From 4ce5ccaada6b51fb8418fa26a35294c4eb9eac74 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Fri, 20 Dec 2024 11:10:09 -0500 Subject: [PATCH] fix(tls): use fixed-length cert CommonNames (#968) * fix(tls): use fixed-length cert CommonNames * certificate change recreation * delete cert secret so they are also recreated --- ...yostat-operator.clusterserviceversion.yaml | 2 +- internal/controllers/certmanager.go | 25 +++++++- .../resource_definitions/certificates.go | 11 ++-- internal/controllers/constants/constants.go | 6 ++ internal/controllers/reconciler_test.go | 60 +++++++++++++++++++ internal/test/resources.go | 35 +++++++++-- 6 files changed, 125 insertions(+), 14 deletions(-) diff --git a/bundle/manifests/cryostat-operator.clusterserviceversion.yaml b/bundle/manifests/cryostat-operator.clusterserviceversion.yaml index 9aa268346..297b82986 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-10-10T18:16:26Z" + createdAt: "2024-11-05T19:02:47Z" description: JVM monitoring and profiling tool operatorframework.io/initialization-resource: |- { diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go index 35df44c10..9305c77ed 100644 --- a/internal/controllers/certmanager.go +++ b/internal/controllers/certmanager.go @@ -24,6 +24,7 @@ import ( "github.com/cryostatio/cryostat-operator/internal/controllers/common" resources "github.com/cryostatio/cryostat-operator/internal/controllers/common/resource_definitions" "github.com/cryostatio/cryostat-operator/internal/controllers/model" + "github.com/google/go-cmp/cmp" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" @@ -398,25 +399,43 @@ func (r *Reconciler) reconcileAgentCertificate(ctx context.Context, cert *certv1 return nil } +var errCertificateModified error = errors.New("certificate has been modified") + func (r *Reconciler) createOrUpdateCertificate(ctx context.Context, cert *certv1.Certificate, owner metav1.Object) error { - certSpec := cert.Spec.DeepCopy() + certCopy := cert.DeepCopy() op, err := controllerutil.CreateOrUpdate(ctx, r.Client, cert, func() error { if owner != nil { if err := controllerutil.SetControllerReference(owner, cert, r.Scheme); err != nil { return err } } - // Update Certificate spec - cert.Spec = *certSpec + + if cert.CreationTimestamp.IsZero() { + cert.Spec = certCopy.Spec + } else if !cmp.Equal(cert.Spec, certCopy.Spec) { + return errCertificateModified + } + return nil }) if err != nil { + if err == errCertificateModified { + return r.recreateCertificate(ctx, certCopy, owner) + } return err } r.Log.Info(fmt.Sprintf("Certificate %s", op), "name", cert.Name, "namespace", cert.Namespace) return nil } +func (r *Reconciler) recreateCertificate(ctx context.Context, cert *certv1.Certificate, owner metav1.Object) error { + err := r.deleteCertWithSecret(ctx, cert) + if err != nil { + return err + } + return r.createOrUpdateCertificate(ctx, cert, owner) +} + func newKeystoreSecret(cr *model.CryostatInstance) *corev1.Secret { return &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ diff --git a/internal/controllers/common/resource_definitions/certificates.go b/internal/controllers/common/resource_definitions/certificates.go index be757c748..55bf76453 100644 --- a/internal/controllers/common/resource_definitions/certificates.go +++ b/internal/controllers/common/resource_definitions/certificates.go @@ -20,6 +20,7 @@ import ( certv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" certMeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" "github.com/cryostatio/cryostat-operator/internal/controllers/common" + "github.com/cryostatio/cryostat-operator/internal/controllers/constants" "github.com/cryostatio/cryostat-operator/internal/controllers/model" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -62,7 +63,7 @@ func NewCryostatCACert(gvk *schema.GroupVersionKind, cr *model.CryostatInstance) Namespace: cr.InstallNamespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf("ca.%s.cert-manager", cr.Name), + CommonName: constants.CryostatCATLSCommonName, SecretName: common.ClusterUniqueNameWithPrefix(gvk, "ca", cr.Name, cr.InstallNamespace), IssuerRef: certMeta.ObjectReference{ Name: cr.Name + "-self-signed", @@ -79,7 +80,7 @@ func NewCryostatCert(cr *model.CryostatInstance, keystoreSecretName string) *cer Namespace: cr.InstallNamespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf("%s.%s.svc", cr.Name, cr.InstallNamespace), + CommonName: constants.CryostatTLSCommonName, DNSNames: []string{ cr.Name, fmt.Sprintf("%s.%s.svc", cr.Name, cr.InstallNamespace), @@ -115,7 +116,7 @@ func NewReportsCert(cr *model.CryostatInstance) *certv1.Certificate { Namespace: cr.InstallNamespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf("%s-reports.%s.svc", cr.Name, cr.InstallNamespace), + CommonName: constants.ReportsTLSCommonName, DNSNames: []string{ cr.Name + "-reports", fmt.Sprintf("%s-reports.%s.svc", cr.Name, cr.InstallNamespace), @@ -140,7 +141,7 @@ func NewAgentCert(cr *model.CryostatInstance, namespace string, gvk *schema.Grou Namespace: cr.InstallNamespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf("*.%s.pod", namespace), + CommonName: constants.AgentsTLSCommonName, DNSNames: []string{ fmt.Sprintf("*.%s.pod", namespace), }, @@ -163,7 +164,7 @@ func NewAgentProxyCert(cr *model.CryostatInstance) *certv1.Certificate { Namespace: cr.InstallNamespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf("%s-agent.%s.svc", cr.Name, cr.InstallNamespace), + CommonName: constants.AgentAuthProxyTLSCommonName, DNSNames: []string{ cr.Name + "-agent", fmt.Sprintf("%s-agent.%s.svc", cr.Name, cr.InstallNamespace), diff --git a/internal/controllers/constants/constants.go b/internal/controllers/constants/constants.go index 59b7bd9b2..5b3b26ec0 100644 --- a/internal/controllers/constants/constants.go +++ b/internal/controllers/constants/constants.go @@ -50,4 +50,10 @@ const ( targetNamespaceCRLabelPrefix = "operator.cryostat.io/" TargetNamespaceCRNameLabel = targetNamespaceCRLabelPrefix + "name" TargetNamespaceCRNamespaceLabel = targetNamespaceCRLabelPrefix + "namespace" + + CryostatCATLSCommonName = "cryostat-ca-cert-manager" + CryostatTLSCommonName = "cryostat" + ReportsTLSCommonName = "cryostat-reports" + AgentsTLSCommonName = "cryostat-agent" + AgentAuthProxyTLSCommonName = "cryostat-agent-proxy" ) diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go index 612c69f4e..40a71f33f 100644 --- a/internal/controllers/reconciler_test.go +++ b/internal/controllers/reconciler_test.go @@ -1795,6 +1795,66 @@ func (c *controllerTest) commonTests() { t.expectCertificates() }) }) + Context("with modified certificates", func() { + var oldCerts []*certv1.Certificate + BeforeEach(func() { + t.objs = append(t.objs, t.NewCryostat().Object, t.OtherCAIssuer()) + oldCerts = []*certv1.Certificate{ + t.OtherCACert(), + t.OtherAgentProxyCert(), + t.OtherCryostatCert(), + t.OtherReportsCert(), + } + // 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("with a modified certificate TLS CommonName", func() { + var oldCerts []*certv1.Certificate + BeforeEach(func() { + oldCerts = []*certv1.Certificate{ + t.NewCryostatCert(), + t.NewReportsCert(), + t.NewAgentProxyCert(), + } + t.objs = append(t.objs, t.NewCryostat().Object, t.OtherCAIssuer()) + for _, cert := range oldCerts { + 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/test/resources.go b/internal/test/resources.go index bc74bc9fe..6c1adf3d2 100644 --- a/internal/test/resources.go +++ b/internal/test/resources.go @@ -1053,7 +1053,7 @@ func (r *TestResources) NewCryostatCert() *certv1.Certificate { Namespace: r.Namespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf(r.Name+".%s.svc", r.Namespace), + CommonName: "cryostat", DNSNames: []string{ r.Name, fmt.Sprintf(r.Name+".%s.svc", r.Namespace), @@ -1084,6 +1084,12 @@ func (r *TestResources) NewCryostatCert() *certv1.Certificate { } } +func (r *TestResources) OtherCryostatCert() *certv1.Certificate { + cert := r.NewCryostatCert() + cert.Spec.CommonName = fmt.Sprintf("%s.%s.svc", r.Name, r.Namespace) + return cert +} + func (r *TestResources) NewReportsCert() *certv1.Certificate { return &certv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ @@ -1091,7 +1097,7 @@ func (r *TestResources) NewReportsCert() *certv1.Certificate { Namespace: r.Namespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf(r.Name+"-reports.%s.svc", r.Namespace), + CommonName: "cryostat-reports", DNSNames: []string{ r.Name + "-reports", fmt.Sprintf(r.Name+"-reports.%s.svc", r.Namespace), @@ -1110,6 +1116,12 @@ func (r *TestResources) NewReportsCert() *certv1.Certificate { } } +func (r *TestResources) OtherReportsCert() *certv1.Certificate { + cert := r.NewReportsCert() + cert.Spec.CommonName = fmt.Sprintf("%s-reports.%s.svc", r.Name, r.Namespace) + return cert +} + func (r *TestResources) NewAgentProxyCert() *certv1.Certificate { return &certv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ @@ -1117,7 +1129,7 @@ func (r *TestResources) NewAgentProxyCert() *certv1.Certificate { Namespace: r.Namespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf(r.Name+"-agent.%s.svc", r.Namespace), + CommonName: "cryostat-agent-proxy", DNSNames: []string{ r.Name + "-agent", fmt.Sprintf(r.Name+"-agent.%s.svc", r.Namespace), @@ -1136,6 +1148,12 @@ func (r *TestResources) NewAgentProxyCert() *certv1.Certificate { } } +func (r *TestResources) OtherAgentProxyCert() *certv1.Certificate { + cert := r.NewAgentProxyCert() + cert.Spec.CommonName = fmt.Sprintf("%s-agent.%s.svc", r.Name, r.Namespace) + return cert +} + func (r *TestResources) NewCACert() *certv1.Certificate { return &certv1.Certificate{ ObjectMeta: metav1.ObjectMeta{ @@ -1143,7 +1161,7 @@ func (r *TestResources) NewCACert() *certv1.Certificate { Namespace: r.Namespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf("ca.%s.cert-manager", r.Name), + CommonName: "cryostat-ca-cert-manager", SecretName: r.getClusterUniqueNameForCA(), IssuerRef: certMeta.ObjectReference{ Name: r.Name + "-self-signed", @@ -1153,6 +1171,13 @@ func (r *TestResources) NewCACert() *certv1.Certificate { } } +func (r *TestResources) OtherCACert() *certv1.Certificate { + cert := r.NewCACert() + cert.Spec.CommonName = fmt.Sprintf("ca.%s.cert-manager", r.Name) + cert.Spec.SecretName = r.Name + "-ca" + return cert +} + func (r *TestResources) NewAgentCert(namespace string) *certv1.Certificate { name := r.getClusterUniqueNameForAgent(namespace) return &certv1.Certificate{ @@ -1161,7 +1186,7 @@ func (r *TestResources) NewAgentCert(namespace string) *certv1.Certificate { Namespace: r.Namespace, }, Spec: certv1.CertificateSpec{ - CommonName: fmt.Sprintf("*.%s.pod", namespace), + CommonName: "cryostat-agent", DNSNames: []string{ fmt.Sprintf("*.%s.pod", namespace), },