security: change client cert expiration caching eviction policy #143081

angles-n-daemons · 2025-03-18T18:58:13Z

security: change client cert expiration caching eviction policy

Previously, we cached and emitted metrics for client certificates
indefinitely or until they expired. However, this strategy ignores
the case where a team rotates their certificates, discontinuing to
use soon-to-expire ones, in which crdb would continue to report on
their expiration.

This change modifies the caching behavior so that eviction from the
cache is based on last usage. It does a few additional things here:

Supports the removal of child metrics by key.
Removes the server.client_cert_expiration_cache.capacity cluster
setting.
Adds a shared utility for adding jitter.
Moves caching and metrics reporting from a single cache to two
caches, one for expiration, one for ttl.

Fixes: #142686
Epic: CRDB-40209

Release note(ops change): Cluster setting
server.client_cert_expiration_cache.capacity removed.
Metrics security.certificate.expiration.client and
security.certificate.ttl.client now report lowest value seen for
user in last 24h.

blathers-crl · 2025-03-18T18:58:17Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

angles-n-daemons · 2025-03-18T18:58:26Z

Note to reviewers, only review the last commit.

cockroach-teamcity · 2025-03-18T18:58:31Z

This change is

pkg/security/certificate_manager.go

angles-n-daemons · 2025-03-18T19:03:39Z

pkg/security/clientcert/cert_expiry_cache.go

+// time window (24h) and groups the certificates by user. When metrics are
+// read, the cache reports for each user the earliest expiration for any of
+// their recently used certificates.
+type Cache struct {


This is the bulk of the new changes.

angles-n-daemons · 2025-03-18T19:49:20Z

pkg/security/clientcert/cert_expiry_cache.go

+	if certs, ok := c.cache[user]; ok {
+		// compute the earliest expiration time.
+		var minExpiration int64
+		for _, cert := range certs {


This does a scan through the certificates for a given user. It's assumed that a given user does not have a lot of certificates.

angles-n-daemons · 2025-03-18T19:50:50Z

pkg/security/cert_expiry_cache.go

-
-// ClientCertExpirationCacheCapacity is the cluster setting that controls the
-// maximum number of client cert expirations in the cache.
-var ClientCertExpirationCacheCapacity = settings.RegisterIntSetting(


This setting is removed, it wasn't immediately clear to me what scale of client certificates the system deals with. Even if it were 1 per connecting node, I'd assume the number of nodes talking a single database node in a given 24h period does not exceed 1e5.

pkg/security/certificate_manager.go

dhartunian · 2025-03-19T17:13:39Z

pkg/security/certificate_manager.go

+	ctx context.Context, identity, serial string, expiration int64,
+) error {
+	for _, cache := range []*clientcert.Cache{cm.clientCertExpirationCache, cm.clientCertTTLCache} {
+		if cache == nil {


This looks wrong. What am I missing?

hmmm..this does look wrong, and was working...let me check.

Definitely a bug, fixed this up.

dhartunian · 2025-03-19T17:14:05Z

pkg/security/clientcert/cert_expiry_cache.go

-	return time.Duration(float64(interval) * (0.75 + 0.5*math_rand.Float64()))
+// evict is a utility function for removing a specific certificate from the
+// cache and removing the corresponding user if they have no more certificates.
+func (c *Cache) evict(ctx context.Context, user, serial string) {


rename this to evictLocked since it assumes mutex is taken

Ah yeah good call, changed.

dhartunian · 2025-03-19T17:27:53Z

pkg/security/certificate_manager.go

+	clientCertExpirationCache *clientcert.Cache
+
+	// Client cert ttl cache.
+	clientCertTTLCache *clientcert.Cache


I'm on the fence about splitting things up into two caches. Their implementations are so similar that it feels like there should be one cache that's specialized to update both metrics since they're supposed to be identical. In this case because these are only used here in 3 specific locations, it's easy to keep the usages contained.

Yeah I'm up for discussing it a little further. For more context behind motivation, making each cache responsible for one metric drastically improves the testing story.

The other improvement is that code duplication would have to exist somewhere, and would likely be multiplied if pushed down. If anything, I'd almost prefer to create an intermediary composite object for managing TTL and Expiration as one surface to the certificate manager.

Actually, I thought this over a bit more and was able to make it better than I worried about. Check out the new change, lmk what you think.

angles-n-daemons · 2025-03-20T17:41:07Z

pkg/util/timeutil/jitter_test.go

@@ -0,0 +1,133 @@
+// Copyright 2025 The Cockroach Authors.


gpt made this

Previously, we cached and emitted metrics for client certificates indefinitely or until they expired. However, this strategy ignores the case where a team rotates their certificates, discontinuing to use soon-to-expire ones, in which crdb would continue to report on their expiration. This change modifies the caching behavior so that eviction from the cache is based on last usage. It does a few additional things here: - Supports the removal of child metrics by key. - Removes the `server.client_cert_expiration_cache.capacity` cluster setting. - Adds a shared utility for adding jitter. - Moves caching and metrics reporting from a single cache to two caches, one for expiration, one for ttl. Fixes: cockroachdb#142686 Epic: CRDB-40209 Release note(ops change): Cluster setting `server.client_cert_expiration_cache.capacity` removed. Metrics `security.certificate.expiration.client` and `security.certificate.ttl.client` now report lowest value seen for user in last 24h.

dhartunian

I like the modification to maintain a single cache, thanks!

In general, I think this PR is not "backport friendly" because the diff is large and contains tangential changes (I agree they're good but just not suited for a backport where we prefer to change as little as possible). Can you restructure into two separate commits potentially? Or perhaps 3 might be necessary.

Smallest possible diff that gets a version of this functionality working. It's not as important in the backport to have these refactors and changes. They make the diff larger, and increase risk.
Refactors etc. that are nice to have but not needed for backports
Second iteration of feature to make the code nicer if necessary on master only.

Let me know if this is tricky or not possible and we can discuss

dhartunian · 2025-03-21T18:51:43Z

pkg/security/clientcert/cert_expiry_cache_test.go

@@ -1,4 +1,4 @@
-// Copyright 2023 The Cockroach Authors.
+// Copyright 2025 The Cockroach Authors.


nit: generally there's no need to bump copyright years in files.

oh nice catch!

angles-n-daemons · 2025-03-21T19:34:07Z

Making the PR smaller is definitely possible, and tricky simultaneously. Is it worth hacking on for a day to see if I can trim down the changeset?

Here are some of the not-quite-required additions:

a shared jitter utility (with tests)
a new package for the cache to simplify namespace collisions
bytes monitor setup upstream
long-held metrics object

On Monday I'll try to put up some example PRs which are smaller, pruning out the easier of the above.

angles-n-daemons · 2025-03-25T17:32:17Z

Closing this in favor of a smaller changeset.

angles-n-daemons requested review from a team as code owners March 18, 2025 18:58

angles-n-daemons requested review from dhartunian, arjunmahishi, aa-joshi and Abhinav1299 and removed request for a team March 18, 2025 18:58

angles-n-daemons commented Mar 18, 2025

View reviewed changes

pkg/security/certificate_manager.go Show resolved Hide resolved

angles-n-daemons force-pushed the client-cert-expiration-by-usage branch 2 times, most recently from b7fc3d0 to 9c78f7a Compare March 18, 2025 19:02

angles-n-daemons commented Mar 18, 2025

View reviewed changes

angles-n-daemons force-pushed the client-cert-expiration-by-usage branch 2 times, most recently from 0f0f0e1 to 6f504b4 Compare March 18, 2025 19:47

angles-n-daemons commented Mar 18, 2025

View reviewed changes

angles-n-daemons force-pushed the client-cert-expiration-by-usage branch from 6f504b4 to a579bf7 Compare March 18, 2025 19:53

dhartunian reviewed Mar 19, 2025

View reviewed changes

angles-n-daemons force-pushed the client-cert-expiration-by-usage branch from a579bf7 to 2e3055a Compare March 19, 2025 18:28

angles-n-daemons commented Mar 20, 2025

View reviewed changes

pkg/util/timeutil/jitter_test.go

@@ -0,0 +1,133 @@

// Copyright 2025 The Cockroach Authors.

Copy link

Contributor Author

angles-n-daemons Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gpt made this

angles-n-daemons force-pushed the client-cert-expiration-by-usage branch 2 times, most recently from a69bbbc to 200ae5a Compare March 21, 2025 16:41

angles-n-daemons force-pushed the client-cert-expiration-by-usage branch from 200ae5a to 522c333 Compare March 21, 2025 18:25

dhartunian reviewed Mar 21, 2025

View reviewed changes

angles-n-daemons closed this Mar 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security: change client cert expiration caching eviction policy #143081

security: change client cert expiration caching eviction policy #143081

angles-n-daemons commented Mar 18, 2025

blathers-crl bot commented Mar 18, 2025

angles-n-daemons commented Mar 18, 2025

cockroach-teamcity commented Mar 18, 2025

angles-n-daemons Mar 18, 2025

angles-n-daemons Mar 18, 2025

angles-n-daemons Mar 18, 2025 •

edited

Loading

dhartunian Mar 19, 2025

angles-n-daemons Mar 20, 2025

angles-n-daemons Mar 20, 2025

dhartunian Mar 19, 2025

angles-n-daemons Mar 20, 2025

dhartunian Mar 19, 2025

angles-n-daemons Mar 20, 2025

angles-n-daemons Mar 21, 2025

angles-n-daemons Mar 20, 2025

dhartunian left a comment

dhartunian Mar 21, 2025

angles-n-daemons Mar 21, 2025

angles-n-daemons commented Mar 21, 2025 •

edited

Loading

angles-n-daemons commented Mar 25, 2025

		@@ -1,4 +1,4 @@
		// Copyright 2023 The Cockroach Authors.
		// Copyright 2025 The Cockroach Authors.

security: change client cert expiration caching eviction policy #143081

security: change client cert expiration caching eviction policy #143081

Conversation

angles-n-daemons commented Mar 18, 2025

blathers-crl bot commented Mar 18, 2025

angles-n-daemons commented Mar 18, 2025

cockroach-teamcity commented Mar 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angles-n-daemons Mar 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhartunian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angles-n-daemons commented Mar 21, 2025 • edited Loading

angles-n-daemons commented Mar 25, 2025

angles-n-daemons Mar 18, 2025 •

edited

Loading

angles-n-daemons commented Mar 21, 2025 •

edited

Loading