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

Back to Draft stage

I decided to step back and make this PR a draft again because I want to cover several things that came up, such as changes to main and some action items from SSH-Stamp security audit performed by Radically Open Security. These are the elements that I will be working on:

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 · 2026-03-05T01:52:09Z

Apologies, I will fix the CI

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

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

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

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


Don't need default feature if it's empty

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

+        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

+        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:52:44Z

+{
+    /// 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

+    }
+
+    /// 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:12:15Z

@@ -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.

jubeormk1 · 2026-04-16T04:39:05Z

Rebasing from mkj/sunset main branch happened from annotation for commit a5e42fa.

The tests in demo/sftp/std/testing/ passed as usual excluding the get_file_long.sh that catches the known issue #40.

jubeormk1 · 2026-04-17T01:44:51Z

Doing a search for lifetimes returns that after @mkj intervention there is no more structures with multiple lifetimes defined.

mkj · 2026-04-22T13:15:06Z

-    type Err;
-
+/// Used in the `OpaqueFileHandleManager` to generate a Key (OpaqueFileHandle)
+pub trait InitFileHandler: Sized {


I'm not sure why this needs to be a trait? init() isn't called by the sftp crate, it can be an implementation detail

mkj · 2026-04-22T13:18:38Z

+        let mut tiny_hash = [0u8; ID_LEN];
        tiny_hash.copy_from_slice(file_handle.0 .0);
-        Ok(DemoOpaqueFileHandle { tiny_hash })
+        Ok(DemoOpaqueFileHandle { handle_id: tiny_hash })
    }


Naming it "hash" here is a bit unclear, since it's just an ID not hashed.

mkj · 2026-04-22T13:20:16Z

-            format!("{:}-{:}", &private_handle.get_path_ref(), salt).as_str(),
-        )
-        .map_err(|_| StatusCode::SSH_FX_FAILURE)?;
+        let handle = K::init();


Would it be easier to call init(counter) rather than having a RNG? The RNG works OK though.

My apologies, I though that It had been merged to the main branch and merged when the env_var (pull 35) feature was included

… use #[sshwire(unknown)] Without changing some visiblitities, the code generated by SSHDecode (SSHDecodeEnum) for proto.rs would try accessing s.ctx().seen_unknown and Unknown::new() throwing errors. Am I doing using SSHDecode wrong?

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

Should have run testing/ci.sh beforehand

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

These should have been added to the previous commit but I did not track the changes: - ci.sh: Changing back teh target folder - sftpserver.rs: Renaming stats to attrs trait and changing all related implementations and calls - sftpsource.rs: Fixing typo peak-> peek in file and related calls

- [x] All tests in testing passing - [ ] Deleting forgotten 512B_random file from the testing output directory

Addressing needed changes in proto.rs. I looked at the code generated by the macro before reverting lib.rs (cargo-expand expand) and applied equivalent code.

@mkj

As pointed out by @mkj, the new(& str) method in `OpaqueFileHandle` is brittle. I added it mindlessly by the DemoFileHandleManager implementation. Now I replaced the `new(&str)`by adding the condition to whoever decides to use `FileHandleManager` trait to implement `InitWithSeed` + `OpaqueFileHandle` for the key.

Sorry about the noise

The read refcount wasn't incremented on ChanIn or ChanInOut clone. This could result in input being discarded if the read refcount hit zero. It isn't clear whether this could result in missed EOF (which has been reported).

This is required by time 0.3.47

These were reported by cargo audit keccak bytes time instant

After cherry picking mkj@a8a27eb

In order to provide a safe example for future SftpServer implementers.

To provide better usage practices as recommended by Radically open Security, I have removed the "tiny_hash" and replaced it by an unpredictable 32 bytes array. All salt and seed mention removed. This should make

@mkj

@mkj thanks for catching this one. I believe that with this name adjustment there is no more references to hash or tiny in variables whiting the demo/sftp/std

jubeormk1 · 2026-05-01T08:17:46Z

Looks like the force-push has caused some unintended changes.

Having channel.rs::Channel.check_window_adjust with inmutable &self makes sense since there is no self mutation.
The other change is a comment, which might be outdated

mkj

I've noted a few future cleanups. I'm OK with merging this as-is though since it's self-contained to the sftp directories and can be marked as "alpha". Thanks for you work on it.

It'll be easier to work on further changes with it in-tree.

mkj · 2026-05-28T23:44:30Z

+                        if !request.sftp_num().is_request() {
+                            error!(
+                                "Unexpected SftpPacket: {:?}",
+                                request.sftp_num()
+                            );
+                            return Err(SunsetError::BadUsage {}.into());
+                        }
+                        match request {


Could the request handler (the whole match below) go in a separate function? Would improve the nesting and make this function shorter.

mkj · 2026-05-28T23:46:05Z

+                        match a.command()?.to_lowercase().as_str() {
+                            "sftp" => {


No need for lowercase, it's spec-defined

mkj · 2026-05-28T23:50:02Z

+        let (mut chan_in, chan_out) = stdio.split();
+
+        let mut sftp_output_pipe = SftpOutputPipe::<BUFFER_OUT_SIZE>::new();
+
+        let (mut output_consumer, output_producer) =
+            sftp_output_pipe.split(chan_out)?;


It wasn't immediately obvious why it needed this extra pipe. Was there a problem trying to write to chan_out directly with send_status() etc? (I guess there could have been async lifetime issues?)
No need to change now, just so there's a reference if refactoring in future. (PR reply is fine, doesn't need to be in the code)

Ah, because the producer isn't mutable. Maybe a Mutex around chan_out could do the same? Can try later.

/// Producer used to send data to a [`ChanOut`] without the restrictions /// of mutable borrows

mkj · 2026-05-29T00:11:46Z

+pub trait SftpServer<T>
+where
+    T: OpaqueFileHandle,
+{
+    /// Opens a file for reading/writing


I think it might be simpler to have type LocalHandle; as an associated type of SftpServer.
Move the to/from Handle bits to SftpServer as

trait SftpServer { type LocalHandle; // not sure if it needs Clone, Eq etc? Perhaps not. Maybe Debug? // ... /// Creates a new `LocalHandle` based on the content of the `FileHandle` fn get_local_handle(&mut self, file_handle: &FileHandle<'_>) -> WireResult<Self::LocalHandle>; /// Returns a FileHandle pointing to the data in the `OpaqueFileHandleTrait` Implementation fn from_handle(&self, h: Self::LocalHandle) -> FileHandle<'_>;

The returned from_handle() would need to borrow from some storage in the SftpServer which isn't ideal, but think it'd work OK where I've seen. I'll give it a try.

I think it should actually be

fn from_handle(&'a mut self, h: &'a Self::LocalHandle) -> FileHandle<'a>;

So then it could borrow from either self (storage in SftpServer) or the LocalHandle (if the implementation for example keeps unique byte arrays in LocalHandle)

The reason I see this being simpler is that SftpHandler<'a, S> only needs a single generic parameter now. Can get rid of OpaqueFileHandleManager and InitFileHandler traits too.

mkj · 2026-05-29T00:13:09Z

+/// The SFTP module user is not required to use it but instead is a suggestion for an exchangeable
+/// trait that facilitates structuring the store and retrieve of 'OpaqueFileHandleTrait' (K),
+/// together with a private handle type or structure (V) that will contains all the details internally stored for the given file.
+///
+/// The only requisite for v is that implements PathFinder, which in fact is another suggested helper to allow the `OpaqueHandleManager`
+/// to look for the file path.
+pub trait OpaqueFileHandleManager<K, V>
+where


I don't think this is needed in sftp crate, but can wait to see how OpaqueFileHandle rework goes

mkj · 2026-05-29T00:15:24Z

+    pub async fn process_loop(
+        &mut self,
+        stdio: ChanInOut<'_>,
+        buffer_in: &mut [u8],
+    ) -> SftpResult<()> {
+        let (mut chan_in, chan_out) = stdio.split();


Eventually it would be nice for this to take chan_in: &mut impl Read, chan_out: &mut impl Write and then you could use it with anything, not just sunset. That'd let it be a drop-in replacement for openssh sftp-server for process stdin/stdout.

Would need some work on the generic error type vs SunsetError

jubeormk1 · 2026-05-31T09:33:59Z

Thanks for your review!

I just read your comments. I am starting to work on the ones that I have answer this week. I will need to digest the comment around sftpserver.rs LocalHandle before I can action anything there.

It is great that you consider incorporating this SFTP as an Alpha version. Shall I resolve conflicts, create issues for every one of your points and work them as small tasks rather than this very big PR? Let me know what you prefer.

mkj · 2026-05-31T12:33:51Z

I've resolved the conflicts in my dev/sftp-rebase branch, if you want to take that for your branch for this PR it'd work. Tidied a few commit messages too there. If you can do that I'll merge this, then you can open PRs for individual changes.

Probably no need to create issues for them - most of them aren't things that definitely need fixing - they're possible future changes.

mkj · 2026-06-01T23:57:31Z

Can you rebase your branch here to main rather than merging? Otherwise there will be all the duplicate cherry pick commits in history. Force push is fine.

The "Co-authored-by: copilot" in "Addressing potential integer overflow" is wrong, can you remove that? Microsoft don't need advertising here.

@mkj

Thanks to @mkj for spotting and proposing a fix

Creating a new module for the read replies and refactoring the SFTP read operations to use the new types. This is a step towards better separation of concerns and clearer code structure, especially around the handling of read operations in the SFTP server. This decluttering allows us to isolate the logic related to read replies, improving readability and maintainability.

This new structures enforce the correct sequence of operations for handling SFTP read requests, guiding the library user to provide the necessary information in simple types in the correct order. Other changes are related to the sftpoutputchannelhandler, where the OutChan has been replaced by a trait bound Write with an associated error type. This helps testing.

…quests - Base structure for readdir replies, similar to readreplies.rs - Added a test to demonstrate how the structure can be used to compose a readdir response - Refactoring helpers - Cleaning comments

I moved the std_helpers to the demo to keep the library more readable and not force the user into a particular implementation. Fixed an issue caused by copy-paste the readreplies.rs where I was not sending the right response back. Refactored sftphandle to use the new interfaces. Refreshing lib.rs, renaming some constants, etc

This made me find many problems in the docs and now the tests are finally part of the CI

- 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). After that I have tested all my ./demo/sftp/std/testing/test_*.sh script and all worked fine except test_get_file_long.sh which triggers a known bug mkj#40 I edited error.rs to avoid a CI check fail around unused use statements. All test passed after this

After rebasing the fork in the current upstream main there where some signature changes

…ated requests

jubeormk1 · 2026-06-02T07:47:57Z

@mkj I just removed the Add from the commit as suggested. See it here.

I have also picked your changes in the matt/sftp-rebase. I have added some more refactors, such as changing process_loop signature to accept Read and Write traits and extracted the sftp packet process into a new method to simplify and making more readable the sfftpHandler match blocks.

I will finish ironing the ci spotted issues and stop making changes in this PR.

Done in github interface. + Manual cargo fmt

mkj · 2026-06-02T12:35:06Z

Thanks for your work on this

jubeormk1 added 2 commits March 4, 2026 10:30

mkj reviewed Mar 9, 2026

View reviewed changes

jubeormk1 marked this pull request as draft April 8, 2026 21:22

mkj reviewed Apr 22, 2026

View reviewed changes

jubeormk1 and others added 22 commits May 1, 2026 17:04

Merge branch 'matt/sftptesting' into dev/sftp-start

636c25e

My apologies, I though that It had been merged to the main branch and merged when the env_var (pull 35) feature was included

CI updated

4b487eb

CI fix: cargo fmt

8b8db45

Should have run testing/ci.sh beforehand

removing size from demo/sftp/std build outputs

0dd3c51

Fixing unnecessary duplicated lifetimes and tidying up

410fb76

- [x] All tests in testing passing - [ ] Deleting forgotten 512B_random file from the testing output directory

Reverting changes to sshwire-derive/src/lib.rs

43f7cdf

Addressing needed changes in proto.rs. I looked at the code generated by the macro before reverting lib.rs (cargo-expand expand) and applied equivalent code.

RUSTSEC-2024-0436: Fixing Paste to version 1.0.25

6803686

Fixing typo in previous commit

3c90e35

Extra typo. Running CI now to make sure that all is good

2cfa4d2

Sorry about the noise

sftp: simplify some lifetimes

8206f86

Fix formatting for "sftp: simplify lifetimes"

bca8569

Fix read refcount for ChanIn and ChanInOut clone()

b303a67

The read refcount wasn't incremented on ChanIn or ChanInOut clone. This could result in input being discarded if the read refcount hit zero. It isn't clear whether this could result in missed EOF (which has been reported).

Rust 1.88 min version

8d93701

This is required by time 0.3.47

Update some outdated dependencies

0f81359

These were reported by cargo audit keccak bytes time instant

Fix some clippy warnings

c6f6736

pretty-hex isn't needed, plain hex format instead

c15e10b

jubeormk1 added 4 commits May 1, 2026 17:05

Fixing fmt to pass CI

68e6abc

After cherry picking mkj@a8a27eb

Adding strict-path to demo/sftp/std

ded1946

In order to provide a safe example for future SftpServer implementers.

Simplifying sftp std example. No seeds + 32 bytes handle_id

283d4b4

To provide better usage practices as recommended by Radically open Security, I have removed the "tiny_hash" and replaced it by an unpredictable 32 bytes array. All salt and seed mention removed. This should make

Fixing unclear naming in OpaqueFileHandle implementation

54573c8

@mkj thanks for catching this one. I believe that with this name adjustment there is no more references to hash or tiny in variables whiting the demo/sftp/std

jubeormk1 force-pushed the dev/sftp-basic branch from d6ce589 to 6714283 Compare May 1, 2026 08:15

mkj reviewed May 29, 2026

View reviewed changes

mkj marked this pull request as ready for review June 1, 2026 23:57

jubeormk1 and others added 12 commits June 2, 2026 07:20

Addressing potential integer overflow

d6046f9

Thanks to @mkj for spotting and proposing a fix

Removing dead docs & Minimal rearrangement of methods

b802a1a

Adding readdirreplies.rs to guide response composition for readdir re…

f7bce96

…quests - Base structure for readdir replies, similar to readreplies.rs - Added a test to demonstrate how the structure can be used to compose a readdir response - Refactoring helpers - Cleaning comments

WIP: Starting to replace DirReply with Enforcing readdirreplies

7d9975f

Adding sunset-sftp testing and doc building to ci.sh

8d1a42a

This made me find many problems in the docs and now the tests are finally part of the CI

Fixing rebase

94805f3

After rebasing the fork in the current upstream main there where some signature changes

sftp: Replace todo!() with Error::bug()

abb09d0

process_loop signature updated && extracted function to process valid…

b77db80

…ated requests

jubeormk1 force-pushed the dev/sftp-basic branch 2 times, most recently from f70b3c6 to e198ada Compare June 2, 2026 07:41

Merge branch 'main' into dev/sftp-basic

1d1a7e0

Done in github interface. + Manual cargo fmt

jubeormk1 force-pushed the dev/sftp-basic branch from e198ada to 1d1a7e0 Compare June 2, 2026 07:51

mkj merged commit 1d1a7e0 into mkj:main Jun 2, 2026
2 checks passed

		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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Intro

Overview of the package

Readability improvements:

Back to Draft stage

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jubeormk1 commented Apr 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jubeormk1 commented Apr 17, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mkj Apr 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jubeormk1 commented May 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

jubeormk1 commented May 31, 2026

Uh oh!

mkj commented May 31, 2026

Uh oh!

mkj commented Jun 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jubeormk1 commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mkj commented Jun 2, 2026

Uh oh!

jubeormk1 commented Mar 5, 2026 •

edited

Loading

jubeormk1 commented Apr 16, 2026 •

edited

Loading

mkj Apr 22, 2026 •

edited

Loading

jubeormk1 commented May 1, 2026 •

edited

Loading

mkj commented Jun 1, 2026 •

edited

Loading

jubeormk1 commented Jun 2, 2026 •

edited

Loading