BUGFIX: issuer-ref and certificate-name annotations are incorrectly updated on Secrets #6147

inteon · 2023-06-13T14:48:35Z

The ensureSecretData function incorrectly uses the certificate name and issuer ref of the current Certificate resource version when creating a patch for the annotations of a Secret.
Instead, the function should re-apply the existing annotations.
New values for these annotations should only be set by the issueCertificate function when the certificate is actually being issued.

The current behavior could cause the annotations of the secret to be changed when the Certificate resource changes. This can then result in the Trigger controller not picking up that eg. the issuer changed & not reconciling because of that.

Kind

/kind bug

Release Note

BUGFIX: the issuer and certificate-name annotations on a Secret were incorrectly updated when other fields are changed.

inteon · 2023-06-13T14:50:28Z

internal/controller/certificates/secrets.go

-	annotations[cmapi.CertificateNameKey] = crt.Name
-	annotations[cmapi.IssuerNameAnnotationKey] = crt.Spec.IssuerRef.Name
-	annotations[cmapi.IssuerKindAnnotationKey] = apiutil.IssuerKind(crt.Spec.IssuerRef)
-	annotations[cmapi.IssuerGroupAnnotationKey] = crt.Spec.IssuerRef.Group


These 4 fields are now set by the setValues function.

inteon · 2023-06-13T14:52:32Z

pkg/controller/certificates/issuing/internal/secret.go

-	PrivateKey, Certificate, CA []byte
+	PrivateKey, Certificate, CA         []byte
+	CertificateName                     string
+	IssuerName, IssuerKind, IssuerGroup string


We now explicitly pass the CertificateName, IssuerName, IssuerKind and IssuerGroup values to setValues instead of reading these values from the cmapi.Certificate resource, since cmapi.Certificate might have been updated since the secret was created.

inteon · 2023-06-13T14:53:35Z

pkg/controller/certificates/issuing/secret_manager.go

+		CertificateName: secret.Annotations[cmapi.CertificateNameKey],
+		IssuerName:      secret.Annotations[cmapi.IssuerNameAnnotationKey],
+		IssuerKind:      secret.Annotations[cmapi.IssuerKindAnnotationKey],
+		IssuerGroup:     secret.Annotations[cmapi.IssuerGroupAnnotationKey],


We now use the existing annotations as input. This prevents this function from overwriting the existing annotations with new values that are based on the latest version of the certificate (which was not used to issue the certificate in the Secret).

… on Secrets to be correct when being updated. Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon · 2023-06-13T14:56:35Z

pkg/controller/certificates/issuing/internal/secret.go

+		secret.Labels = make(map[string]string)
+	}
+
+	if crt.Spec.SecretTemplate != nil {


I changed the order: first, we apply the SecretTemplate; afterwards, we apply the cert-manager annotations.
This prevents us from overwriting cert-manager annotations using the SecretTemplate.

I'm sure that this was the original behaviour when @jonathansp originally created the feature. See

feat: add support to secretTemplates #3828 (review)

I wonder when and how this bug got introduced?
Which versions are affected? Do we need to fix this bug in a separate PR and backport to the affected versions?
Is there a unit test for this? If not, please add one.

This issue was introduced in #4746

inteon · 2023-06-13T14:57:59Z

pkg/controller/certificates/issuing/internal/secret.go

+	secret.Annotations[cmapi.CertificateNameKey] = data.CertificateName
+	secret.Annotations[cmapi.IssuerNameAnnotationKey] = data.IssuerName
+	secret.Annotations[cmapi.IssuerKindAnnotationKey] = data.IssuerKind
+	secret.Annotations[cmapi.IssuerGroupAnnotationKey] = data.IssuerGroup


Previously, AnnotationsForCertificate was setting these annotations. In this PR, I moved that step out of the AnnotationsForCertificate function, since it now uses values from the data object.

EDDIE-DAV

Thanks Tim, Richard and I spotted one typo while reading through the code.

internal/controller/certificates/policies/checks.go

Co-authored-by: EDDIE-DAV <136573637+EDDIE-DAV@users.noreply.github.com> Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

wallrj

I asked some questions about parts of the PR that I don't understand.

I have a feeling that the secretTemplate annotations override bug should be fixed in a separate PR with its own release note, and be backported to previous versions if necessary.

internal/controller/certificates/secrets.go

wallrj · 2023-06-14T11:30:44Z

pkg/controller/certificates/issuing/internal/secret.go

+		secret.Labels = make(map[string]string)
+	}
+
+	if crt.Spec.SecretTemplate != nil {


I'm sure that this was the original behaviour when @jonathansp originally created the feature. See

feat: add support to secretTemplates #3828 (review)

I wonder when and how this bug got introduced?
Which versions are affected? Do we need to fix this bug in a separate PR and backport to the affected versions?
Is there a unit test for this? If not, please add one.

pkg/controller/certificates/issuing/internal/secret.go

wallrj · 2023-06-14T11:41:55Z

pkg/controller/certificates/issuing/temporary.go

-		PrivateKey:  pkData,
+		Certificate:     certData,
+		PrivateKey:      pkData,
+		CertificateName: crt.Name,


So when a certificate is issued, this is how the certificate name annotation value gets populated.
What about IssuerName, IssuerKind, IssuerGroup? Don't those also get added as annotations to this temporary secret?

I did not add those annotations for a temporary secret, since the temporary secret actually does not use the configured issuer (I try to prevent the user from being confused).
Not sure if there is a technical reason to (not) set those values here.

pkg/controller/certificates/issuing/issuing_controller.go

…r the current behaviour Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

wallrj

Thanks Tim,

Thanks for answering all those previous points.

/lgtm

jetstack-bot · 2023-06-14T14:43:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon, wallrj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [inteon,wallrj]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This PR contains the following updates: | Package | Update | Change | |---|---|---| | [cert-manager](https://github.com/cert-manager/cert-manager) | minor | `v1.12.4` -> `v1.13.0` | --- ### Release Notes <details> <summary>cert-manager/cert-manager (cert-manager)</summary> ### [`v1.13.0`](https://github.com/cert-manager/cert-manager/releases/tag/v1.13.0) [Compare Source](cert-manager/cert-manager@v1.12.4...v1.13.0) cert-manager is the easiest way to automatically manage certificates in Kubernetes and OpenShift clusters. This is the 1.13 release of cert-manager! #### Community Welcome to these new cert-manager members (more info - cert-manager/cert-manager#6260): [@jsoref](https://github.com/jsoref) [@FlorianLiebhart](https://github.com/FlorianLiebhart) [@hawksight](https://github.com/hawksight) [@erikgb](https://github.com/erikgb) Thanks again to all open-source contributors with commits in this release, including: [@AcidLeroy](https://github.com/AcidLeroy) [@FlorianLiebhart](https://github.com/FlorianLiebhart) [@lucacome](https://github.com/lucacome) [@cypres](https://github.com/cypres) [@erikgb](https://github.com/erikgb) [@ubergesundheit](https://github.com/ubergesundheit) [@jkroepke](https://github.com/jkroepke) [@jsoref](https://github.com/jsoref) [@gdvalle](https://github.com/gdvalle) [@rouke-broersma](https://github.com/rouke-broersma) [@schrodit](https://github.com/schrodit) [@zhangzhiqiangcs](https://github.com/zhangzhiqiangcs) [@arukiidou](https://github.com/arukiidou) [@hawksight](https://github.com/hawksight) [@Richardds](https://github.com/Richardds) [@kahirokunn](https://github.com/kahirokunn) Thanks also to the following cert-manager maintainers for their contributions during this release: [@SgtCoDFish](https://github.com/SgtCoDFish) [@maelvls](https://github.com/maelvls) [@irbekrm](https://github.com/irbekrm) [@inteon](https://github.com/inteon) Equally thanks to everyone who provided feedback, helped users and raised issues on Github and Slack and joined our meetings! Special thanks to [@AcidLeroy](https://github.com/AcidLeroy) for adding "load options from a versioned config file" support for the cert-manager controller! This has been on our wishlist for a very long time. (see cert-manager/cert-manager#5337) Also, thanks a lot to [@FlorianLiebhart](https://github.com/FlorianLiebhart) for adding support for DNS over HTTPS for the ACME DNS self-check. This is very useful in case all traffic must be HTTP(S) trafic, eg. when using a HTTPS_PROXY. (see cert-manager/cert-manager#5003) Thanks also to the [CNCF](https://www.cncf.io/), which provides resources and support, and to the AWS open source team for being good community members and for their maintenance of the [PrivateCA Issuer](https://github.com/cert-manager/aws-privateca-issuer). In addition, massive thanks to [Venafi](https://www.venafi.com/) for contributing developer time and resources towards the continued maintenance of cert-manager projects. #### Changes since v1.12.0 ##### Feature - Add support for logging options to webhook config file. ([#6243](cert-manager/cert-manager#6243), [@inteon](https://github.com/inteon)) - Add view permissions to the well-known (Openshift) user-facing `cluster-reader` aggregated cluster role ([#6241](cert-manager/cert-manager#6241), [@erikgb](https://github.com/erikgb)) - Certificate Shim: distinguish dns names and ip address in certificate ([#6267](cert-manager/cert-manager#6267), [@zhangzhiqiangcs](https://github.com/zhangzhiqiangcs)) - Cmctl can now be imported by third parties. ([#6049](cert-manager/cert-manager#6049), [@SgtCoDFish](https://github.com/SgtCoDFish)) - Make `enableServiceLinks` configurable for all Deployments and `startupapicheck` Job in Helm chart. ([#6292](cert-manager/cert-manager#6292), [@ubergesundheit](https://github.com/ubergesundheit)) - Promoted the StableCertificateRequestName and SecretsFilteredCaching feature gates to Beta (enabled by default). ([#6298](cert-manager/cert-manager#6298), [@inteon](https://github.com/inteon)) - The cert-manager controller options are now configurable using a configuration file. ([#5337](cert-manager/cert-manager#5337), [@AcidLeroy](https://github.com/AcidLeroy)) - The pki CertificateTemplate functions now perform validation of the CSR blob, making sure we sign a Certificate that matches the IsCA and (Extended)KeyUsages that are defined in the CertificateRequest resource. ([#6199](cert-manager/cert-manager#6199), [@inteon](https://github.com/inteon)) - \[helm] Add prometheus.servicemonitor.endpointAdditionalProperties to define additional properties on a ServiceMonitor endpoint, e.g. relabelings ([#6110](cert-manager/cert-manager#6110), [@jkroepke](https://github.com/jkroepke)) ##### Design - DNS over HTTPS (DoH) is now possible for doing the self-checks during the ACME verification. The DNS check method to be used is controlled through the command line flag: `--dns01-recursive-nameservers-only=true` in combination with `--dns01-recursive-nameservers=https://<DoH-endpoint>` (e.g. `https://8.8.8.8/dns-query`). It keeps using DNS lookup as a default method. ([#5003](cert-manager/cert-manager#5003), [@FlorianLiebhart](https://github.com/FlorianLiebhart)) ##### Bug or Regression - Allow overriding default pdb .minAvailable with .maxUnavailable without setting .minAvailable to null ([#6087](cert-manager/cert-manager#6087), [@rouke-broersma](https://github.com/rouke-broersma)) - BUGFIX: `cmctl check api --wait 0` exited without output and exit code 1; we now make sure we perform the API check at least once and return with the correct error code ([#6109](cert-manager/cert-manager#6109), [@inteon](https://github.com/inteon)) - BUGFIX: the issuer and certificate-name annotations on a Secret were incorrectly updated when other fields are changed. ([#6147](cert-manager/cert-manager#6147), [@inteon](https://github.com/inteon)) - BUGFIX\[cainjector]: 1-character bug was causing invalid log messages and a memory leak ([#6232](cert-manager/cert-manager#6232), [@inteon](https://github.com/inteon)) - Fix CloudDNS issuers stuck in propagation check, when multiple instances are issuing for the same FQDN ([#6088](cert-manager/cert-manager#6088), [@cypres](https://github.com/cypres)) - Fix indentation of Webhook NetworkPolicy matchLabels in helm chart. ([#6220](cert-manager/cert-manager#6220), [@ubergesundheit](https://github.com/ubergesundheit)) - Fixed Cloudflare DNS01 challenge provider race condition when validating multiple domains ([#6191](cert-manager/cert-manager#6191), [@Richardds](https://github.com/Richardds)) - Fixes a bug where webhook was pulling in controller's feature gates. ⚠️ ⚠️ BREAKING ⚠️ ⚠️ : If you deploy cert-manager using helm and have `.featureGates` value set, the features defined there will no longer be passed to cert-manager webhook, only to cert-manager controller. Use `webhook.featureGates` field instead to define features to be enabled on webhook. **Potentially breaking**: If you were, for some reason, passing cert-manager controller's features to webhook's `--feature-gates` flag, this will now break (unless the webhook actually has a feature by that name). ([#6093](cert-manager/cert-manager#6093), [@irbekrm](https://github.com/irbekrm)) - Fixes an issue where cert-manager would incorrectly reject two IP addresses as being unequal when they should have compared equal. This would be most noticeable when using an IPv6 address which doesn't match how Go's `net.IP.String()` function would have printed that address. ([#6293](cert-manager/cert-manager#6293), [@SgtCoDFish](https://github.com/SgtCoDFish)) - We disabled the `enableServiceLinks` option for our ACME http solver pods, because the option caused the pod to be in a crash loop in a cluster with lot of services. ([#6143](cert-manager/cert-manager#6143), [@schrodit](https://github.com/schrodit)) - ⚠️ possibly breaking: Webhook validation of CertificateRequest resources is stricter now: all KeyUsages and ExtendedKeyUsages must be defined directly in the CertificateRequest resource, the encoded CSR can never contain more usages that defined there. ([#6182](cert-manager/cert-manager#6182), [@inteon](https://github.com/inteon)) ##### Other (Cleanup or Flake) - A subset of the klogs flags have been deprecated and will be removed in the future. ([#5879](cert-manager/cert-manager#5879), [@maelvls](https://github.com/maelvls)) - All service links in helm chart deployments have been disabled. ([#6144](cert-manager/cert-manager#6144), [@schrodit](https://github.com/schrodit)) - Cert-manager will now re-issue a certificate if the public key in the latest CertificateRequest resource linked to a Certificate resource does not match the public key of the key encoded in the Secret linked to that Certificate resource ([#6168](cert-manager/cert-manager#6168), [@inteon](https://github.com/inteon)) - Chore: When hostNetwork is enabled, dnsPolicy is now set to ClusterFirstWithHostNet. ([#6156](cert-manager/cert-manager#6156), [@kahirokunn](https://github.com/kahirokunn)) - Cleanup the controller configfile structure by introducing sub-structs. ([#6242](cert-manager/cert-manager#6242), [@inteon](https://github.com/inteon)) - Don't run API Priority and Fairness controller in webhook's extension apiserver ([#6085](cert-manager/cert-manager#6085), [@irbekrm](https://github.com/irbekrm)) - Helm: Add apache 2.0 license annotation ([#6225](cert-manager/cert-manager#6225), [@arukiidou](https://github.com/arukiidou)) - Make apis/acme/v1/ACMEIssuer.PreferredChain optional in JSON serialization. ([#6034](cert-manager/cert-manager#6034), [@gdvalle](https://github.com/gdvalle)) - The SecretPostIssuancePolicyChain now also makes sure that the `cert-manager.io/common-name`, `cert-manager.io/alt-names`, ... annotations on Secrets are kept at their correct value. ([#6176](cert-manager/cert-manager#6176), [@inteon](https://github.com/inteon)) - The cmctl logging has been improved and support for json logging has been added. ([#6247](cert-manager/cert-manager#6247), [@inteon](https://github.com/inteon)) - Updates Kubernetes libraries to `v0.27.2`. ([#6077](cert-manager/cert-manager#6077), [@lucacome](https://github.com/lucacome)) - Updates Kubernetes libraries to `v0.27.4`. ([#6227](cert-manager/cert-manager#6227), [@lucacome](https://github.com/lucacome)) - We now only check that the issuer name, kind and group annotations on a Secret match in case those annotations are set. ([#6152](cert-manager/cert-manager#6152), [@inteon](https://github.com/inteon)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.home/nrdufour/home-ops/pulls/84 Co-authored-by: Renovate <renovate@ptinem.io> Co-committed-by: Renovate <renovate@ptinem.io>

inteon commented Jun 13, 2023

View reviewed changes

fix a bug that caused the issuer-ref and certificate-name annotations…

8ddf016

… on Secrets to be correct when being updated. Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the fix_wrong_annotations branch from b91e314 to 8ddf016 Compare June 13, 2023 14:54

inteon commented Jun 13, 2023

View reviewed changes

inteon requested review from wallrj and irbekrm and removed request for wallrj June 13, 2023 15:19

EDDIE-DAV suggested changes Jun 14, 2023

View reviewed changes

internal/controller/certificates/policies/checks.go Outdated Show resolved Hide resolved

Update internal/controller/certificates/policies/checks.go

3aa7b82

Co-authored-by: EDDIE-DAV <136573637+EDDIE-DAV@users.noreply.github.com> Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

wallrj requested changes Jun 14, 2023

View reviewed changes

inteon added 2 commits June 14, 2023 14:51

add TODO comment that explains that we don't understand the reason fo…

9c9e833

…r the current behaviour Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

re-add TODO comment and make the message more clear

fe4f4e4

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

wallrj approved these changes Jun 14, 2023

View reviewed changes

jetstack-bot assigned wallrj Jun 14, 2023

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2023

jetstack-bot merged commit 2d01fbe into cert-manager:master Jun 14, 2023
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUGFIX: issuer-ref and certificate-name annotations are incorrectly updated on Secrets #6147

BUGFIX: issuer-ref and certificate-name annotations are incorrectly updated on Secrets #6147

inteon commented Jun 13, 2023 •

edited

inteon Jun 13, 2023 •

edited

inteon Jun 13, 2023 •

edited

inteon Jun 13, 2023 •

edited

inteon Jun 13, 2023

wallrj Jun 14, 2023

inteon Jun 14, 2023 •

edited

inteon Jun 13, 2023

EDDIE-DAV left a comment

wallrj left a comment

wallrj Jun 14, 2023

wallrj Jun 14, 2023

inteon Jun 14, 2023

wallrj left a comment

jetstack-bot commented Jun 14, 2023

BUGFIX: issuer-ref and certificate-name annotations are incorrectly updated on Secrets #6147

BUGFIX: issuer-ref and certificate-name annotations are incorrectly updated on Secrets #6147

Conversation

inteon commented Jun 13, 2023 • edited

Kind

Release Note

inteon Jun 13, 2023 • edited

Choose a reason for hiding this comment

inteon Jun 13, 2023 • edited

Choose a reason for hiding this comment

inteon Jun 13, 2023 • edited

Choose a reason for hiding this comment

inteon Jun 13, 2023

Choose a reason for hiding this comment

wallrj Jun 14, 2023

Choose a reason for hiding this comment

inteon Jun 14, 2023 • edited

Choose a reason for hiding this comment

inteon Jun 13, 2023

Choose a reason for hiding this comment

EDDIE-DAV left a comment

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

wallrj Jun 14, 2023

Choose a reason for hiding this comment

wallrj Jun 14, 2023

Choose a reason for hiding this comment

inteon Jun 14, 2023

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

jetstack-bot commented Jun 14, 2023

inteon commented Jun 13, 2023 •

edited

inteon Jun 13, 2023 •

edited

inteon Jun 13, 2023 •

edited

inteon Jun 13, 2023 •

edited

inteon Jun 14, 2023 •

edited