diff --git a/pkg/operator/certrotation/annotations.go b/pkg/operator/certrotation/annotations.go index fa9709ec06..c7c3356305 100644 --- a/pkg/operator/certrotation/annotations.go +++ b/pkg/operator/certrotation/annotations.go @@ -8,6 +8,16 @@ import ( ) const ( + // CertificateNotBeforeAnnotation contains the certificate expiration date in RFC3339 format. + CertificateNotBeforeAnnotation = "auth.openshift.io/certificate-not-before" + // CertificateNotAfterAnnotation contains the certificate expiration date in RFC3339 format. + CertificateNotAfterAnnotation = "auth.openshift.io/certificate-not-after" + // CertificateIssuer contains the common name of the certificate that signed another certificate. + CertificateIssuer = "auth.openshift.io/certificate-issuer" + // CertificateHostnames contains the hostnames used by a signer. + CertificateHostnames = "auth.openshift.io/certificate-hostnames" + // AutoRegenerateAfterOfflineExpiryAnnotation contains a link to PR and an e2e test name which verifies + // that TLS artifact is correctly regenerated after it has expired AutoRegenerateAfterOfflineExpiryAnnotation string = "certificates.openshift.io/auto-regenerate-after-offline-expiry" ) @@ -19,6 +29,10 @@ type AdditionalAnnotations struct { // AutoRegenerateAfterOfflineExpiry contains a link to PR and an e2e test name which verifies // that TLS artifact is correctly regenerated after it has expired AutoRegenerateAfterOfflineExpiry string + // NotBefore contains certificate the certificate creation date in RFC3339 format. + NotBefore string + // NotAfter contains certificate the certificate validity date in RFC3339 format. + NotAfter string } func (a AdditionalAnnotations) EnsureTLSMetadataUpdate(meta *metav1.ObjectMeta) bool { @@ -44,6 +58,14 @@ func (a AdditionalAnnotations) EnsureTLSMetadataUpdate(meta *metav1.ObjectMeta) meta.Annotations[AutoRegenerateAfterOfflineExpiryAnnotation] = a.AutoRegenerateAfterOfflineExpiry modified = true } + if len(a.NotBefore) > 0 && meta.Annotations[CertificateNotBeforeAnnotation] != a.NotBefore { + meta.Annotations[CertificateNotBeforeAnnotation] = a.NotBefore + modified = true + } + if len(a.NotAfter) > 0 && meta.Annotations[CertificateNotAfterAnnotation] != a.NotAfter { + meta.Annotations[CertificateNotAfterAnnotation] = a.NotAfter + modified = true + } return modified } diff --git a/pkg/operator/certrotation/client_cert_rotation_controller.go b/pkg/operator/certrotation/client_cert_rotation_controller.go index b7a7a69c7a..0d6ffe6738 100644 --- a/pkg/operator/certrotation/client_cert_rotation_controller.go +++ b/pkg/operator/certrotation/client_cert_rotation_controller.go @@ -15,14 +15,6 @@ import ( ) const ( - // CertificateNotBeforeAnnotation contains the certificate expiration date in RFC3339 format. - CertificateNotBeforeAnnotation = "auth.openshift.io/certificate-not-before" - // CertificateNotAfterAnnotation contains the certificate expiration date in RFC3339 format. - CertificateNotAfterAnnotation = "auth.openshift.io/certificate-not-after" - // CertificateIssuer contains the common name of the certificate that signed another certificate. - CertificateIssuer = "auth.openshift.io/certificate-issuer" - // CertificateHostnames contains the hostnames used by a signer. - CertificateHostnames = "auth.openshift.io/certificate-hostnames" // RunOnceContextKey is a context value key that can be used to call the controller Sync() and make it only run the syncWorker once and report error. RunOnceContextKey = "cert-rotation-controller.openshift.io/run-once" ) diff --git a/pkg/operator/certrotation/signer.go b/pkg/operator/certrotation/signer.go index de9eb11b72..c2b300858e 100644 --- a/pkg/operator/certrotation/signer.go +++ b/pkg/operator/certrotation/signer.go @@ -92,7 +92,7 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (* reason = "secret doesn't exist" } c.EventRecorder.Eventf("SignerUpdateRequired", "%q in %q requires a new signing cert/key pair: %v", c.Name, c.Namespace, reason) - if err := setSigningCertKeyPairSecret(signingCertKeyPairSecret, c.Validity); err != nil { + if err := setSigningCertKeyPairSecret(signingCertKeyPairSecret, c.Validity, c.AdditionalAnnotations); err != nil { return nil, false, err } @@ -200,7 +200,7 @@ func getValidityFromAnnotations(annotations map[string]string) (notBefore time.T } // setSigningCertKeyPairSecret creates a new signing cert/key pair and sets them in the secret -func setSigningCertKeyPairSecret(signingCertKeyPairSecret *corev1.Secret, validity time.Duration) error { +func setSigningCertKeyPairSecret(signingCertKeyPairSecret *corev1.Secret, validity time.Duration, annotations AdditionalAnnotations) error { signerName := fmt.Sprintf("%s_%s@%d", signingCertKeyPairSecret.Namespace, signingCertKeyPairSecret.Name, time.Now().Unix()) ca, err := crypto.MakeSelfSignedCAConfigForDuration(signerName, validity) if err != nil { @@ -221,9 +221,11 @@ func setSigningCertKeyPairSecret(signingCertKeyPairSecret *corev1.Secret, validi } signingCertKeyPairSecret.Data["tls.crt"] = certBytes.Bytes() signingCertKeyPairSecret.Data["tls.key"] = keyBytes.Bytes() - signingCertKeyPairSecret.Annotations[CertificateNotAfterAnnotation] = ca.Certs[0].NotAfter.Format(time.RFC3339) - signingCertKeyPairSecret.Annotations[CertificateNotBeforeAnnotation] = ca.Certs[0].NotBefore.Format(time.RFC3339) + annotations.NotBefore = ca.Certs[0].NotBefore.Format(time.RFC3339) + annotations.NotAfter = ca.Certs[0].NotAfter.Format(time.RFC3339) signingCertKeyPairSecret.Annotations[CertificateIssuer] = ca.Certs[0].Issuer.CommonName + _ = annotations.EnsureTLSMetadataUpdate(&signingCertKeyPairSecret.ObjectMeta) + return nil } diff --git a/pkg/operator/certrotation/target.go b/pkg/operator/certrotation/target.go index 65dd87eadd..71a568ad4d 100644 --- a/pkg/operator/certrotation/target.go +++ b/pkg/operator/certrotation/target.go @@ -258,8 +258,8 @@ func setTargetCertKeyPairSecret(targetCertKeyPairSecret *corev1.Secret, validity if err != nil { return err } - targetCertKeyPairSecret.Annotations[CertificateNotAfterAnnotation] = certKeyPair.Certs[0].NotAfter.Format(time.RFC3339) - targetCertKeyPairSecret.Annotations[CertificateNotBeforeAnnotation] = certKeyPair.Certs[0].NotBefore.Format(time.RFC3339) + annotations.NotBefore = certKeyPair.Certs[0].NotBefore.Format(time.RFC3339) + annotations.NotAfter = certKeyPair.Certs[0].NotAfter.Format(time.RFC3339) targetCertKeyPairSecret.Annotations[CertificateIssuer] = certKeyPair.Certs[0].Issuer.CommonName _ = annotations.EnsureTLSMetadataUpdate(&targetCertKeyPairSecret.ObjectMeta) diff --git a/pkg/operator/csr/cert_controller.go b/pkg/operator/csr/cert_controller.go index d56c88df54..ba4f1b358f 100644 --- a/pkg/operator/csr/cert_controller.go +++ b/pkg/operator/csr/cert_controller.go @@ -3,7 +3,9 @@ package csr import ( "context" "crypto/tls" + "crypto/x509" "crypto/x509/pkix" + "encoding/pem" "fmt" "math/rand" "time" @@ -166,7 +168,7 @@ func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory. // reconcile pending csr if exists if len(c.csrName) > 0 { - newSecretConfig, err := c.syncCSR(secret) + newSecretConfig, leaf, err := c.syncCSR(secret) if err != nil { c.reset() return err @@ -179,6 +181,12 @@ func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory. newSecretConfig[k] = v } secret.Data = newSecretConfig + + // Update not-before/not-after annotations + c.AdditionalAnnotations.NotBefore = leaf.NotBefore.Format(time.RFC3339) + c.AdditionalAnnotations.NotAfter = leaf.NotAfter.Format(time.RFC3339) + _ = c.AdditionalAnnotations.EnsureTLSMetadataUpdate(&secret.ObjectMeta) + // save the changes into secret if err := c.saveSecret(secret); err != nil { return err @@ -231,10 +239,10 @@ func (c *clientCertificateController) sync(ctx context.Context, syncCtx factory. return nil } -func (c *clientCertificateController) syncCSR(secret *corev1.Secret) (map[string][]byte, error) { +func (c *clientCertificateController) syncCSR(secret *corev1.Secret) (map[string][]byte, *x509.Certificate, error) { // skip if there is no ongoing csr if len(c.csrName) == 0 { - return nil, fmt.Errorf("no ongoing csr") + return nil, nil, fmt.Errorf("no ongoing csr") } // skip if csr no longer exists @@ -244,38 +252,48 @@ func (c *clientCertificateController) syncCSR(secret *corev1.Secret) (map[string // fallback to fetching csr from hub apiserver in case it is not cached by informer yet csr, err = c.hubCSRClient.Get(context.Background(), c.csrName, metav1.GetOptions{}) if errors.IsNotFound(err) { - return nil, fmt.Errorf("unable to get csr %q. It might have already been deleted.", c.csrName) + return nil, nil, fmt.Errorf("unable to get csr %q. It might have already been deleted.", c.csrName) } case err != nil: - return nil, err + return nil, nil, err } // skip if csr is not approved yet if !isCSRApproved(csr) { - return nil, nil + return nil, nil, nil } // skip if csr has no certificate in its status yet if len(csr.Status.Certificate) == 0 { - return nil, nil + return nil, nil, nil } klog.V(4).Infof("Sync csr %v", c.csrName) // check if cert in csr status matches with the corresponding private key if c.keyData == nil { - return nil, fmt.Errorf("No private key found for certificate in csr: %s", c.csrName) + return nil, nil, fmt.Errorf("No private key found for certificate in csr: %s", c.csrName) } _, err = tls.X509KeyPair(csr.Status.Certificate, c.keyData) if err != nil { - return nil, fmt.Errorf("Private key does not match with the certificate in csr: %s", c.csrName) + return nil, nil, fmt.Errorf("Private key does not match with the certificate in csr: %s", c.csrName) + } + // verify that the recieved data is a valid x509 certificate + var block *pem.Block + block, _ = pem.Decode(csr.Status.Certificate) + if block == nil || block.Type != "CERTIFICATE" || len(block.Headers) != 0 { + return nil, nil, fmt.Errorf("invalid first block found for certificate in csr: %s", c.csrName) } + certBytes := block.Bytes + parsedCert, err := x509.ParseCertificate(certBytes) + if err != nil { + return nil, nil, fmt.Errorf("failed to parse the certificate in csr %s: %v", c.csrName, err) + } data := map[string][]byte{ TLSCertFile: csr.Status.Certificate, TLSKeyFile: c.keyData, } - - return data, nil + return data, parsedCert, nil } func (c *clientCertificateController) createCSR(ctx context.Context) (string, error) { diff --git a/pkg/operator/csr/cert_controller_test.go b/pkg/operator/csr/cert_controller_test.go new file mode 100644 index 0000000000..9d03adb9a6 --- /dev/null +++ b/pkg/operator/csr/cert_controller_test.go @@ -0,0 +1,313 @@ +package csr + +import ( + "context" + "crypto/x509/pkix" + "errors" + "strings" + "testing" + "time" + + "github.com/openshift/api/annotations" + "github.com/openshift/library-go/pkg/operator/certrotation" + "github.com/openshift/library-go/pkg/operator/csr/csrtestinghelpers" + "github.com/openshift/library-go/pkg/operator/events" + "github.com/stretchr/testify/require" + + certificates "k8s.io/api/certificates/v1" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/watch" + certificatesv1 "k8s.io/client-go/applyconfigurations/certificates/v1" + "k8s.io/client-go/informers" + "k8s.io/client-go/kubernetes/fake" + clienttesting "k8s.io/client-go/testing" + "k8s.io/client-go/util/workqueue" + clocktesting "k8s.io/utils/clock/testing" +) + +const ( + testControllerNamespace = "test-ns" + testControllerSecretName = "test-secret" + testControllerCSRName = "test-csr" +) + +func TestReset(t *testing.T) { + ctrl, _ := newTestControllerWithClient() + ctrl.csrName = "test-csr" + ctrl.keyData = []byte("test-key") + + ctrl.reset() + + if ctrl.csrName != "" { + t.Errorf("expected csrName to be empty, got %q", ctrl.csrName) + } + if ctrl.keyData != nil { + t.Errorf("expected keyData to be nil, got %v", ctrl.keyData) + } +} + +func TestControllerSync(t *testing.T) { + testCert := csrtestinghelpers.NewTestCert("test", time.Hour) + + tests := []struct { + name string + ctrlPrepare func(*clientCertificateController) + fakeClientPrepare func(*fake.Clientset) + errorExpected bool + errorContains string + validateCtrl func(*testing.T, *clientCertificateController, error) + validateSecret func(*testing.T, *corev1.Secret, error) + }{ + { + name: "secret not found", + fakeClientPrepare: func(fakeClient *fake.Clientset) { + fakeClient.PrependReactor("get", "secrets", func(action clienttesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewNotFound(schema.GroupResource{Resource: "secrets"}, testControllerSecretName) + }) + }, + validateSecret: func(t *testing.T, secret *corev1.Secret, err error) { + require.Equal(t, err, apierrors.NewNotFound(schema.GroupResource{Resource: "secrets"}, testControllerSecretName), "error message") + }, + }, + { + name: "secret get error", + fakeClientPrepare: func(fakeClient *fake.Clientset) { + fakeClient.PrependReactor("get", "secrets", func(action clienttesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("api error") + }) + }, + errorExpected: true, + errorContains: "api error", + validateSecret: func(t *testing.T, secret *corev1.Secret, err error) { + require.Equal(t, err, errors.New("api error"), "error message") + }, + }, + { + name: "secret exists", + fakeClientPrepare: func(fakeClient *fake.Clientset) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testControllerSecretName, + Namespace: testControllerNamespace, + }, + Data: map[string][]byte{ + TLSCertFile: []byte("some-cert-data"), + }, + } + fakeClient.PrependReactor("get", "secrets", func(action clienttesting.Action) (bool, runtime.Object, error) { + return true, secret, nil + }) + }, + validateSecret: func(t *testing.T, secret *corev1.Secret, err error) { + require.NoError(t, err) + require.NotNil(t, secret) + require.NotNil(t, secret.Annotations) + require.Equal(t, "test-component", secret.Annotations[annotations.OpenShiftComponent], "unexpected component") + }, + }, + { + name: "secret with metadata update", + ctrlPrepare: func(ctrl *clientCertificateController) { + ctrl.AdditionalAnnotations = certrotation.AdditionalAnnotations{ + JiraComponent: "test-component", + } + }, + fakeClientPrepare: func(fakeClient *fake.Clientset) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testControllerSecretName, + Namespace: testControllerNamespace, + }, + Data: map[string][]byte{ + TLSCertFile: []byte("some-cert-data"), + }, + } + fakeClient.PrependReactor("get", "secrets", func(action clienttesting.Action) (bool, runtime.Object, error) { + return true, secret, nil + }) + fakeClient.PrependReactor("update", "secrets", func(action clienttesting.Action) (bool, runtime.Object, error) { + return true, secret, nil + }) + }, + validateSecret: func(t *testing.T, secret *corev1.Secret, err error) { + require.NoError(t, err) + require.NotNil(t, secret) + require.NotNil(t, secret.Annotations) + require.Equal(t, "test-component", secret.Annotations[annotations.OpenShiftComponent], "unexpected component") + }, + }, + { + name: "pending csr", + ctrlPrepare: func(ctrl *clientCertificateController) { + ctrl.csrName = testControllerCSRName + ctrl.keyData = []byte("pending-key") + + // Mock approved CSR with certificate + approvedCSR := csrtestinghelpers.NewApprovedCSR(csrtestinghelpers.CSRHolder{Name: testControllerCSRName}) + approvedCSR.Status.Certificate = testCert.Cert + ctrl.hubCSRClient = &fakeCSRClient{ + csr: approvedCSR, + } + ctrl.keyData = testCert.Key + }, + validateSecret: func(t *testing.T, secret *corev1.Secret, err error) { + require.NoError(t, err) + require.NotNil(t, secret) + require.NotNil(t, secret.Annotations) + require.Equal(t, "test-component", secret.Annotations[annotations.OpenShiftComponent], "unexpected component") + require.NotNil(t, secret.Data) + require.NotNil(t, secret.Data[corev1.TLSCertKey]) + require.Equal(t, testCert.Cert, secret.Data[corev1.TLSCertKey], "unexpected certificate") + require.Equal(t, testCert.Key, secret.Data[corev1.TLSPrivateKeyKey], "unexpected private key") + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl, fakeClient := newTestControllerWithClient() + + if tt.ctrlPrepare != nil { + tt.ctrlPrepare(ctrl) + } + + if tt.fakeClientPrepare != nil { + tt.fakeClientPrepare(fakeClient) + } + + ctx := context.Background() + syncCtx := &testSyncContext{} + err := ctrl.sync(ctx, syncCtx) + + if tt.errorExpected && err == nil { + t.Error("expected error, got nil") + } + if !tt.errorExpected && err != nil && tt.validateCtrl == nil { + t.Errorf("unexpected error: %v", err) + } + if tt.errorContains != "" && (err == nil || !strings.Contains(err.Error(), tt.errorContains)) { + t.Errorf("expected error to contain %q, got %q", tt.errorContains, err) + } + + if tt.validateCtrl != nil { + tt.validateCtrl(t, ctrl, err) + } + + if tt.validateSecret != nil { + secret, err := ctrl.spokeCoreClient.Secrets(testControllerNamespace).Get(ctx, testControllerSecretName, metav1.GetOptions{}) + tt.validateSecret(t, secret, err) + } + }) + } +} + +// Test helper functions and types +func newTestController(client *fake.Clientset) *clientCertificateController { + clientCertOption := ClientCertOption{ + SecretNamespace: testControllerNamespace, + SecretName: testControllerSecretName, + AdditonalSecretData: map[string][]byte{"test": []byte("data")}, + AdditionalAnnotations: certrotation.AdditionalAnnotations{ + JiraComponent: "test-component", + }, + } + csrOption := CSROption{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: "test-csr-", + }, + Subject: &pkix.Name{CommonName: "test"}, + SignerName: certificates.KubeAPIServerClientSignerName, + DNSNames: []string{"localhost"}, + EventFilterFunc: func(obj interface{}) bool { return true }, + } + informerFactory := informers.NewSharedInformerFactory(client, 0) + return &clientCertificateController{ + clientCertOption, + csrOption, + informerFactory.Certificates().V1().CertificateSigningRequests().Lister(), + client.CertificatesV1().CertificateSigningRequests(), + client.CoreV1(), + "test-controller", + "", + []byte{}, + } +} + +func newTestControllerWithClient() (*clientCertificateController, *fake.Clientset) { + client := fake.NewSimpleClientset() + ctrl := newTestController(client) + return ctrl, client +} + +type testSyncContext struct{} + +func (t *testSyncContext) Queue() workqueue.RateLimitingInterface { return nil } +func (t *testSyncContext) QueueKey() string { return "test-key" } +func (t *testSyncContext) Recorder() events.Recorder { + return events.NewInMemoryRecorder("test", clocktesting.NewFakeClock(time.Now())) +} + +type fakeCSRClient struct { + csr *certificates.CertificateSigningRequest + err error +} + +func (f *fakeCSRClient) Get(ctx context.Context, name string, opts metav1.GetOptions) (*certificates.CertificateSigningRequest, error) { + if f.err != nil { + return nil, f.err + } + return f.csr, nil +} + +func (f *fakeCSRClient) Create(ctx context.Context, csr *certificates.CertificateSigningRequest, opts metav1.CreateOptions) (*certificates.CertificateSigningRequest, error) { + if f.err != nil { + return nil, f.err + } + csr.Name = "test-csr" + return csr, nil +} + +func (f *fakeCSRClient) Update(ctx context.Context, csr *certificates.CertificateSigningRequest, opts metav1.UpdateOptions) (*certificates.CertificateSigningRequest, error) { + panic("not implemented") +} + +func (f *fakeCSRClient) UpdateStatus(ctx context.Context, csr *certificates.CertificateSigningRequest, opts metav1.UpdateOptions) (*certificates.CertificateSigningRequest, error) { + panic("not implemented") +} + +func (f *fakeCSRClient) UpdateApproval(ctx context.Context, certificateSigningRequestName string, certificateSigningRequest *certificates.CertificateSigningRequest, opts metav1.UpdateOptions) (*certificates.CertificateSigningRequest, error) { + panic("not implemented") +} + +func (f *fakeCSRClient) Delete(ctx context.Context, name string, opts metav1.DeleteOptions) error { + panic("not implemented") +} + +func (f *fakeCSRClient) DeleteCollection(ctx context.Context, opts metav1.DeleteOptions, listOpts metav1.ListOptions) error { + panic("not implemented") +} + +func (f *fakeCSRClient) List(ctx context.Context, opts metav1.ListOptions) (*certificates.CertificateSigningRequestList, error) { + panic("not implemented") +} + +func (f *fakeCSRClient) Watch(ctx context.Context, opts metav1.ListOptions) (watch.Interface, error) { + panic("not implemented") +} + +func (f *fakeCSRClient) Patch(ctx context.Context, name string, pt types.PatchType, data []byte, opts metav1.PatchOptions, subresources ...string) (*certificates.CertificateSigningRequest, error) { + panic("not implemented") +} + +func (f *fakeCSRClient) Apply(ctx context.Context, certificateSigningRequest *certificatesv1.CertificateSigningRequestApplyConfiguration, opts metav1.ApplyOptions) (*certificates.CertificateSigningRequest, error) { + panic("not implemented") +} + +func (f *fakeCSRClient) ApplyStatus(ctx context.Context, certificateSigningRequest *certificatesv1.CertificateSigningRequestApplyConfiguration, opts metav1.ApplyOptions) (*certificates.CertificateSigningRequest, error) { + panic("not implemented") +}