ccsr: trust a server certificate that exactly matches the configured CA - #39459
jasonhernandez wants to merge 1 commit into
Conversation
ec2cbca to
ef9f076
Compare
|
sorry - this is some small followup to minimize changes in behavior handling ssl certs moving to rustls @antiguru |
antiguru
left a comment
There was a problem hiding this comment.
The approach looks right. Preconfiguring rustls with rustls-platform-verifier::new_with_extra_roots and the aws-lc-rs provider matches reqwest 0.13.5's default path for the options ccsr uses, and handshake signatures still go through inner, so a pinned cert still requires its private key.
Main open question: the exact-match path skips hostname verification, which is broader than the OpenSSL behavior this restores (inline). Also a doc comment that contradicts the validity check, and test gaps for the name-skip and the CA + client-identity handshake. Happy to approve once those are settled.
Minor: x509-cert is a new dependency used only for notBefore/notAfter. Fine given der/spki are already in the tree, but worth a line in the description that webpki exposes no validity accessor.
Posted by Claude Code on behalf of @antiguru.
| /// | ||
| /// Certificates in the system's certificate store are trusted by default. | ||
| /// A server certificate identical to `cert` is trusted regardless of its | ||
| /// name, validity period or extensions. |
There was a problem hiding this comment.
This contradicts the implementation: ExactRootMatch does check the validity period and returns NotValidYet/Expired, and the test asserts it. Drop "validity period" here.
There was a problem hiding this comment.
Fixed: the doc now says an identical certificate is trusted even if it isn't a valid end-entity certificate, and its name and validity period are still checked.
| if now > not_after { | ||
| return Err(rustls::Error::InvalidCertificate(CertificateError::Expired)); | ||
| } | ||
| return Ok(ServerCertVerified::assertion()); |
There was a problem hiding this comment.
Skipping the name check goes further than OpenSSL parity. Under native-tls (before #39415), reqwest accepted the CA:TRUE self-signed cert but still verified the hostname. The QA finding is only about CaUsedAsEndEntity.
Could we keep the name check on the exact-match path? webpki::EndEntityCert::try_from(end_entity)?.verify_is_valid_for_subject_name(server_name) (rustls-webpki is already in the tree) checks names only, not basic constraints, so the CA case still passes, and the no-CN-fallback behavior stays uniform. If skipping the name is intentional, please state why it is safe when the registry is reached through PrivateLink or an SSH tunnel.
There was a problem hiding this comment.
Agreed, restored in 5e95bc0. On an exact match the verifier checks validity, then rustls::client::verify_server_name (the public wrapper over webpki's verify_is_valid_for_subject_name), so it skips only the chain, basic-constraints and key-usage checks. A self-signed CA:TRUE cert with a matching SAN still passes, and there is no CN fallback.
|
|
||
| // webpki rejects a `CA:TRUE` server certificate as `CaUsedAsEndEntity`. | ||
| // It is accepted because it is identical to the configured root. | ||
| let self_signed_ca = TestCert::new(TLS_TEST_HOST, Some(TLS_TEST_HOST), true, None); |
There was a problem hiding this comment.
This cert's SAN matches TLS_TEST_HOST, so the documented "regardless of its name" behavior is not exercised. Please add a pinned cert with a mismatched SAN and assert whichever outcome we settle on.
Separately, the client identity now flows through rustls_config whenever roots are set, but test_pem_identity only calls build() and never does a handshake. A case where the test server requires a client certificate would cover CA + mTLS, unless an mzcompose suite (kafka-auth?) already covers CSR mTLS in CI.
There was a problem hiding this comment.
Added both: a pinned self-signed CA with SAN other.test is rejected with the name error (fails without the check), and test_tls_client_identity_with_roots does an mTLS handshake with configured roots, with and without an identity. kafka-auth's test-schema-registry-mssl.td also covers CSR mTLS with a CA in PR CI.
|
|
||
| if let Some(ident) = self.identity { | ||
| // A preconfigured TLS backend makes reqwest ignore its own root | ||
| // certificate and identity settings, so `rustls_config` handles both. |
There was a problem hiding this comment.
A preconfigured backend makes reqwest ignore all of its TLS builder settings (ALPN, SNI, min/max TLS version, CRLs), not only roots and identity. Worth saying so here, so a later builder.min_tls_version(..) isn't silently dropped when roots are configured.
There was a problem hiding this comment.
Added a NOTE: a preconfigured backend makes reqwest ignore all its TLS builder settings (roots, identity, ALPN, SNI, TLS versions, CRLs), so they must go into rustls_config.
| None => builder.with_no_client_auth(), | ||
| }; | ||
| // reqwest only sets ALPN on TLS configurations it builds itself. | ||
| config.alpn_protocols = vec![b"h2".to_vec(), b"http/1.1".to_vec()]; |
There was a problem hiding this comment.
nit: this mirrors reqwest only while the workspace enables reqwest's http2 feature. Mention that dependency in the comment.
There was a problem hiding this comment.
Done, the comment now ties the ALPN choice to the workspace enabling reqwest's http2 feature.
antiguru
left a comment
There was a problem hiding this comment.
Looks good! My bot raises some points, but once they're fixed this should be good to go.
ef9f076 to
5e95bc0
Compare
QA LLM Review1. MEDIUM -- The new name check on the exact-match path still rejects the self-signed
|
With rustls, webpki rejects a self-signed `CA:TRUE` certificate that is both the schema registry's server certificate and the connection's `SSL CERTIFICATE AUTHORITY`, reporting `CaUsedAsEndEntity`. OpenSSL accepted it. When a schema registry client has configured root certificates, build the rustls configuration in mz-ccsr and hand it to reqwest as a preconfigured TLS backend. Its verifier accepts a server certificate that is byte-for-byte identical to a configured root, which is equivalent to pinning that certificate, and otherwise defers to `rustls-platform-verifier` with the same native and extra roots that reqwest uses by default. Handshake signatures are always checked by the inner verifier. The client identity moves into the rustls configuration, because reqwest ignores its own identity setting with a preconfigured backend. Clients without configured roots keep reqwest's default TLS path. A CN-only leaf without a subjectAltName is still rejected. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5e95bc0 to
f763bd6
Compare
|
Addressed in f763bd6: on an exact match the name check now falls back to the subject CN when the pinned certificate has no DNS or IP subjectAltName, like OpenSSL (exact, ASCII case-insensitive, no wildcards, last CN). Every other certificate stays SAN-only. Tests cover the CN-only CA:TRUE cert you used (accepted), a wrong CN (rejected), and a SAN for another host with a matching CN (rejected). @antiguru this adds a CN fallback to the exact-match path after your approval, could you take another look at |
Motivation
This addresses the QA review finding on #39415. Under rustls, a self-signed
CA:TRUEschema registry certificate supplied as its ownSSL CERTIFICATE AUTHORITYfails withCaUsedAsEndEntity, though OpenSSL accepted it. Follows #39415.Description
tls_backend_preconfigured.x509-certbecause rustls-webpki exposes no validity accessor.rustls-platform-verifier, with the same native and extra roots reqwest uses. Handshake signatures are always verified.Verification
New
test_tls_server_verificationruns against a local TLS server. It accepts a SAN leaf and exact-match self-signed CAs matched by SAN or by CN, and rejects an exact match with the wrong SAN or CN, an expired or not-yet-valid exact match, a different self-signed cert, and a CN-only leaf. Newtest_tls_client_identity_with_rootscovers mTLS with configured roots. Each check was confirmed to make its test fail when removed.🤖 Generated with Claude Code