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

[bindings] Apply async blinding #3356

Merged
merged 3 commits into from
Jun 13, 2022
Merged

[bindings] Apply async blinding #3356

merged 3 commits into from
Jun 13, 2022

Conversation

lrstewart
Copy link
Contributor

Description of changes:

This one's a little long, but mostly tests.

Currently, TlsStream uses the default built-in blinding, which means that a blinded error causes the underlying s2n-tls library to block the thread until the blinding delay (10-30s) has passed. Instead, we need to use self-service blinding and wait asynchronously on the blinding.

I added the wait to poll_shutdown(). s2n-tls actually enforces the wait immediately when the error occurs (so in s2n_negotiate or s2n_recv), but that complicated the implementation since I'd need to save the result of s2n_negotiate and s2n_recv to be returned after the blinding. Enforcing the blinding in shutdown should be sufficient, since shutdown is what responds to the peer and closes the connection.

I also added logic to call shutdown if the handshake fails. Because the TlsStream isn't returned to the caller if the handshake fails, the caller can't be responsible for gracefully shutting down connections after failed handshakes. We need to do it for them.

Currently, calling s2n_shutdown usually doesn't work:

  • If a close_notify alert is received during s2n_negotiate, it is not considered an error and the handshake tries to continue. However, there will be no more data, so the handshake just hangs. I added a minimal fix for this by at least checking that the connection isn't closed before trying to continue the handshake.
  • If a blinded error occurs during s2n_recv, and we try to call s2n_negotiate afterwards, usually the fatal error in s2n_recv will have left the connection in such a state that it can't receive the peer's close_notify and s2n_shutdown will fail. I did not fix this bug as part of this PR.

Testing:

Testing this proved a little tricky, since it required specific errors from send / receive. All the existing test utilities I found required knowing the exact sequence of reads and writes, which we don't because of the handshake. So I added a little test stream that allows read and write to be temporarily overridden, but usually just calls a real stream.

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

@lrstewart lrstewart requested review from camshaft and maddeleine June 9, 2022 17:52
@github-actions github-actions bot added the s2n-core team label Jun 9, 2022
@lrstewart lrstewart changed the title Apply async blinding [bindings] Apply async blinding Jun 9, 2022
@lrstewart lrstewart marked this pull request as ready for review June 9, 2022 18:35
/// then the -1 error result wraps around to the maximum value.
/// The maximum value must not be possible otherwise.
///
/// For example, [`s2n_connection_get_delay`] can't return
Copy link
Contributor

Choose a reason for hiding this comment

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

So I guess what you're trying to say is that s2n_connection_get_delay isn't Fallible? I am not following this explanation.

Copy link
Contributor Author

@lrstewart lrstewart Jun 13, 2022

Choose a reason for hiding this comment

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

s2n_connection_get_delay is Fallible, just in a really unfortunate way. I updated the comment-- does that makes it clearer?

Probably important to note that this is "impl Fallible for u64", meaning it's for an unsigned int.

@lrstewart lrstewart enabled auto-merge (squash) June 13, 2022 16:29
@lrstewart lrstewart merged commit 0459b41 into aws:main Jun 13, 2022
@lrstewart lrstewart deleted the rust_blinding branch June 13, 2022 17:39
dougch pushed a commit to dougch/s2n-tls that referenced this pull request Jun 27, 2022
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