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

feature(bindings): scheduled renegotiation via poll_recv #4764

Merged
merged 12 commits into from
Oct 3, 2024

Conversation

lrstewart
Copy link
Contributor

@lrstewart lrstewart commented Sep 11, 2024

Description of changes:

This adds an option to let s2n-tls handle renegotiation automatically when poll_recv is called. It makes renegotiation more usable with higher level abstractions like s2n-tls-tokio and s2n-tls-hyper.

This option can't handle poll_send. I tried a couple different approaches, but there are two major problems:

  1. If poll_send can also renegotiate, then it may block on server application data send after the client hello. I'm not sure this problem is solvable, although it could be circumvented by not supporting application data sent after the hello request.
  2. If poll_send can clear buffered send data to unblock renegotiation, then it also needs to be able to wake any receive operations blocked on buffered send data. The mid-level bindings have no concept of IO wakers, so cannot do that.

This is a mess, but it's a mess that's compile-time confined to renegotiate.rs, so I'm not particularly worried about the effect on other customers.

Could customers use renegotiation + tokio or hyper without this change?

I think so, but it would require fairly advanced manipulation of s2n-tls and futures, which s2n-tls-tokio and s2n-tls-hyper are designed to avoid.

For s2n-tls-tokio: The application could implement a wrapper around the TlsStream returned by TlsConnector. Like TlsStream, that wrapper would implement AsyncRead and AsyncWrite. It would mostly just poll the underlying TlsStream, but could perform renegotiation if necessary. The logic would likely look similar to what I've implemented here.

For s2n-tls-hyper: The application could implement a wrapper around the future returned by send_request. That wrapper would also implement Future. It would mostly just poll the underlying future, but could perform renegotiaton if necessary. I think it could assume no partial reads or writes blocking wiping, because 1) you wouldn't read a response until the request is completely sent 2) if renegotiation was actually mandatory then the server wouldn't write a response until after the second handshake. You might be able to get away with only renegotiating if the wipe succeeds, and carrying on as normal otherwise. If you can't assume the server won't respond until after renegotiation, things get trickier.

Call-outs:

  • I've tried to comment the worst parts, but this IO is complex so please ask questions where things aren't clear so I can add even more documentation.
  • This doesn't cover s2n-tls-tokio tests, which I think we need to really prove that this works. I've got a branch with working tests: bbd55a7 It uses a pretty hacky model suggested by camshaft to avoid "unstable-renegotiate" infecting other tests.

Testing:

I updated the existing tests and added more cases.

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

@github-actions github-actions bot added the s2n-core team label Sep 11, 2024
@lrstewart lrstewart force-pushed the bindings_renegotiate branch 3 times, most recently from 61d3eab to dbeeb27 Compare September 11, 2024 20:20
@lrstewart lrstewart marked this pull request as ready for review September 11, 2024 20:32
bindings/rust/s2n-tls/src/connection.rs Outdated Show resolved Hide resolved
api/unstable/renegotiate.h Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
Comment on lines +205 to 206
self.as_mut().set_blinding(Blinding::SelfService)?;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While updating my doc example I thought, "I'll use set_blinding instead of set_server_name" and then realized that s2n-tls-tokio probably did that automatically. I'm moving the call here so that the blinding can't be changed out from underneath s2n-tls-tokio.

I also checked for other connection-level configuration methods in s2n-tls-tokio and found none.

@lrstewart lrstewart changed the title feature: pivot renegotiation bindings to managed by s2n-tls feature(bindings): scheduled renegotiation via poll_recv Sep 23, 2024
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
@lrstewart lrstewart requested a review from goatgoose September 27, 2024 01:54
bindings/rust/s2n-tls/src/error.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/error.rs Outdated Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Show resolved Hide resolved
bindings/rust/s2n-tls/src/renegotiate.rs Outdated Show resolved Hide resolved
&mut self,
buf_ptr: *mut libc::c_void,
buf_len: isize,
) -> (Poll<Result<(), Error>>, usize) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The return signature is a bit awkward here... Would it be better to have something like:

Suggested change
) -> (Poll<Result<(), Error>>, usize) {
) -> Poll<Result<usize, Error>> {

Seems like all of the callers have the same match:

match self.poll_renegotiate_raw(buf_ptr, buf_len) {
    (Ready(Err(err)), _) => Ready(Err(err)),
    (Ready(Ok(())), 0) => ...,
    (Pending, 0) => Pending,
    (_, bytes) => Ready(Ok(bytes)),
}

I think if we just returned the bytes in Ok, the callers would just be this, which is a bit easier to follow:

match self.poll_renegotiate_raw(buf_ptr, buf_len) {
    Ready(Err(err)) => Ready(Err(err)),
    Ready(Ok(0)) => ...,
    Pending => Pending,
    Ready(Ok(bytes)) => Ready(Ok(bytes)),
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that that "usize" is unrelated to the actual completion of the poll_renegotiate. It's just something that can happen along the way. If I switch to Poll<Result<usize, Error>>, the caller will need to keep polling after Ready, because Ready does not mean finished. There aren't any real rules for general poll_* methods, but there are rules for Future::poll, and that violates the "Once a future has finished, clients should not poll it again" one. Needing to say "renegotiate is only done once you get Ready(Ok(0)) instead of Ready(Ok(x>0))" seems worse than the awkward but explicit signature :/

@lrstewart lrstewart force-pushed the bindings_renegotiate branch from 03d84a0 to c417476 Compare October 2, 2024 05:01
@lrstewart lrstewart enabled auto-merge (squash) October 2, 2024 23:07
@lrstewart lrstewart merged commit 0983bee into aws:main Oct 3, 2024
38 checks passed
@lrstewart lrstewart deleted the bindings_renegotiate branch October 3, 2024 01:29
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