fix(tls): detect modified CA and reissue certs (#897)

* fix(tls): detect modified CA and reissue certs

* Ignore certificate not found when deleting

bundle/manifests/cryostat-operator.clusterserviceversion.yaml

@@ -30,7 +30,7 @@ metadata:
     capabilities: Seamless Upgrades
     categories: Monitoring, Developer Tools
     containerImage: quay.io/cryostat/cryostat-operator:4.0.0-dev
-    createdAt: "2024-06-10T14:47:15Z"
+    createdAt: "2024-06-15T00:21:26Z"
     description: JVM monitoring and profiling tool
     operatorframework.io/initialization-resource: |-
       {

internal/controllers/certmanager.go

@@ -24,9 +24,11 @@ import (
 	resources "github.com/cryostatio/cryostat-operator/internal/controllers/common/resource_definitions"
 	"github.com/cryostatio/cryostat-operator/internal/controllers/model"
 	corev1 "k8s.io/api/core/v1"
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 )
 
@@ -212,6 +214,14 @@ func (r *Reconciler) createOrUpdateIssuer(ctx context.Context, issuer *certv1.Is
 		if err := controllerutil.SetControllerReference(owner, issuer, r.Scheme); err != nil {
 			return err
 		}
+		// Check if the issuer's CA has changed
+		if !issuer.CreationTimestamp.IsZero() && r.issuerCAChanged(issuer.Spec.CA, issuerSpec.CA) {
+			// Issuer CA has changed, delete all certificates the previous CA issued
+			err := r.deleteCertChain(ctx, issuer.Namespace, issuerSpec.CA.SecretName, owner)
+			if err != nil {
+				return err
+			}
+		}
 		// Update Issuer spec
 		issuer.Spec = *issuerSpec
 		return nil
@@ -223,6 +233,61 @@ func (r *Reconciler) createOrUpdateIssuer(ctx context.Context, issuer *certv1.Is
 	return nil
 }
 
+func (r *Reconciler) issuerCAChanged(current *certv1.CAIssuer, updated *certv1.CAIssuer) bool {
+	// Compare the .spec.ca.secretName in the current and updated Issuer. Return whether they differ.
+	if current == nil {
+		return false
+	}
+	currentSecret := current.SecretName
+	updatedSecret := ""
+	if updated != nil {
+		updatedSecret = updated.SecretName
+	}
+
+	if currentSecret != updatedSecret {
+		r.Log.Info("certificate authority has changed, deleting issued certificates",
+			"current", currentSecret, "updated", updatedSecret)
+		return true
+	}
+	return false
+}
+
+func (r *Reconciler) deleteCertChain(ctx context.Context, namespace string, caSecretName string, owner metav1.Object) error {
+	// Look up all certificates in this namespace
+	certs := &certv1.CertificateList{}
+	err := r.Client.List(ctx, certs, &client.ListOptions{
+		Namespace: namespace,
+	})
+	if err != nil {
+		return err
+	}
+
+	for i, cert := range certs.Items {
+		// Is the certificate owned by this CR, and not the CA itself?
+		if metav1.IsControlledBy(&certs.Items[i], owner) && cert.Spec.SecretName != caSecretName {
+			// Clean up secret referenced by the cert
+			secret := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      cert.Spec.SecretName,
+					Namespace: cert.Namespace,
+				},
+			}
+
+			err := r.deleteSecret(ctx, secret)
+			if err != nil {
+				return err
+			}
+			// Delete the certificate
+			err = r.deleteCertificate(ctx, &certs.Items[i])
+			if err != nil {
+				return err
+			}
+		}
+	}
+
+	return nil
+}
+
 func (r *Reconciler) createOrUpdateCertificate(ctx context.Context, cert *certv1.Certificate, owner metav1.Object) error {
 	certSpec := cert.Spec.DeepCopy()
 	op, err := controllerutil.CreateOrUpdate(ctx, r.Client, cert, func() error {
@@ -292,3 +357,16 @@ func (r *Reconciler) getCertficateBytes(ctx context.Context, cert *certv1.Certif
 	}
 	return secret.Data[corev1.TLSCertKey], nil
 }
+
+func (r *Reconciler) deleteCertificate(ctx context.Context, cert *certv1.Certificate) error {
+	err := r.Client.Delete(ctx, cert)
+	if err != nil {
+		if kerrors.IsNotFound(err) {
+			return nil
+		}
+		r.Log.Error(err, "Could not delete certificate", "name", cert.Name, "namespace", cert.Namespace)
+		return err
+	}
+	r.Log.Info("deleted Certificate", "name", cert.Name, "namespace", cert.Namespace)
+	return nil
+}

internal/controllers/reconciler_test.go

@@ -1711,6 +1711,36 @@ func (c *controllerTest) commonTests() {
 				})
 			})
 		})
+		Context("with a modified CA certificate", func() {
+			var oldCerts []*certv1.Certificate
+			BeforeEach(func() {
+				t.objs = append(t.objs, t.NewCryostat().Object, t.OtherCAIssuer())
+				oldCerts = []*certv1.Certificate{
+					t.NewCryostatCert(),
+					t.NewReportsCert(),
+				}
+				// Add an annotation for each cert, the test will assert that
+				// the annotation is gone.
+				for i, cert := range oldCerts {
+					metav1.SetMetaDataAnnotation(&oldCerts[i].ObjectMeta, "bad", "cert")
+					t.objs = append(t.objs, cert)
+				}
+			})
+			JustBeforeEach(func() {
+				cr := t.getCryostatInstance()
+				for _, cert := range oldCerts {
+					// Make the old certs owned by the Cryostat CR
+					err := controllerutil.SetControllerReference(cr.Object, cert, t.Client.Scheme())
+					Expect(err).ToNot(HaveOccurred())
+					err = t.Client.Update(context.Background(), cert)
+					Expect(err).ToNot(HaveOccurred())
+				}
+				t.reconcileCryostatFully()
+			})
+			It("should recreate certificates", func() {
+				t.expectCertificates()
+			})
+		})
 
 		Context("reconciling a multi-namespace request", func() {
 			targetNamespaces := []string{"multi-test-one", "multi-test-two"}

internal/controllers/secrets.go

@@ -165,7 +165,10 @@ func (r *Reconciler) createOrUpdateSecret(ctx context.Context, secret *corev1.Se
 
 func (r *Reconciler) deleteSecret(ctx context.Context, secret *corev1.Secret) error {
 	err := r.Client.Delete(ctx, secret)
-	if err != nil && !kerrors.IsNotFound(err) {
+	if err != nil {
+		if kerrors.IsNotFound(err) {
+			return nil
+		}
 		r.Log.Error(err, "Could not delete secret", "name", secret.Name, "namespace", secret.Namespace)
 		return err
 	}

internal/test/resources.go

@@ -1039,6 +1039,22 @@ func (r *TestResources) NewCryostatCAIssuer() *certv1.Issuer {
 	}
 }
 
+func (r *TestResources) OtherCAIssuer() *certv1.Issuer {
+	return &certv1.Issuer{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      r.Name + "-ca",
+			Namespace: r.Namespace,
+		},
+		Spec: certv1.IssuerSpec{
+			IssuerConfig: certv1.IssuerConfig{
+				CA: &certv1.CAIssuer{
+					SecretName: r.Name + "-ca",
+				},
+			},
+		},
+	}
+}
+
 func (r *TestResources) newPVC(spec *corev1.PersistentVolumeClaimSpec, labels map[string]string,
 	annotations map[string]string) *corev1.PersistentVolumeClaim {
 	return &corev1.PersistentVolumeClaim{