Improve Trigger, Readiness and PostIssuance Policy chains #6152

inteon · 2023-06-14T18:34:57Z

This PR improves the Trigger, Readiness and PostIssuance Policy chains.
It contains the following improvements:

made names of the check functions more consistent
made Issuer secret annotations check optional (only if the annotation is present)

This PR improves readability of the Check code and makes cert-manager a bit more permissive in case annotations on a Secret are missing (allowing users to manually provide a Secret before cert-manager takes over).

Kind

/kind cleanup

Release Note

We now only check that the issuer name, kind and group annotations on a Secret match in case those annotations are set.

internal/controller/certificates/policies/checks.go

inteon · 2023-06-14T18:42:05Z

internal/controller/certificates/policies/checks.go

@@ -347,134 +473,159 @@ func SecretTemplateMismatchesSecretManagedFields(fieldManager string) Func {
 			}
 		}

-		baseAnnotations, err := internalcertificates.AnnotationsForCertificate(x509cert)
+		expCertificateDataAnnotations, err := internalcertificates.AnnotationsForCertificate(x509cert)


This SecretManagedLabelsAndAnnotationsManagedFieldsMismatch function makes sure the Secret's certificate details annotations all exist. It uses the output of AnnotationsForCertificate to build the set of expected annotations. NOTE: this function does not check the contents of these annotations, that happens in SecretCertificateDetailsAnnotationsMismatch.

inteon · 2023-06-15T06:46:42Z

internal/controller/certificates/policies/policies.go

+		SecretIsMissingData,    // Make sure the Secret has the required keys set
+		SecretPublicKeysDiffer, // Make sure the PrivateKey and PublicKey match in the Secret
+
+		SecretCertificateNameAnnotationMismatch, // Make sure the Secret's CertificateName annotation matches the Certificate Name


Question: should we reissue when the certificate names don't match or should we allow a certificate to "adopt" an existing secret?

internal/controller/certificates/policies/checks.go

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

wallrj

Thanks @inteon

I suggest adding a release note to the PR, explaining how the re-issuing checks have been improved and give an example of a mismatch that will now trigger a re-issuance.

/lgtm

wallrj · 2023-07-24T14:45:27Z

internal/controller/certificates/policies/checks.go

 	if err != nil {
-		return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains an invalid key-pair: %v", err), true
+		return InvalidKeyPair, fmt.Sprintf("Issuing certificate as Secret contains invalid private key data: %v", err), true
+	}


Is there any risk that the error will contain the invalid private key bytes?
I don't know whether the original tls.X509KeyPair would or not.

wallrj · 2023-07-24T14:49:37Z

internal/controller/certificates/policies/checks_test.go

@@ -92,7 +93,7 @@ func Test_NewTriggerPolicyChain(t *testing.T) {
 				},
 			},
 			reason:  InvalidKeyPair,
-			message: "Issuing certificate as Secret contains an invalid key-pair: tls: failed to find any PEM data in certificate input",
+			message: "Issuing certificate as Secret contains invalid private key data: error decoding private key PEM block",


Ah, it looks like the error probably would not contain the bytes.

wallrj · 2023-07-24T14:52:39Z

internal/controller/certificates/policies/policies.go

+		SecretPrivateKeyMismatchesSpec,                      // Make sure the PrivateKey Type and Size match the Certificate spec
+		SecretPublicKeyDiffersFromCurrentCertificateRequest, // Make sure the Secret's PublicKey matches the current CertificateRequest
+		CurrentCertificateRequestMismatchesSpec,             // Make sure the current CertificateRequest matches the Certificate spec
+		CurrentCertificateHasExpired(c),                     // Make sure the Certificate in the Secret has not expired


Thanks for all the explanatory comments.

jetstack-bot · 2023-07-24T14:54:39Z

[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 force-pushed the improve_policy_chain_v0 branch from 50be3d8 to 8257b78 Compare June 14, 2023 18:35

jetstack-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 14, 2023

inteon commented Jun 14, 2023

View reviewed changes

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

inteon commented Jun 14, 2023

View reviewed changes

inteon requested a review from wallrj June 14, 2023 18:43

inteon force-pushed the improve_policy_chain_v0 branch from 8257b78 to bdc4b60 Compare June 14, 2023 18:54

inteon mentioned this pull request Jun 14, 2023

Add Certificate Hash #6155

Draft

inteon commented Jun 15, 2023

View reviewed changes

wallrj requested changes Jun 15, 2023

View reviewed changes

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

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2023

inteon force-pushed the improve_policy_chain_v0 branch from bdc4b60 to 404e178 Compare July 4, 2023 11:42

jetstack-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 4, 2023

inteon force-pushed the improve_policy_chain_v0 branch from 404e178 to a2cda5c Compare July 7, 2023 11:39

inteon requested a review from wallrj July 24, 2023 07:36

inteon force-pushed the improve_policy_chain_v0 branch from a2cda5c to 39ee979 Compare July 24, 2023 07:40

improve Trigger, Readiness and PostIssuance Policy chains

36ddf19

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

inteon force-pushed the improve_policy_chain_v0 branch from 39ee979 to 36ddf19 Compare July 24, 2023 07:42

wallrj approved these changes Jul 24, 2023

View reviewed changes

jetstack-bot assigned wallrj Jul 24, 2023

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

jetstack-bot merged commit 4de06a1 into cert-manager:master Jul 24, 2023
6 checks passed

jetstack-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jul 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Trigger, Readiness and PostIssuance Policy chains #6152

Improve Trigger, Readiness and PostIssuance Policy chains #6152

inteon commented Jun 14, 2023 •

edited

inteon Jun 14, 2023

inteon Jun 15, 2023

wallrj left a comment

wallrj Jul 24, 2023

wallrj Jul 24, 2023

wallrj Jul 24, 2023

jetstack-bot commented Jul 24, 2023

Improve Trigger, Readiness and PostIssuance Policy chains #6152

Improve Trigger, Readiness and PostIssuance Policy chains #6152

Conversation

inteon commented Jun 14, 2023 • edited

Kind

Release Note

inteon Jun 14, 2023

Choose a reason for hiding this comment

inteon Jun 15, 2023

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

wallrj Jul 24, 2023

Choose a reason for hiding this comment

wallrj Jul 24, 2023

Choose a reason for hiding this comment

wallrj Jul 24, 2023

Choose a reason for hiding this comment

jetstack-bot commented Jul 24, 2023

inteon commented Jun 14, 2023 •

edited