Repository navigation
feat: Add client TLS configuration, with built-in support for rustls - #547
Conversation
09eaaeb to
6a30581
Compare
|
/ok-to-test sha=6a30581 |
6a30581 to
161ebc0
Compare
|
/ok-to-test sha=161ebc0 |
djc
left a comment
djc
left a comment
There was a problem hiding this comment.
Here's some (fairly opinionated) feedback. This seems very much designed from the inside out (that is, what does the rustls API support) rather than outside in (what do users actually need), and as a result it feels overengineered to me.
| reqwest-retry = "0.7.0" | ||
| reqwest-tracing = "0.5.8" | ||
| rustls = "0.23" | ||
| rustls-pki-types = "1.12.0" |
There was a problem hiding this comment.
pki-types is re-exported via rustls as rustls::pki_types, so you could probably skip the separate dependency.
| /// # Security | ||
| /// | ||
| /// The lack of support for TLS 1.0 and TLS 1.1 is intentional. | ||
| pub struct AllowedTlsVersionsConfig { |
There was a problem hiding this comment.
Do you have an actual use case for this today? If not, I'd skip it -- can always add it later and there's a fairly obvious default.
There was a problem hiding this comment.
I've pushed more than a few TLS1.3-only deployments, hence the inclusion!
| #[serde(rename_all = "snake_case")] | ||
| #[non_exhaustive] | ||
| /// Supported encodings for the root certificate in [`RootCertificate::File`]. | ||
| pub enum RootCertificateFileEncoding { |
There was a problem hiding this comment.
Do you have an actual use case for DER-encoded files? PEM is widespread and it seems like over-engineering to support both.
There was a problem hiding this comment.
As far as I am aware, DER is still pretty common in the Windows ecosystem and adjacent universes (e.g. .NET).
| /// A DER-encoded X.509 certificate; as specified in [RFC 5280](https://datatracker.ietf.org/doc/html/rfc5280#section-4.1). | ||
| /// | ||
| /// Since DER is a binary format, we expect the data to be [base64-encoded](https://datatracker.ietf.org/doc/html/rfc4648#section-4). | ||
| Base64Der, |
There was a problem hiding this comment.
What's the value of supporting a non-standard base64 DER in YAML encoding? PEM is pretty much base64 DER anyway, and is widely supported. Seems like YAGNI.
There was a problem hiding this comment.
Thinking about it again, you're 100% right.
| #[serde(rename_all = "kebab-case", tag = "name")] | ||
| #[non_exhaustive] | ||
| /// Which implementation to use for TLS cryptographic operations. | ||
| pub enum CryptoProviderConfig { |
There was a problem hiding this comment.
It seems strange to me to provide run-time configuration support for crypto stacks since it's mostly a compile-time constraint. Even if you did want to do this, I'd suggest just simplifying to having separate variants for aws-lc-rs and aws-lc-rs-fips.
There was a problem hiding this comment.
I strongly dislike the use of compile-time features to control default choices (or to make choices at all).
I prefer to use features to control the availability of a certain piece of functionality while using runtime configuration to decide which of that functionality should be used.
| fn parse_certificates_from_pem_bytes(data: &[u8]) -> Result<Vec<CertificateDer<'static>>> { | ||
| let mut certs = Vec::new(); | ||
| for outcome in | ||
| <(SectionKind, Vec<u8>) as rustls_pki_types::pem::PemObject>::pem_slice_iter(data) |
There was a problem hiding this comment.
Why not <CertificateDer as _>::pem_slice_iter()?
There was a problem hiding this comment.
Different behaviour if a certificate of a different kind is encountered—I prefer to be strict and fail if the bundle contains other certificate kinds (e.g. a private key) rather than ignoring it.
161ebc0 to
49adb83
Compare
|
First of all, thanks for having a look!
It's interesting you say that, because the design journey started rather far from |
4411579 to
4b8f5ed
Compare
4b8f5ed to
50eac48
Compare
No description provided.