From 2d28d1da19e22d6adca60c39ee6916426e229c50 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 11 Mar 2021 16:20:25 -0500 Subject: [PATCH] Implement all optional methods in dynamic certs provider Signed-off-by: Monis Khan --- cmd/local-user-authenticator/main.go | 2 +- cmd/local-user-authenticator/main_test.go | 5 +- .../dynamiccertauthority_test.go | 42 ++++--- .../concierge/impersonator/impersonator.go | 16 +-- .../impersonator/impersonator_test.go | 13 ++- internal/concierge/server/server.go | 6 +- .../controller/apicerts/certs_observer.go | 7 +- .../apicerts/certs_observer_test.go | 52 ++++++--- .../impersonatorconfig/impersonator_config.go | 25 +++-- .../impersonator_config_test.go | 14 +-- internal/controller/kubecertagent/execer.go | 27 +++-- .../controller/kubecertagent/execer_test.go | 67 +++++++++++- internal/dynamiccert/provider.go | 103 +++++++++++++----- 13 files changed, 268 insertions(+), 111 deletions(-) diff --git a/cmd/local-user-authenticator/main.go b/cmd/local-user-authenticator/main.go index 379cde21..f4f2249d 100644 --- a/cmd/local-user-authenticator/main.go +++ b/cmd/local-user-authenticator/main.go @@ -355,7 +355,7 @@ func run() error { kubeinformers.WithNamespace(namespace), ) - dynamicCertProvider := dynamiccert.New() + dynamicCertProvider := dynamiccert.New("local-user-authenticator-tls-serving-certificate") startControllers(ctx, dynamicCertProvider, client.Kubernetes, kubeInformers) plog.Debug("controllers are ready") diff --git a/cmd/local-user-authenticator/main_test.go b/cmd/local-user-authenticator/main_test.go index b755f1ac..632fbde0 100644 --- a/cmd/local-user-authenticator/main_test.go +++ b/cmd/local-user-authenticator/main_test.go @@ -473,8 +473,9 @@ func newCertProvider(t *testing.T) (dynamiccert.Provider, []byte, string) { certPEM, keyPEM, err := certauthority.ToPEM(cert) require.NoError(t, err) - certProvider := dynamiccert.New() - certProvider.Set(certPEM, keyPEM) + certProvider := dynamiccert.New(t.Name()) + err = certProvider.SetCertKeyContent(certPEM, keyPEM) + require.NoError(t, err) return certProvider, ca.Bundle(), serverName } diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go index 5482af71..41e0f291 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go @@ -17,7 +17,7 @@ import ( func TestCAIssuePEM(t *testing.T) { t.Parallel() - provider := dynamiccert.New() + provider := dynamiccert.New(t.Name()) ca := New(provider) goodCACrtPEM0, goodCAKeyPEM0, err := testutil.CreateCertificate( @@ -44,12 +44,12 @@ func TestCAIssuePEM(t *testing.T) { { name: "only cert", caCrtPEM: goodCACrtPEM0, - wantError: "could not load CA: tls: failed to find any PEM data in key input", + wantError: "TestCAIssuePEM: attempt to set invalid key pair: tls: failed to find any PEM data in key input", }, { name: "only key", caKeyPEM: goodCAKeyPEM0, - wantError: "could not load CA: tls: failed to find any PEM data in certificate input", + wantError: "TestCAIssuePEM: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input", }, { name: "new cert+key", @@ -68,19 +68,19 @@ func TestCAIssuePEM(t *testing.T) { name: "bad cert", caCrtPEM: []byte("this is not a cert"), caKeyPEM: goodCAKeyPEM0, - wantError: "could not load CA: tls: failed to find any PEM data in certificate input", + wantError: "TestCAIssuePEM: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input", }, { name: "bad key", caCrtPEM: goodCACrtPEM0, caKeyPEM: []byte("this is not a key"), - wantError: "could not load CA: tls: failed to find any PEM data in key input", + wantError: "TestCAIssuePEM: attempt to set invalid key pair: tls: failed to find any PEM data in key input", }, { name: "mismatch cert+key", caCrtPEM: goodCACrtPEM0, caKeyPEM: goodCAKeyPEM1, - wantError: "could not load CA: tls: private key does not match public key", + wantError: "TestCAIssuePEM: attempt to set invalid key pair: tls: private key does not match public key", }, { name: "good cert+key again", @@ -94,17 +94,7 @@ func TestCAIssuePEM(t *testing.T) { // Can't run these steps in parallel, because each one depends on the previous steps being // run. - if step.caCrtPEM != nil || step.caKeyPEM != nil { - provider.Set(step.caCrtPEM, step.caKeyPEM) - } - - crtPEM, keyPEM, err := ca.IssuePEM( - pkix.Name{ - CommonName: "some-common-name", - }, - []string{"some-dns-name", "some-other-dns-name"}, - time.Hour*24, - ) + crtPEM, keyPEM, err := issuePEM(provider, ca, step.caCrtPEM, step.caKeyPEM) if step.wantError != "" { require.EqualError(t, err, step.wantError) @@ -126,3 +116,21 @@ func TestCAIssuePEM(t *testing.T) { }) } } + +func issuePEM(provider dynamiccert.Provider, ca *CA, 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 { + return nil, nil, err + } + } + + // otherwise check to see if their is an issuing error + return ca.IssuePEM( + pkix.Name{ + CommonName: "some-common-name", + }, + []string{"some-dns-name", "some-other-dns-name"}, + time.Hour*24, + ) +} diff --git a/internal/concierge/impersonator/impersonator.go b/internal/concierge/impersonator/impersonator.go index e6b5d5fe..a14e87bc 100644 --- a/internal/concierge/impersonator/impersonator.go +++ b/internal/concierge/impersonator/impersonator.go @@ -12,11 +12,10 @@ import ( "strings" "time" - "k8s.io/apimachinery/pkg/util/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/endpoints/request" @@ -27,6 +26,7 @@ import ( "k8s.io/client-go/transport" "go.pinniped.dev/internal/constable" + "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/plog" @@ -39,22 +39,22 @@ import ( // Instead, call the factory function again to get a new start function. type FactoryFunc func( port int, - dynamicCertProvider dynamiccertificates.CertKeyContentProvider, - impersonationProxySignerCA dynamiccertificates.CAContentProvider, + dynamicCertProvider dynamiccert.Private, + impersonationProxySignerCA dynamiccert.Public, ) (func(stopCh <-chan struct{}) error, error) func New( port int, - dynamicCertProvider dynamiccertificates.CertKeyContentProvider, // TODO: we need to check those optional interfaces and see what we need to implement - impersonationProxySignerCA dynamiccertificates.CAContentProvider, // TODO: we need to check those optional interfaces and see what we need to implement + dynamicCertProvider dynamiccert.Private, + impersonationProxySignerCA dynamiccert.Public, ) (func(stopCh <-chan struct{}) error, error) { return newInternal(port, dynamicCertProvider, impersonationProxySignerCA, nil, nil) } func newInternal( //nolint:funlen // yeah, it's kind of long. port int, - dynamicCertProvider dynamiccertificates.CertKeyContentProvider, - impersonationProxySignerCA dynamiccertificates.CAContentProvider, + dynamicCertProvider dynamiccert.Private, + impersonationProxySignerCA dynamiccert.Public, clientOpts []kubeclient.Option, // for unit testing, should always be nil in production recOpts func(*genericoptions.RecommendedOptions), // for unit testing, should always be nil in production ) (func(stopCh <-chan struct{}) error, error) { diff --git a/internal/concierge/impersonator/impersonator_test.go b/internal/concierge/impersonator/impersonator_test.go index 31e9aae9..66cbd3d3 100644 --- a/internal/concierge/impersonator/impersonator_test.go +++ b/internal/concierge/impersonator/impersonator_test.go @@ -18,7 +18,6 @@ import ( "k8s.io/apiserver/pkg/authentication/user" "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/features" - "k8s.io/apiserver/pkg/server/dynamiccertificates" genericoptions "k8s.io/apiserver/pkg/server/options" utilfeature "k8s.io/apiserver/pkg/util/feature" "k8s.io/client-go/rest" @@ -26,6 +25,7 @@ import ( featuregatetesting "k8s.io/component-base/featuregate/testing" "go.pinniped.dev/internal/certauthority" + "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/testutil" ) @@ -35,11 +35,16 @@ func TestNew(t *testing.T) { ca, err := certauthority.New(pkix.Name{CommonName: "ca"}, time.Hour) require.NoError(t, err) + caKey, err := ca.PrivateKeyToPEM() + require.NoError(t, err) + caContent := dynamiccert.New("ca") + err = caContent.SetCertKeyContent(ca.Bundle(), caKey) + require.NoError(t, err) + cert, key, err := ca.IssuePEM(pkix.Name{CommonName: "example.com"}, []string{"example.com"}, time.Hour) require.NoError(t, err) - certKeyContent, err := dynamiccertificates.NewStaticCertKeyContent("cert-key", cert, key) - require.NoError(t, err) - caContent, err := dynamiccertificates.NewStaticCAContent("ca", ca.Bundle()) + certKeyContent := dynamiccert.New("cert-key") + err = certKeyContent.SetCertKeyContent(cert, key) require.NoError(t, err) // Punch out just enough stuff to make New actually run without error. diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index d485ee19..8602d07d 100644 --- a/internal/concierge/server/server.go +++ b/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() + dynamicServingCertProvider := dynamiccert.New("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() + dynamicSigningCertProvider := dynamiccert.New("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() + impersonationProxySigningCertProvider := dynamiccert.New("impersonation-proxy-signing-cert") // Get the "real" name of the login concierge API group (i.e., the API group name with the // injected suffix). diff --git a/internal/controller/apicerts/certs_observer.go b/internal/controller/apicerts/certs_observer.go index c6380741..c8b6f2b8 100644 --- a/internal/controller/apicerts/certs_observer.go +++ b/internal/controller/apicerts/certs_observer.go @@ -57,12 +57,15 @@ func (c *certsObserverController) Sync(_ controllerlib.Context) error { if notFound { klog.Info("certsObserverController Sync found that the secret does not exist yet or was deleted") // The secret does not exist yet or was deleted. - c.dynamicCertProvider.Set(nil, nil) + c.dynamicCertProvider.UnsetCertKeyContent() return nil } // Mutate the in-memory cert provider to update with the latest cert values. - c.dynamicCertProvider.Set(certSecret.Data[TLSCertificateChainSecretKey], certSecret.Data[tlsPrivateKeySecretKey]) + if err := c.dynamicCertProvider.SetCertKeyContent(certSecret.Data[TLSCertificateChainSecretKey], certSecret.Data[tlsPrivateKeySecretKey]); err != nil { + return fmt.Errorf("failed to set serving cert/key content from secret %s/%s: %w", c.namespace, c.certsSecretResourceName, err) + } + klog.Info("certsObserverController Sync updated certs in the dynamic cert provider") return nil } diff --git a/internal/controller/apicerts/certs_observer_test.go b/internal/controller/apicerts/certs_observer_test.go index bd1540cc..553e66f3 100644 --- a/internal/controller/apicerts/certs_observer_test.go +++ b/internal/controller/apicerts/certs_observer_test.go @@ -5,7 +5,9 @@ package apicerts import ( "context" + "strings" "testing" + "time" "github.com/sclevine/spec" "github.com/sclevine/spec/report" @@ -94,6 +96,7 @@ func TestObserverControllerInformerFilters(t *testing.T) { } func TestObserverControllerSync(t *testing.T) { + name := t.Name() spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { const installedInNamespace = "some-namespace" const certsSecretResourceName = "some-resource-name" @@ -142,7 +145,7 @@ func TestObserverControllerSync(t *testing.T) { kubeInformerClient = kubernetesfake.NewSimpleClientset() kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0) - dynamicCertProvider = dynamiccert.New() + dynamicCertProvider = dynamiccert.New(name) }) it.After(func() { @@ -160,7 +163,14 @@ func TestObserverControllerSync(t *testing.T) { err := kubeInformerClient.Tracker().Add(unrelatedSecret) r.NoError(err) - dynamicCertProvider.Set([]byte("some cert"), []byte("some private key")) + crt, key, err := testutil.CreateCertificate( + time.Now().Add(-time.Hour), + time.Now().Add(time.Hour), + ) + require.NoError(t, err) + + err = dynamicCertProvider.SetCertKeyContent(crt, key) + r.NoError(err) }) it("sets the dynamicCertProvider's cert and key to nil", func() { @@ -176,6 +186,12 @@ 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( + time.Now().Add(-time.Hour), + time.Now().Add(time.Hour), + ) + require.NoError(t, err) + apiServingCertSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: certsSecretResourceName, @@ -183,24 +199,29 @@ func TestObserverControllerSync(t *testing.T) { }, Data: map[string][]byte{ "caCertificate": []byte("fake cert"), - "tlsPrivateKey": []byte("fake private key"), - "tlsCertificateChain": []byte("fake cert chain"), + "tlsPrivateKey": key, + "tlsCertificateChain": crt, }, } - err := kubeInformerClient.Tracker().Add(apiServingCertSecret) + err = kubeInformerClient.Tracker().Add(apiServingCertSecret) r.NoError(err) - dynamicCertProvider.Set(nil, nil) + dynamicCertProvider.UnsetCertKeyContent() }) it("updates the dynamicCertProvider's cert and key", func() { startInformersAndController() + + actualCertChain, actualKey := dynamicCertProvider.CurrentCertKeyContent() + r.Nil(actualCertChain) + r.Nil(actualKey) + err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - actualCertChain, actualKey := dynamicCertProvider.CurrentCertKeyContent() - r.Equal("fake cert chain", string(actualCertChain)) - r.Equal("fake private key", string(actualKey)) + 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)) }) }) @@ -216,17 +237,22 @@ func TestObserverControllerSync(t *testing.T) { err := kubeInformerClient.Tracker().Add(apiServingCertSecret) r.NoError(err) - dynamicCertProvider.Set(nil, nil) + dynamicCertProvider.UnsetCertKeyContent() }) - it("set the missing values in the dynamicCertProvider as nil", func() { + it("returns an error and does not change the dynamicCertProvider", func() { startInformersAndController() - err := controllerlib.TestSync(t, subject, *syncContext) - r.NoError(err) actualCertChain, actualKey := dynamicCertProvider.CurrentCertKeyContent() r.Nil(actualCertChain) r.Nil(actualKey) + + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, "failed to set serving cert/key content from secret some-namespace/some-resource-name: TestObserverControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input") + + actualCertChain, actualKey = dynamicCertProvider.CurrentCertKeyContent() + r.Nil(actualCertChain) + r.Nil(actualKey) }) }) }, spec.Parallel(), spec.Report(report.Terminal{})) diff --git a/internal/controller/impersonatorconfig/impersonator_config.go b/internal/controller/impersonatorconfig/impersonator_config.go index 389905d7..6f81901d 100644 --- a/internal/controller/impersonatorconfig/impersonator_config.go +++ b/internal/controller/impersonatorconfig/impersonator_config.go @@ -117,7 +117,7 @@ func NewImpersonatorConfigController( clock: clock, impersonationSigningCertProvider: impersonationSigningCertProvider, impersonatorFunc: impersonatorFunc, - tlsServingCertDynamicCertProvider: dynamiccert.New(), + tlsServingCertDynamicCertProvider: dynamiccert.New("impersonation-proxy-serving-cert"), }, }, withInformer( @@ -238,7 +238,7 @@ func (c *impersonatorConfigController) doSync(syncCtx controllerlib.Context) (*v nameInfo, err := c.findDesiredTLSCertificateName(config) if err != nil { // Unexpected error while determining the name that should go into the certs, so clear any existing certs. - c.tlsServingCertDynamicCertProvider.Set(nil, nil) + c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent() return nil, err } @@ -371,7 +371,7 @@ func (c *impersonatorConfigController) ensureImpersonatorIsStarted(syncCtx contr startImpersonatorFunc, err := c.impersonatorFunc( impersonationProxyPort, c.tlsServingCertDynamicCertProvider, - dynamiccert.NewCAProvider(c.impersonationSigningCertProvider), + c.impersonationSigningCertProvider, ) if err != nil { return err @@ -750,16 +750,17 @@ func (c *impersonatorConfigController) createNewTLSSecret(ctx context.Context, c func (c *impersonatorConfigController) loadTLSCertFromSecret(tlsSecret *v1.Secret) error { certPEM := tlsSecret.Data[v1.TLSCertKey] keyPEM := tlsSecret.Data[v1.TLSPrivateKeyKey] - _, err := tls.X509KeyPair(certPEM, keyPEM) - if err != nil { - c.tlsServingCertDynamicCertProvider.Set(nil, nil) + + if err := c.tlsServingCertDynamicCertProvider.SetCertKeyContent(certPEM, keyPEM); err != nil { + c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent() return fmt.Errorf("could not parse TLS cert PEM data from Secret: %w", err) } + plog.Info("Loading TLS certificates for impersonation proxy", "certPEM", string(certPEM), "secret", c.tlsSecretName, "namespace", c.namespace) - c.tlsServingCertDynamicCertProvider.Set(certPEM, keyPEM) + return nil } @@ -779,7 +780,7 @@ func (c *impersonatorConfigController) ensureTLSSecretIsRemoved(ctx context.Cont return err } - c.tlsServingCertDynamicCertProvider.Set(nil, nil) + c.tlsServingCertDynamicCertProvider.UnsetCertKeyContent() return nil } @@ -798,8 +799,8 @@ func (c *impersonatorConfigController) loadSignerCA(status v1alpha1.StrategyStat certPEM := signingCertSecret.Data[apicerts.CACertificateSecretKey] keyPEM := signingCertSecret.Data[apicerts.CACertificatePrivateKeySecretKey] - _, err = tls.X509KeyPair(certPEM, keyPEM) - if err != nil { + + if err := c.impersonationSigningCertProvider.SetCertKeyContent(certPEM, keyPEM); err != nil { return fmt.Errorf("could not load the impersonator's credential signing secret: %w", err) } @@ -807,13 +808,13 @@ func (c *impersonatorConfigController) loadSignerCA(status v1alpha1.StrategyStat "certPEM", string(certPEM), "fromSecret", c.impersonationSignerSecretName, "namespace", c.namespace) - c.impersonationSigningCertProvider.Set(certPEM, keyPEM) + return nil } func (c *impersonatorConfigController) clearSignerCA() { plog.Info("Clearing credential signing certificate for impersonation proxy") - c.impersonationSigningCertProvider.Set(nil, nil) + c.impersonationSigningCertProvider.UnsetCertKeyContent() } func (c *impersonatorConfigController) doSyncResult(nameInfo *certNameInfo, config *impersonator.Config, ca *certauthority.CA) *v1alpha1.CredentialIssuerStrategy { diff --git a/internal/controller/impersonatorconfig/impersonator_config_test.go b/internal/controller/impersonatorconfig/impersonator_config_test.go index 60d029fb..f926208b 100644 --- a/internal/controller/impersonatorconfig/impersonator_config_test.go +++ b/internal/controller/impersonatorconfig/impersonator_config_test.go @@ -30,7 +30,6 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/clock" - "k8s.io/apiserver/pkg/server/dynamiccertificates" kubeinformers "k8s.io/client-go/informers" kubernetesfake "k8s.io/client-go/kubernetes/fake" coretesting "k8s.io/client-go/testing" @@ -266,6 +265,7 @@ func TestImpersonatorConfigControllerOptions(t *testing.T) { } func TestImpersonatorConfigControllerSync(t *testing.T) { + name := t.Name() spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { const installedInNamespace = "some-namespace" const configMapResourceName = "some-configmap-resource-name" @@ -306,8 +306,8 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { var impersonatorFunc = func( port int, - dynamicCertProvider dynamiccertificates.CertKeyContentProvider, - impersonationProxySignerCAProvider dynamiccertificates.CAContentProvider, + dynamicCertProvider dynamiccert.Private, + impersonationProxySignerCAProvider dynamiccert.Public, ) (func(stopCh <-chan struct{}) error, error) { impersonatorFuncWasCalled++ r.Equal(8444, port) @@ -972,7 +972,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() + signingCertProvider = dynamiccert.New(name) ca := newCA() signingCACertPEM = ca.Bundle() @@ -2408,7 +2408,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it("returns the error", func() { startInformersAndController() - errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input` + errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input` r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) requireSigningCertProviderIsEmpty() @@ -2423,7 +2423,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { it("returns the error", func() { startInformersAndController() - errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input` + errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input` r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) requireSigningCertProviderIsEmpty() @@ -2459,7 +2459,7 @@ func TestImpersonatorConfigControllerSync(t *testing.T) { addSecretToTrackers(updatedSigner, kubeInformerClient) waitForObjectToAppearInInformer(updatedSigner, kubeInformers.Core().V1().Secrets()) - errString := `could not load the impersonator's credential signing secret: tls: failed to find any PEM data in certificate input` + errString := `could not load the impersonator's credential signing secret: TestImpersonatorConfigControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in certificate input` r.EqualError(runControllerSync(), errString) requireCredentialIssuer(newErrorStrategy(errString)) requireSigningCertProviderIsEmpty() diff --git a/internal/controller/kubecertagent/execer.go b/internal/controller/kubecertagent/execer.go index b8ff1c83..691af1c4 100644 --- a/internal/controller/kubecertagent/execer.go +++ b/internal/controller/kubecertagent/execer.go @@ -11,9 +11,9 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/clock" + "k8s.io/apimachinery/pkg/util/errors" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/tools/clientcmd" - "k8s.io/klog/v2" configv1alpha1 "go.pinniped.dev/generated/latest/apis/concierge/config/v1alpha1" pinnipedclientset "go.pinniped.dev/generated/latest/client/concierge/clientset/versioned" @@ -119,8 +119,7 @@ func (c *execerController) Sync(ctx controllerlib.Context) error { c.pinnipedAPIClient, strategyError(c.clock, err), ) - klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuer with strategy success") - return err + return newAggregate(err, strategyResultUpdateErr) } keyPEM, err := c.podCommandExecutor.Exec(agentPod.Namespace, agentPod.Name, "cat", keyPath) @@ -132,11 +131,20 @@ func (c *execerController) Sync(ctx controllerlib.Context) error { c.pinnipedAPIClient, strategyError(c.clock, err), ) - klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuer with strategy success") - return err + return newAggregate(err, strategyResultUpdateErr) } - c.dynamicCertProvider.Set([]byte(certPEM), []byte(keyPEM)) + if err := c.dynamicCertProvider.SetCertKeyContent([]byte(certPEM), []byte(keyPEM)); err != nil { + err = fmt.Errorf("failed to set signing cert/key content from agent pod %s/%s: %w", agentPod.Namespace, agentPod.Name, err) + strategyResultUpdateErr := issuerconfig.UpdateStrategy( + ctx.Context, + c.credentialIssuerLocationConfig.Name, + c.credentialIssuerLabels, + c.pinnipedAPIClient, + strategyError(c.clock, err), + ) + return newAggregate(err, strategyResultUpdateErr) + } apiInfo, err := c.getTokenCredentialRequestAPIInfo() if err != nil { @@ -153,8 +161,7 @@ func (c *execerController) Sync(ctx controllerlib.Context) error { LastUpdateTime: metav1.NewTime(c.clock.Now()), }, ) - klog.ErrorS(strategyResultUpdateErr, "could not create or update CredentialIssuer with strategy success") - return err + return newAggregate(err, strategyResultUpdateErr) } return issuerconfig.UpdateStrategy( @@ -219,3 +226,7 @@ func (c *execerController) getKeypairFilePaths(pod *v1.Pod) (string, string) { return certPath, keyPath } + +func newAggregate(errs ...error) error { + return errors.NewAggregate(errs) +} diff --git a/internal/controller/kubecertagent/execer_test.go b/internal/controller/kubecertagent/execer_test.go index 7d1092c9..fe19d198 100644 --- a/internal/controller/kubecertagent/execer_test.go +++ b/internal/controller/kubecertagent/execer_test.go @@ -132,6 +132,7 @@ func (s *fakePodExecutor) Exec(podNamespace string, podName string, commandAndAr } func TestManagerControllerSync(t *testing.T) { + name := t.Name() spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { const agentPodNamespace = "some-namespace" const agentPodName = "some-agent-pod-name-123" @@ -139,8 +140,6 @@ func TestManagerControllerSync(t *testing.T) { const keyPathAnnotationName = "kube-cert-agent.pinniped.dev/key-path" const fakeCertPath = "/some/cert/path" const fakeKeyPath = "/some/key/path" - const defaultDynamicCertProviderCert = "initial-cert" - const defaultDynamicCertProviderKey = "initial-key" const credentialIssuerResourceName = "ci-resource-name" var r *require.Assertions @@ -159,6 +158,8 @@ func TestManagerControllerSync(t *testing.T) { var fakeCertPEM, fakeKeyPEM string var credentialIssuerGVR schema.GroupVersionResource var frozenNow time.Time + var defaultDynamicCertProviderCert string + var defaultDynamicCertProviderKey string // Defer starting the informers until the last possible moment so that the // nested Before's can keep adding things to the informer caches. @@ -228,14 +229,23 @@ func TestManagerControllerSync(t *testing.T) { it.Before(func() { r = require.New(t) + crt, key, err := testutil.CreateCertificate( + time.Now().Add(-time.Hour), + time.Now().Add(time.Hour), + ) + require.NoError(t, err) + defaultDynamicCertProviderCert = string(crt) + defaultDynamicCertProviderKey = string(key) + cancelContext, cancelContextCancelFunc = context.WithCancel(context.Background()) pinnipedAPIClient = pinnipedfake.NewSimpleClientset() kubeClientset = kubernetesfake.NewSimpleClientset() 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() - dynamicCertProvider.Set([]byte(defaultDynamicCertProviderCert), []byte(defaultDynamicCertProviderKey)) + dynamicCertProvider = dynamiccert.New(name) + err = dynamicCertProvider.SetCertKeyContent([]byte(defaultDynamicCertProviderCert), []byte(defaultDynamicCertProviderKey)) + r.NoError(err) loadFile := func(filename string) string { bytes, err := ioutil.ReadFile(filename) @@ -669,6 +679,55 @@ func TestManagerControllerSync(t *testing.T) { r.Equal([]coretesting.Action{expectedGetAction, expectedCreateAction, expectedUpdateAction}, pinnipedAPIClient.Actions()) }) }) + + when("the third resulting pod exec has invalid key data", func() { + var keyParseErrorMessage string + + it.Before(func() { + keyParseErrorMessage = "failed to set signing cert/key content from agent pod some-namespace/some-agent-pod-name-123: TestManagerControllerSync: attempt to set invalid key pair: tls: failed to find any PEM data in key input" + fakeExecutor.errorsToReturn = []error{nil, nil} + fakeExecutor.resultsToReturn = []string{fakeCertPEM, ""} + startInformersAndController() + }) + + it("does not update the dynamic certificates provider", func() { + r.EqualError(controllerlib.TestSync(t, subject, *syncContext), keyParseErrorMessage) + requireDynamicCertProviderHasDefaultValues() + }) + + it("creates or updates the the CredentialIssuer status field with an error", func() { + r.EqualError(controllerlib.TestSync(t, subject, *syncContext), keyParseErrorMessage) + + expectedCreateCredentialIssuer := &configv1alpha1.CredentialIssuer{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: credentialIssuerResourceName, + }, + } + + expectedCredentialIssuer := &configv1alpha1.CredentialIssuer{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: credentialIssuerResourceName, + }, + Status: configv1alpha1.CredentialIssuerStatus{ + Strategies: []configv1alpha1.CredentialIssuerStrategy{ + { + Type: configv1alpha1.KubeClusterSigningCertificateStrategyType, + Status: configv1alpha1.ErrorStrategyStatus, + Reason: configv1alpha1.CouldNotFetchKeyStrategyReason, + Message: keyParseErrorMessage, + LastUpdateTime: metav1.NewTime(frozenNow), + }, + }, + }, + } + expectedGetAction := coretesting.NewRootGetAction(credentialIssuerGVR, credentialIssuerResourceName) + expectedCreateAction := coretesting.NewRootCreateAction(credentialIssuerGVR, expectedCreateCredentialIssuer) + expectedUpdateAction := coretesting.NewRootUpdateSubresourceAction(credentialIssuerGVR, "status", expectedCredentialIssuer) + r.Equal([]coretesting.Action{expectedGetAction, expectedCreateAction, expectedUpdateAction}, pinnipedAPIClient.Actions()) + }) + }) }) }, spec.Parallel(), spec.Report(report.Terminal{})) } diff --git a/internal/dynamiccert/provider.go b/internal/dynamiccert/provider.go index 77687fb1..8cef78bd 100644 --- a/internal/dynamiccert/provider.go +++ b/internal/dynamiccert/provider.go @@ -4,69 +4,112 @@ package dynamiccert import ( + "crypto/tls" "crypto/x509" + "fmt" "sync" "k8s.io/apiserver/pkg/server/dynamiccertificates" ) -// Provider provides a getter, CurrentCertKeyContent(), and a setter, Set(), for a PEM-formatted -// certificate and matching key. type Provider interface { + Private + Public +} + +type Private interface { dynamiccertificates.CertKeyContentProvider - // TODO dynamiccertificates.Notifier - // TODO dynamiccertificates.ControllerRunner ??? - Set(certPEM, keyPEM []byte) + SetCertKeyContent(certPEM, keyPEM []byte) error + UnsetCertKeyContent() + + notifier +} + +type Public interface { + dynamiccertificates.CAContentProvider + + notifier +} + +type notifier interface { + dynamiccertificates.Notifier + dynamiccertificates.ControllerRunner // we do not need this today, but it could grow and change in the future } type provider struct { - certPEM []byte - keyPEM []byte - mutex sync.RWMutex + name string + + // mutex guards all the fields below it + mutex sync.RWMutex + certPEM []byte + keyPEM []byte + listeners []dynamiccertificates.Listener } // New returns an empty Provider. The returned Provider is thread-safe. -func New() Provider { - return &provider{} -} - -func (p *provider) Set(certPEM, keyPEM []byte) { - p.mutex.Lock() // acquire a write lock - defer p.mutex.Unlock() - p.certPEM = certPEM - p.keyPEM = keyPEM +func New(name string) Provider { + return &provider{name: name} } func (p *provider) Name() string { - return "DynamicCertProvider" + return p.name // constant after struct initialization and thus does not need locking } func (p *provider) CurrentCertKeyContent() (cert []byte, key []byte) { - p.mutex.RLock() // acquire a read lock + p.mutex.RLock() defer p.mutex.RUnlock() + return p.certPEM, p.keyPEM } -func NewCAProvider(delegate dynamiccertificates.CertKeyContentProvider) dynamiccertificates.CAContentProvider { - return &caContentProvider{delegate: delegate} +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 { + return fmt.Errorf("%s: attempt to set invalid key pair: %w", p.name, err) + } + + p.setCertKeyContent(certPEM, keyPEM) + + return nil } -type caContentProvider struct { - delegate dynamiccertificates.CertKeyContentProvider +func (p *provider) UnsetCertKeyContent() { + p.setCertKeyContent(nil, nil) } -func (c *caContentProvider) Name() string { - return "DynamicCAProvider" +func (p *provider) setCertKeyContent(certPEM, keyPEM []byte) { + p.mutex.Lock() + defer p.mutex.Unlock() + + p.certPEM = certPEM + p.keyPEM = keyPEM + + for _, listener := range p.listeners { + listener.Enqueue() + } } -func (c *caContentProvider) CurrentCABundleContent() []byte { - ca, _ := c.delegate.CurrentCertKeyContent() +func (p *provider) CurrentCABundleContent() []byte { + ca, _ := p.CurrentCertKeyContent() return ca } -func (c *caContentProvider) VerifyOptions() (x509.VerifyOptions, bool) { +func (p *provider) VerifyOptions() (x509.VerifyOptions, bool) { return x509.VerifyOptions{}, false // assume we are unioned via dynamiccertificates.NewUnionCAContentProvider } -// TODO look at both the serving side union struct and the ca side union struct for all optional interfaces -// and then implement everything that makes sense for us to implement +func (p *provider) AddListener(listener dynamiccertificates.Listener) { + p.mutex.Lock() + defer p.mutex.Unlock() + + p.listeners = append(p.listeners, listener) +} + +func (p *provider) RunOnce() error { + return nil // no-op, but we want to make sure to stay in sync with dynamiccertificates.ControllerRunner +} + +func (p *provider) Run(workers int, stopCh <-chan struct{}) { + // no-op, but we want to make sure to stay in sync with dynamiccertificates.ControllerRunner +}