Merge pull request #15 from erikgb/renew-cert

Ensure certificates are renewed

Author: cert-manager-prow[bot]

go.mod

@@ -3,7 +3,7 @@ module github.com/cert-manager/webhook-cert-lib
 go 1.24.0
 
 require (
-	github.com/go-logr/logr v1.4.3
+	github.com/stretchr/testify v1.10.0
 	k8s.io/api v0.33.2
 	k8s.io/apimachinery v0.33.2
 	k8s.io/client-go v0.33.2
@@ -19,6 +19,7 @@ require (
 	github.com/evanphx/json-patch/v5 v5.9.11 // indirect
 	github.com/fsnotify/fsnotify v1.9.0 // indirect
 	github.com/fxamacker/cbor/v2 v2.8.0 // indirect
+	github.com/go-logr/logr v1.4.3 // indirect
 	github.com/go-openapi/jsonpointer v0.21.1 // indirect
 	github.com/go-openapi/jsonreference v0.21.0 // indirect
 	github.com/go-openapi/swag v0.23.1 // indirect
@@ -34,6 +35,7 @@ require (
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
 	github.com/pkg/errors v0.9.1 // indirect
+	github.com/pmezard/go-difflib v1.0.0 // indirect
 	github.com/prometheus/client_golang v1.22.0 // indirect
 	github.com/prometheus/client_model v0.6.2 // indirect
 	github.com/prometheus/common v0.65.0 // indirect

internal/pki/tls.go

@@ -0,0 +1,41 @@
+/*
+Copyright 2020 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package pki
+
+import (
+	"crypto"
+	"crypto/tls"
+	"crypto/x509"
+)
+
+func ToTLSCertificate(cert *x509.Certificate, pk crypto.Signer) (tls.Certificate, error) {
+	pkData, err := EncodePrivateKey(pk)
+	if err != nil {
+		return tls.Certificate{}, err
+	}
+
+	certData, err := EncodeX509(cert)
+	if err != nil {
+		return tls.Certificate{}, err
+	}
+
+	tlsCert, err := tls.X509KeyPair(certData, pkData)
+	if err != nil {
+		return tls.Certificate{}, err
+	}
+	return tlsCert, nil
+}

pkg/authority/ca_secret_controller.go

@@ -19,9 +19,9 @@ package authority
 import (
 	"context"
 	"crypto"
+	"crypto/tls"
 	"crypto/x509"
 
-	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -63,40 +63,51 @@ func (r *CASecretReconciler) SetupWithManager(mgr ctrl.Manager) error {
 }
 
 func (r *CASecretReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	return ctrl.Result{}, r.reconcileSecret(ctx, req)
-}
-
-func (r *CASecretReconciler) reconcileSecret(ctx context.Context, req ctrl.Request) error {
 	secret := &corev1.Secret{}
 	if err := r.Cache.Get(ctx, req.NamespacedName, secret); err != nil {
 		if !errors.IsNotFound(err) {
-			return err
+			return ctrl.Result{}, err
 		}
-		// Secret does not exist - let's create it
+		// Secret does not exist - let's create it by setting namespace/name
 		secret.Namespace = req.Namespace
 		secret.Name = req.Name
 	}
 
-	generate, caCert, caPk := r.needsGenerate(secret)
+	caCert, err := r.reconcileSecret(ctx, secret)
+	return ctrl.Result{RequeueAfter: certificate.RenewAfter(caCert)}, err
+}
+
+func (r *CASecretReconciler) reconcileSecret(ctx context.Context, secret *corev1.Secret) (caCert *x509.Certificate, err error) {
+	var caPk crypto.Signer
+
+	if required, reason := caRequiresRegeneration(secret); required {
+		log.FromContext(ctx).Info("Will regenerate CA", "reason", reason)
 
-	if generate || secret.Annotations[api.RenewCertificateSecretAnnotation] != secret.Annotations[api.RenewHandledCertificateSecretAnnotation] {
-		var err error
 		caCert, caPk, err = certificate.GenerateCA(r.CAOptions.Duration)
 		if err != nil {
-			return err
+			return caCert, err
+		}
+	} else {
+		caCert, err = pki.DecodeX509CertificateBytes(secret.Data[corev1.TLSCertKey])
+		if err != nil {
+			return caCert, err
+		}
+		caPk, err = pki.DecodePrivateKeyBytes(secret.Data[corev1.TLSPrivateKeyKey])
+		if err != nil {
+			return caCert, err
 		}
 	}
 
 	certBytes, err := pki.EncodeX509(caCert)
 	if err != nil {
-		return err
+		return caCert, err
 	}
 	pkBytes, err := pki.EncodePrivateKey(caPk)
 	if err != nil {
-		return err
+		return caCert, err
 	}
 
-	caBundleBytes := addCertToCABundle(log.FromContext(ctx), secret.Data[api.TLSCABundleKey], caCert)
+	caBundleBytes := addCertToCABundle(ctx, secret.Data[api.TLSCABundleKey], caCert)
 
 	ac := corev1ac.Secret(secret.Name, secret.Namespace).
 		WithLabels(map[string]string{
@@ -115,36 +126,51 @@ func (r *CASecretReconciler) reconcileSecret(ctx context.Context, req ctrl.Reque
 		})
 	}
 
-	return r.Patcher.Patch(ctx, secret, ssa.NewApplyPatch(ac), client.ForceOwnership, ssa.FieldOwner)
+	return caCert, r.Patcher.Patch(ctx, secret, ssa.NewApplyPatch(ac), client.ForceOwnership, ssa.FieldOwner)
 }
 
-func addCertToCABundle(logger logr.Logger, caBundleBytes []byte, caCert *x509.Certificate) []byte {
+func addCertToCABundle(ctx context.Context, caBundleBytes []byte, caCert *x509.Certificate) []byte {
 	certPool := pki.NewCertPool(pki.WithFilteredExpiredCerts(true))
 
 	if err := certPool.AddCertsFromPEM(caBundleBytes); err != nil {
-		logger.Error(err, "failed to re-use existing CAs in new set of CAs")
+		log.FromContext(ctx).Error(err, "failed to re-use existing CAs in new set of CAs")
 	}
 	// TODO: handle AddCert returning false? I expect this will never happen.
 	certPool.AddCert(caCert)
 
 	return []byte(certPool.PEM())
 }
 
-func (r *CASecretReconciler) needsGenerate(secret *corev1.Secret) (bool, *x509.Certificate, crypto.Signer) {
-	caCert, err := pki.DecodeX509CertificateBytes(secret.Data[corev1.TLSCertKey])
-	if err != nil {
-		return true, nil, nil
+// caRequiresRegeneration will check data in a Secret resource and return true
+// if the CA needs to be regenerated for any reason.
+func caRequiresRegeneration(s *corev1.Secret) (bool, string) {
+	if s.Annotations[api.RenewCertificateSecretAnnotation] != s.Annotations[api.RenewHandledCertificateSecretAnnotation] {
+		return true, "Forced renewal."
+	}
+
+	if s.Data == nil {
+		return true, "Missing data in CA secret."
 	}
-	caPk, err := pki.DecodePrivateKeyBytes(secret.Data[corev1.TLSPrivateKeyKey])
+	pkData := s.Data[corev1.TLSPrivateKeyKey]
+	certData := s.Data[corev1.TLSCertKey]
+	if len(pkData) == 0 || len(certData) == 0 {
+		return true, "Missing data in CA secret."
+	}
+	cert, err := tls.X509KeyPair(certData, pkData)
 	if err != nil {
-		return true, nil, nil
+		return true, "Failed to parse data in CA secret."
 	}
 
-	equal, err := pki.PublicKeysEqual(caCert.PublicKey, caPk.Public())
-	if !equal || err != nil {
-		return true, nil, nil
+	x509Cert, err := x509.ParseCertificate(cert.Certificate[0])
+	if err != nil {
+		return true, "Internal error parsing x509 certificate."
+	}
+	if !x509Cert.IsCA {
+		return true, "Stored certificate is not marked as a CA."
+	}
+	if certificate.RenewAfter(x509Cert) < 0 {
+		return true, "CA certificate is nearing expiry."
 	}
 
-	// TODO: Trigger renew check due
-	return false, caCert, caPk
+	return false, ""
 }

pkg/authority/ca_secret_controller_test.go

@@ -0,0 +1,179 @@
+/*
+Copyright 2020 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package authority
+
+import (
+	"crypto/rand"
+	"crypto/x509"
+	"crypto/x509/pkix"
+	"math/big"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	corev1 "k8s.io/api/core/v1"
+
+	"github.com/cert-manager/webhook-cert-lib/internal/pki"
+)
+
+var serialNumberLimit = new(big.Int).Lsh(big.NewInt(1), 128)
+
+func Test__caRequiresRegeneration(t *testing.T) {
+	generateSecretData := func(mod func(*x509.Certificate)) map[string][]byte {
+		// Generate a certificate and private key pair
+		pk, err := pki.GenerateECPrivateKey(384)
+		assert.NoError(t, err)
+		pkBytes, err := pki.EncodePrivateKey(pk)
+		assert.NoError(t, err)
+		serialNumber, err := rand.Int(rand.Reader, serialNumberLimit)
+		assert.NoError(t, err)
+		cert := &x509.Certificate{
+			Version:               3,
+			BasicConstraintsValid: true,
+			SerialNumber:          serialNumber,
+			PublicKeyAlgorithm:    x509.ECDSA,
+			Subject: pkix.Name{
+				CommonName: "cert-manager-webhook-ca",
+			},
+			IsCA:      true,
+			NotBefore: time.Now(),
+			NotAfter:  time.Now().Add(5 * time.Minute),
+			KeyUsage:  x509.KeyUsageDigitalSignature | x509.KeyUsageKeyEncipherment | x509.KeyUsageCertSign,
+		}
+		if mod != nil {
+			mod(cert)
+		}
+		_, cert, err = pki.SignCertificate(cert, cert, pk.Public(), pk)
+		assert.NoError(t, err)
+		certBytes, err := pki.EncodeX509(cert)
+		assert.NoError(t, err)
+
+		return map[string][]byte{
+			"tls.crt": certBytes,
+			"ca.crt":  certBytes,
+			"tls.key": pkBytes,
+		}
+	}
+
+	tests := []struct {
+		name         string
+		secret       *corev1.Secret
+		expect       bool
+		expectReason string
+	}{
+		{
+			name: "Missing data in CA secret (nil data)",
+			secret: &corev1.Secret{
+				Data: nil,
+			},
+			expect:       true,
+			expectReason: "Missing data in CA secret.",
+		},
+		{
+			name: "Missing data in CA secret (missing ca.crt)",
+			secret: &corev1.Secret{
+				Data: map[string][]byte{
+					"tls.key": []byte("private key"),
+				},
+			},
+			expect:       true,
+			expectReason: "Missing data in CA secret.",
+		},
+		{
+			name: "Failed to parse data in CA secret",
+			secret: &corev1.Secret{
+				Data: map[string][]byte{
+					"tls.crt": []byte("cert"),
+					"ca.crt":  []byte("cert"),
+					"tls.key": []byte("secret"),
+				},
+			},
+			expect:       true,
+			expectReason: "Failed to parse data in CA secret.",
+		},
+		{
+			name: "Stored certificate is not marked as a CA",
+			secret: &corev1.Secret{
+				Data: generateSecretData(
+					func(cert *x509.Certificate) {
+						cert.IsCA = false
+					},
+				),
+			},
+			expect:       true,
+			expectReason: "Stored certificate is not marked as a CA.",
+		},
+		{
+			name: "Root CA certificate is JUST nearing expiry",
+			secret: &corev1.Secret{
+				Data: generateSecretData(
+					func(cert *x509.Certificate) {
+						cert.NotBefore = time.Now().Add(-2*time.Hour - 1*time.Minute)
+						cert.NotAfter = cert.NotBefore.Add(3 * time.Hour)
+					},
+				),
+			},
+			expect:       true,
+			expectReason: "CA certificate is nearing expiry.",
+		},
+		{
+			name: "Root CA certificate is ALMOST nearing expiry",
+			secret: &corev1.Secret{
+				Data: generateSecretData(
+					func(cert *x509.Certificate) {
+						cert.NotBefore = time.Now().Add(-2*time.Hour + 1*time.Minute)
+						cert.NotAfter = cert.NotBefore.Add(3 * time.Hour)
+					},
+				),
+			},
+			expect: false,
+		},
+		{
+			name: "Root CA certificate is expired",
+			secret: &corev1.Secret{
+				Data: generateSecretData(
+					func(cert *x509.Certificate) {
+						cert.NotBefore = time.Now().Add(-1 * time.Hour)
+						cert.NotAfter = time.Now().Add(-1 * time.Minute)
+					},
+				),
+			},
+			expect:       true,
+			expectReason: "CA certificate is nearing expiry.",
+		},
+		{
+			name: "Ok",
+			secret: &corev1.Secret{
+				Data: generateSecretData(nil),
+			},
+			expect:       false,
+			expectReason: "",
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			required, reason := caRequiresRegeneration(test.secret)
+			if required != test.expect {
+				t.Errorf("Expected %v, but got %v", test.expect, required)
+			}
+			if reason != test.expectReason {
+				t.Errorf("Expected %q, but got %q", test.expectReason, reason)
+			}
+		})
+	}
+}

pkg/authority/certificate/tls.go

@@ -117,3 +117,10 @@ func (h *Holder) GetCertificate(_ *tls.ClientHelloInfo) (*tls.Certificate, error
 func (h *Holder) SetCertificate(cert *tls.Certificate) {
 	h.certP.Store(cert)
 }
+
+// RenewAfter returns the duration until the certificate should be renewed.
+func RenewAfter(cert *x509.Certificate) time.Duration {
+	lifetime := cert.NotAfter.Sub(cert.NotBefore)
+	renewTime := cert.NotBefore.Add(lifetime * 2 / 3)
+	return time.Until(renewTime)
+}