Skip to content

Commit

Permalink
fix(tls): use fixed-length cert CommonNames (#968)
Browse files Browse the repository at this point in the history 
* fix(tls): use fixed-length cert CommonNames

* certificate change recreation

* delete cert secret so they are also recreated
  • Loading branch information
@andrewazores
andrewazores authored Dec 20, 2024
1 parent d058b54 commit 4ce5cca
Show file tree
Hide file tree
Showing 6 changed files with 125 additions and 14 deletions.
2 changes: 1 addition & 1 deletion bundle/manifests/cryostat-operator.clusterserviceversion.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: |-
{
Expand Down
25 changes: 22 additions & 3 deletions internal/controllers/certmanager.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{
Expand Down
11 changes: 6 additions & 5 deletions internal/controllers/common/resource_definitions/certificates.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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",
Expand All @@ -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),
Expand Down Expand Up @@ -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),
Expand All @@ -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),
},
Expand All @@ -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),
Expand Down
6 changes: 6 additions & 0 deletions internal/controllers/constants/constants.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
60 changes: 60 additions & 0 deletions internal/controllers/reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"}
Expand Down
35 changes: 30 additions & 5 deletions internal/test/resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down Expand Up @@ -1084,14 +1084,20 @@ 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{
Name: r.Name + "-reports",
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),
Expand All @@ -1110,14 +1116,20 @@ 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{
Name: r.Name + "-agent-proxy",
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),
Expand All @@ -1136,14 +1148,20 @@ 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{
Name: r.Name + "-ca",
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",
Expand All @@ -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{
Expand All @@ -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),
},
Expand Down

0 comments on commit 4ce5cca

Please sign in to comment.