security: fix memory accounting issue in client certificate cache

The client certificate cache is a simple utility cache which keeps track
of the nearest certificate expiration times by user. It does this by
reading in certificates as they are used, and then updating the gauge
associated with the user so that our operators can alert on expiring
certificates.

It has been identified that there is a memory accounting issue with this
utility class, in that if we see the same certificate multiple times, we
report that our cache is growing for each one. In reality however,
though we may change the reference in our cache, the old value will be
cleaned up, and memory should remain relatively low.

This PR both fixes the accounting bug, and adds a limit so that the
client certificate cache can't negatively affect SQL operation.

Epic: none
Fixes: #151040
Release note (bug fix): Fixes a memory accounting issue with the client
certificate cache, where we were accidentally reporting multiple
allocations for the same memory.

Author: angles-n-daemons

pkg/security/clientcert/cert_expiry_cache.go

@@ -33,6 +33,13 @@ var ClientCertExpirationCacheCapacity = settings.RegisterIntSetting(
 	1000,
 	settings.WithPublic)
 
+var certCacheMemLimit = settings.RegisterByteSizeSetting(
+	settings.ApplicationLevel,
+	"security.client_cert.cache_memory_limit",
+	"memory limit for the client certificate expiration cache",
+	1<<29, // 512MiB
+)
+
 // certInfo holds information about a certificate, including its expiration
 // time and the last time it was seen.
 type certInfo struct {
@@ -89,8 +96,9 @@ func NewClientCertExpirationCache(
 	}
 
 	c.mu.cache = make(map[string]map[string]certInfo)
+	limit := certCacheMemLimit.Get(&st.SV)
 	c.mon = mon.NewMonitorInheritWithLimit(
-		"client-expiration-cache", 0 /* limit */, parentMon, true, /* longLiving */
+		"client-expiration-cache", limit, parentMon, true, /* longLiving */
 	)
 	c.mu.acc = c.mon.MakeBoundAccount()
 	c.mon.StartNoReserved(ctx, parentMon)
@@ -158,16 +166,19 @@ func (c *ClientCertExpirationCache) MaybeUpsert(
 	if _, ok := c.mu.cache[user]; !ok {
 		err := c.mu.acc.Grow(ctx, 2*GaugeSize)
 		if err != nil {
-			log.Ops.Warningf(ctx, "no memory available to cache cert expiry: %v", err)
+			log.Warningf(ctx, "no memory available to cache cert expiry: %v", err)
 			return
 		}
 		c.mu.cache[user] = map[string]certInfo{}
 	}
 
-	err := c.mu.acc.Grow(ctx, CertInfoSize)
-	if err != nil {
-		log.Warningf(ctx, "no memory available to cache cert expiry: %v", err)
-		c.evictLocked(ctx, user, serial)
+	// if the serial hasn't been seen, report it in the memory accounting.
+	if _, ok := c.mu.cache[user][serial]; !ok {
+		err := c.mu.acc.Grow(ctx, CertInfoSize)
+		if err != nil {
+			log.Warningf(ctx, "no memory available to cache cert expiry: %v", err)
+			return
+		}
 	}
 
 	// insert / update the certificate expiration time.

pkg/security/clientcert/cert_expiry_cache_test.go

@@ -371,6 +371,7 @@ func TestAllocationTracking(t *testing.T) {
 		cache.MaybeUpsert(ctx, "user1", "serial2", 120)
 		cache.MaybeUpsert(ctx, "user2", "serial3", 65)
 		clock.Advance(time.Minute)
+		require.Equal(t, (3*certInfoSize)+(4*gaugeSize), account.Used())
 		cache.Purge(ctx)
 		cache.MaybeUpsert(ctx, "user2", "serial4", 75)
 		require.Equal(t, certInfoSize+(2*gaugeSize), account.Used())
@@ -383,9 +384,22 @@ func TestAllocationTracking(t *testing.T) {
 		cache.MaybeUpsert(ctx, "user1", "serial2", 120)
 		cache.MaybeUpsert(ctx, "user2", "serial3", 65)
 		cache.MaybeUpsert(ctx, "user2", "serial4", 75)
+		require.Equal(t, (4*certInfoSize)+(4*gaugeSize), account.Used())
 		cache.Clear()
 		require.Equal(t, int64(0), account.Used())
 	})
+
+	t.Run("overwriting an existing certificate does not change the reported memory allocation", func(t *testing.T) {
+		cache := newCache(ctx, clock, stopper)
+		account := cache.Account()
+		require.Equal(t, int64(0), account.Used())
+		cache.MaybeUpsert(ctx, "user1", "serial1", 100)
+		require.Equal(t, certInfoSize+(2*gaugeSize), account.Used())
+		cache.MaybeUpsert(ctx, "user1", "serial1", 100)
+		require.Equal(t, certInfoSize+(2*gaugeSize), account.Used())
+		cache.MaybeUpsert(ctx, "user1", "serial1", 100)
+		require.Equal(t, certInfoSize+(2*gaugeSize), account.Used())
+	})
 }
 
 func TestGetExpiration(t *testing.T) {