Merge pull request #151141 from angles-n-daemons/backport24.3-151041

release-24.3: security: fix memory accounting issue in client certificate cache

Author: kyle-a-wong

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) {