Skip to content

fix: prevent one-sided auth when stream teardown races connection drop#6

Open
gusinacio wants to merge 1 commit into
rustonbsd:mainfrom
basilica-digital:main
Open

fix: prevent one-sided auth when stream teardown races connection drop#6
gusinacio wants to merge 1 commit into
rustonbsd:mainfrom
basilica-digital:main

Conversation

@gusinacio

Copy link
Copy Markdown

end_of_auth awaited send.stopped() and propagated any error up the call stack. In practice this raced with the peer dropping its Connection as soon as it finished auth, so the slower side frequently observed connection lost from stopped().await and returned an error before calling add_authenticated. The faster side ended up authenticated, the slower side did not, and the real application connection timed out in after_handshake waiting for the watcher to tick.

Two changes fix this:

  1. end_of_auth becomes best-effort. All SPAKE2 tokens and HKDF-derived keys are already written and ACKed at the QUIC layer by the time we reach this function; the stream FIN is cosmetic and should not be able to fail auth.

  2. add_authenticated is moved before end_of_auth in both auth_accept and auth_open. Even if teardown is ever made non-best-effort again, the authenticated set is committed first.

The error branches in both paths also call end_of_auth best-effort so the stream is still finalized on mismatch without masking the real ...FailedAndBlock error.

All existing tests continue to pass.

`end_of_auth` awaited `send.stopped()` and propagated any error up the call
stack. In practice this raced with the peer dropping its `Connection` as
soon as *it* finished auth, so the slower side frequently observed
`connection lost` from `stopped().await` and returned an error *before*
calling `add_authenticated`. The faster side ended up authenticated, the
slower side did not, and the real application connection timed out in
`after_handshake` waiting for the watcher to tick.

Two changes fix this:

1. `end_of_auth` becomes best-effort. All SPAKE2 tokens and HKDF-derived
   keys are already written and ACKed at the QUIC layer by the time we
   reach this function; the stream FIN is cosmetic and should not be able
   to fail auth.

2. `add_authenticated` is moved *before* `end_of_auth` in both
   `auth_accept` and `auth_open`. Even if teardown is ever made
   non-best-effort again, the authenticated set is committed first.

The error branches in both paths also call `end_of_auth` best-effort so
the stream is still finalized on mismatch without masking the real
`...FailedAndBlock` error.

All existing tests continue to pass.
@rustonbsd

Copy link
Copy Markdown
Owner

Hi @gusinacio, thank you for the PR!

The issue in end_of_auth is definitely a bug. I have already implemented a fix along with a lot of stability improvements based on my network degradation testing in #5.

I suggest that after I push #5 to main we rebase this branch and keep your ordering changes and if required adopt the the infallible end_of_auth approach. I agree that add_authenticated should happen before we call end_of_auth and if after the changes from #5 we still fail the end_of_auth function asymmetrically, then we use your approach if still required.

My solution was to use recv.read_to_end(usize::MAX).await instead of stopp. This should not fail even if the other stream is already closed, but if the read_to_end or the finish call still fail, we use your version.

iroh-auth/src/lib.rs

Lines 317 to 331 in 08e8930

if let Err(err) = recv.read_to_end(usize::MAX).await.map_err(|err| {
if open {
AuthenticatorError::OpenFailed(format!(
"Failed to wait for stream stopped: {}",
err
))
} else {
AuthenticatorError::AcceptFailed(format!(
"Failed to wait for stream stopped: {}",
err
))
}
}) {
warn!("[end_of_auth] {}", err);
}

I will write you here once I have updated main. Do you want to rebase it after or should I? (don't want to mess around with your fork if you don't agree ^^). Let me know what you think!

@rustonbsd rustonbsd added the bug Something isn't working label Apr 17, 2026
@gusinacio

Copy link
Copy Markdown
Author

Feel free to rebase it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants