Repository navigation
crypto: add rustls TLS foundation alongside openssl - #36085
jasonhernandez wants to merge 3 commits into
Conversation
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
4f06772 to
c319014
Compare
Add the rustls/aws-lc-rs building blocks next to the existing openssl paths so that downstream consumers can migrate one at a time: - ore: add a `crypto` feature that installs the aws-lc-rs rustls CryptoProvider process-wide via ctor, and rustls AsyncReady impls alongside the openssl one. - tls-util: add `make_tls_rustls` and a rustls-backed `MakeTlsConnect` implementation for tokio-postgres. The openssl `make_tls` and PKCS#12 helpers are unchanged. - pgwire-common: `Conn::Ssl` now holds a `TlsStream` enum that carries either an openssl or a rustls stream. The `Openssl` variant is removed once no consumer constructs it anymore. - pgwire, balancerd: mechanically wrap their openssl streams in `TlsStream::Openssl`. Balancerd's SNI extraction moves behind `TlsStream::server_name`. No consumer changes TLS backend in this commit. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Co-Authored-By: Claude Fable 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_018ztDqVCrRgMdtjJxvPWa4B
c319014 to
c7e4283
Compare
def-
left a comment
There was a problem hiding this comment.
I'll post the QA LLM review verbatim, hope that's ok:
Finding 1 (MEDIUM): make_tls_rustls drops the system/default CA trust store, so verify-full/verify-ca without an explicit CA will fail every connection
File: src/tls-util/src/lib.rs (make_tls_rustls, root-store construction)
The openssl make_tls builds its connector with
SslConnector::builder(SslMethod::tls_client()), which calls
set_default_verify_paths() internally and therefore trusts the process/system
default CA bundle. In verify-full (or verify-ca) mode with no explicit
sslrootcert, a server presenting a publicly/system-trusted certificate
verifies successfully. This matches libpq: with no ~/.postgresql/root.crt,
libpq falls back to the system CA store.
make_tls_rustls instead starts from rustls::RootCertStore::empty() and only
ever add()s certificates from config.get_ssl_root_cert(). There is no
rustls-native-certs / webpki-roots fallback (confirmed: neither crate is a
workspace dependency). So when verify_mode == true and no CA was supplied, the
root store is empty and the default WebPkiServerVerifier rejects every
server certificate — the TLS handshake can never succeed.
This is reachable via a real, user-configurable path. For CREATE CONNECTION ... TO POSTGRES, src/sql/.../ddl/connection.rs:445-457 maps SSL MODE 'verify-full' / 'verify-ca' to SslMode::VerifyFull / VerifyCa and, unlike
the SQL Server path, does not require SSL CERTIFICATE AUTHORITY
(tls_root_cert may be None). That config flows through
storage-types/connections.rs → mz-postgres-util → make_tls today. When
#36087 swaps that call to make_tls_rustls, any such connection that relied on
system/public CA trust silently starts failing at handshake time (source
ingestion / CSR outage), with no test to catch it (testdrive PG fixtures always
pin an explicit CA, and CI has no publicly-trusted-cert server).
It is fail-closed (a broken connection, not a verification bypass), so this is an
availability/correctness regression rather than a security hole. But it defeats
the function's stated contract of being "the rustls equivalent of make_tls",
and — unlike the related hostname deviation — it is not tracked anywhere
(see the note below).
Suggested fix: seed the root store from the platform trust store before
adding the user CA, matching openssl's set_default_verify_paths(). e.g. add
rustls-native-certs and, when verify_mode, load
rustls_native_certs::load_native_certs() into root_store in addition to any
sslrootcert entries (or fall back to webpki-roots). Add a regression case for
verify-full/verify-ca with no explicit CA against a system-trusted endpoint.
Related, already-tracked: verify-ca collapses into verify-full
For verify-ca (and require+CA), openssl uses (PEER, verify_hostname=false)
— it validates the chain but skips the hostname. make_tls_rustls folds both
VerifyCa and VerifyFull into verify_mode = true and uses rustls's standard
verifier, which always checks the hostname. So verify-ca behaves like
verify-full (stricter than libpq), breaking connections whose cert name does
not match the connection host (e.g. connecting by IP or internal name).
This deviation is already known and tracked in Linear SEC-192 ("rustls's
standard verifier always checks the hostname, so sslmode=verify-ca currently
behaves like verify-full ... Either add a custom verifier that skips the server
name check or document the stricter behavior as intended"), so I am not
re-reporting it — noting it only because the same root-store construction is
where both the tracked hostname issue and the untracked system-CA issue in
Finding 1 live, and both should be fixed together when the verifier is reworked.
Finding 2 (LOW): rustls client stream reports no channel binding, dropping SCRAM-SHA-256-PLUS
File: src/tls-util/src/lib.rs — RustlsTlsStream::channel_binding returns
ChannelBinding::none().
The openssl path (postgres-openssl) supplies tls-server-end-point channel
binding, so tokio-postgres negotiates SCRAM-SHA-256-PLUS when the upstream
offers it. The rustls stream returns ChannelBinding::none(), so channel binding
is never available and the client always falls back to plain SCRAM-SHA-256.
Impact is limited: no consumer sets channel_binding=require (confirmed — the
only channel_binding references in-tree are the server-side pgwire SCRAM
decoder, unrelated), so tokio-postgres runs with the default prefer, where the
loss is a silent no-op rather than a failure, and an active MITM can already
strip the -PLUS offer under prefer. Still, in sslmode=require (no cert
verification — the recommended Materialize PG source mode, since prefer is
intentionally disallowed) channel binding was the only MITM defense the openssl
path offered against a network attacker, and this removes it. Not tracked in
SEC-192. Worth restoring tls-server-end-point (rustls exposes the peer cert via
TlsStream::get_ref().1.peer_certificates()) when the client flip lands, or at
least recording the gap.
Address review findings on the rustls connector: Seed the rustls root store with the platform's native certificates when verifying, matching the openssl connector whose builder trusts the system default CA paths. Without this, verify-full/verify-ca with no explicit sslrootcert rejected every server certificate. Implement RFC 5929 tls-server-end-point channel binding for the rustls stream, so SCRAM-SHA-256-PLUS stays available after the migration off postgres-openssl. Signature algorithms without a well-defined hash (Ed25519, RSASSA-PSS) report no channel binding, matching openssl. Includes parity tests against openssl's X509_digest. Co-Authored-By: Claude Fable 5 <[email protected]>
Add hermetic end-to-end tests driving both TLS connectors over real handshakes against an in-process tokio-rustls server: verify-full with no explicit sslrootcert trusting a CA installed via SSL_CERT_FILE (guarding the native root loading in make_tls_rustls), and client certificate presentation against a server that requires one. Add a Nightly-only test that completes a verify-full handshake with s3.amazonaws.com through the rustls connector, validating the platform trust store against a public CA end to end. The openssl connector is excluded there: vendored openssl compiles its default trust store path to /usr/local/ssl, which does not exist at runtime, so it cannot validate public CAs without SSL_CERT_FILE. Wired into Nightly via a --tls-external mode on the cargo-test composition. Co-Authored-By: Claude Fable 5 <[email protected]>
|
@def- I made some fixes and added some more test coverage (I would suggest we consider keeping this through the openssl -> rustls migration then possibly dropping / shrinking it). Unfortunately that makes this PR bigger. I could refactor the tests out into another PR but would appreciate some brief feedback on this general test approach. Claude did point out that a live TLS test against s3.amazonaws.com would make our tests depend on AWS's availability ;). |
mz-aws-util was the last user of hyper 0.14 in the workspace. It had stayed there because the AWS SDK's hyper 1 client offers only rustls and s2n-tls as TLS providers, and its hook for wrapping a custom TLS connector is crate-private. Native TLS therefore meant either staying on hyper 0.14 or writing our own SDK HTTP client, including the SDK's timeout handling and retry error classification. This PR takes the SDK's own hyper 1 client with rustls on the aws-lc-rs provider. That matches the direction of the [SEC-218](https://linear.app/materializeinc/issue/SEC-218) migration from OpenSSL to rustls on aws-lc-rs (#36085), and aws-lc-rs is already in the tree through jsonwebtoken and launchdarkly. The client trusts the system's root certificates. `http_client_with_resolver` passes `MzAwsResolver` to `build_with_resolver`, so connections to AWS endpoints still reject non-global addresses when enforcement is on. This drops hyper 0.14, hyper-tls 0.5, h2 0.3, http-body 0.4, socket2 0.5 and the direct aws-smithy-runtime dependency, along with their deny.toml skips and the RUSTSEC-2026-0258 ignore for h2 0.3. The resolver tests now go through `ResolveDns`. Locally, both clients listed a public S3 bucket over TLS. Until the rest of the [SEC-218](https://linear.app/materializeinc/issue/SEC-218) stack lands, AWS traffic uses rustls while other TLS consumers stay on OpenSSL. FIPS is out of scope here, as in #36085. Posted by Claude Code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <[email protected]>
|
Closing in favor of a smaller stack: #39428 (OpenSSL handshake tests) → #39430 (rustls Postgres connector) → #39431 (channel binding) and #39432 (nightly public-CA check), plus #39433 (pgwire-common TlsStream bridge, based on main). The mz_ore::crypto provider module is dropped: binaries install aws-lc-rs explicitly and tls-util passes the provider directly. |
### Motivation First PR in the TLS migration stack replacing #36085: #39428 (this, OpenSSL handshake tests) → #39430 (rustls Postgres connector) → #39431 (channel binding) and #39432 (nightly public-CA check). #39433 (pgwire-common `TlsStream` bridge) is independent, based on main. These tests pin down today's OpenSSL TLS client behavior in `mz-tls-util` ahead of the rustls migration, which will extend each test with a rustls leg. Split out of #36085. ### Description - `verify_full_trusts_default_store`: with `sslmode=verify-full` and no `sslrootcert`, `make_tls` trusts a CA from the default store (`SSL_CERT_FILE`). - `client_certs_accepted`: `make_tls` presents the configured client certificate to a server that requires one. - Harness: a throwaway `TestCa`, a blocking OpenSSL `SslAcceptor` server on a background thread, and connect/assert helpers. - `tempfile` and `tokio` added as dev-dependencies (both existing workspace deps). No production code changes. ### Verification Ran the tests repeatedly under nextest. Confirmed each fails when its property is broken (no `SSL_CERT_FILE`, no client cert). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Jason Hernandez <[email protected]> Co-authored-by: Claude Opus 5.5 <[email protected]>
Part of the crypto migration from native-tls/OpenSSL to rustls/aws-lc-rs.
Motivation
First PR of the TLS migration stack. Earlier iterations rewrote the shared TLS types in place, which could never pass CI standalone because consumers (balancerd, mz-debug, pgwire) still used the openssl API. This version is additive: the rustls machinery lands next to the openssl paths, and each later PR flips exactly one consumer.
This PR deliberately contains no FIPS machinery. It standardizes rustls on the aws-lc-rs provider, and the FIPS build story lands separately once the TLS stack swap is complete.
Changes
cryptofeature installs the aws-lc-rs rustlsCryptoProviderprocess-wide viactor, so rustls configurations built anywhere in the process pick it up without selecting a provider explicitly. rustlsAsyncReadyimpls added alongside the openssl one.make_tls_rustlsplus a rustls-backedMakeTlsConnectimpl for tokio-postgres. Enablesmz-ore/crypto, so linking tls-util guarantees the provider is installed. The opensslmake_tlsand PKCS#12 helpers are untouched.Conn::Sslnow holds aTlsStreamenum (Openssl| rustlsServer| rustlsClient). TheOpensslvariant is a migration bridge and is deleted by the final cleanup PR. SNI extraction is unified behindTlsStream::server_name.TlsStream::Opensslonly. No behavior change.Net diff: ~430 insertions, 17 deletions. No consumer changes TLS backend in this PR.
Stack
This PR → #36086 (server-core/pgwire) and #36087 (postgres-util/postgres-client/ccsr) in parallel → #36088 (balancerd), #36089 (environmentd), #36090 (test infra) → final cleanup PR removes the openssl paths and the
TlsStream::Opensslvariant.Testing
Existing TLS behavior is unchanged (openssl paths untouched, verified by full workspace build + existing test suite in CI). The rustls machinery gains coverage as each consumer flips in its own PR.
Part of SEC-218. Related to SEC-192.