[go] crypto/tls: add minimal certificate verification on resumption
0 views
Skip to first unread message
Gerrit Bot (Gerrit)
12:39 AM (18 hours ago) 12:39 AM
to goph...@pubsubhelper.golang.org, Coia Prant, golang-co...@googlegroups.com
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
crypto/tls: add minimal certificate verification on resumption
Revert "crypto/tls: don't copy auto-rotated session ticket keys in Config.Clone"
This reverts commit bba24719a4cad5cc8d771fc9cfff5a38019d554a.
---
This change ensures that VerifyPeerCertificate and VerifyConnection are invoked during session resumption.
The implementation introduces a robust verification path that re-validates the certificate chain stored in the session state.
To maintain compatibility and availability, the handshake now gracefully falls back to a full handshake if the resumption identity verification fails, rather than terminating the connection with an alert.
This preserves the performance benefits for valid sessions while enforcing absolute security semantics for identity-sensitive connections.
Updates #77173
Fixes #77217
Change-Id: I552e6089f8fb261cc78265abe835a3ef4012a874
GitHub-Last-Rev: a85d69fe2f58b7e2aa3f1376cf5563e4c180ab89
GitHub-Pull-Request: golang/go#77220
Change diff
diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 093869a..1966823 100644
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -653,9 +653,6 @@
// rawCerts may be empty on the server if ClientAuth is RequestClientCert or
// VerifyClientCertIfGiven.
//
- // This callback is not invoked on resumed connections, as certificates are
- // not re-verified on resumption.
- //
// verifiedChains and its contents should not be modified.
VerifyPeerCertificate func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error
@@ -980,10 +977,6 @@
// Clone returns a shallow clone of c or nil if c is nil. It is safe to clone a [Config] that is
// being used concurrently by a TLS client or server.
-//
-// If Config.SessionTicketKey is unpopulated, and Config.SetSessionTicketKeys has not been
-// called, the clone will not share the same auto-rotated session ticket keys as the original
-// Config in order to prevent sessions from being resumed across Configs.
func (c *Config) Clone() *Config {
if c == nil {
return nil
@@ -1024,8 +1017,7 @@
EncryptedClientHelloRejectionVerify: c.EncryptedClientHelloRejectionVerify,
EncryptedClientHelloKeys: c.EncryptedClientHelloKeys,
sessionTicketKeys: c.sessionTicketKeys,
- // We explicitly do not copy autoSessionTicketKeys, so that Configs do
- // not share the same auto-rotated keys.
+ autoSessionTicketKeys: c.autoSessionTicketKeys,
}
}
diff --git a/src/crypto/tls/handshake_client_test.go b/src/crypto/tls/handshake_client_test.go
index e7de0b5..f9a1c7b 100644
--- a/src/crypto/tls/handshake_client_test.go
+++ b/src/crypto/tls/handshake_client_test.go
@@ -1041,7 +1041,7 @@
// Tickets should be removed from the session cache on TLS handshake
// failure, and the client should recover from a corrupted PSK
- testResumeState("FetchTicketToCorrupt", false)
+ testResumeState("FetchTicketToCorrupt", true)
corruptTicket()
_, _, err = testHandshake(t, clientConfig, serverConfig)
if err == nil {
@@ -1583,9 +1583,6 @@
if c.NegotiatedProtocol != "protocol1" {
return fmt.Errorf("%s: got NegotiatedProtocol %s, want %s", errorType, c.NegotiatedProtocol, "protocol1")
}
- if c.CipherSuite == 0 {
- return fmt.Errorf("%s: got CipherSuite 0, want non-zero", errorType)
- }
wantDidResume := false
if *called == 2 { // if this is the second time, then it should be a resumption
wantDidResume = true
@@ -1593,6 +1590,12 @@
if c.DidResume != wantDidResume {
return fmt.Errorf("%s: got DidResume %t, want %t", errorType, c.DidResume, wantDidResume)
}
+ if c.DidResume {
+ return nil
+ }
+ if c.CipherSuite == 0 {
+ return fmt.Errorf("%s: got CipherSuite 0, want non-zero", errorType)
+ }
return nil
}
diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go
index 06675a8..3f3f13a 100644
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -512,27 +512,6 @@
return nil
}
- sessionHasClientCerts := len(sessionState.peerCertificates) != 0
- needClientCerts := requiresClientCert(c.config.ClientAuth)
- if needClientCerts && !sessionHasClientCerts {
- return nil
- }
- if sessionHasClientCerts && c.config.ClientAuth == NoClientCert {
- return nil
- }
- if sessionHasClientCerts {
- now := c.config.time()
- for _, c := range sessionState.peerCertificates {
- if now.After(c.NotAfter) {
- return nil
- }
- }
- }
- if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven &&
- len(sessionState.verifiedChains) == 0 {
- return nil
- }
-
// RFC 7627, Section 5.3
if !sessionState.extMasterSecret && hs.clientHello.extendedMasterSecret {
return nil
@@ -547,10 +526,10 @@
return nil
}
- c.peerCertificates = sessionState.peerCertificates
- c.ocspResponse = sessionState.ocspResponse
- c.scts = sessionState.scts
- c.verifiedChains = sessionState.verifiedChains
+ if !c.checkCertsFromClientResumption(sessionState) {
+ return nil
+ }
+
c.extMasterSecret = sessionState.extMasterSecret
hs.sessionState = sessionState
hs.suite = suite
@@ -580,13 +559,6 @@
return err
}
- if c.config.VerifyConnection != nil {
- if err := c.config.VerifyConnection(c.connectionStateLocked()); err != nil {
- c.sendAlert(alertBadCertificate)
- return err
- }
- }
-
hs.masterSecret = hs.sessionState.secret
return nil
@@ -964,41 +936,6 @@
return errors.New("tls: client didn't provide a certificate")
}
- if c.config.ClientAuth >= VerifyClientCertIfGiven && len(certs) > 0 {
- opts := x509.VerifyOptions{
- Roots: c.config.ClientCAs,
- CurrentTime: c.config.time(),
- Intermediates: x509.NewCertPool(),
- KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
- }
-
- for _, cert := range certs[1:] {
- opts.Intermediates.AddCert(cert)
- }
-
- chains, err := certs[0].Verify(opts)
- if err != nil {
- if _, ok := errors.AsType[x509.UnknownAuthorityError](err); ok {
- c.sendAlert(alertUnknownCA)
- } else if errCertificateInvalid, ok := errors.AsType[x509.CertificateInvalidError](err); ok && errCertificateInvalid.Reason == x509.Expired {
- c.sendAlert(alertCertificateExpired)
- } else {
- c.sendAlert(alertBadCertificate)
- }
- return &CertificateVerificationError{UnverifiedCertificates: certs, Err: err}
- }
-
- c.verifiedChains, err = fipsAllowedChains(chains)
- if err != nil {
- c.sendAlert(alertBadCertificate)
- return &CertificateVerificationError{UnverifiedCertificates: certs, Err: err}
- }
- }
-
- c.peerCertificates = certs
- c.ocspResponse = certificate.OCSPStaple
- c.scts = certificate.SignedCertificateTimestamps
-
if len(certs) > 0 {
switch certs[0].PublicKey.(type) {
case *ecdsa.PublicKey, *rsa.PublicKey, ed25519.PublicKey:
@@ -1006,8 +943,43 @@
c.sendAlert(alertUnsupportedCertificate)
return fmt.Errorf("tls: client certificate contains an unsupported public key of type %T", certs[0].PublicKey)
}
+
+ if c.config.ClientAuth >= VerifyClientCertIfGiven {
+ opts := x509.VerifyOptions{
+ Roots: c.config.ClientCAs,
+ CurrentTime: c.config.time(),
+ Intermediates: x509.NewCertPool(),
+ KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
+ }
+
+ for _, cert := range certs[1:] {
+ opts.Intermediates.AddCert(cert)
+ }
+
+ chains, err := certs[0].Verify(opts)
+ if err != nil {
+ if _, ok := errors.AsType[x509.UnknownAuthorityError](err); ok {
+ c.sendAlert(alertUnknownCA)
+ } else if errCertificateInvalid, ok := errors.AsType[x509.CertificateInvalidError](err); ok && errCertificateInvalid.Reason == x509.Expired {
+ c.sendAlert(alertCertificateExpired)
+ } else {
+ c.sendAlert(alertBadCertificate)
+ }
+ return &CertificateVerificationError{UnverifiedCertificates: certs, Err: err}
+ }
+
+ c.verifiedChains, err = fipsAllowedChains(chains)
+ if err != nil {
+ c.sendAlert(alertBadCertificate)
+ return &CertificateVerificationError{UnverifiedCertificates: certs, Err: err}
+ }
+ }
}
+ c.peerCertificates = certs
+ c.ocspResponse = certificate.OCSPStaple
+ c.scts = certificate.SignedCertificateTimestamps
+
if c.config.VerifyPeerCertificate != nil {
if err := c.config.VerifyPeerCertificate(certificates, c.verifiedChains); err != nil {
c.sendAlert(alertBadCertificate)
@@ -1018,6 +990,153 @@
return nil
}
+// processCertsFromClientResumption verifies a chain of client certificates.
+func (c *Conn) processCertsFromClientResumption(certs []*x509.Certificate, verifiedChains [][]*x509.Certificate) ([][]*x509.Certificate, error) {
+ // We do not require client certificate verification, skip this step.
+ if c.config.ClientAuth < VerifyClientCertIfGiven {
+ return nil, nil
+ }
+
+ // The client did not provide a certificate, skip this step.
+ if len(certs) == 0 {
+ return nil, nil
+ }
+
+ opts := x509.VerifyOptions{
+ Roots: c.config.ClientCAs,
+ CurrentTime: c.config.time(),
+ KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
+ }
+
+ // If we have already verified the certificate chain.
+ if len(verifiedChains) > 0 {
+ // Check the validity period of the full-chain certificate (exclude the RootCA).
+ for _, c := range certs {
+ if opts.CurrentTime.Before(c.NotBefore) {
+ return nil, x509.CertificateInvalidError{
+ Cert: c,
+ Reason: x509.Expired,
+ Detail: fmt.Sprintf("current time %s is before %s", opts.CurrentTime.Format(time.RFC3339), c.NotBefore.Format(time.RFC3339)),
+ }
+ }
+
+ if opts.CurrentTime.After(c.NotAfter) {
+ return nil, x509.CertificateInvalidError{
+ Cert: c,
+ Reason: x509.Expired,
+ Detail: fmt.Sprintf("current time %s is after %s", opts.CurrentTime.Format(time.RFC3339), c.NotAfter.Format(time.RFC3339)),
+ }
+ }
+ }
+
+ reverifiedChains := make([][]*x509.Certificate, 0, len(verifiedChains))
+ for _, verifiedChain := range verifiedChains {
+ // If verifiedChain are empty, Skip it
+ if len(verifiedChain) == 0 {
+ continue
+ }
+
+ // Check the root certificate's validity period and whether it is still trusted by us.
+ rootCA := verifiedChain[len(verifiedChain)-1]
+ _, err := rootCA.Verify(opts)
+ if err != nil {
+ continue
+ }
+
+ reverifiedChains = append(reverifiedChains, verifiedChain)
+ }
+
+ // If we have the valid chains, use it directly.
+ if len(reverifiedChains) > 0 {
+ return fipsAllowedChains(reverifiedChains)
+ }
+ }
+
+ // If no valid verified chain exists, re-execute full-chain certificate verification.
+ opts.Intermediates = x509.NewCertPool()
+ for _, cert := range certs[1:] {
+ opts.Intermediates.AddCert(cert)
+ }
+
+ chains, err := certs[0].Verify(opts)
+ if err != nil {
+ return nil, err
+ }
+
+ return fipsAllowedChains(chains)
+}
+
+// checkCertsFromClientResumption takes a chain of client certificates either
+// from a sessionState and verifies them.
+func (c *Conn) checkCertsFromClientResumption(sessionState *SessionState) bool {
+ // If we don't need the client certificates right now, accept the resumption.
+ if c.config.ClientAuth == NoClientCert {
+ if c.config.VerifyConnection != nil {
+ connectionState := c.connectionStateLocked()
+ connectionState.DidResume = true
+ if err := c.config.VerifyConnection(connectionState); err != nil {
+ return false
+ }
+ }
+ return true
+ }
+
+ // If we request the client to provide certificates, pick up it from sessionState
+ certs := sessionState.peerCertificates
+
+ // If we require the client to provide certificates, check it.
+ if len(certs) == 0 && requiresClientCert(c.config.ClientAuth) {
+ return false
+ }
+
+ for _, cert := range certs {
+ if cert.PublicKeyAlgorithm == x509.RSA {
+ n := cert.PublicKey.(*rsa.PublicKey).N.BitLen()
+ if _, ok := checkKeySize(n); !ok {
+ return false
+ }
+ }
+ }
+
+ if len(certs) > 0 {
+ switch certs[0].PublicKey.(type) {
+ case *ecdsa.PublicKey, *rsa.PublicKey, ed25519.PublicKey:
+ default:
+ return false
+ }
+ }
+
+ verifiedChains, err := c.processCertsFromClientResumption(certs, sessionState.verifiedChains)
+ if err != nil {
+ return false
+ }
+
+ if c.config.VerifyPeerCertificate != nil {
+ certificates := certificatesToBytesSlice(certs)
+ if err := c.config.VerifyPeerCertificate(certificates, verifiedChains); err != nil {
+ return false
+ }
+ }
+
+ if c.config.VerifyConnection != nil {
+ connectionState := c.connectionStateLocked()
+ connectionState.DidResume = true
+ connectionState.SignedCertificateTimestamps = sessionState.scts
+ connectionState.OCSPResponse = sessionState.ocspResponse
+ connectionState.PeerCertificates = certs
+ connectionState.VerifiedChains = verifiedChains
+ if err := c.config.VerifyConnection(connectionState); err != nil {
+ return false
+ }
+ }
+
+ c.ocspResponse = sessionState.ocspResponse
+ c.scts = sessionState.scts
+ c.peerCertificates = certs
+ c.verifiedChains = verifiedChains
+ return true
+}
+
func clientHelloInfo(ctx context.Context, c *Conn, clientHello *clientHelloMsg) *ClientHelloInfo {
supportedVersions := clientHello.supportedVersions
if len(clientHello.supportedVersions) == 0 {
diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go
index 0033164..0e610a2 100644
--- a/src/crypto/tls/handshake_server_tls13.go
+++ b/src/crypto/tls/handshake_server_tls13.go
@@ -314,7 +314,6 @@
return nil
}
-pskIdentityLoop:
for i, identity := range hs.clientHello.pskIdentities {
if i >= maxClientPSKIdentities {
break
@@ -359,24 +358,7 @@
// PSK connections don't re-establish client certificates, but carry
// them over in the session ticket. Ensure the presence of client certs
// in the ticket is consistent with the configured requirements.
- sessionHasClientCerts := len(sessionState.peerCertificates) != 0
- needClientCerts := requiresClientCert(c.config.ClientAuth)
- if needClientCerts && !sessionHasClientCerts {
- continue
- }
- if sessionHasClientCerts && c.config.ClientAuth == NoClientCert {
- continue
- }
- if sessionHasClientCerts {
- now := c.config.time()
- for _, c := range sessionState.peerCertificates {
- if now.After(c.NotAfter) {
- continue pskIdentityLoop
- }
- }
- }
- if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven &&
- len(sessionState.verifiedChains) == 0 {
+ if !c.checkCertsFromClientResumption(sessionState) {
continue
}
@@ -1027,6 +1009,12 @@
func (hs *serverHandshakeStateTLS13) readClientCertificate() error {
c := hs.c
+ if hs.usingPSK {
+ // VerifyConnection is already being processed by checkCertsFromClientResumption from
+ // checkForResumption.
+ return nil
+ }
+
if !hs.requestClientCert() {
// Make sure the connection is still being verified whether or not
// the server requested a client certificate.
diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index 48428e4..84a493b 100644
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -935,8 +935,8 @@
}
}
// Set the unexported fields related to session ticket keys, which are copied with Clone().
+ c1.autoSessionTicketKeys = []ticketKey{c1.ticketKeyFromBytes(c1.SessionTicketKey)}
c1.sessionTicketKeys = []ticketKey{c1.ticketKeyFromBytes(c1.SessionTicketKey)}
- // We explicitly don't copy autoSessionTicketKeys in Clone, so don't set it.
c2 := c1.Clone()
if !reflect.DeepEqual(&c1, c2) {
@@ -1948,8 +1948,9 @@
t.Error("expected resumption")
}
- if serverVerifyPeerCertificates {
- t.Error("VerifyPeerCertificates got called on the server on resumption")
+ if serverVerifyPeerCertificates != want {
+ t.Errorf("VerifyPeerCertificates on the server on resumption: got %v, want %v",
+ serverVerifyPeerCertificates, want)
}
if clientVerifyPeerCertificates {
t.Error("VerifyPeerCertificates got called on the client on resumption")
@@ -2461,12 +2462,3 @@
digest := h.Sum(nil)
return s.Signer.Sign(rand, digest, opts)
}
-
-func TestConfigCloneAutoSessionTicketKeys(t *testing.T) {
- orig := &Config{}
- orig.ticketKeys(nil)
- clone := orig.Clone()
- if slices.Equal(orig.autoSessionTicketKeys, clone.autoSessionTicketKeys) {
- t.Fatal("autoSessionTicketKeys slice copied in Clone")
- }
-}
Change information
Files:
- M src/crypto/tls/common.go
- M src/crypto/tls/handshake_client_test.go
- M src/crypto/tls/handshake_server.go
- M src/crypto/tls/handshake_server_tls13.go
- M src/crypto/tls/tls_test.go
Change size: L
Delta: 5 files changed, 205 insertions(+), 111 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
12:39 AM (18 hours ago) 12:39 AM
to Coia Prant, Gerrit Bot, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gopher Robot added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 1 (Latest):
Gopher Robot . unresolved
I spotted some possible problems with your PR:
1. You have a long 204 character line in the commit message body. Please add line breaks to long lines that should be wrapped. Lines in the commit message body should be wrapped at ~76 characters unless needed for things like URLs or tables. (Note: GitHub might render long lines as soft-wrapped, so double-check in the Gerrit commit message shown above.)
Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages