src/crypto/tls/common.go | 7 ++++++- src/crypto/tls/handshake_server.go | 9 +++++++-- src/crypto/tls/handshake_server_test.go | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++++ src/crypto/tls/handshake_server_tls13.go | 10 ++++++++-- src/crypto/tls/tls_test.go | 11 ++++++++++- diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go index d6942d2ef14a27b532dee6d1b502f5632e156ea0..6b73796920882eaa35854769116bbe572f52e3e8 100644 --- a/src/crypto/tls/common.go +++ b/src/crypto/tls/common.go @@ -920,6 +920,10 @@ const maxSessionTicketLifetime = 7 * 24 * time.Hour // 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 @@ -959,7 +963,8 @@ EncryptedClientHelloConfigList: c.EncryptedClientHelloConfigList, EncryptedClientHelloRejectionVerify: c.EncryptedClientHelloRejectionVerify, EncryptedClientHelloKeys: c.EncryptedClientHelloKeys, sessionTicketKeys: c.sessionTicketKeys, - autoSessionTicketKeys: c.autoSessionTicketKeys, + // We explicitly do not copy autoSessionTicketKeys, so that Configs do + // not share the same auto-rotated keys. } } diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go index 6aebb742229a50b0172b4a0210b3f4c021530b20..cdb15afc02ff6aaaf87529a1a08c9d1a66382b6e 100644 --- a/src/crypto/tls/handshake_server.go +++ b/src/crypto/tls/handshake_server.go @@ -510,8 +510,13 @@ } if sessionHasClientCerts && c.config.ClientAuth == NoClientCert { return nil } - if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) { - 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 { diff --git a/src/crypto/tls/handshake_server_test.go b/src/crypto/tls/handshake_server_test.go index f533023afba5b991e68eb3c15c9a70046c0faf98..bd586dd7b7af9d0afc15392746a9cbaabef4e273 100644 --- a/src/crypto/tls/handshake_server_test.go +++ b/src/crypto/tls/handshake_server_test.go @@ -13,6 +13,7 @@ "crypto/elliptic" "crypto/rand" "crypto/tls/internal/fips140tls" "crypto/x509" + "crypto/x509/pkix" "encoding/pem" "errors" "fmt" @@ -2122,3 +2123,103 @@ if err := <-clientErr; err != nil { t.Errorf("Unexpected client error: %v", err) } } + +func TestHandshakeChainExpiryResumptionTLS12(t *testing.T) { + t.Run("TLS1.2", func(t *testing.T) { + testHandshakeChainExpiryResumption(t, VersionTLS12) + }) + t.Run("TLS1.3", func(t *testing.T) { + testHandshakeChainExpiryResumption(t, VersionTLS13) + }) +} + +func testHandshakeChainExpiryResumption(t *testing.T, version uint16) { + now := time.Now() + createChain := func(leafNotAfter, rootNotAfter time.Time) (certDER []byte, root *x509.Certificate) { + tmpl := &x509.Certificate{ + Subject: pkix.Name{CommonName: "root"}, + NotBefore: rootNotAfter.Add(-time.Hour * 24), + NotAfter: rootNotAfter, + IsCA: true, + BasicConstraintsValid: true, + } + rootDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + if err != nil { + t.Fatalf("CreateCertificate: %v", err) + } + root, err = x509.ParseCertificate(rootDER) + if err != nil { + t.Fatalf("ParseCertificate: %v", err) + } + + tmpl = &x509.Certificate{ + Subject: pkix.Name{}, + DNSNames: []string{"expired-resume.example.com"}, + NotBefore: leafNotAfter.Add(-time.Hour * 24), + NotAfter: leafNotAfter, + KeyUsage: x509.KeyUsageDigitalSignature, + } + certDER, err = x509.CreateCertificate(rand.Reader, tmpl, root, &testECDSAPrivateKey.PublicKey, testECDSAPrivateKey) + if err != nil { + t.Fatalf("CreateCertificate: %v", err) + } + + return certDER, root + } + + initialLeafDER, initialRoot := createChain(now.Add(time.Hour), now.Add(2*time.Hour)) + + serverConfig := testConfig.Clone() + serverConfig.MaxVersion = version + serverConfig.Certificates = []Certificate{{ + Certificate: [][]byte{initialLeafDER}, + PrivateKey: testECDSAPrivateKey, + }} + serverConfig.ClientCAs = x509.NewCertPool() + serverConfig.ClientCAs.AddCert(initialRoot) + serverConfig.ClientAuth = RequireAndVerifyClientCert + serverConfig.Time = func() time.Time { + return now + } + + clientConfig := testConfig.Clone() + clientConfig.MaxVersion = version + clientConfig.Certificates = []Certificate{{ + Certificate: [][]byte{initialLeafDER}, + PrivateKey: testECDSAPrivateKey, + }} + clientConfig.RootCAs = x509.NewCertPool() + clientConfig.RootCAs.AddCert(initialRoot) + clientConfig.ServerName = "expired-resume.example.com" + clientConfig.ClientSessionCache = NewLRUClientSessionCache(32) + + testResume := func(t *testing.T, sc, cc *Config, expectResume bool) { + t.Helper() + ss, cs, err := testHandshake(t, cc, sc) + if err != nil { + t.Fatalf("handshake: %v", err) + } + if cs.DidResume != expectResume { + t.Fatalf("DidResume = %v; want %v", cs.DidResume, expectResume) + } + if ss.DidResume != expectResume { + t.Fatalf("DidResume = %v; want %v", cs.DidResume, expectResume) + } + } + + testResume(t, serverConfig, clientConfig, false) + testResume(t, serverConfig, clientConfig, true) + + freshLeafDER, freshRoot := createChain(now.Add(2*time.Hour), now.Add(3*time.Hour)) + clientConfig.Certificates = []Certificate{{ + Certificate: [][]byte{freshLeafDER}, + PrivateKey: testECDSAPrivateKey, + }} + serverConfig.Time = func() time.Time { + return now.Add(1*time.Hour + 30*time.Minute) + } + serverConfig.ClientCAs = x509.NewCertPool() + serverConfig.ClientCAs.AddCert(freshRoot) + + testResume(t, serverConfig, clientConfig, false) +} diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go index a1a5d3101b9853cac3999e88eefccf6b2dbeaa21..e9e58755110ebe513fb3b185d2b06d0cdae86c83 100644 --- a/src/crypto/tls/handshake_server_tls13.go +++ b/src/crypto/tls/handshake_server_tls13.go @@ -352,6 +352,7 @@ if len(hs.clientHello.pskIdentities) == 0 { return nil } +pskIdentityLoop: for i, identity := range hs.clientHello.pskIdentities { if i >= maxClientPSKIdentities { break @@ -404,8 +405,13 @@ } if sessionHasClientCerts && c.config.ClientAuth == NoClientCert { continue } - if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) { - 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 { diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go index 76a9a222a94bd74b5251e08fa72e0e0f28dfd3f6..c5bbeb971366660faf4354b768a6be61c34f30ce 100644 --- a/src/crypto/tls/tls_test.go +++ b/src/crypto/tls/tls_test.go @@ -930,8 +930,8 @@ t.Errorf("all fields must be accounted for, but saw unknown field %q", fn) } } // 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) { @@ -2249,3 +2249,12 @@ if !cs.VerifiedChains[0][0].Equal(secretCert) { t.Fatal("unexpected certificate") } } + +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") + } +}