feat(bindings): add renegotiate to the rust bindings #4668
Conversation
lrstewart
force-pushed
the
bindings_renegotiate
branch
from
326df2f
to
ab79241
Compare
lrstewart
force-pushed
the
bindings_renegotiate
branch
from
ab79241
to
c24c9ff
Compare
| /// # Safety | ||
| /// | ||
| /// The `context` pointer must live at least as long as the connection | ||
| pub unsafe fn set_send_context(&mut self, context: *mut c_void) -> Result<&mut Self, Error> { | ||
| s2n_connection_set_send_ctx(self.connection.as_ptr(), context).into_result()?; | ||
| Ok(self) | ||
| } |
There was a problem hiding this comment.
I just moved this bc it was weird that it wasn't with the other IO methods
|
|
||
| // std::task::ready is unstable | ||
| fn unwrap_poll<T>( |
There was a problem hiding this comment.
Is there really not a better way to do this? :/
| struct ServerTestStream(TestPairIO); | ||
|
|
||
| // For server testing purposes, we read from the client output stream | ||
| impl Read for ServerTestStream { |
There was a problem hiding this comment.
What do you think about implementing these on the underlying TestPairIO? Since it's such generic functionality. Also the name ServerTestStream feels a little unclear because I don't think there's anything Server specific about it?
Then you'd be able to
let ossl_stream = SslStream::new(openssl_ssl, test_pair.io)?;
There was a problem hiding this comment.
No, it's server specific. You have to implement the reverse stream for a client, because you read and write from the opposite buffers. So we'd need a separate ClientTestStream if I wanted to create an openssl client.
There was a problem hiding this comment.
Or I suppose you could have a generic in/out, but then you really can't use the same struct for the TestPair because the TestPair needs them specific for the client/server.
There was a problem hiding this comment.
I don't think it's specifically useful in this context, but if we find ourselves doing stuff like this relatively frequently it would probably be worth investigating what it would look like to unify the individual harnesses and the benchmark harness.
So we'd just have a single harness that lives in bindings/s2n-tls, and then the benchmarks could take a dependency on that.
There was a problem hiding this comment.
Idk, we'd have to be real careful. Our testing framework was so difficult to write tests with before your recent refactors because it was written for benchmarking, not for testing.
| @@ -542,7 +546,7 @@ impl Connection { | |||
| // Poll the connection future if it exists. | |||
| // | |||
| // If the future returns Pending, then re-set it back on the Connection. | |||
| fn poll_async_task(&mut self) -> Option<Poll<Result<(), Error>>> { | |||
| pub(crate) fn poll_async_task(&mut self) -> Option<Poll<Result<(), Error>>> { | |||
There was a problem hiding this comment.
It looks like the visibility is being expanded because of the implementation of poll_renegotiate and friends in the sibling renegotiate module?
I think I'd instead prefer to have the connection impls inside the connection module, and just gate them behind the unstable-renegotiate flag. That let's us keep the private visibility for internal functions, and also keeps all of the direct connection functionality in one module.
And I'd vote for the same treatment for the renegotiate config::Builder impl.
There was a problem hiding this comment.
I really don't want renegotiate polluting our main modules. The average developer should never know renegotiate exists. Also, while the renegotiate source code was only like ~200 lines, I had ~500 lines of testing. connection.rs and config.rs are already very large.
But I'm not a fan of all the random pub(crate)s either. I've done a bit of a refactor, see if you like this better:
- I made wipe_for_renegotiate and renegotiate share the same logic as wipe and negotiate. This means that we only have two well-define pub(crate) methods, AND that we won't accidentally forget to update the renegotiation logic when we update the main logic.
- I surfaced a new context_mut method for the config builder instead of piecemeal slapping pub(crate) on things. That'll also give us a clear path to other modules defining callback setters.
There was a problem hiding this comment.
Hmm, I do like it better, but I'd still prefer it to be part of the main connection/config implementation. But this is a two way door, so I'm also fine with merging this the way it is.
As for the connection.rs and friend being very large, I agree but we'll need to fix that eventually anyways 😅.
Description of changes:
Add the Rust code necessary to enable the renegotiate feature from the Rust bindings.
Callouts
I put the code in its own module. That required me to mark some of the connection / config private methods pub(crate), but I think keeping all the renegotiate code in one place is worth it.
Testing:
Testing this was tricky. s2n-tls doesn't actually support renegotiation on the server side. In the C unit tests we work around that limitation by doing things like manually constructing the hello request message, but that isn't possible from outside the C library (the message must be encrypted properly). Instead, I had to use openssl as my server, and even openssl didn't actually surface its negotiate methods from its Rust bindings.
I reused TestPair to setup my s2n connection, then discarded the TestPair's server and used the TestPair IO to configure my openssl server. I also had to refactor how the testing framework handles certs-- none of our test code had ever used non-default certs before, and the struct really wasn't very usable.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.