K8SPXC-1847: detect CA rotation and re-issue leaf TLS certificates - #2412
Conversation
|
|
larainema
changed the title
| if k8serr.IsNotFound(err) { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("get CA secret %s: %v", caSecretName, err) |
There was a problem hiding this comment.
please wrap errors here and lines below. preferably with errors.Wrap or fmt.Errorf("...: %w")
egegunes
changed the title
fa0054b to
9578100
Compare
|
@larainema I wonder if this is reproducible by simple running |
@egegunes I think this is reproducible with cmctl renew — specifically by renewing the CA certificate cmctl renew <cluster-name>-ca-cert(Note: I think we can trigger renewal without installing kubectl annotate certificate <cluster-name>-ca-cert cert-manager.io/renew-after=$(date -u +%Y-%m-%dT%H:%M:%SZ) --overwrite |
|
@larainema could you please confirm this triggers the renewal of ca.crt? if it does, we need to add this one of the cert-manager tests. probably tls-issue-cert-manager |
@egegunes I tested this on my environment. Here's what I found: The kubectl annotate cert-manager.io/renew-after approach does NOT work — this annotation is not recognized by cert-manager (it's not in the official annotations list). I was mistaken in my earlier comment. cmctl renew works by setting an Issuing status condition, not an annotation. cmctl renew works, but with an important caveat: by default, cert-manager reuses the existing private key on renewal. This means the CA cert gets a new serial/dates but the same key pair — so the leaf certs remain valid and the bug is NOT triggered. To fully reproduce the bug, the CA Certificate needs privateKey.rotationPolicy: Always, which forces cert-manager to generate a new key pair on renewal. With that set: # Set rotation policy (one-time setup for the test)
kubectl patch certificate <cluster>-ca-cert --type=merge \ -p '{"spec":{"privateKey":{"rotationPolicy":"Always"}}}'
# Trigger renewal
cmctl renew <cluster>-ca-certAfter this, I confirmed: ✅ CA secret gets a new key pair (different modulus/SKI) |
|
thanks for checking @larainema. can you add this case into tls-issue-cert-manager e2e test? if you need any guidance please let me know. |
4a02717 to
db7f4f2
Compare
1d34c9d to
7a4a05c
Compare
When cert-manager renews a self-signed CA certificate, it generates a new key pair. The leaf certificates (ssl and ssl-internal) become invalid because they were signed by the old CA, but reconcileSSL short-circuits when their secrets already exist, never detecting the CA mismatch. Add reconcileCARotation() that compares ca.crt in leaf secrets against the current CA secret tls.crt. On mismatch, delete the stale leaf Secrets so cert-manager re-issues them from the still-existing Certificate CRs using the new CA. Pod restarts are handled automatically by the existing percona.com/ssl-hash annotation mechanism in updatePod(). Fixes percona#2411
7a4a05c to
9379593
Compare
| desc 'restart all PXC and HAProxy pods to pick up new certs simultaneously' | ||
| # A rolling restart cannot work for CA rotation because pods with new CA | ||
| # certs cannot do SSL handshakes with pods still running old CA certs. | ||
| # Delete all pods at once so they all restart with the new certs. | ||
| sleep 30 # wait for kubelet to sync new cert files into volume mounts | ||
| kubectl_bin delete pods -l app.kubernetes.io/instance=$cluster,app.kubernetes.io/managed-by=percona-xtradb-cluster-operator --force --grace-period=0 |
There was a problem hiding this comment.
@larainema I wonder if it's possible to do it without downtime. Do we have the old ca.crt accessible to the operator after rotation? Maybe we can keep both CA certs available to allow rolling restart?
There was a problem hiding this comment.
@egegunes Good point — a zero-downtime rolling restart is technically achievable, but it requires a more involved CA-bundle approach. Here's the trade-off:
Why a plain rolling restart fails today
After CA rotation, ca.crt in the leaf secrets contains only the new CA. During a rolling restart:
Pod-0 restarts → loads new leaf cert (signed by new CA) and trusts only new CA
Pod-1/Pod-2 still in memory → hold old leaf certs (signed by old CA) and trust only old CA
Galera SST/IST handshake between Pod-0 ↔ Pod-1 fails in both directions (neither side trusts the peer's cert chain) → cluster never re-syncs
Zero-downtime path (CA bundle)
To make rolling restart safe, every pod's ca.crt would need to trust both the old and new CA simultaneously during the transition window:
On CA rotation detection, build a bundle containing old_ca || new_ca and write it to the leaf secrets' ca.crt field (instead of just new_ca).
Trigger a rolling restart — every pod now trusts both CAs, so old-leaf and new-leaf pods can mutually authenticate.
After all pods are restarted with the new leaf cert, run a second reconcile that prunes the old CA from ca.crt, leaving only new_ca.
Optionally, do another rolling restart to drop trust in the old CA (purely a hygiene step).
Why I didn't include this in this PR
We do still have the old CA accessible right after rotation: cert-manager keeps it in the previous tls.crt data of the leaf secrets (we read it in reconcileCARotation precisely to detect the mismatch). So the inputs are available.
However, implementing the bundle approach correctly requires:
A new state machine in the operator (bundling → rotating → pruning) to know which phase we're in across reconciles.
Patching cert-manager Certificate resources or post-processing the secrets after cert-manager writes them (cert-manager owns ca.crt, so we'd be fighting the controller unless we add additionalOutputFormats or a webhook).
Coordination with the existing ssl-hash annotation flow so the rolling restart fires at the right phase.
I felt this expanded the scope significantly beyond the original bug (operator silently keeping stale leaf certs after CA rotation).
Proposal
If you're OK with it, I'd suggest landing this PR as-is to fix the correctness bug (no more silent stale certs), then opening a follow-up issue/PR for the zero-downtime CA-bundle work. That way users running CA rotation get a working, consistent state today, and the optimization can be designed and reviewed separately.
WDYT?
There was a problem hiding this comment.
@larainema yes, implementing in another PR sounds reasonable
There was a problem hiding this comment.
| // Skip manually created secrets. | ||
| if sslSecret.Annotations["cert-manager.io/issuer-kind"] == "" { | ||
| return nil | ||
| } | ||
|
|
||
| caSecretName := cr.Name + "-ca-cert" |
There was a problem hiding this comment.
Lets move this to the naming package, might require refactoring in the other cert issuing functions too
There was a problem hiding this comment.
Done in 60c8687. Added [naming.CAIssuerName], [naming.IssuerName], [naming.CACertificateName] and switched both createSSLByCertManager and deleteCerts to use them.
| certNames = append(certNames, cr.Name+"-ssl") | ||
| } | ||
| if sslInternalMismatch && cr.Spec.PXC.SSLSecretName != cr.Spec.PXC.SSLInternalSecretName { | ||
| certNames = append(certNames, cr.Name+"-ssl-internal") |
There was a problem hiding this comment.
Lets move it to naming package too
There was a problem hiding this comment.
Done in 60c8687. Added [naming.SSLCertificateName] / [naming.SSLInternalCertificateName] and switched both createSSLByCertManager, reconcileCARotation, and deleteCerts to use them.
| if sslMismatch { | ||
| certNames = append(certNames, cr.Name+"-ssl") | ||
| } | ||
| if sslInternalMismatch && cr.Spec.PXC.SSLSecretName != cr.Spec.PXC.SSLInternalSecretName { |
There was a problem hiding this comment.
Why do we need the cr.Spec.PXC.SSLSecretName != cr.Spec.PXC.SSLInternalSecretName check?
There was a problem hiding this comment.
This is for the shared-secret case. When SSLSecretName == SSLInternalSecretName we only manage a single leaf Certificate (see createSSLByCertManager, which short-circuits and waits on a single secret), so the SSLCertificateName entry above already covers it. Without this guard we’d try to re-issue a non-existent *-ssl-internal Certificate. I added a comment explaining this in 60c8687.
| alreadyIssuing := false | ||
| for _, c := range cert.Status.Conditions { | ||
| if c.Type == cm.CertificateConditionIssuing && c.Status == cmmeta.ConditionTrue { | ||
| alreadyIssuing = true | ||
| break | ||
| } | ||
| } | ||
| if alreadyIssuing { | ||
| log.Info("Certificate already issuing, skipping", "certificate", name) | ||
| continue | ||
| } | ||
|
|
||
| log.Info("Triggering leaf certificate re-issuance", "certificate", name) | ||
|
|
||
| // Set the Issuing condition to True to trigger cert-manager re-issuance. | ||
| issuing := cm.CertificateCondition{ | ||
| Type: cm.CertificateConditionIssuing, | ||
| Status: cmmeta.ConditionTrue, | ||
| Reason: "ManuallyTriggered", | ||
| Message: "Re-issuing due to CA rotation", | ||
| LastTransitionTime: &now, | ||
| } | ||
|
|
||
| found := false | ||
| for i, c := range cert.Status.Conditions { | ||
| if c.Type == cm.CertificateConditionIssuing { | ||
| cert.Status.Conditions[i] = issuing | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| cert.Status.Conditions = append(cert.Status.Conditions, issuing) | ||
| } |
There was a problem hiding this comment.
Can't we just use apiutil.SetCertificateCondition instead?
See how cmctl does it - https://github.com/cert-manager/cmctl/blob/main/pkg/renew/renew.go#L217C2-L217C33
There was a problem hiding this comment.
Good call — switched to [cmapiutil.SetCertificateCondition] and [cmapiutil.CertificateHasCondition] in 60c8687, matching what cmctl renew does.
… handling - Move cert-manager Certificate and Issuer name helpers into the naming package (CAIssuerName, IssuerName, CACertificateName, SSLCertificateName, SSLInternalCertificateName) and use them from createSSLByCertManager, reconcileCARotation, and deleteCerts. - In reconcileCARotation, replace the inline CertificateConditionIssuing mutation with cert-manager's apiutil.SetCertificateCondition and CertificateHasCondition helpers, matching the behaviour of cmctl renew. - Document why the SSLSecretName != SSLInternalSecretName guard is needed when collecting certificates to re-issue (shared-secret single-Certificate case).
commit: a49dbbc |
|
Hi @larainema, thank you for your contribution |
Resolve conflicts with percona#2412 (CA rotation) which has been merged to main: - pkg/controller/pxc/tls.go: combine reconcileCARotation + naming helpers from percona#2412 with reconcileCertManagerCertificateSpecs + build helpers from this PR. Build helpers now use the shared naming constants. - e2e-tests/tls-issue-cert-manager/run: keep cert-manager readiness check, inline cluster creation, and full-pod restart for CA rotation from percona#2412; preserve certificate spec update test from this PR.
7 participants
Problem
When cert-manager renews a self-signed CA certificate (created by the operator), it generates a new key pair. The leaf TLS certificates (
sslandssl-internal) become invalid because they were signed by the old CA.However,
reconcileSSL()short-circuits with an earlyreturn nilwhen both leaf secrets already exist, never detecting that the CA has changed. The leaf certs remain signed by the old (now-expired) CA, eventually causing TLS handshake failures across the PXC cluster.Fixes #2411
Root Cause
Three interacting issues:
reconcileSSLearly return — when both SSL secrets exist, the function returns immediately without checking if the CA is still valid or consistent.createSSLByCertManagerusesr.client.Create()withIsAlreadyExistsguard, so it never updates existing Certificate CRs.Fix
Add
reconcileCARotation()that runs when both leaf secrets exist. It:tls.crtagainstca.crtin each leaf secret usingbytes.EqualcreateSSLByCertManagerwhich re-creates them signed by the new CAPod rolling restarts are handled automatically by the existing
percona.com/ssl-hashannotation mechanism inupdatePod().Edge Cases Handled
spec.tls.issuerConfset) — skipped, CA is user-managedcert-manager.io/issuer-kindannotation) — skippedSSLSecretName == SSLInternalSecretName) — only one deletion targetIsNotFoundguards on all deletes