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: 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/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.RegisterIntSetting(
+	settings.SystemVisible,
+	"security.clienc_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

@@ -105,10 +105,13 @@ func (c *Cache) Upsert(ctx context.Context, user string, serial string, newExpir
 		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

@@ -947,7 +947,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")