feat: Add client TLS configuration, with built-in support for rustls #547
Conversation
LukeMathWalker
force-pushed
the
client-tls-config
branch
from
09eaaeb to
6a30581
Compare
|
/ok-to-test sha=6a30581 |
LukeMathWalker
force-pushed
the
client-tls-config
branch
from
6a30581 to
161ebc0
Compare
|
/ok-to-test sha=161ebc0 |
libs/Cargo.toml
Outdated
| 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).
libs/pavex/src/tls/client/config.rs
Outdated
| /// 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.
LukeMathWalker
force-pushed
the
client-tls-config
branch
from
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 |
LukeMathWalker
force-pushed
the
client-tls-config
branch
7 times, most recently
from
4411579 to
4b8f5ed
Compare
LukeMathWalker
force-pushed
the
client-tls-config
branch
from
4b8f5ed to
50eac48
Compare
No description provided.