fix(tls): use fixed-length cert CommonNames #968
Conversation
|
/build_test |
|
|
There was a problem hiding this comment.
Yeah, I'm getting a 503 error after upgrading using the steps in #897. I think the easiest fix would be expand the check from #897 to compare the entire certificate spec and not just the secret name. This doesn't solve the certificate expiry issue (#119), but at least should get us back to the current state.
|
The check from #897 looks like it's just checking the SecretName referenced by the CA Issuer, and the Issuer's Spec only really contains the SecretName. If I'm understanding the problem correctly, it's that the Certificates' Specs have changed (because the CommonName field has changed) - the Issuers and theirs Specs actually haven't changed. But I suppose on upgrade, a new CA gets created again and the old one needs to be invalidated and all its Certificates deleted? |
Right that's what needs to happen on upgrade. You're right, detecting this will be a little more complicated than just modifying the check done in #897. One possibility is treating the certificate spec like an immutable field, and calling the cryostat-operator/internal/controllers/reconciler.go Lines 512 to 518 in 0ec9a84 |
|
I found that the |
|
/build_test |
|
|
|
/build_test |
|
|
|
I'm still not really sure what's causing diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go
index 0be2db9..fcf156d 100644
--- a/internal/controllers/certmanager.go
+++ b/internal/controllers/certmanager.go
@@ -410,6 +410,7 @@ func (r *Reconciler) createOrUpdateCertificate(ctx context.Context, cert *certv1
}
}
+ fmt.Printf("Cert Name: %s\nDiff:\n%s\n", cert.Name, cmp.Diff(cert.Spec, *specCopy))
if !cmp.Equal(cert.Spec.CommonName, specCopy.CommonName) &&
!cmp.Equal(cert.Spec.DNSNames, specCopy.DNSNames) &&
!cmp.Equal(cert.Spec.Duration, specCopy.Duration) &&I tried running through the #897 upgrade steps again with this log line added so that I could spot whatever difference there is in the specs, but to my surprise: ie. only the CA had a couple of small and expected changes. That doesn't seem to match the Any ideas on what I could be missing? Or, is the current property-by-property check OK? if !cmp.Equal(cert.Spec.CommonName, specCopy.CommonName) &&
!cmp.Equal(cert.Spec.DNSNames, specCopy.DNSNames) &&
!cmp.Equal(cert.Spec.Duration, specCopy.Duration) &&
!cmp.Equal(cert.Spec.RenewBefore, specCopy.RenewBefore) &&
!cmp.Equal(cert.Spec.IPAddresses, specCopy.IPAddresses) &&
!cmp.Equal(cert.Spec.URIs, specCopy.URIs) &&
!cmp.Equal(cert.Spec.EmailAddresses, specCopy.EmailAddresses) &&
!cmp.Equal(cert.Spec.SecretName, specCopy.SecretName) &&
!cmp.Equal(cert.Spec.SecretTemplate, specCopy.SecretTemplate) &&
!cmp.Equal(cert.Spec.IssuerRef, specCopy.IssuerRef) &&
!cmp.Equal(cert.Spec.IsCA, specCopy.IsCA) &&
!cmp.Equal(cert.Spec.Keystores, specCopy.Keystores) &&
!cmp.Equal(cert.Spec.PrivateKey, specCopy.PrivateKey) &&
!cmp.Equal(cert.Spec.EncodeUsagesInRequest, specCopy.EncodeUsagesInRequest) &&
!cmp.Equal(cert.Spec.Usages, specCopy.Usages) {
return errCertificateModified
} |
| 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 |
There was a problem hiding this comment.
I think we still need this line to set the Certificate spec?
There was a problem hiding this comment.
I'm somewhat forgetting what I was doing here, but I think the point was that we check for spec changes and either do nothing (spec is functionally equivalent), or delete-and-recreate. So there is no more reconciliation of the states within this function, just the errCertificateModified status.
There was a problem hiding this comment.
Editted: I combined comments and organized my thoughts a bit for easier read :D
I guess we still need it in cases where the certificate is not yet available? That is: The certificate is being recreated (i.e. deleted and created).
either do nothing (spec is functionally equivalent), or delete-and-recreate.
In recreateCertificate, we are calling func createOrUpdateCertificate again so it does need the new cert spec to be set, otherwise it will use the old spec?
I guess that's explained the weird behavior?
I'm still not really sure what's causing cmp.Equal() to return false unexpectedly.
There was a problem hiding this comment.
The operator is installed for the first time.
That said, we will also need to check if the cert exists before doing comparison? Something like:
There was a problem hiding this comment.
I think I tried that before, though it may not have been committed to git history. Taking another stab at it:
- if !cmp.Equal(cert.Spec.CommonName, specCopy.CommonName) &&
- !cmp.Equal(cert.Spec.DNSNames, specCopy.DNSNames) &&
- !cmp.Equal(cert.Spec.Duration, specCopy.Duration) &&
- !cmp.Equal(cert.Spec.RenewBefore, specCopy.RenewBefore) &&
- !cmp.Equal(cert.Spec.IPAddresses, specCopy.IPAddresses) &&
- !cmp.Equal(cert.Spec.URIs, specCopy.URIs) &&
- !cmp.Equal(cert.Spec.EmailAddresses, specCopy.EmailAddresses) &&
- !cmp.Equal(cert.Spec.SecretName, specCopy.SecretName) &&
- !cmp.Equal(cert.Spec.SecretTemplate, specCopy.SecretTemplate) &&
- !cmp.Equal(cert.Spec.IssuerRef, specCopy.IssuerRef) &&
- !cmp.Equal(cert.Spec.IsCA, specCopy.IsCA) &&
- !cmp.Equal(cert.Spec.Keystores, specCopy.Keystores) &&
- !cmp.Equal(cert.Spec.PrivateKey, specCopy.PrivateKey) &&
- !cmp.Equal(cert.Spec.EncodeUsagesInRequest, specCopy.EncodeUsagesInRequest) &&
- !cmp.Equal(cert.Spec.Usages, specCopy.Usages) {
+ if cert.CreationTimestamp.IsZero() {
+ cert.Spec = *specCopy
+ } else if !cmp.Equal(cert.Spec, specCopy) {
return errCertificateModified
}make test-envtest
This results in a stack overflow :-) skipping the tests and deploying it regardless also shows bad behaviour of the operator controller Pod, which seems to hang at the same point where the envtest crashes, and fails to ever bring up a Cryostat instance. Even after deleting the Cryostat CR, the operator controller Pod logs show it hanging at the same spot, and the deletion finalizer doesn't seem to run.
So I think the cert object's creation timestamp isn't getting set as expected, and this is probably falling into the !cmp.Equal deep equality strangeness, leading to it recursively calling between createOrUpdateCertificate and recreateCertificate.
There was a problem hiding this comment.
Do you think it is due to the recreateCertificate is calling createOrUpdateCertificate with the old certificate? Since we are trying to delete the old cert, but uses it again to call createOrUpdateCertificate.
func (r *Reconciler) recreateCertificate(ctx context.Context, cert *certv1.Certificate, owner metav1.Object) error {
err := r.deleteCertWithSecret(ctx, cert)
if err != nil {
return err
}
// The cert here is the old one. It should be the new cert with new specs?
return r.createOrUpdateCertificate(ctx, cert, owner)
}There was a problem hiding this comment.
I was thinking: if the certificate exists when reconciling, the cert pointer is updated with existing specs. Thus, it now has the old specs. By using it in subsequent steps, we are actually trying to reconcile the old cert.
There was a problem hiding this comment.
diff --git a/internal/controllers/certmanager.go b/internal/controllers/certmanager.go
index e156309..9305c77 100644
--- a/internal/controllers/certmanager.go
+++ b/internal/controllers/certmanager.go
@@ -402,7 +402,7 @@ func (r *Reconciler) reconcileAgentCertificate(ctx context.Context, cert *certv1
var errCertificateModified error = errors.New("certificate has been modified")
func (r *Reconciler) createOrUpdateCertificate(ctx context.Context, cert *certv1.Certificate, owner metav1.Object) error {
- specCopy := 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 {
@@ -410,21 +410,9 @@ func (r *Reconciler) createOrUpdateCertificate(ctx context.Context, cert *certv1
}
}
- if !cmp.Equal(cert.Spec.CommonName, specCopy.CommonName) &&
- !cmp.Equal(cert.Spec.DNSNames, specCopy.DNSNames) &&
- !cmp.Equal(cert.Spec.Duration, specCopy.Duration) &&
- !cmp.Equal(cert.Spec.RenewBefore, specCopy.RenewBefore) &&
- !cmp.Equal(cert.Spec.IPAddresses, specCopy.IPAddresses) &&
- !cmp.Equal(cert.Spec.URIs, specCopy.URIs) &&
- !cmp.Equal(cert.Spec.EmailAddresses, specCopy.EmailAddresses) &&
- !cmp.Equal(cert.Spec.SecretName, specCopy.SecretName) &&
- !cmp.Equal(cert.Spec.SecretTemplate, specCopy.SecretTemplate) &&
- !cmp.Equal(cert.Spec.IssuerRef, specCopy.IssuerRef) &&
- !cmp.Equal(cert.Spec.IsCA, specCopy.IsCA) &&
- !cmp.Equal(cert.Spec.Keystores, specCopy.Keystores) &&
- !cmp.Equal(cert.Spec.PrivateKey, specCopy.PrivateKey) &&
- !cmp.Equal(cert.Spec.EncodeUsagesInRequest, specCopy.EncodeUsagesInRequest) &&
- !cmp.Equal(cert.Spec.Usages, specCopy.Usages) {
+ if cert.CreationTimestamp.IsZero() {
+ cert.Spec = certCopy.Spec
+ } else if !cmp.Equal(cert.Spec, certCopy.Spec) {
return errCertificateModified
}
@@ -432,7 +420,7 @@ func (r *Reconciler) createOrUpdateCertificate(ctx context.Context, cert *certv1
})
if err != nil {
if err == errCertificateModified {
- return r.recreateCertificate(ctx, cert, owner)
+ return r.recreateCertificate(ctx, certCopy, owner)
}
return err
}This seems to pass the env-test. I won't have a chance to test out today on working cluster but will verify later :D But the idea is to deep copy the entire certificate and pass it to recreateCertificate.
internal/controllers/certmanager.go
Outdated
| // Update Certificate spec | ||
| cert.Spec = *certSpec | ||
|
|
||
| if !cmp.Equal(cert.Spec.CommonName, specCopy.CommonName) && |
There was a problem hiding this comment.
Any ideas on what I could be missing? Or, is the current property-by-property check OK?
I just have a question here: since we are treating the spec as immutable, any changes to the spec fields should return error errCertificateModified?
There was a problem hiding this comment.
Yes, I think that's the idea.
internal/controllers/certmanager.go
Outdated
| 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.deleteCertificate(ctx, cert) |
There was a problem hiding this comment.
Do we also need to delete the certificate's secret?
There was a problem hiding this comment.
I think that makes sense.
|
I noticed one other funny effect from the #897 upgrade steps - the old separate Grafana Route gets left behind. That makes sense since it was an upgrade from 2.4.0 to 4.0.0-dev, and in 2.x we exposed Grafana as a separate Route. In 3.0 it's exposed as a path on the auth proxy with the same Route. I think the old leftover Route is harmless, but also useless. |
Oh right, how about we delete the old Grafana route in the reconciler, similar to how we do it for CORS modifications? cryostat-operator/internal/controllers/openshift.go Lines 41 to 42 in 0ec9a84 |
diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go
index 612c69f..dce34d5 100644
--- a/internal/controllers/reconciler_test.go
+++ b/internal/controllers/reconciler_test.go
@@ -1795,6 +1795,34 @@ func (c *controllerTest) commonTests() {
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"}Is this something like what you're thinking of? The |
Oh yup, I guess what I had in mind was to have a scenario where there are existing/old certificates (i.e. with specs defined from previous operator versions) and let the test expect if these certificates are reconciled correctly. Since the changes in this PR apply to all managed certificates, we do like something below. What do you think? diff --git a/internal/controllers/reconciler_test.go b/internal/controllers/reconciler_test.go
index 612c69f..0db9d3f 100644
--- a/internal/controllers/reconciler_test.go
+++ b/internal/controllers/reconciler_test.go
@@ -1765,13 +1765,16 @@ func (c *controllerTest) commonTests() {
})
})
})
- Context("with a modified CA certificate", func() {
+
+ 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.NewCryostatCert(),
- t.NewReportsCert(),
+ t.OtherCACert(),
+ t.OtherAgentProxyCert(),
+ t.OtherCryostatCert(),
+ t.OtherReportsCert(),
}
// Add an annotation for each cert, the test will assert that
// the annotation is gone.
diff --git a/internal/test/resources.go b/internal/test/resources.go
index bcad916..b5e89a0 100644
--- a/internal/test/resources.go
+++ b/internal/test/resources.go
@@ -1058,6 +1058,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{
@@ -1084,6 +1090,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{
@@ -1110,6 +1122,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{
@@ -1127,6 +1145,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{ |
| @@ -1795,6 +1795,66 @@ func (c *controllerTest) commonTests() { | |||
| t.expectCertificates() | |||
| }) | |||
| }) | |||
| Context("with modified certificates", func() { | |||
There was a problem hiding this comment.
I suppose this case also cover the previous test context with a modified CA certificate?
There was a problem hiding this comment.
I think it is effectively the same right now, but I thought I may as well retain the two different contexts since the cert.Spec.CommonNames differ in the setup - even though the execution and assertions are the same.
Welcome to Cryostat! 👋
Before contributing, make sure you have:
mainbranch[chore, ci, docs, feat, fix, test]git commit -S -m "YOUR_COMMIT_MESSAGE"Fixes: #964
Description of the change:
This change adds allows the users to provide...
Motivation for the change:
This change is helpful because users may want to...
How to manually test: