Dev/sftp-basic by jubeormk1 · Pull Request #41 · mkj/sunset

jubeormk1 · 2026-03-05T01:47:36Z

Intro

Please consider this simplified and improved version of the pull request dev/sftp-start #29. It privides basic sftp server functionality responding to request in order to:

read directory files
read file attributes
put files of arbitrary size
get files up to 5MB (see issue Server Channel Window length is reduced to zero when long data is sent from server to client #40)

Overview of the package

The Sftp requests and responses are implemented in proto.rs.

Uses SSHEncode and SSHDecode together with SftpSink (SshSink) and SftpSource (SshSource) to handle the packets, even when they are received fragmented.

The point of entry of the package is SftpHandler, which will deserialise request and pass them to an structure implementing the SftpServer trait. This allows the library user to program an implementation suitable for its use case. See the provided demo/sftp/std example for more details.

Readability improvements:

The original pull request followed every local commit made, including a lot of trial and error, and would add too many details for main.

For that reason I have decided to make the next improvements:

Groups different parts of the code in commits so it is easier to follow the changes and avoids all the intermediate steps
improved testing scripts

changes First commit summarizing the implementation in the draft pull request contained on the dev/sftp-start branch. I am trying to keep the commit number low and focused on different parts of the implementation. - sshwire-derive/src/lib.rs: Modified enconde_enum to allow encoding of enums discriminants - Added the full proto and sftpsource definitions and Cargo.toml for the sftp crate. - sftp/src/lib.rs: Will experience many changes as the functionality is implemented, but for now it just re-exports the proto and sftpsource modules.

- lib.rs: Now it contains the main library code for the sunset-sftp crate, including module declarations and public exports. Updated documentation to reflect the current state of the library and its features including issue mkj#40. Main additions include: - sftphandler module: Implementation of the main entrypoint for the SFTP server, which will handle incoming SFTP requests and manage the server's state. - sftpserver.rs: Contains the trait definition for the SFTP server that is to be implemented by the user of the library, defining the required methods for handling SFTP operations. - sftperror.rs: Defines error types and handling for the SFTP server operations. Additional files: - sftpsink.rs: An implementation of SSHSink with extra functionality for handling SFTP packets - opaquefilehandle.rs: Collection of traits that a filehandle is expected to implement. About SftpHandler: Main entry point for the SFTP server. It requires to take ownership of an async_channel.rs::ChanInOut in order to write long responses to the client. This makes it not exactly sans-io and not completely observable, but this compromise facilitates the implementation of the SftpServer trait thanks to an internal embassy pipe (See sftpoutputchannelhandler.rs).

@jubeormk1

… challenges and other changes to sunset From 'matt/sftptesting' into dev/sftp-start commits: - 947cd6e - 2869501 And @jubeormk1 - a44f70c

TODO: Improve the tests. Running them is a bit hairy. You need to run the demo server from the root of the repo, and then run the test scripts from the testing folder. This is an implementation of the sunset-sftp basic functionality. It also has a testing folder with scripts to test different operations

- Can be run from the repo base folder. Other pwd will fail - All of them will return a value coherent with the test result so they can be used for CI - Improved, but complicating dir listing and stat listing test: require expect and use expect script for the test

jubeormk1 · 2026-03-05T01:52:09Z

Apologies, I will fix the CI

Should have run testing/ci.sh beforehand

mkj

Thanks for your efforts and getting that tidied up.
I will have to add more doc comments to the rest of Sunset to keep up!

I've added a few first-pass review comments looking over it, fairly superficial. I'll have a closer look at the details later.

mkj · 2026-03-09T14:07:49Z

sshwire-derive/src/lib.rs

Don't think this file should be included, it reverts some more recent upstream changes

mkj · 2026-03-09T14:08:05Z

sftp/Cargo.toml

+edition = "2024"
+
+[features]
+default = []


Don't need default feature if it's empty

mkj · 2026-03-09T14:10:28Z

sftp/src/sftpsource.rs

+        debug!("New source with content: : {:?}", buffer);
+        SftpSource { buffer: buffer, index: 0 }
+    }
+    /// Peaks the buffer for packet type [`SftpNum`]. This does not advance


"Peek" rather than "Peak"

mkj · 2026-03-09T14:14:01Z

sftp/src/proto.rs

+        impl<'a: 'de, 'de> SSHDecode<'de> for SftpPacket<'a>
+        where 'de: 'a // This implies that both lifetimes are equal


The where 'de: 'a doesn't seem right, as you comment it implies just a single lifetime 'a or 'de could be used there.

I'll try some experiments simplifying it.

mkj · 2026-03-09T14:15:40Z

sftp/src/proto.rs

+        /// **Warning**: No Sequence Id can be infered from a Packet Type
+        impl<'a> From<$request_packet_type> for SftpPacket<'a> {
+            fn from(s: $request_packet_type) -> SftpPacket<'a> {
+                warn!("Casting from {:?} to SftpPacket cannot set Request Id",$request_ssh_fxp_name);


I don't think a warn! makes sense here, it's not a useful warning to end up in logs.

I'm not sure how they are used yet though, so I'll check.

mkj · 2026-03-09T14:52:44Z

sftp/src/opaquefilehandle.rs

+{
+    /// Creates a new instance using a given string slice as `seed` which
+    /// content should not clearly related to the seed
+    fn new(seed: &str) -> Self;


I'm not sure that OpaqueFileHandle should have a constructor from a string, it seems brittle unless using >128 bit identifiers and cryptographic hashes.

To me it seems that it would be better for OpaqueFileHandleManager to have private knowledge of the OpaqueFileHandle, then it could construct it directly with a plain counter or something like that, and avoid duplicates by checking its own handle_map etc. Wouldn't need to worry about salts either.

(Can leave it for the time being though)

mkj · 2026-03-09T14:55:12Z

sftp/src/sftpserver.rs

+    }
+
+    /// Provides the stats of the given file path
+    fn stats(


Maybe call it attrs()? Or stat() to match the spec/posix syscall.

mkj · 2026-03-09T15:02:57Z

testing/ci.sh

 set -e

-export CARGO_TARGET_DIR=target/ci
+export CARGO_TARGET_DIR=testing/target


This reverts recent upstream 342a515

mkj · 2026-03-09T15:04:53Z

testing/ci.sh

+cargo build --release
+cargo test --release
+cargo bloat --release -n 100 | tee "$OUT/sftp-std-bloat.txt"
+cargo bloat --release --crates | tee "$OUT/sftp-std-bloat-crates.txt"
+)
+size ./target/release/sunset-demo-sftp-std | tee "$OUT/sftp-std-size.txt"


Since this is for non-embedded can probably build without --release and don't need to run bloat or record size.
The one for picow was to check size on embedded targets, we aren't worrying about that for std demos.

mkj · 2026-03-09T15:12:15Z

src/packets.rs

@@ -882,7 +882,7 @@ pub struct ParseContext {

    // Set to true if an unknown variant is encountered.
    // Packet length checks should be omitted in that case.
-    pub(crate) seen_unknown: bool,


Maybe the changes in packets.rs should be their own commit, since they're good to keep.

The other changes in that commit are bodge workarounds that might get reverted later, wouldn't want to lose the packet.rs changes.

Thanks for the review, you have risen some good points. I am going to continue addressing your review, for now these are my changes: - removed default = [] as it is unnecessary - warn->debug for From request_packet_type for SftpPacket - requestholder.rs::RequestHolder.valid_request() : explicit None on Err() try_get_ref() - SftpServer.rs::SftpServe.stats()->attrs() and uses replaced - sftpsource.rs::SftpSource.peak_packet_type()->peek_packet_type() - ci.sh undo revert from [342a515](mkj@342a515) and now builds `demo/sftp/std` without release or bloat

jubeormk1 added 6 commits March 4, 2026 10:30

[skip ci] Adding @mkj contributions troubleshooting channel reception…

1f38033

… challenges and other changes to sunset From 'matt/sftptesting' into dev/sftp-start commits: - 947cd6e - 2869501 And @jubeormk1 - a44f70c

CI updated

8ef777f

CI fix: cargo fmt

5259179

Should have run testing/ci.sh beforehand

mkj reviewed Mar 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev/sftp-basic#41

Dev/sftp-basic#41
jubeormk1 wants to merge 8 commits intomkj:mainfrom
jubeormk1:dev/sftp-basic

jubeormk1 commented Mar 5, 2026

Uh oh!

jubeormk1 commented Mar 5, 2026

Uh oh!

mkj left a comment

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

mkj Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		impl<'a: 'de, 'de> SSHDecode<'de> for SftpPacket<'a>
		where 'de: 'a // This implies that both lifetimes are equal

Conversation

jubeormk1 commented Mar 5, 2026

Intro

Overview of the package

Readability improvements:

Uh oh!

jubeormk1 commented Mar 5, 2026

Uh oh!

mkj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants