Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

s2n-tls-tokio: use s2n_shutdown_send instead of s2n_shutdown #4374

Merged
merged 4 commits into from
Jan 31, 2024

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Jan 23, 2024

Description of changes:

We implemented write-only close / half-close in s2n-tls by adding a new method, s2n_shutdown_send, instead of changing the old method, s2n_shutdown. The tokio bindings pre-date half-close and call s2n_shutdown.

However, AsyncWrite::poll_shutdown should only close the write side of the connection (half-close). First, it's a method on the AsyncWrite trait (there's also an AsyncRead trait), so should really only write, particularly if you consider the case where split is called on the stream. Second, that's what other implementations of the AsyncWrite trait do; for example, TcpStream implements poll_shutdown by calling TcpStream::shutdown(Shutdown::Write).

Call-outs:

This is a behavior change, but it should be relatively safe. Calling s2n_shutdown causes s2n-tls to discard any remaining ApplicationData anyway, so truncation attacks shouldn't be a concern. And the close_notify message is still sent, so the peer isn't affected.

Currently, calling read or write after shutdown causes an error. After this change, that behavior doesn't change for TLS1.2. However, TLS1.3 will allow the application to continue to call read until the close_notify message is received.

Testing:

Updated the existing tests. I could simplify most of the tests now that we're only doing half-close, but I was trying to modify the tests as little as possible to prove that there's very limited effect on an application that calls shutdown. I primarily changed how errors triggered (they have to occur on write now, not on read) and how we verify that the close_notify was actually sent (we can't rely on the peer calling shutdown anymore).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Comment on lines -373 to +378
// s2n_shutdown reading might have triggered blinding again
ready!(self.as_mut().poll_blinding(ctx))?;
// s2n_shutdown_send only writes, so will never trigger blinding again.
// So we do not need to poll_blinding again after this error.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back and forth on whether to leave the call to poll_blinding in or not. We could leave it in just in case we ever introduce a possible blinded error in s2n_shutdown_send, but that should never happen since we only blind when reading. In the end I removed it because I can't test it anymore without doing something crazy and undefined like calling s2n_recv from inside my mocked tcp write call. That's also why I deleted the "shutdown_with_blinding_bad_close_record" test-- I can't reasonably trigger that case anymore.

I left a comment as a reminder of the "double-blinding" problem just in case. But again, we should never read here, since it's the AsyncWrite trait :)

@lrstewart lrstewart marked this pull request as ready for review January 30, 2024 10:26
@lrstewart lrstewart enabled auto-merge (squash) January 31, 2024 18:52
@lrstewart lrstewart merged commit 7227160 into aws:main Jan 31, 2024
30 of 31 checks passed
@lrstewart lrstewart deleted the half_close branch January 31, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants