tls: include every CA in the trusted bundle when generating TLS session IDs - #47405
Conversation
…on IDs DefaultCertValidator::updateDigestForSessionId() hashed only ca_cert_, which is assigned to the first X509 encountered in the trusted CA PEM bundle. Consequently a change to any CA after the first one, e.g. a rotation or removal via an xDS update, did not invalidate previously issued session IDs, and a resumed session could be accepted without being validated against the current trust bundle. Hash the digest of every certificate in the shared parsed bundle (shared_ca_certs_->certs) instead, mirroring what the SPIFFE validator already does for its CA list. A bundle containing a single certificate produces byte-identical digest input to the previous behavior; multi-certificate bundles will reissue session IDs once after the upgrade, causing a one-time increase in full handshakes. Signed-off-by: abhinav-phi <alpha9coder@gmail.com>
|
Hi @abhinav-phi, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
| // the client connection. This ensures that the client is always validated against | ||
| // the correct settings, even if session resumption across different listeners | ||
| // is enabled. | ||
| if (ca_cert_ != nullptr) { |
There was a problem hiding this comment.
Is this field still necessary?
There was a problem hiding this comment.
It is still needed, but only for first-CA reporting. ca_cert_ is assigned at default_validator.cc:224 and read by getCaCertInformation(), initializeCertExpirationStats() and daysUntilFirstCertExpires(). Now that the digest no longer uses it, it is only a duplicate reference to shared_ca_certs_->certs[0] (CaCertCache::getOrCreate() never returns an empty bundle), so those three can read the first entry of the shared bundle directly and the member can be dropped.
Happy to push that as an extra commit on this PR, but it is a pure no-behavior-change refactor of the cert-info/expiration reporting path, so say the word if you would rather keep this PR limited to the digest fix and see it as a separate change.
There was a problem hiding this comment.
Not needed. BTW since this sounds like Claude, please disclose the AI usage.
3 participants
Commit Message: tls: include every CA in the trusted bundle when generating TLS session IDs
DefaultCertValidator::updateDigestForSessionId()hashed onlyca_cert_, which is assigned once to the first X509 encountered in the trusted CA PEM bundle. A change to any CA after the first one (e.g. a rotation or removal through an xDS update) therefore did not invalidate previously issued session IDs, and a resumed session could be accepted without validation against the current trust bundle.The digest now includes the SHA-256 of every certificate in the parsed bundle (
shared_ca_certs_->certs), mirroring what the SPIFFE validator already does for its CA list.Compatibility: a trusted bundle with a single certificate produces byte-identical digest input to the previous behavior. Multi-CA bundles will reissue session IDs once after the upgrade (one-time increase in full handshakes), which is the intended security fix, so no runtime guard is added; the SPIFFE validator ships the same behavior unguarded.
Fixes #47238
DefaultCertValidatorTest.SessionIdDigestCoversAllCaCertificatesintest/common/tls/cert_validator/default_validator_test.cc, which asserts that (a) adding a CA after the first one changes the digest and (b) rotating only a non-first CA changes the digest — both regressions of the old behavior. Existing TLS unit tests cover the single-CA byte-compatibility path.changelogs/current/bug_fixes/tls__session-id-digest-covers-all-ca-certs.rst.