Restrict which packages are aware of the keys used in Pinniped-generated certificate secrets

Author: joshuatcasey

internal/concierge/server/prepare_controllers.go

@@ -162,6 +162,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol
 			apicerts.NewAPIServiceUpdaterController(
 				c.ServerInstallationInfo.Namespace,
 				c.NamesConfig.ServingCertificateSecret,
+				apicerts.RetrieveCAFromSecret,
 				loginConciergeGroupData.APIServiceName(),
 				client.Aggregation,
 				informers.installationNamespaceK8s.Core().V1().Secrets(),
@@ -173,6 +174,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol
 			apicerts.NewAPIServiceUpdaterController(
 				c.ServerInstallationInfo.Namespace,
 				c.NamesConfig.ServingCertificateSecret,
+				apicerts.RetrieveCAFromSecret,
 				identityConciergeGroupData.APIServiceName(),
 				client.Aggregation,
 				informers.installationNamespaceK8s.Core().V1().Secrets(),
@@ -184,6 +186,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol
 			apicerts.NewCertsObserverController(
 				c.ServerInstallationInfo.Namespace,
 				c.NamesConfig.ServingCertificateSecret,
+				apicerts.RetrieveCertificateFromSecret,
 				c.DynamicServingCertProvider,
 				informers.installationNamespaceK8s.Core().V1().Secrets(),
 				controllerlib.WithInformer,
@@ -198,7 +201,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol
 				informers.installationNamespaceK8s.Core().V1().Secrets(),
 				controllerlib.WithInformer,
 				c.ServingCertRenewBefore,
-				apicerts.TLSCertificateChainSecretKey,
+				apicerts.RetrieveCertificateFromSecret,
 				plog.New(),
 			),
 			singletonWorker,
@@ -281,6 +284,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol
 				clock.RealClock{},
 				impersonator.New,
 				c.NamesConfig.ImpersonationSignerSecret,
+				apicerts.RetrieveCAFromSecret,
 				c.ImpersonationSigningCertProvider,
 				plog.New(),
 				c.ImpersonationProxyTokenCache,
@@ -310,7 +314,7 @@ func PrepareControllers(c *Config) (controllerinit.RunnerBuilder, error) { //nol
 				informers.installationNamespaceK8s.Core().V1().Secrets(),
 				controllerlib.WithInformer,
 				365*24*time.Hour-time.Hour, // 1 year minus 1 hour hard coded value (i.e. wait until the last moment to break the signer)
-				apicerts.CACertificateSecretKey,
+				apicerts.RetrieveCAFromSecret,
 				plog.New(),
 			),
 			singletonWorker,

internal/controller/apicerts/apiservice_updater.go

@@ -18,6 +18,7 @@ import (
 type apiServiceUpdaterController struct {
 	namespace               string
 	certsSecretResourceName string
+	certificateRetriever    RetrieveFromSecretFunc
 	aggregatorClient        aggregatorclient.Interface
 	secretInformer          corev1informers.SecretInformer
 	apiServiceName          string
@@ -26,6 +27,7 @@ type apiServiceUpdaterController struct {
 func NewAPIServiceUpdaterController(
 	namespace string,
 	certsSecretResourceName string,
+	certificateRetriever RetrieveFromSecretFunc,
 	apiServiceName string,
 	aggregatorClient aggregatorclient.Interface,
 	secretInformer corev1informers.SecretInformer,
@@ -37,6 +39,7 @@ func NewAPIServiceUpdaterController(
 			Syncer: &apiServiceUpdaterController{
 				namespace:               namespace,
 				certsSecretResourceName: certsSecretResourceName,
+				certificateRetriever:    certificateRetriever,
 				aggregatorClient:        aggregatorClient,
 				secretInformer:          secretInformer,
 				apiServiceName:          apiServiceName,
@@ -63,13 +66,15 @@ func (c *apiServiceUpdaterController) Sync(ctx controllerlib.Context) error {
 		return nil
 	}
 
+	caCertPEM, _ := c.certificateRetriever(certSecret)
+
 	// Update the APIService to give it the new CA bundle.
 	if err := UpdateAPIService(
 		ctx.Context,
 		c.aggregatorClient,
 		c.apiServiceName,
 		c.namespace,
-		certSecret.Data[CACertificateSecretKey],
+		caCertPEM,
 	); err != nil {
 		return fmt.Errorf("could not update the API service: %w", err)
 	}

internal/controller/apicerts/apiservice_updater_test.go

@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package apicerts
@@ -42,6 +42,9 @@ func TestAPIServiceUpdaterControllerOptions(t *testing.T) {
 			_ = NewAPIServiceUpdaterController(
 				installedInNamespace,
 				certsSecretResourceName,
+				func(secret *corev1.Secret) ([]byte, []byte) {
+					return secret.Data["some-key-for-ca-certificate"], []byte("this value does not matter")
+				},
 				loginv1alpha1.SchemeGroupVersion.Version+"."+loginv1alpha1.GroupName,
 				nil,
 				secretsInformer,
@@ -122,6 +125,9 @@ func TestAPIServiceUpdaterControllerSync(t *testing.T) {
 			subject = NewAPIServiceUpdaterController(
 				installedInNamespace,
 				certsSecretResourceName,
+				func(secret *corev1.Secret) ([]byte, []byte) {
+					return secret.Data["some-key-for-ca-certificate"], []byte("this value does not matter")
+				},
 				loginv1alpha1.SchemeGroupVersion.Version+"."+loginv1alpha1.GroupName,
 				aggregatorAPIClient,
 				kubeInformers.Core().V1().Secrets(),
@@ -185,9 +191,9 @@ func TestAPIServiceUpdaterControllerSync(t *testing.T) {
 						Namespace: installedInNamespace,
 					},
 					Data: map[string][]byte{
-						"caCertificate":       []byte("fake CA cert"),
-						"tlsPrivateKey":       []byte("fake private key"),
-						"tlsCertificateChain": []byte("fake cert chain"),
+						"some-key-for-ca-certificate": []byte("fake CA cert"),
+						"serving-cert-key-EXTRA":      []byte("fake cert chain"),
+						"private-key-EXTRA":           []byte("fake private key"),
 					},
 				}
 				err := kubeInformerClient.Tracker().Add(apiServingCertSecret)

internal/controller/apicerts/certs_creator.go

@@ -19,11 +19,17 @@ import (
 	"go.pinniped.dev/internal/plog"
 )
 
+// The following key names are unexported, to prevent a leaky abstraction.
+// Even the string literals should only be used in a very limited set of places:
+// - The unit tests for this file
+// - The unit tests for retrieve_from_secret.go
+// - Integration tests
+// Comment must end in a period, so here's a period: .
 const (
-	CACertificateSecretKey           = "caCertificate"
-	CACertificatePrivateKeySecretKey = "caCertificatePrivateKey"
+	caCertificateSecretKey           = "caCertificate"
+	caCertificatePrivateKeySecretKey = "caCertificatePrivateKey"
+	tlsCertificateChainSecretKey     = "tlsCertificateChain"
 	tlsPrivateKeySecretKey           = "tlsPrivateKey"
-	TLSCertificateChainSecretKey     = "tlsCertificateChain"
 )
 
 type certsCreatorController struct {
@@ -111,8 +117,8 @@ func (c *certsCreatorController) Sync(ctx controllerlib.Context) error {
 			Labels:    c.certsSecretLabels,
 		},
 		Data: map[string][]byte{
-			CACertificateSecretKey:           ca.Bundle(),
-			CACertificatePrivateKeySecretKey: caPrivateKeyPEM,
+			caCertificateSecretKey:           ca.Bundle(),
+			caCertificatePrivateKeySecretKey: caPrivateKeyPEM,
 		},
 	}
 
@@ -131,7 +137,7 @@ func (c *certsCreatorController) Sync(ctx controllerlib.Context) error {
 		}
 
 		secret.Data[tlsPrivateKeySecretKey] = tlsPrivateKeyPEM
-		secret.Data[TLSCertificateChainSecretKey] = tlsCertChainPEM
+		secret.Data[tlsCertificateChainSecretKey] = tlsCertChainPEM
 	}
 
 	_, err = c.k8sClient.CoreV1().Secrets(c.namespace).Create(ctx.Context, &secret, metav1.CreateOptions{})

internal/controller/apicerts/certs_expirer.go

@@ -31,7 +31,7 @@ type certsExpirerController struct {
 	// this controller will start to try to rotate it.
 	renewBefore time.Duration
 
-	secretKey string
+	certificateRetriever RetrieveFromSecretFunc
 
 	logger plog.Logger
 }
@@ -46,7 +46,7 @@ func NewCertsExpirerController(
 	secretInformer corev1informers.SecretInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 	renewBefore time.Duration,
-	secretKey string,
+	certificateRetriever RetrieveFromSecretFunc,
 	logger plog.Logger,
 ) controllerlib.Controller {
 	const name = "certs-expirer-controller"
@@ -59,7 +59,7 @@ func NewCertsExpirerController(
 				k8sClient:               k8sClient,
 				secretInformer:          secretInformer,
 				renewBefore:             renewBefore,
-				secretKey:               secretKey,
+				certificateRetriever:    certificateRetriever,
 				logger:                  logger.WithName(name),
 			},
 		},
@@ -83,15 +83,14 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error {
 			"controller", ctx.Name,
 			"namespace", c.namespace,
 			"name", c.certsSecretResourceName,
-			"key", c.secretKey,
 			"renewBefore", c.renewBefore.String(),
 		)
 		return nil
 	}
 
 	notBefore, notAfter, err := c.getCertBounds(secret)
 	if err != nil {
-		return fmt.Errorf("failed to get cert bounds for secret %q with key %q: %w", secret.Name, c.secretKey, err)
+		return fmt.Errorf("failed to get cert bounds for secret %q: %w", secret.Name, err)
 	}
 
 	certAge := time.Since(notBefore)
@@ -100,7 +99,6 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error {
 		"controller", ctx.Name,
 		"namespace", c.namespace,
 		"name", c.certsSecretResourceName,
-		"key", c.secretKey,
 		"renewBefore", c.renewBefore.String(),
 		"notBefore", notBefore.String(),
 		"notAfter", notAfter.String(),
@@ -130,7 +128,7 @@ func (c *certsExpirerController) Sync(ctx controllerlib.Context) error {
 // getCertBounds returns the NotBefore and NotAfter fields of the TLS
 // certificate in the provided secret, or an error.
 func (c *certsExpirerController) getCertBounds(secret *corev1.Secret) (time.Time, time.Time, error) {
-	certPEM := secret.Data[c.secretKey]
+	certPEM, _ := c.certificateRetriever(secret)
 	if certPEM == nil {
 		return time.Time{}, time.Time{}, constable.Error("failed to find certificate")
 	}

internal/controller/apicerts/certs_expirer_test.go

@@ -102,8 +102,8 @@ func TestExpirerControllerFilters(t *testing.T) {
 				nil, // k8sClient, not needed
 				secretsInformer,
 				withInformer.WithInformer,
-				0,  // renewBefore, not needed
-				"", // not needed
+				0,   // renewBefore, not needed
+				nil, // not needed
 				plog.TestLogger(t, io.Discard),
 			)
 
@@ -134,14 +134,14 @@ func TestExpirerControllerSync(t *testing.T) {
 	}{
 		{
 			name:       "secret does not exist",
-			wantLog:    `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"certs-expirer-controller","caller":"apicerts/certs_expirer.go:<line>$apicerts.(*certsExpirerController).Sync","message":"secret does not exist yet or was deleted","controller":"","namespace":"some-namespace","name":"some-resource-name","key":"some-awesome-key","renewBefore":"0s"}`,
+			wantLog:    `{"level":"info","timestamp":"2099-08-08T13:57:36.123456Z","logger":"certs-expirer-controller","caller":"apicerts/certs_expirer.go:<line>$apicerts.(*certsExpirerController).Sync","message":"secret does not exist yet or was deleted","controller":"","namespace":"some-namespace","name":"some-resource-name","renewBefore":"0s"}`,
 			wantDelete: false,
 		},
 		{
 			name:           "secret missing key",
 			fillSecretData: func(t *testing.T, m map[string][]byte) {},
 			wantDelete:     false,
-			wantError:      `failed to get cert bounds for secret "some-resource-name" with key "some-awesome-key": failed to find certificate`,
+			wantError:      `failed to get cert bounds for secret "some-resource-name": failed to find certificate`,
 		},
 		{
 			name:        "lifetime below threshold",
@@ -214,7 +214,7 @@ func TestExpirerControllerSync(t *testing.T) {
 				require.NoError(t, err)
 			},
 			wantDelete: false,
-			wantError:  `failed to get cert bounds for secret "some-resource-name" with key "some-awesome-key": failed to decode certificate PEM`,
+			wantError:  `failed to get cert bounds for secret "some-resource-name": failed to decode certificate PEM`,
 		},
 	}
 	for _, test := range tests {
@@ -265,7 +265,9 @@ func TestExpirerControllerSync(t *testing.T) {
 				kubeInformers.Core().V1().Secrets(),
 				controllerlib.WithInformer,
 				test.renewBefore,
-				fakeTestKey,
+				func(secret *corev1.Secret) ([]byte, []byte) {
+					return secret.Data[fakeTestKey], nil
+				},
 				plog.TestLogger(t, &log),
 			)
 

internal/controller/apicerts/certs_observer.go

@@ -18,13 +18,15 @@ import (
 type certsObserverController struct {
 	namespace               string
 	certsSecretResourceName string
+	certificateRetriever    RetrieveFromSecretFunc
 	dynamicCertProvider     dynamiccert.Private
 	secretInformer          corev1informers.SecretInformer
 }
 
 func NewCertsObserverController(
 	namespace string,
 	certsSecretResourceName string,
+	certificateRetriever RetrieveFromSecretFunc,
 	dynamicCertProvider dynamiccert.Private,
 	secretInformer corev1informers.SecretInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
@@ -35,6 +37,7 @@ func NewCertsObserverController(
 			Syncer: &certsObserverController{
 				namespace:               namespace,
 				certsSecretResourceName: certsSecretResourceName,
+				certificateRetriever:    certificateRetriever,
 				dynamicCertProvider:     dynamicCertProvider,
 				secretInformer:          secretInformer,
 			},
@@ -62,7 +65,7 @@ func (c *certsObserverController) Sync(_ controllerlib.Context) error {
 	}
 
 	// Mutate the in-memory cert provider to update with the latest cert values.
-	if err := c.dynamicCertProvider.SetCertKeyContent(certSecret.Data[TLSCertificateChainSecretKey], certSecret.Data[tlsPrivateKeySecretKey]); err != nil {
+	if err := c.dynamicCertProvider.SetCertKeyContent(c.certificateRetriever(certSecret)); err != nil {
 		return fmt.Errorf("failed to set serving cert/key content from secret %s/%s: %w", c.namespace, c.certsSecretResourceName, err)
 	}
 

internal/controller/apicerts/certs_observer_test.go

@@ -1,10 +1,11 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 package apicerts
 
 import (
 	"context"
+	_ "embed"
 	"strings"
 	"testing"
 	"time"
@@ -23,6 +24,9 @@ import (
 	"go.pinniped.dev/internal/testutil"
 )
 
+//go:embed testdata/private_key_prefix.txt
+var privateKeyPrefix string
+
 func TestObserverControllerInformerFilters(t *testing.T) {
 	spec.Run(t, "informer filters", func(t *testing.T, when spec.G, it spec.S) {
 		const installedInNamespace = "some-namespace"
@@ -40,6 +44,7 @@ func TestObserverControllerInformerFilters(t *testing.T) {
 				installedInNamespace,
 				certsSecretResourceName,
 				nil,
+				nil,
 				secretsInformer,
 				observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters
 			)
@@ -119,6 +124,9 @@ func TestObserverControllerSync(t *testing.T) {
 			subject = NewCertsObserverController(
 				installedInNamespace,
 				certsSecretResourceName,
+				func(secret *corev1.Secret) ([]byte, []byte) {
+					return secret.Data["some-key-for-certificate"], secret.Data["some-key-for-private-key"]
+				},
 				dynamicCertProvider,
 				kubeInformers.Core().V1().Secrets(),
 				controllerlib.WithInformer,
@@ -211,9 +219,9 @@ func TestObserverControllerSync(t *testing.T) {
 						Namespace: installedInNamespace,
 					},
 					Data: map[string][]byte{
-						"caCertificate":       []byte("fake cert"),
-						"tlsPrivateKey":       key,
-						"tlsCertificateChain": crt,
+						"some-pretend-ca-EXTRA":    []byte("fake cert"),
+						"some-key-for-certificate": crt,
+						"some-key-for-private-key": key,
 					},
 				}
 				err = kubeInformerClient.Tracker().Add(apiServingCertSecret)
@@ -234,7 +242,7 @@ func TestObserverControllerSync(t *testing.T) {
 
 				actualCertChain, actualKey = dynamicCertProvider.CurrentCertKeyContent()
 				r.True(strings.HasPrefix(string(actualCertChain), `-----BEGIN CERTIFICATE-----`), "not a cert:\n%s", string(actualCertChain))
-				r.True(strings.HasPrefix(string(actualKey), `-----BEGIN PRIVATE KEY-----`), "not a key:\n%s", string(actualKey))
+				r.True(strings.HasPrefix(string(actualKey), privateKeyPrefix), "not a key:\n%s", string(actualKey))
 			})
 		})