crypto/tls: don't reverify but check certificate expiration on resumption

We used to inconsistently run certificate verification on the server on
resumption, but not on the client. This made TLS 1.3 resumption pretty
much useless, as it didn't save bytes, CPU, or round-trips.

This requires serializing the verified chains into the session ticket,
so it's a tradeoff making the ticket bigger to save computation (and for
consistency).

The previous behavior also had a "stickyness" issue: if a ticket
contained invalid certificates, they would be used even if the client
had in the meantime configured valid certificates for a full handshake.

We also didn't check expiration on the client side on resumption if
InsecureSkipVerify was set. Again for consistency, we do that now.

Also, we used to run VerifyPeerCertificates on resumption even if
NoClientCerts was set.

Fixes #31641

Change-Id: Icc88269ea4adb544fa81158114aae76f3c91a15f
Reviewed-on: https://go-review.googlesource.com/c/go/+/497895
Reviewed-by: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Filippo Valsorda <filippo@golang.org>
Reviewed-by: Roland Shoemaker <roland@golang.org>
Auto-Submit: Filippo Valsorda <filippo@golang.org>

common.go

@@ -618,10 +618,16 @@ type Config struct {
 	// non-nil error, the handshake is aborted and that error results.
 	//
 	// If normal verification fails then the handshake will abort before
-	// considering this callback. If normal verification is disabled by
+	// considering this callback. If normal verification is disabled (on the
-	// setting InsecureSkipVerify, or (for a server) when ClientAuth is
+	// client when InsecureSkipVerify is set, or on a server when ClientAuth is
-	// RequestClientCert or RequireAnyClientCert, then this callback will
+	// RequestClientCert or RequireAnyClientCert), then this callback will be
-	// be considered but the verifiedChains argument will always be nil.
+	// considered but the verifiedChains argument will always be nil. When
+	// ClientAuth is NoClientCert, this callback is not called on the server.
+	// 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
@@ -632,8 +638,9 @@ type Config struct {
 	// and that error results.
 	//
 	// If normal verification fails then the handshake will abort before
-	// considering this callback. This callback will run for all connections
+	// considering this callback. This callback will run for all connections,
-	// regardless of InsecureSkipVerify or ClientAuth settings.
+	// including resumptions, regardless of InsecureSkipVerify or ClientAuth
+	// settings.
 	VerifyConnection func(ConnectionState) error
 
 	// RootCAs defines the set of root certificate authorities

handshake_client.go

@@ -319,18 +319,17 @@ func (c *Conn) loadSession(hello *clientHelloMsg) (
 	// Check that the cached server certificate is not expired, and that it's
 	// valid for the ServerName. This should be ensured by the cache key, but
 	// protect the application from a faulty ClientSessionCache implementation.
+	if c.config.time().After(session.peerCertificates[0].NotAfter) {
+		// Expired certificate, delete the entry.
+		c.config.ClientSessionCache.Put(cacheKey, nil)
+		return nil, nil, nil, nil
+	}
 	if !c.config.InsecureSkipVerify {
 		if len(session.verifiedChains) == 0 {
 			// The original connection had InsecureSkipVerify, while this doesn't.
 			return nil, nil, nil, nil
 		}
-		serverCert := session.peerCertificates[0]
+		if err := session.peerCertificates[0].VerifyHostname(c.config.ServerName); err != nil {
-		if c.config.time().After(serverCert.NotAfter) {
-			// Expired certificate, delete the entry.
-			c.config.ClientSessionCache.Put(cacheKey, nil)
-			return nil, nil, nil, nil
-		}
-		if err := serverCert.VerifyHostname(c.config.ServerName); err != nil {
 			return nil, nil, nil, nil
 		}
 	}

handshake_server.go

@@ -471,6 +471,13 @@ func (hs *serverHandshakeState) checkForResumption() error {
 	if sessionHasClientCerts && c.config.ClientAuth == NoClientCert {
 		return nil
 	}
+	if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].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 {
@@ -482,6 +489,10 @@ func (hs *serverHandshakeState) checkForResumption() error {
 		return errors.New("tls: session supported extended_master_secret but client does not")
 	}
 
+	c.peerCertificates = sessionState.peerCertificates
+	c.ocspResponse = sessionState.ocspResponse
+	c.scts = sessionState.scts
+	c.verifiedChains = sessionState.verifiedChains
 	c.extMasterSecret = sessionState.extMasterSecret
 	hs.sessionState = sessionState
 	hs.suite = suite
@@ -510,10 +521,6 @@ func (hs *serverHandshakeState) doResumeHandshake() error {
 		return err
 	}
 
-	if err := c.processCertsFromClient(hs.sessionState.certificate()); err != nil {
-		return err
-	}
-
 	if c.config.VerifyConnection != nil {
 		if err := c.config.VerifyConnection(c.connectionStateLocked()); err != nil {
 			c.sendAlert(alertBadCertificate)
@@ -847,8 +854,7 @@ func (hs *serverHandshakeState) sendFinished(out []byte) error {
 }
 
 // processCertsFromClient takes a chain of client certificates either from a
-// Certificates message or from a sessionState and verifies them. It returns
+// Certificates message and verifies them.
-// the public key of the leaf certificate.
 func (c *Conn) processCertsFromClient(certificate Certificate) error {
 	certificates := certificate.Certificate
 	certs := make([]*x509.Certificate, len(certificates))

handshake_server_tls13.go

@@ -387,6 +387,13 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error {
 		if sessionHasClientCerts && c.config.ClientAuth == NoClientCert {
 			continue
 		}
+		if sessionHasClientCerts && c.config.time().After(sessionState.peerCertificates[0].NotAfter) {
+			continue
+		}
+		if sessionHasClientCerts && c.config.ClientAuth >= VerifyClientCertIfGiven &&
+			len(sessionState.verifiedChains) == 0 {
+			continue
+		}
 
 		hs.earlySecret = hs.suite.extract(sessionState.secret, nil)
 		binderKey := hs.suite.deriveSecret(hs.earlySecret, resumptionBinderLabel, nil)
@@ -422,9 +429,10 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error {
 		}
 
 		c.didResume = true
-		if err := c.processCertsFromClient(sessionState.certificate()); err != nil {
+		c.peerCertificates = sessionState.peerCertificates
-			return err
+		c.ocspResponse = sessionState.ocspResponse
-		}
+		c.scts = sessionState.scts
+		c.verifiedChains = sessionState.verifiedChains
 
 		hs.hello.selectedIdentityPresent = true
 		hs.hello.selectedIdentity = uint16(i)

ticket.go

@@ -37,16 +37,16 @@ type SessionState struct {
 	//       uint8 ext_master_secret = { 0, 1 };
 	//       uint8 early_data = { 0, 1 };
 	//       CertificateEntry certificate_list<0..2^24-1>;
+	//       CertificateChain verified_chains<0..2^24-1>; /* excluding leaf */
 	//       select (SessionState.early_data) {
 	//           case 0: Empty;
 	//           case 1: opaque alpn<1..2^8-1>;
 	//       };
 	//       select (SessionState.type) {
-	//           case server: /* empty */;
+	//           case server: Empty;
 	//           case client: struct {
-	//               CertificateChain verified_chains<0..2^24-1>; /* excluding leaf */
 	//               select (SessionState.version) {
-	//                   case VersionTLS10..VersionTLS12: /* empty */;
+	//                   case VersionTLS10..VersionTLS12: Empty;
 	//                   case VersionTLS13: struct {
 	//                       uint64 use_by;
 	//                       uint32 age_add;
@@ -87,10 +87,8 @@ type SessionState struct {
 	activeCertHandles []*activeCert
 	ocspResponse      []byte
 	scts              [][]byte
-	alpnProtocol      string // only set if EarlyData is true
-
-	// Client-side fields.
 	verifiedChains [][]*x509.Certificate
+	alpnProtocol      string // only set if EarlyData is true
 
 	// Client-side TLS 1.3-only fields.
 	useBy  uint64 // seconds since UNIX epoch
@@ -129,29 +127,33 @@ func (s *SessionState) Bytes() ([]byte, error) {
 	} else {
 		b.AddUint8(0)
 	}
-	marshalCertificate(&b, s.certificate())
+	marshalCertificate(&b, Certificate{
+		Certificate:                 certificatesToBytesSlice(s.peerCertificates),
+		OCSPStaple:                  s.ocspResponse,
+		SignedCertificateTimestamps: s.scts,
+	})
+	b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
+		for _, chain := range s.verifiedChains {
+			b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
+				// We elide the first certificate because it's always the leaf.
+				if len(chain) == 0 {
+					b.SetError(errors.New("tls: internal error: empty verified chain"))
+					return
+				}
+				for _, cert := range chain[1:] {
+					b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
+						b.AddBytes(cert.Raw)
+					})
+				}
+			})
+		}
+	})
 	if s.EarlyData {
 		b.AddUint8LengthPrefixed(func(b *cryptobyte.Builder) {
 			b.AddBytes([]byte(s.alpnProtocol))
 		})
 	}
 	if s.isClient {
-		b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
-			for _, chain := range s.verifiedChains {
-				b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
-					// We elide the first certificate because it's always the leaf.
-					if len(chain) == 0 {
-						b.SetError(errors.New("tls: internal error: empty verified chain"))
-						return
-					}
-					for _, cert := range chain[1:] {
-						b.AddUint24LengthPrefixed(func(b *cryptobyte.Builder) {
-							b.AddBytes(cert.Raw)
-						})
-					}
-				})
-			}
-		})
 		if s.version >= VersionTLS13 {
 			addUint64(&b, s.useBy)
 			b.AddUint32(s.ageAdd)
@@ -160,14 +162,6 @@ func (s *SessionState) Bytes() ([]byte, error) {
 	return b.Bytes()
 }
 
-func (s *SessionState) certificate() Certificate {
-	return Certificate{
-		Certificate:                 certificatesToBytesSlice(s.peerCertificates),
-		OCSPStaple:                  s.ocspResponse,
-		SignedCertificateTimestamps: s.scts,
-	}
-}
-
 func certificatesToBytesSlice(certs []*x509.Certificate) [][]byte {
 	s := make([][]byte, 0, len(certs))
 	for _, c := range certs {
@@ -221,6 +215,34 @@ func ParseSessionState(data []byte) (*SessionState, error) {
 	}
 	ss.ocspResponse = cert.OCSPStaple
 	ss.scts = cert.SignedCertificateTimestamps
+	var chainList cryptobyte.String
+	if !s.ReadUint24LengthPrefixed(&chainList) {
+		return nil, errors.New("tls: invalid session encoding")
+	}
+	for !chainList.Empty() {
+		var certList cryptobyte.String
+		if !chainList.ReadUint24LengthPrefixed(&certList) {
+			return nil, errors.New("tls: invalid session encoding")
+		}
+		var chain []*x509.Certificate
+		if len(ss.peerCertificates) == 0 {
+			return nil, errors.New("tls: invalid session encoding")
+		}
+		chain = append(chain, ss.peerCertificates[0])
+		for !certList.Empty() {
+			var cert []byte
+			if !readUint24LengthPrefixed(&certList, &cert) {
+				return nil, errors.New("tls: invalid session encoding")
+			}
+			c, err := globalCertCache.newCert(cert)
+			if err != nil {
+				return nil, err
+			}
+			ss.activeCertHandles = append(ss.activeCertHandles, c)
+			chain = append(chain, c.cert)
+		}
+		ss.verifiedChains = append(ss.verifiedChains, chain)
+	}
 	if ss.EarlyData {
 		var alpn []byte
 		if !readUint8LengthPrefixed(&s, &alpn) {
@@ -238,31 +260,6 @@ func ParseSessionState(data []byte) (*SessionState, error) {
 	if len(ss.peerCertificates) == 0 {
 		return nil, errors.New("tls: no server certificates in client session")
 	}
-	var chainList cryptobyte.String
-	if !s.ReadUint24LengthPrefixed(&chainList) {
-		return nil, errors.New("tls: invalid session encoding")
-	}
-	for !chainList.Empty() {
-		var certList cryptobyte.String
-		if !chainList.ReadUint24LengthPrefixed(&certList) {
-			return nil, errors.New("tls: invalid session encoding")
-		}
-		var chain []*x509.Certificate
-		chain = append(chain, ss.peerCertificates[0])
-		for !certList.Empty() {
-			var cert []byte
-			if !readUint24LengthPrefixed(&certList, &cert) {
-				return nil, errors.New("tls: invalid session encoding")
-			}
-			c, err := globalCertCache.newCert(cert)
-			if err != nil {
-				return nil, err
-			}
-			ss.activeCertHandles = append(ss.activeCertHandles, c)
-			chain = append(chain, c.cert)
-		}
-		ss.verifiedChains = append(ss.verifiedChains, chain)
-	}
 	if ss.version < VersionTLS13 {
 		if !s.Empty() {
 			return nil, errors.New("tls: invalid session encoding")
@@ -278,13 +275,6 @@ func ParseSessionState(data []byte) (*SessionState, error) {
 // sessionState returns a partially filled-out [SessionState] with information
 // from the current connection.
 func (c *Conn) sessionState() (*SessionState, error) {
-	var verifiedChains [][]*x509.Certificate
-	if c.isClient {
-		verifiedChains = c.verifiedChains
-		if len(c.peerCertificates) == 0 {
-			return nil, errors.New("tls: internal error: empty peer certificates")
-		}
-	}
 	return &SessionState{
 		version:           c.vers,
 		cipherSuite:       c.cipherSuite,
@@ -296,7 +286,7 @@ func (c *Conn) sessionState() (*SessionState, error) {
 		scts:              c.scts,
 		isClient:          c.isClient,
 		extMasterSecret:   c.extMasterSecret,
-		verifiedChains:    verifiedChains,
+		verifiedChains:    c.verifiedChains,
 	}, nil
 }