Add s2n-tls client TLS provider #3965
Conversation
| // TODO(hyper1) s2n support | ||
| /// TLS provider based on [s2n-tls](https://github.com/aws/s2n-tls) | ||
| #[cfg(feature = "s2n-tls")] | ||
| S2ntls, |
There was a problem hiding this comment.
style/naming-bikehsed: Is this the capitalization we want? I realize Rustls is this way but that matches the full name Rustls and the way they refer to it in their own project.
There was a problem hiding this comment.
I agree that this capitalization looks strange! I wanted to avoid going against Rust's naming conventions for enums. But if this isn't a concern, something like s2n_tls does look better. I updated the name and ignored the style warning.
There was a problem hiding this comment.
There may also be a name that aligns more with the s2n-tls capitalization scheme and doesn't break Rust's naming conventions, like S2nTls maybe. I'll change the name to this for now and I will get more input on the name.
There was a problem hiding this comment.
We tentatively agreed on S2nTls with the admission that there isn't really any good way to represent s2n-tls as a Rust enum value. If you have a better alternative please let me know!
9e4d3da to
3924a04
Compare
There was a problem hiding this comment.
Thanks for contributing this! Overall looks good.
One question I have is about more advanced TLS configuration (e.g. ca roots, peer verification, etc). I'm wondering if we should define a TlsContext or similar that defines common TLS options we think we could support across providers. Might be useful to have your team's help/input with this.
| // S2n, | ||
| // TODO(hyper1) s2n support | ||
| /// TLS provider based on [s2n-tls](https://github.com/aws/s2n-tls) | ||
| #[cfg(feature = "s2n-tls")] |
There was a problem hiding this comment.
Do we need to worry about or consider that s2n only works on unix platforms? Should this cfg be gated on both the feature and platform support?
There was a problem hiding this comment.
Yeah I considered doing that as well. My concern was someone enabling s2n-tls on a non-unix platform and then trying to figure out why they can't select the s2n-tls provider. A build failure after enabling the feature seemed like it might be less confusing. But maybe this would also be confusing. Let me know what you think, I'm happy to change it!
There was a problem hiding this comment.
We can leave it for now unless someone else feels strongly at the moment.
Yeah we'd be happy to help with that! I believe that our QUIC library, s2n-quic, does something similar. The s2n-tls and rustls client/server s2n-quic builders wrap the underlying s2n-tls and rustls APIs. And both builders share APIs with overlapping functionality, like |
Motivation and Context
Adds s2n-tls as an optional TLS provider to the hyper1 branch.
Related issue: #2446
Description
Currently, smithy-rs clients use rustls as the TLS provider. This PR adds s2n-tls as an optional provider, allowing users to configure smithy-rs to secure their HTTP requests with s2n-tls.
Testing
I added a new client smoke test for s2n-tls. I also added a test that makes sure s2n-tls is actually set as the provider when configured. I was looking for more tests that invoke rustls that I could add an equivalent s2n-tls test for, but was having trouble discovering them. If there are tests that make sense to have please let me know and I will add them!
Some of the CI jobs are still failing. However, they appear to be unrelated to this change. These jobs also failed in the last hyper1 commit:
Also, the canary will need to be run manually if necessary for this change.
Checklist
.changelogdirectory, specifying "client," "server," or both in theapplies_tokey..changelogdirectory, specifying "aws-sdk-rust" in theapplies_tokey.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.