dynamiccert: split into serving cert and CA providers

Signed-off-by: Monis Khan <mok@vmware.com>

cmd/local-user-authenticator/main.go

@@ -54,12 +54,12 @@ const (
 )
 
 type webhook struct {
-	certProvider   dynamiccert.Provider
+	certProvider   dynamiccert.Private
 	secretInformer corev1informers.SecretInformer
 }
 
 func newWebhook(
-	certProvider dynamiccert.Provider,
+	certProvider dynamiccert.Private,
 	secretInformer corev1informers.SecretInformer,
 ) *webhook {
 	return &webhook{
@@ -281,7 +281,7 @@ func respondWithAuthenticated(
 
 func startControllers(
 	ctx context.Context,
-	dynamicCertProvider dynamiccert.Provider,
+	dynamicCertProvider dynamiccert.Private,
 	kubeClient kubernetes.Interface,
 	kubeInformers kubeinformers.SharedInformerFactory,
 ) {
@@ -328,7 +328,7 @@ func startControllers(
 func startWebhook(
 	ctx context.Context,
 	l net.Listener,
-	dynamicCertProvider dynamiccert.Provider,
+	dynamicCertProvider dynamiccert.Private,
 	secretInformer corev1informers.SecretInformer,
 ) error {
 	return newWebhook(dynamicCertProvider, secretInformer).start(ctx, l)
@@ -355,7 +355,7 @@ func run() error {
 		kubeinformers.WithNamespace(namespace),
 	)
 
-	dynamicCertProvider := dynamiccert.New("local-user-authenticator-tls-serving-certificate")
+	dynamicCertProvider := dynamiccert.NewServingCert("local-user-authenticator-tls-serving-certificate")
 
 	startControllers(ctx, dynamicCertProvider, client.Kubernetes, kubeInformers)
 	plog.Debug("controllers are ready")

cmd/local-user-authenticator/main_test.go

@@ -458,7 +458,7 @@ func createSecretInformer(ctx context.Context, t *testing.T, kubeClient kubernet
 // newClientProvider returns a dynamiccert.Provider configured
 // with valid serving cert, the CA bundle that can be used to verify the serving
 // cert, and the server name that can be used to verify the TLS peer.
-func newCertProvider(t *testing.T) (dynamiccert.Provider, []byte, string) {
+func newCertProvider(t *testing.T) (dynamiccert.Private, []byte, string) {
 	t.Helper()
 
 	serverName := "local-user-authenticator"
@@ -472,7 +472,7 @@ func newCertProvider(t *testing.T) (dynamiccert.Provider, []byte, string) {
 	certPEM, keyPEM, err := certauthority.ToPEM(cert)
 	require.NoError(t, err)
 
-	certProvider := dynamiccert.New(t.Name())
+	certProvider := dynamiccert.NewServingCert(t.Name())
 	err = certProvider.SetCertKeyContent(certPEM, keyPEM)
 	require.NoError(t, err)
 

internal/certauthority/certauthority.go

@@ -19,6 +19,8 @@ import (
 	"math/big"
 	"net"
 	"time"
+
+	"go.pinniped.dev/internal/constable"
 )
 
 // certBackdate is the amount of time before time.Now() that will be used to set
@@ -71,7 +73,7 @@ func secureEnv() env {
 }
 
 // ErrInvalidCACertificate is returned when the contents of the loaded CA certificate do not meet our assumptions.
-var ErrInvalidCACertificate = fmt.Errorf("invalid CA certificate")
+const ErrInvalidCACertificate = constable.Error("invalid CA certificate")
 
 // Load a certificate authority from an existing certificate and private key (in PEM format).
 func Load(certPEM string, keyPEM string) (*CA, error) {
@@ -82,6 +84,13 @@ func Load(certPEM string, keyPEM string) (*CA, error) {
 	if certCount := len(cert.Certificate); certCount != 1 {
 		return nil, fmt.Errorf("%w: expected a single certificate, found %d certificates", ErrInvalidCACertificate, certCount)
 	}
+	x509Cert, err := x509.ParseCertificate(cert.Certificate[0])
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse key pair as x509 cert: %w", err)
+	}
+	if !x509Cert.IsCA {
+		return nil, fmt.Errorf("%w: passed in key pair is not a CA", ErrInvalidCACertificate)
+	}
 	return &CA{
 		caCertBytes: cert.Certificate[0],
 		signer:      cert.PrivateKey.(crypto.Signer),

internal/certauthority/dynamiccertauthority/dynamiccertauthority.go

@@ -11,25 +11,33 @@ import (
 	"k8s.io/apiserver/pkg/server/dynamiccertificates"
 
 	"go.pinniped.dev/internal/certauthority"
+	"go.pinniped.dev/internal/issuer"
 )
 
-// CA is a type capable of issuing certificates.
+// ca is a type capable of issuing certificates.
-type CA struct {
+type ca struct {
 	provider dynamiccertificates.CertKeyContentProvider
 }
 
-// New creates a new CA, ready to issue certs whenever the provided provider has a keypair to
+// New creates a ClientCertIssuer, ready to issue certs whenever
-// provide.
+// the given CertKeyContentProvider has a keypair to provide.
-func New(provider dynamiccertificates.CertKeyContentProvider) *CA {
+func New(provider dynamiccertificates.CertKeyContentProvider) issuer.ClientCertIssuer {
-	return &CA{
+	return &ca{
 		provider: provider,
 	}
 }
 
+func (c *ca) Name() string {
+	return c.provider.Name()
+}
+
 // IssueClientCertPEM issues a new client certificate for the given identity and duration, returning it as a
 // pair of PEM-formatted byte slices for the certificate and private key.
-func (c *CA) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) {
+func (c *ca) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) {
 	caCrtPEM, caKeyPEM := c.provider.CurrentCertKeyContent()
+	// in the future we could split dynamiccert.Private into two interfaces (Private and PrivateRead)
+	// and have this code take PrivateRead as input.  We would then add ourselves as a listener to
+	// the PrivateRead.  This would allow us to only reload the CA contents when they actually change.
 	ca, err := certauthority.Load(string(caCrtPEM), string(caKeyPEM))
 	if err != nil {
 		return nil, nil, err

internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go

@@ -10,13 +10,14 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"go.pinniped.dev/internal/dynamiccert"
+	"go.pinniped.dev/internal/issuer"
 	"go.pinniped.dev/internal/testutil"
 )
 
 func TestCAIssuePEM(t *testing.T) {
 	t.Parallel()
 
-	provider := dynamiccert.New(t.Name())
+	provider := dynamiccert.NewCA(t.Name())
 	ca := New(provider)
 
 	goodCACrtPEM0, goodCAKeyPEM0, err := testutil.CreateCertificate(
@@ -115,7 +116,7 @@ func TestCAIssuePEM(t *testing.T) {
 	}
 }
 
-func issuePEM(provider dynamiccert.Provider, ca *CA, caCrt, caKey []byte) ([]byte, []byte, error) {
+func issuePEM(provider dynamiccert.Provider, ca issuer.ClientCertIssuer, caCrt, caKey []byte) ([]byte, []byte, error) {
 	// if setting fails, look at that error
 	if caCrt != nil || caKey != nil {
 		if err := provider.SetCertKeyContent(caCrt, caKey); err != nil {

internal/concierge/impersonator/impersonator_test.go

@@ -42,13 +42,13 @@ func TestImpersonator(t *testing.T) {
 	require.NoError(t, err)
 	caKey, err := ca.PrivateKeyToPEM()
 	require.NoError(t, err)
-	caContent := dynamiccert.New("ca")
+	caContent := dynamiccert.NewCA("ca")
 	err = caContent.SetCertKeyContent(ca.Bundle(), caKey)
 	require.NoError(t, err)
 
 	cert, key, err := ca.IssueServerCertPEM(nil, []net.IP{net.ParseIP("127.0.0.1")}, time.Hour)
 	require.NoError(t, err)
-	certKeyContent := dynamiccert.New("cert-key")
+	certKeyContent := dynamiccert.NewServingCert("cert-key")
 	err = certKeyContent.SetCertKeyContent(cert, key)
 	require.NoError(t, err)
 

internal/concierge/server/server.go

@@ -114,15 +114,15 @@ func (a *App) runServer(ctx context.Context) error {
 	// is stored in a k8s Secret. Therefore it also effectively acting as
 	// an in-memory cache of what is stored in the k8s Secret, helping to
 	// keep incoming requests fast.
-	dynamicServingCertProvider := dynamiccert.New("concierge-serving-cert")
+	dynamicServingCertProvider := dynamiccert.NewServingCert("concierge-serving-cert")
 
 	// This cert provider will be used to provide the Kube signing key to the
 	// cert issuer used to issue certs to Pinniped clients wishing to login.
-	dynamicSigningCertProvider := dynamiccert.New("concierge-kube-signing-cert")
+	dynamicSigningCertProvider := dynamiccert.NewCA("concierge-kube-signing-cert")
 
 	// This cert provider will be used to provide the impersonation proxy signing key to the
 	// cert issuer used to issue certs to Pinniped clients wishing to login.
-	impersonationProxySigningCertProvider := dynamiccert.New("impersonation-proxy-signing-cert")
+	impersonationProxySigningCertProvider := dynamiccert.NewCA("impersonation-proxy-signing-cert")
 
 	// Get the "real" name of the login concierge API group (i.e., the API group name with the
 	// injected suffix).
@@ -182,7 +182,7 @@ func (a *App) runServer(ctx context.Context) error {
 
 // Create a configuration for the aggregated API server.
 func getAggregatedAPIServerConfig(
-	dynamicCertProvider dynamiccert.Provider,
+	dynamicCertProvider dynamiccert.Private,
 	authenticator credentialrequest.TokenCredentialRequestAuthenticator,
 	issuer issuer.ClientCertIssuer,
 	startControllersPostStartHook func(context.Context),

internal/controller/apicerts/certs_observer.go

@@ -18,14 +18,14 @@ import (
 type certsObserverController struct {
 	namespace               string
 	certsSecretResourceName string
-	dynamicCertProvider     dynamiccert.Provider
+	dynamicCertProvider     dynamiccert.Private
 	secretInformer          corev1informers.SecretInformer
 }
 
 func NewCertsObserverController(
 	namespace string,
 	certsSecretResourceName string,
-	dynamicCertProvider dynamiccert.Provider,
+	dynamicCertProvider dynamiccert.Private,
 	secretInformer corev1informers.SecretInformer,
 	withInformer pinnipedcontroller.WithInformerOptionFunc,
 ) controllerlib.Controller {

internal/controller/apicerts/certs_observer_test.go

@@ -17,6 +17,7 @@ import (
 	kubeinformers "k8s.io/client-go/informers"
 	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 
+	"go.pinniped.dev/internal/certauthority"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/dynamiccert"
 	"go.pinniped.dev/internal/testutil"
@@ -109,7 +110,7 @@ func TestObserverControllerSync(t *testing.T) {
 		var cancelContext context.Context
 		var cancelContextCancelFunc context.CancelFunc
 		var syncContext *controllerlib.Context
-		var dynamicCertProvider dynamiccert.Provider
+		var dynamicCertProvider dynamiccert.Private
 
 		// Defer starting the informers until the last possible moment so that the
 		// nested Before's can keep adding things to the informer caches.
@@ -145,7 +146,7 @@ func TestObserverControllerSync(t *testing.T) {
 
 			kubeInformerClient = kubernetesfake.NewSimpleClientset()
 			kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0)
-			dynamicCertProvider = dynamiccert.New(name)
+			dynamicCertProvider = dynamiccert.NewServingCert(name)
 		})
 
 		it.After(func() {
@@ -163,12 +164,18 @@ func TestObserverControllerSync(t *testing.T) {
 				err := kubeInformerClient.Tracker().Add(unrelatedSecret)
 				r.NoError(err)
 
-				crt, key, err := testutil.CreateCertificate(
+				caCrt, caKey, err := testutil.CreateCertificate(
 					time.Now().Add(-time.Hour),
 					time.Now().Add(time.Hour),
 				)
 				require.NoError(t, err)
 
+				ca, err := certauthority.Load(string(caCrt), string(caKey))
+				require.NoError(t, err)
+
+				crt, key, err := ca.IssueServerCertPEM(nil, nil, time.Hour)
+				require.NoError(t, err)
+
 				err = dynamicCertProvider.SetCertKeyContent(crt, key)
 				r.NoError(err)
 			})
@@ -186,12 +193,18 @@ func TestObserverControllerSync(t *testing.T) {
 
 		when("there is a serving cert Secret with the expected keys already in the installation namespace", func() {
 			it.Before(func() {
-				crt, key, err := testutil.CreateCertificate(
+				caCrt, caKey, err := testutil.CreateCertificate(
 					time.Now().Add(-time.Hour),
 					time.Now().Add(time.Hour),
 				)
 				require.NoError(t, err)
 
+				ca, err := certauthority.Load(string(caCrt), string(caKey))
+				require.NoError(t, err)
+
+				crt, key, err := ca.IssueServerCertPEM(nil, nil, time.Hour)
+				require.NoError(t, err)
+
 				apiServingCertSecret := &corev1.Secret{
 					ObjectMeta: metav1.ObjectMeta{
 						Name:      certsSecretResourceName,

internal/controller/impersonatorconfig/impersonator_config.go

@@ -72,7 +72,7 @@ type impersonatorConfigController struct {
 	hasControlPlaneNodes              *bool
 	serverStopCh                      chan struct{}
 	errorCh                           chan error
-	tlsServingCertDynamicCertProvider dynamiccert.Provider
+	tlsServingCertDynamicCertProvider dynamiccert.Private
 }
 
 func NewImpersonatorConfigController(
@@ -116,7 +116,7 @@ func NewImpersonatorConfigController(
 				clock:                             clock,
 				impersonationSigningCertProvider:  impersonationSigningCertProvider,
 				impersonatorFunc:                  impersonatorFunc,
-				tlsServingCertDynamicCertProvider: dynamiccert.New("impersonation-proxy-serving-cert"),
+				tlsServingCertDynamicCertProvider: dynamiccert.NewServingCert("impersonation-proxy-serving-cert"),
 			},
 		},
 		withInformer(

internal/controller/impersonatorconfig/impersonator_config_test.go

@@ -974,7 +974,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) {
 			kubeAPIClient = kubernetesfake.NewSimpleClientset()
 			pinnipedAPIClient = pinnipedfake.NewSimpleClientset()
 			frozenNow = time.Date(2021, time.March, 2, 7, 42, 0, 0, time.Local)
-			signingCertProvider = dynamiccert.New(name)
+			signingCertProvider = dynamiccert.NewCA(name)
 
 			ca := newCA()
 			signingCACertPEM = ca.Bundle()

internal/controller/kubecertagent/execer.go

@@ -33,7 +33,7 @@ type execerController struct {
 	credentialIssuerLocationConfig *CredentialIssuerLocationConfig
 	credentialIssuerLabels         map[string]string
 	discoveryURLOverride           *string
-	dynamicCertProvider            dynamiccert.Provider
+	dynamicCertProvider            dynamiccert.Private
 	podCommandExecutor             PodCommandExecutor
 	clock                          clock.Clock
 	pinnipedAPIClient              pinnipedclientset.Interface
@@ -51,7 +51,7 @@ func NewExecerController(
 	credentialIssuerLocationConfig *CredentialIssuerLocationConfig,
 	credentialIssuerLabels map[string]string,
 	discoveryURLOverride *string,
-	dynamicCertProvider dynamiccert.Provider,
+	dynamicCertProvider dynamiccert.Private,
 	podCommandExecutor PodCommandExecutor,
 	pinnipedAPIClient pinnipedclientset.Interface,
 	clock clock.Clock,

internal/controller/kubecertagent/execer_test.go

@@ -243,7 +243,7 @@ func TestManagerControllerSync(t *testing.T) {
 			kubeInformerFactory = kubeinformers.NewSharedInformerFactory(kubeClientset, 0)
 			fakeExecutor = &fakePodExecutor{r: r}
 			frozenNow = time.Date(2020, time.September, 23, 7, 42, 0, 0, time.Local)
-			dynamicCertProvider = dynamiccert.New(name)
+			dynamicCertProvider = dynamiccert.NewCA(name)
 			err = dynamicCertProvider.SetCertKeyContent([]byte(defaultDynamicCertProviderCert), []byte(defaultDynamicCertProviderKey))
 			r.NoError(err)
 

internal/controllermanager/prepare_controllers.go

@@ -63,12 +63,12 @@ type Config struct {
 	DiscoveryURLOverride *string
 
 	// DynamicServingCertProvider provides a setter and a getter to the Pinniped API's serving cert.
-	DynamicServingCertProvider dynamiccert.Provider
+	DynamicServingCertProvider dynamiccert.Private
 
 	// DynamicSigningCertProvider provides a setter and a getter to the Pinniped API's
 	// signing cert, i.e., the cert that it uses to sign certs for Pinniped clients wishing to login.
 	// This is filled with the Kube API server's signing cert by a controller, if it can be found.
-	DynamicSigningCertProvider dynamiccert.Provider
+	DynamicSigningCertProvider dynamiccert.Private
 
 	// ImpersonationSigningCertProvider provides a setter and a getter to the CA cert that should be
 	// used to sign client certs for authentication to the impersonation proxy. This CA is used by

internal/dynamiccert/provider.go

@@ -10,6 +10,8 @@ import (
 	"sync"
 
 	"k8s.io/apiserver/pkg/server/dynamiccertificates"
+
+	"go.pinniped.dev/internal/plog"
 )
 
 type Provider interface {
@@ -36,8 +38,12 @@ type notifier interface {
 	dynamiccertificates.ControllerRunner // we do not need this today, but it could grow and change in the future
 }
 
+var _ Provider = &provider{}
+
 type provider struct {
+	// these fields are constant after struct initialization and thus do not need locking
 	name string
+	isCA bool
 
 	// mutex guards all the fields below it
 	mutex     sync.RWMutex
@@ -46,13 +52,20 @@ type provider struct {
 	listeners []dynamiccertificates.Listener
 }
 
-// New returns an empty Provider. The returned Provider is thread-safe.
+// NewServingCert returns a Private that is go routine safe.
-func New(name string) Provider {
+// It can only hold key pairs that have IsCA=false.
+func NewServingCert(name string) Private {
 	return &provider{name: name}
 }
 
+// NewCA returns a Provider that is go routine safe.
+// It can only hold key pairs that have IsCA=true.
+func NewCA(name string) Provider {
+	return &provider{name: name, isCA: true}
+}
+
 func (p *provider) Name() string {
-	return p.name // constant after struct initialization and thus does not need locking
+	return p.name
 }
 
 func (p *provider) CurrentCertKeyContent() (cert []byte, key []byte) {
@@ -65,10 +78,25 @@ func (p *provider) CurrentCertKeyContent() (cert []byte, key []byte) {
 func (p *provider) SetCertKeyContent(certPEM, keyPEM []byte) error {
 	// always make sure that we have valid PEM data, otherwise
 	// dynamiccertificates.NewUnionCAContentProvider.VerifyOptions will panic
-	if _, err := tls.X509KeyPair(certPEM, keyPEM); err != nil {
+	cert, err := tls.X509KeyPair(certPEM, keyPEM)
+	if err != nil {
 		return fmt.Errorf("%s: attempt to set invalid key pair: %w", p.name, err)
 	}
 
+	// these checks should always pass if tls.X509KeyPair did not error
+	if len(cert.Certificate) == 0 {
+		return fmt.Errorf("%s: key pair has empty cert slice", p.name)
+	}
+	x509Cert, err := x509.ParseCertificate(cert.Certificate[0])
+	if err != nil {
+		return fmt.Errorf("%s: failed to parse key pair as x509 cert: %w", p.name, err)
+	}
+
+	// confirm that we are not trying to use a CA as a serving cert and vice versa
+	if p.isCA != x509Cert.IsCA {
+		return fmt.Errorf("%s: attempt to set x509 cert with unexpected IsCA=%v", p.name, x509Cert.IsCA)
+	}
+
 	p.setCertKeyContent(certPEM, keyPEM)
 
 	return nil
@@ -85,17 +113,27 @@ func (p *provider) setCertKeyContent(certPEM, keyPEM []byte) {
 	p.certPEM = certPEM
 	p.keyPEM = keyPEM
 
+	// technically this only reads a read lock but we already have the write lock
 	for _, listener := range p.listeners {
 		listener.Enqueue()
 	}
 }
 
 func (p *provider) CurrentCABundleContent() []byte {
+	if !p.isCA {
+		panic("*provider from NewServingCert was cast into wrong CA interface")
+	}
+
 	ca, _ := p.CurrentCertKeyContent()
 	return ca
 }
 
 func (p *provider) VerifyOptions() (x509.VerifyOptions, bool) {
+	if !p.isCA {
+		panic("*provider from NewServingCert was cast into wrong CA interface")
+	}
+
+	plog.Warning("unexpected call to *provider.VerifyOptions; CA union logic is broken")
 	return x509.VerifyOptions{}, false // assume we are unioned via dynamiccertificates.NewUnionCAContentProvider
 }
 

internal/issuer/issuer.go

@@ -4,6 +4,8 @@
 package issuer
 
 import (
+	"fmt"
+	"strings"
 	"time"
 
 	"k8s.io/apimachinery/pkg/util/errors"
@@ -14,6 +16,7 @@ import (
 const defaultCertIssuerErr = constable.Error("failed to issue cert")
 
 type ClientCertIssuer interface {
+	Name() string
 	IssueClientCertPEM(username string, groups []string, ttl time.Duration) (certPEM, keyPEM []byte, err error)
 }
 
@@ -21,13 +24,26 @@ var _ ClientCertIssuer = ClientCertIssuers{}
 
 type ClientCertIssuers []ClientCertIssuer
 
+func (c ClientCertIssuers) Name() string {
+	if len(c) == 0 {
+		return "empty-client-cert-issuers"
+	}
+
+	names := make([]string, 0, len(c))
+	for _, issuer := range c {
+		names = append(names, issuer.Name())
+	}
+
+	return strings.Join(names, ",")
+}
+
 func (c ClientCertIssuers) IssueClientCertPEM(username string, groups []string, ttl time.Duration) ([]byte, []byte, error) {
 	var errs []error
 
 	for _, issuer := range c {
 		certPEM, keyPEM, err := issuer.IssueClientCertPEM(username, groups, ttl)
 		if err != nil {
-			errs = append(errs, err)
+			errs = append(errs, fmt.Errorf("%s failed to issue client cert: %w", issuer.Name(), err))
 			continue
 		}
 		return certPEM, keyPEM, nil

internal/mocks/issuermocks/issuermocks.go

@@ -53,3 +53,17 @@ func (mr *MockClientCertIssuerMockRecorder) IssueClientCertPEM(arg0, arg1, arg2
 	mr.mock.ctrl.T.Helper()
 	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IssueClientCertPEM", reflect.TypeOf((*MockClientCertIssuer)(nil).IssueClientCertPEM), arg0, arg1, arg2)
 }
+
+// Name mocks base method.
+func (m *MockClientCertIssuer) Name() string {
+	m.ctrl.T.Helper()
+	ret := m.ctrl.Call(m, "Name")
+	ret0, _ := ret[0].(string)
+	return ret0
+}
+
+// Name indicates an expected call of Name.
+func (mr *MockClientCertIssuerMockRecorder) Name() *gomock.Call {
+	mr.mock.ctrl.T.Helper()
+	return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Name", reflect.TypeOf((*MockClientCertIssuer)(nil).Name))
+}