Skip to content

fix(tokio): send close_notify on finalize_retr_stream to fix TLS 1.3 data-channel 426 - #174

Merged
veeso merged 3 commits into
veeso:mainfrom
Smooveemaan:fix/tokio-finalize-retr-close-notify
Aug 31, 2026
Merged

fix(tokio): send close_notify on finalize_retr_stream to fix TLS 1.3 data-channel 426#174
veeso merged 3 commits into
veeso:mainfrom
Smooveemaan:fix/tokio-finalize-retr-close-notify

Conversation

@Smooveemaan

Copy link
Copy Markdown
Contributor

Summary

finalize_retr_stream() on the tokio backend (crates/suppaftp/src/async_ftp/tokio_ftp.rs) drops the data-connection stream without a graceful shutdown, unlike finalize_put_stream(), which already calls stream.shutdown().await. For a plain TCP data stream this is harmless, but for a TLS-secured FTPS data channel it means no close_notify is ever sent.

TLS 1.2 servers tolerate the abrupt close. TLS-1.3-strict servers reply 426 Transfer failed (unable to close data connection gracefully) even though the transfer already completed — this is what forces downstream consumers of this crate to cap their rustls::ClientConfig at TLS 1.2 for FTPS to work at all.

Root cause / how I verified it

Reproduced live against test.rebex.net (public FTPS server, TLS 1.3) with RUST_LOG=trace. My first hypothesis was a TLS-session-resumption mismatch between the control and data channel (session-ID vs. session-ticket schemes), but the trace ruled that out: rustls logs Resuming using PSK, and the full LIST payload is read from the data stream before the 426 shows up. The 426 only disappears once the data stream is shut down cleanly before being dropped — i.e. it's purely about the missing close_notify, not resumption.

I exercised this with a small standalone client running two scenarios against test.rebex.net: an unrestricted rustls::ClientConfig (negotiates TLS 1.3) and one capped at TLS 1.2. Before this fix, only the TLS-1.2-capped path completed LIST; after this fix, both do.

Change

Widens finalize_retr_stream()'s bound from impl AsyncRead to impl AsyncRead + AsyncWriteExt + Unpin (matching finalize_put_stream's existing bound) and sends close_notify before dropping. All existing callers (retr(), list()/nlst()/mlsd() via stream_lines(), and the test suite) already pass a DataStream<T> or a BufReader wrapping one, both of which implement AsyncWrite, so no call sites needed to change.

Shutdown errors are ignored deliberately: the data has already been fully read by the time finalize_retr_stream runs, so a failed shutdown must not fail an otherwise-successful transfer. This is intentionally looser than finalize_put_stream, which still propagates the shutdown error (for uploads, the write isn't confirmed complete until shutdown succeeds).

Not included

sync_ftp.rs and the async-std smol_ftp.rs backend have the same drop-without-shutdown pattern in their finalize_retr_stream. I only patched and verified the tokio backend (the one exercised above) — the other two likely need the equivalent fix but I haven't tested them, so I left them out of this PR rather than guess.

Testing

  • cargo build -p suppaftp --no-default-features --features tokio-rustls-ring — compiles clean.
  • Live reproduction via a standalone client against test.rebex.net (both TLS 1.2-capped and unrestricted/TLS 1.3 configs) — both complete LIST after this fix; only the capped one did before.

@cocogitto-bot

cocogitto-bot Bot commented Aug 29, 2026

Copy link
Copy Markdown

✔️ ebc6885 - Conventional commits check succeeded.

Smooveemaan and others added 2 commits August 31, 2026 10:45
finalize_retr_stream() (used by retr()/list()/nlst()/mlsd() on the tokio
backend) dropped the data-connection stream without a graceful shutdown,
unlike finalize_put_stream() which already calls stream.shutdown().

For a plain TCP data stream this is harmless, but for a TLS-secured FTPS
data channel it means no close_notify is sent. TLS 1.2 servers tolerate
the abrupt close (session-ID based resumption apparently masks it), but
TLS-1.3-strict servers reply "426 Transfer failed (unable to close data
connection gracefully)" even though the transfer already completed —
reproduced live against test.rebex.net (public FTPS server, TLS 1.3) with
RUST_LOG=trace: rustls confirms `Resuming using PSK` and the full LIST
payload is read before the 426 appears, ruling out a session-resumption
mismatch. The 426 only goes away when the data stream shuts down cleanly.

This widens finalize_retr_stream()'s bound from `impl AsyncRead` to
`impl AsyncRead + AsyncWriteExt + Unpin` (matching finalize_put_stream's
existing bound) and sends the close_notify before dropping. Shutdown
errors are ignored, mirroring the fact that the data has already been
fully read by this point — a failed shutdown must not fail an
otherwise-successful transfer (finalize_put_stream is stricter here since
for uploads the write isn't confirmed complete until shutdown succeeds).

Verified with a small standalone client exercising both an unrestricted
rustls config (negotiates TLS 1.3) and one capped at TLS 1.2 against
test.rebex.net: before this fix, only the TLS-1.2-capped path completed
LIST; after, both do.

Note: sync_ftp.rs and the async-std smol_ftp.rs backend have the same
drop-without-shutdown pattern in their finalize_retr_stream. I only
patched and verified the tokio backend (the one exercised above); the
other two likely need the equivalent fix but I haven't tested them.

BREAKING CHANGE: tokio finalize_retr_stream now requires streams to implement AsyncWrite and Unpin.
Mirror Tokio retrieval finalization by closing smol data streams before reading the final control response. Add deterministic coverage for both runtimes and document the response-authority policy.

BREAKING CHANGE: smol finalize_retr_stream now requires streams to implement AsyncWrite and Unpin.
@veeso
veeso force-pushed the fix/tokio-finalize-retr-close-notify branch from 5393c5c to 66f6db4 Compare August 31, 2026 08:47
@veeso
veeso merged commit f3b22c1 into veeso:main Aug 31, 2026
63 of 64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants