From fd1becca584d4dc0de1c865b56eeefea324b19e1 Mon Sep 17 00:00:00 2001 From: Elliott Baron Date: Mon, 17 Jun 2024 11:31:47 -0400 Subject: [PATCH] fix(tls): detect modified CA and reissue certs (#897) * fix(tls): detect modified CA and reissue certs * Ignore certificate not found when deleting --- ...yostat-operator.clusterserviceversion.yaml | 2 +- internal/controllers/certmanager.go | 78 +++++++++++++++++++ internal/controllers/reconciler_test.go | 30 +++++++ internal/controllers/secrets.go | 5 +- internal/test/resources.go | 16 ++++ 5 files changed, 129 insertions(+), 2 deletions(-) diff --git a/bundle/manifests/cryostat-operator.clusterserviceversion.yaml b/bundle/manifests/cryostat-operator.clusterserviceversion.yaml index 9a446c46..a219bb8d 100644 --- a/bundle/manifests/cryostat-operator.clusterserviceversion.yaml +++ b/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: |- { diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index a355dcf9..33c60623 100644 --- a/internal/controllers/certmanager.go +++ b/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 +} diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go index 18551529..35a0e78a 100644 --- a/internal/controllers/reconciler_test.go +++ b/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"} diff --git a/internal/controllers/secrets.go b/internal/controllers/secrets.go index d0074754..a2862d1d 100644 --- a/internal/controllers/secrets.go +++ b/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 } diff --git a/internal/test/resources.go b/internal/test/resources.go index 11c6b0d3..31f1d002 100644 --- a/internal/test/resources.go +++ b/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{