-
Notifications
You must be signed in to change notification settings - Fork 712
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
Conversation
61d3eab
to
dbeeb27
Compare
self.as_mut().set_blinding(Blinding::SelfService)?; | ||
|
There was a problem hiding this comment.
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.
24456fb
to
794f8ef
Compare
&mut self, | ||
buf_ptr: *mut libc::c_void, | ||
buf_len: isize, | ||
) -> (Poll<Result<(), Error>>, usize) { |
There was a problem hiding this comment.
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:
) -> (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)),
}
There was a problem hiding this comment.
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 :/
24b8d88
to
03d84a0
Compare
... rather than managed by the application, like in the C library. This is required for use with s2n-tls-tokio and s2n-tls-hyper.
Co-authored-by: Sam Clark <3758302+goatgoose@users.noreply.github.com>
03d84a0
to
c417476
Compare
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:
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:
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.