Merge pull request #1889 from vrutkovs/cert-annotations-not-before-not-after

OCPBUGS-44842: certrotation: set not-before/not-after annotations

Author: openshift-merge-bot[bot]

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
 }
 

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"
 )

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
 }

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)

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) {