security: clear client cert expiration cache on SIGHUP

Release note (security update): The SIGHUP signal will now clear the
cached expiration times for client certificates that are reported by
the security.certificate.expiration.client metric.

pkg/security/BUILD.bazel

@@ -106,54 +106,67 @@ go_test(
     ] + select({
         "@io_bazel_rules_go//go/platform:aix": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:android": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:darwin": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:dragonfly": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:freebsd": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:illumos": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:ios": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:js": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:linux": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:netbsd": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:openbsd": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:plan9": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "@io_bazel_rules_go//go/platform:solaris": [
             "//pkg/util/log/eventpb",
+            "@com_github_prometheus_client_model//go",
             "@org_golang_x_sys//unix",
         ],
         "//conditions:default": [],

pkg/security/cert_expiry_cache.go

@@ -88,6 +88,9 @@ func NewClientCertExpirationCache(
 		},
 		OnEvictedEntry: func(entry *cache.Entry) {
 			gauge := entry.Value.(*aggmetric.Gauge)
+			// The child metric will continue to report into the parent metric even
+			// after unlinking, so we also reset it to 0.
+			gauge.Update(0)
 			gauge.Unlink()
 			c.mu.acc.Shrink(ctx, int64(unsafe.Sizeof(*gauge)))
 		},

pkg/security/certificate_manager.go

@@ -170,6 +170,9 @@ func (cm *CertificateManager) RegisterSignalHandler(
 				return
 			case sig := <-ch:
 				log.Ops.Infof(ctx, "received signal %q, triggering certificate reload", sig)
+				if cache := cm.clientCertExpirationCache; cache != nil {
+					cache.Clear()
+				}
 				if err := cm.LoadCertificates(); err != nil {
 					log.Ops.Warningf(ctx, "could not reload certificates: %v", err)
 					log.StructuredEvent(ctx, &eventpb.CertsReload{Success: false, ErrorMessage: err.Error()})
@@ -187,9 +190,9 @@ func (cm *CertificateManager) RegisterExpirationCache(cache *ClientCertExpiratio
 	cm.clientCertExpirationCache = cache
 }
 
-// MaybeUpsertClientExpiration updates the updates or inserts the expiration
-// time for the given client certificate. An update is contingent on whether the
-// old expiration is after the new expiration.
+// MaybeUpsertClientExpiration updates or inserts the expiration time for the
+// given client certificate. An update is contingent on whether the old
+// expiration is after the new expiration.
 func (cm *CertificateManager) MaybeUpsertClientExpiration(
 	ctx context.Context, identity username.SQLUsername, expiration int64,
 ) {

pkg/security/certs_rotation_test.go

@@ -25,6 +25,7 @@ import (
 	"regexp"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer"
@@ -41,6 +42,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/log/eventpb"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
+	prometheusgo "github.com/prometheus/client_model/go"
+	"github.com/stretchr/testify/require"
 	"golang.org/x/sys/unix"
 )
 
@@ -56,7 +59,7 @@ func TestRotateCerts(t *testing.T) {
 	defer ResetTest()
 	certsDir := t.TempDir()
 
-	if err := generateBaseCerts(certsDir); err != nil {
+	if err := generateBaseCerts(certsDir, 48*time.Hour); err != nil {
 		t.Fatal(err)
 	}
 
@@ -99,6 +102,22 @@ func TestRotateCerts(t *testing.T) {
 		return goDB
 	}
 
+	checkCertExpirationMetric := func() int64 {
+		cm, err := s.RPCContext().SecurityContext.GetCertificateManager()
+		if err != nil {
+			t.Fatal(err)
+		}
+		ret := int64(0)
+		cm.Metrics().ClientExpiration.Each(nil, func(pm *prometheusgo.Metric) {
+			for _, l := range pm.GetLabel() {
+				if l.GetName() == "sql_user" && l.GetValue() == username.RootUser {
+					ret = int64(pm.GetGauge().GetValue())
+				}
+			}
+		})
+		return ret
+	}
+
 	// Some errors codes.
 	const kBadAuthority = "certificate signed by unknown authority"
 	const kBadCertificate = "tls: bad certificate"
@@ -117,6 +136,9 @@ func TestRotateCerts(t *testing.T) {
 		t.Fatalf("could not create http client: %v", err)
 	}
 
+	// Before any client has connected, the expiration metric should be zero.
+	require.Zero(t, checkCertExpirationMetric())
+
 	if err := clientTest(firstClient); err != nil {
 		t.Fatal(err)
 	}
@@ -128,12 +150,15 @@ func TestRotateCerts(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Delete certs and re-generate them.
+	// Now the expiration metric should be set.
+	require.Greater(t, checkCertExpirationMetric(), timeutil.Now().Add(40*time.Hour).Unix())
+
+	// Delete certs and re-generate them with a longer client cert lifetime.
 	// New clients will fail with CA errors.
 	if err := os.RemoveAll(certsDir); err != nil {
 		t.Fatal(err)
 	}
-	if err := generateBaseCerts(certsDir); err != nil {
+	if err := generateBaseCerts(certsDir, 54*time.Hour); err != nil {
 		t.Fatal(err)
 	}
 
@@ -195,6 +220,9 @@ func TestRotateCerts(t *testing.T) {
 			return nil
 		})
 
+	// The expiration metric should be zero after the SIGHUP.
+	require.Zero(t, checkCertExpirationMetric())
+
 	// Check that the structured event was logged.
 	// We use SucceedsSoon here because there may be a delay between
 	// the moment SIGHUP is processed and certs are reloaded, and
@@ -242,13 +270,16 @@ func TestRotateCerts(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	// The expiration metric should be set again, but should now show a higher value.
+	require.Greater(t, checkCertExpirationMetric(), timeutil.Now().Add(50*time.Hour).Unix())
+
 	// Now regenerate certs, but keep the CA cert around.
 	// We still need to delete the key.
 	// New clients with certs will fail with bad certificate (CA not yet loaded).
 	if err := os.Remove(filepath.Join(certsDir, certnames.EmbeddedCAKey)); err != nil {
 		t.Fatal(err)
 	}
-	if err := generateBaseCerts(certsDir); err != nil {
+	if err := generateBaseCerts(certsDir, 58*time.Hour); err != nil {
 		t.Fatal(err)
 	}
 
@@ -301,6 +332,9 @@ func TestRotateCerts(t *testing.T) {
 			}
 			return nil
 		})
+
+	// The expiration metric should be updated to be larger again.
+	require.Greater(t, checkCertExpirationMetric(), timeutil.Now().Add(57*time.Hour).Unix())
 }
 
 var cmLogRe = regexp.MustCompile(`event_log\.go`)

pkg/security/certs_test.go

@@ -263,7 +263,7 @@ func TestGenerateNodeCerts(t *testing.T) {
 // client.root.crt: client certificate for the root user.
 // client-tenant.10.crt: tenant client certificate for tenant 10.
 // tenant-signing.10.crt: tenant signing certificate for tenant 10.
-func generateBaseCerts(certsDir string) error {
+func generateBaseCerts(certsDir string, clientCertLifetime time.Duration) error {
 	{
 		caKey := filepath.Join(certsDir, certnames.EmbeddedCAKey)
 
@@ -285,7 +285,7 @@ func generateBaseCerts(certsDir string) error {
 			certsDir,
 			caKey,
 			testKeySize,
-			time.Hour*48,
+			clientCertLifetime,
 			true,
 			username.RootUserName(),
 			[]roachpb.TenantID{roachpb.SystemTenantID},
@@ -328,7 +328,7 @@ func generateBaseCerts(certsDir string) error {
 // client.node.crt: node client cert: signed by ca-client.crt
 // client.root.crt: root client cert: signed by ca-client.crt
 func generateSplitCACerts(certsDir string) error {
-	if err := generateBaseCerts(certsDir); err != nil {
+	if err := generateBaseCerts(certsDir, 48*time.Hour); err != nil {
 		return err
 	}
 
@@ -381,7 +381,7 @@ func TestUseCerts(t *testing.T) {
 	defer ResetTest()
 	certsDir := t.TempDir()
 
-	if err := generateBaseCerts(certsDir); err != nil {
+	if err := generateBaseCerts(certsDir, 48*time.Hour); err != nil {
 		t.Fatal(err)
 	}