crypto/tls: replace custom intern cache with weak cache

Uses the new weak package to replace the existing custom intern cache
with a map of weak.Pointers instead. This simplifies the cache, and
means we don't need to store a slice of handles on the Conn anymore.

Change-Id: I5c2bf6ef35fac4255e140e184f4e48574b34174c
Reviewed-on: https://go-review.googlesource.com/c/go/+/644176
TryBot-Bypass: Roland Shoemaker <roland@golang.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Roland Shoemaker <roland@golang.org>

cache.go

@@ -8,78 +8,19 @@ import (
 	"crypto/x509"
 	"runtime"
 	"sync"
-	"sync/atomic"
+	"weak"
 )
 
-type cacheEntry struct {
+// weakCertCache provides a cache of *x509.Certificates, allowing multiple
-	refs atomic.Int64
+// connections to reuse parsed certificates, instead of re-parsing the
-	cert *x509.Certificate
+// certificate for every connection, which is an expensive operation.
-}
+type weakCertCache struct{ sync.Map }
 
-// certCache implements an intern table for reference counted x509.Certificates,
+func (wcc *weakCertCache) newCert(der []byte) (*x509.Certificate, error) {
-// implemented in a similar fashion to BoringSSL's CRYPTO_BUFFER_POOL. This
+	if entry, ok := wcc.Load(string(der)); ok {
-// allows for a single x509.Certificate to be kept in memory and referenced from
+		if v := entry.(weak.Pointer[x509.Certificate]).Value(); v != nil {
-// multiple Conns. Returned references should not be mutated by callers. Certificates
+			return v, nil
-// are still safe to use after they are removed from the cache.
-//
-// Certificates are returned wrapped in an activeCert struct that should be held by
-// the caller. When references to the activeCert are freed, the number of references
-// to the certificate in the cache is decremented. Once the number of references
-// reaches zero, the entry is evicted from the cache.
-//
-// The main difference between this implementation and CRYPTO_BUFFER_POOL is that
-// CRYPTO_BUFFER_POOL is a more  generic structure which supports blobs of data,
-// rather than specific structures. Since we only care about x509.Certificates,
-// certCache is implemented as a specific cache, rather than a generic one.
-//
-// See https://boringssl.googlesource.com/boringssl/+/master/include/openssl/pool.h
-// and https://boringssl.googlesource.com/boringssl/+/master/crypto/pool/pool.c
-// for the BoringSSL reference.
-type certCache struct {
-	sync.Map
 		}
-
-var globalCertCache = new(certCache)
-
-// activeCert is a handle to a certificate held in the cache. Once there are
-// no alive activeCerts for a given certificate, the certificate is removed
-// from the cache by a cleanup.
-type activeCert struct {
-	cert *x509.Certificate
-}
-
-// active increments the number of references to the entry, wraps the
-// certificate in the entry in an activeCert, and sets the cleanup.
-//
-// Note that there is a race between active and the cleanup set on the
-// returned activeCert, triggered if active is called after the ref count is
-// decremented such that refs may be > 0 when evict is called. We consider this
-// safe, since the caller holding an activeCert for an entry that is no longer
-// in the cache is fine, with the only side effect being the memory overhead of
-// there being more than one distinct reference to a certificate alive at once.
-func (cc *certCache) active(e *cacheEntry) *activeCert {
-	e.refs.Add(1)
-	a := &activeCert{e.cert}
-	runtime.AddCleanup(a, func(ce *cacheEntry) {
-		if ce.refs.Add(-1) == 0 {
-			cc.evict(ce)
-		}
-	}, e)
-	return a
-}
-
-// evict removes a cacheEntry from the cache.
-func (cc *certCache) evict(e *cacheEntry) {
-	cc.Delete(string(e.cert.Raw))
-}
-
-// newCert returns a x509.Certificate parsed from der. If there is already a copy
-// of the certificate in the cache, a reference to the existing certificate will
-// be returned. Otherwise, a fresh certificate will be added to the cache, and
-// the reference returned. The returned reference should not be mutated.
-func (cc *certCache) newCert(der []byte) (*activeCert, error) {
-	if entry, ok := cc.Load(string(der)); ok {
-		return cc.active(entry.(*cacheEntry)), nil
 	}
 
 	cert, err := x509.ParseCertificate(der)
@@ -87,9 +28,17 @@ func (cc *certCache) newCert(der []byte) (*activeCert, error) {
 		return nil, err
 	}
 
-	entry := &cacheEntry{cert: cert}
+	wp := weak.Make(cert)
-	if entry, loaded := cc.LoadOrStore(string(der), entry); loaded {
+	if entry, loaded := wcc.LoadOrStore(string(der), wp); !loaded {
-		return cc.active(entry.(*cacheEntry)), nil
+		runtime.AddCleanup(cert, func(_ any) { wcc.CompareAndDelete(string(der), entry) }, any(string(der)))
+	} else if v := entry.(weak.Pointer[x509.Certificate]).Value(); v != nil {
+		return v, nil
+	} else {
+		if wcc.CompareAndSwap(string(der), entry, wp) {
+			runtime.AddCleanup(cert, func(_ any) { wcc.CompareAndDelete(string(der), wp) }, any(string(der)))
 		}
-	return cc.active(entry), nil
 	}
+	return cert, nil
+}
+
+var globalCertCache = new(weakCertCache)

conn.go

@@ -58,9 +58,6 @@ type Conn struct {
 	ocspResponse     []byte   // stapled OCSP response
 	scts             [][]byte // signed certificate timestamps from server
 	peerCertificates []*x509.Certificate
-	// activeCertHandles contains the cache handles to certificates in
-	// peerCertificates that are used to track active references.
-	activeCertHandles []*activeCert
 	// verifiedChains contains the certificate chains that we built, as
 	// opposed to the ones presented by the server.
 	verifiedChains [][]*x509.Certificate

handshake_client.go

@@ -949,7 +949,6 @@ func (hs *clientHandshakeState) processServerHello() (bool, error) {
 	hs.masterSecret = hs.session.secret
 	c.extMasterSecret = hs.session.extMasterSecret
 	c.peerCertificates = hs.session.peerCertificates
-	c.activeCertHandles = hs.c.activeCertHandles
 	c.verifiedChains = hs.session.verifiedChains
 	c.ocspResponse = hs.session.ocspResponse
 	// Let the ServerHello SCTs override the session SCTs from the original
@@ -1100,7 +1099,6 @@ func checkKeySize(n int) (max int, ok bool) {
 // verifyServerCertificate parses and verifies the provided chain, setting
 // c.verifiedChains and c.peerCertificates or sending the appropriate alert.
 func (c *Conn) verifyServerCertificate(certificates [][]byte) error {
-	activeHandles := make([]*activeCert, len(certificates))
 	certs := make([]*x509.Certificate, len(certificates))
 	for i, asn1Data := range certificates {
 		cert, err := globalCertCache.newCert(asn1Data)
@@ -1108,15 +1106,14 @@ func (c *Conn) verifyServerCertificate(certificates [][]byte) error {
 			c.sendAlert(alertDecodeError)
 			return errors.New("tls: failed to parse certificate from server: " + err.Error())
 		}
-		if cert.cert.PublicKeyAlgorithm == x509.RSA {
+		if cert.PublicKeyAlgorithm == x509.RSA {
-			n := cert.cert.PublicKey.(*rsa.PublicKey).N.BitLen()
+			n := cert.PublicKey.(*rsa.PublicKey).N.BitLen()
 			if max, ok := checkKeySize(n); !ok {
 				c.sendAlert(alertBadCertificate)
 				return fmt.Errorf("tls: server sent certificate containing RSA key larger than %d bits", max)
 			}
 		}
-		activeHandles[i] = cert
+		certs[i] = cert
-		certs[i] = cert.cert
 	}
 
 	echRejected := c.config.EncryptedClientHelloConfigList != nil && !c.echAccepted
@@ -1181,7 +1178,6 @@ func (c *Conn) verifyServerCertificate(certificates [][]byte) error {
 		return fmt.Errorf("tls: server's certificate contains an unsupported type of public key: %T", certs[0].PublicKey)
 	}
 
-	c.activeCertHandles = activeHandles
 	c.peerCertificates = certs
 
 	if c.config.VerifyPeerCertificate != nil && !echRejected {

handshake_client_tls13.go

@@ -466,7 +466,6 @@ func (hs *clientHandshakeStateTLS13) processServerHello() error {
 	hs.usingPSK = true
 	c.didResume = true
 	c.peerCertificates = hs.session.peerCertificates
-	c.activeCertHandles = hs.session.activeCertHandles
 	c.verifiedChains = hs.session.verifiedChains
 	c.ocspResponse = hs.session.ocspResponse
 	c.scts = hs.session.scts

ticket.go

@@ -88,7 +88,6 @@ type SessionState struct {
 	secret           []byte // master secret for TLS 1.2, or the PSK for TLS 1.3
 	extMasterSecret  bool
 	peerCertificates []*x509.Certificate
-	activeCertHandles []*activeCert
 	ocspResponse     []byte
 	scts             [][]byte
 	verifiedChains   [][]*x509.Certificate
@@ -239,8 +238,7 @@ func ParseSessionState(data []byte) (*SessionState, error) {
 		if err != nil {
 			return nil, err
 		}
-		ss.activeCertHandles = append(ss.activeCertHandles, c)
+		ss.peerCertificates = append(ss.peerCertificates, c)
-		ss.peerCertificates = append(ss.peerCertificates, c.cert)
 	}
 	if ss.isClient && len(ss.peerCertificates) == 0 {
 		return nil, errors.New("tls: no server certificates in client session")
@@ -270,8 +268,7 @@ func ParseSessionState(data []byte) (*SessionState, error) {
 			if err != nil {
 				return nil, err
 			}
-			ss.activeCertHandles = append(ss.activeCertHandles, c)
+			chain = append(chain, c)
-			chain = append(chain, c.cert)
 		}
 		ss.verifiedChains = append(ss.verifiedChains, chain)
 	}
@@ -305,7 +302,6 @@ func (c *Conn) sessionState() *SessionState {
 		createdAt:        uint64(c.config.time().Unix()),
 		alpnProtocol:     c.clientProtocol,
 		peerCertificates: c.peerCertificates,
-		activeCertHandles: c.activeCertHandles,
 		ocspResponse:     c.ocspResponse,
 		scts:             c.scts,
 		isClient:         c.isClient,