Merge pull request #151136 from cockroachdb/blathers/backport-release-25.3-151041

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

Author: angles-n-daemons

pkg/security/certificate_manager.go

@@ -13,6 +13,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security/certnames"
 	"github.com/cockroachdb/cockroach/pkg/security/clientcert"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/settings"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 	"github.com/cockroachdb/cockroach/pkg/util/log/severity"
@@ -196,15 +198,24 @@ func (cm *CertificateManager) RegisterSignalHandler(
 	})
 }
 
+var certCacheMemLimit = settings.RegisterByteSizeSetting(
+	settings.ApplicationLevel,
+	"security.client_cert.cache_memory_limit",
+	"memory limit for the client certificate expiration cache",
+	1<<29, // 512MiB
+)
+
 // RegisterExpirationCache registers a cache for client certificate expiration.
 // It is called during server startup.
 func (cm *CertificateManager) RegisterExpirationCache(
 	ctx context.Context,
 	stopper *stop.Stopper,
 	timeSrc timeutil.TimeSource,
 	parentMon *mon.BytesMonitor,
+	st *cluster.Settings,
 ) error {
-	m := mon.NewMonitorInheritWithLimit(mon.MakeName("client-expiration-caches"), 0 /* limit */, parentMon, true /* longLiving */)
+	limit := certCacheMemLimit.Get(&st.SV)
+	m := mon.NewMonitorInheritWithLimit(mon.MakeName("client-expiration-caches"), limit, parentMon, true /* longLiving */)
 	acc := m.MakeConcurrentBoundAccount()
 	m.StartNoReserved(ctx, parentMon)
 

pkg/security/clientcert/cert_expiry_cache.go

@@ -99,16 +99,19 @@ func (c *Cache) Upsert(ctx context.Context, user string, serial string, newExpir
 	if _, ok := c.cache[user]; !ok {
 		err := c.account.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.cache[user] = map[string]certInfo{}
 	}
 
-	err := c.account.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.cache[user][serial]; !ok {
+		err := c.account.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

@@ -313,6 +313,7 @@ func TestAllocationTracking(t *testing.T) {
 		cache.Upsert(ctx, "user1", "serial2", 120)
 		cache.Upsert(ctx, "user2", "serial3", 65)
 		clock.Advance(time.Minute)
+		require.Equal(t, (3*certInfoSize)+(4*gaugeSize), account.Used())
 		cache.Purge(ctx)
 		cache.Upsert(ctx, "user2", "serial4", 75)
 		require.Equal(t, certInfoSize+(2*gaugeSize), account.Used())
@@ -324,9 +325,21 @@ func TestAllocationTracking(t *testing.T) {
 		cache.Upsert(ctx, "user1", "serial2", 120)
 		cache.Upsert(ctx, "user2", "serial3", 65)
 		cache.Upsert(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, account := newCacheAndAccount(ctx, clock, stopper)
+		require.Equal(t, int64(0), account.Used())
+		cache.Upsert(ctx, "user1", "serial1", 100)
+		require.Equal(t, certInfoSize+(2*gaugeSize), account.Used())
+		cache.Upsert(ctx, "user1", "serial1", 100)
+		require.Equal(t, certInfoSize+(2*gaugeSize), account.Used())
+		cache.Upsert(ctx, "user1", "serial1", 100)
+		require.Equal(t, certInfoSize+(2*gaugeSize), account.Used())
+	})
 }
 
 func TestExpiration(t *testing.T) {

pkg/server/server_sql.go

@@ -944,7 +944,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 			return nil, errors.Wrap(err, "initializing certificate manager")
 		}
 		err = certMgr.RegisterExpirationCache(
-			ctx, cfg.stopper, &timeutil.DefaultTimeSource{}, rootSQLMemoryMonitor,
+			ctx, cfg.stopper, &timeutil.DefaultTimeSource{}, rootSQLMemoryMonitor, cfg.Settings,
 		)
 		if err != nil {
 			return nil, errors.Wrap(err, "adding clientcert cache")