Allow ingress-shim to opt-in for privatekey rotation 'Always'

[ravilr]

Is your feature request related to a problem? Please describe.
Yes. Certificate resource created by ingress-shim defaults to privateKey.rotationPolicy of Never. This limits the use of ingress-shim with External Issuers which doesn't allow private/public key reuse in the CSR.

Describe the solution you'd like
ingress-shim should support Ingress resources to opt-in to privateKey.rotationPolicy of Always through an annotation.
Alternatively, change the default rotationPolicy to Always in the certificate controller for certmanager.io/v1 Certificate resource, as it is the recommended best security practice to rotate keys on cert renewals.

Describe alternatives you've considered
N/A

Additional context
N/A

Environment details (remove if not applicable):

• Kubernetes version: v1.20.6
• Cloud-provider/provisioner: baremetal
• cert-manager version: v1.3.1

/kind feature

[munnerz]

I'd love for us to find a way to change the default value for this field to be 'Always'.

That said, there will be users out there who have relied on this behaviour and use private/public key pinning (despite general advise being against this practice). These users may see their private keys lost if we flip this flag to 'Always', which could cause quite difficult to unwind outages (i.e. loss of private key).

I think this could be resolved if we came up with a way to:

1. default this field on reads to be 'Never' (i.e. any existing resources would continue to have a value of 'Never' if the resource is read from etcd and does not have the field set)
2. default this field on writes to be 'Always'

I'm not too sure if this is possible (nor advisable) from an API machinery perspective however, because I think OpenAPI defaulting (on writes) happens before mutating webhooks are called, which means we'd never know if the user explicitly set the field to Never or if it had just been defaulted.

We could consider making this behaviour configurable via a flag to the controller - alternatively, the CertificatePreset proposal (#2239) would allow for this sort of thing to be configured on a per-namespace (or potentially per-cluster, if we had a ClusterCertificatePreset resource too) level.

---

For the ingress-shim case, I think we could definitely expose an annotation for this to allow configuration (though as you've noted this would need to be opt-in)

/area api

[7ing]

Up vote this feature
In addition to controller level configuration, we also could add an annotation to ingress resource to indicate the rotationPolicy.
Looking forward to see this feature in next release.

[ravilr]

@munnerz yes, I agree that for any new feature that introduces change in behavior, it is best to initially introduce a feature flag at the controllers level. That way, it enables the cluster admins who wants to ensure best practices for their cluster users, to right away adapt those changes for their cluster cert-manager deployments. Additionally, have a feature maturity/deprecation process similar to Kubernetes, where the behavior controlled by the feature flag becomes the default in a future (X+3) release (promoting reasonable defaults for all new deployments out-of-the-box).

We want our cluster end-users wanting to acquire a signed certificate for their workload Ingress use case to be able to just specify bare minimum required annotations (issuer ref and dns names). certificate privateKey rotation policy, certificateRequest revision GC policy, etc. are cluster admin concerns, hence should be able to be configured and enforced globally at the cert-manager deployment level.

[maelvls]

Hi! Triaging party going on here, I'll add the missing "priority" label, I hope "backlog" makes sense 😅

/priority backlog

[devonwarren]

I was looking for this functionality as well. Hoping it makes it into the main codebase but in the meantime I was able to get it working with a MutatingWebhook, you can use my initial codebase at https://github.com/devonwarren/admission-control-examples/tree/master/cert-manager-rotate-policy to get started if you want to go that route

[jetstack-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle stale

[7ing]

@munnerz have we finalized the design for this fix ?

[jetstack-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
Send feedback to jetstack.
/lifecycle rotten
/remove-lifecycle stale

[matpen]

would it be possible to do that with an annotation, perhaps?

[jetstack-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

[jetstack-bot]

@jetstack-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.
Send feedback to jetstack.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[matpen]

Sorry, is it possible to keep this issue open? I for one, but I am sure others in this thread, still need this feature. Thanks!

[irbekrm]

For the ingress-shim case, I think we could definitely expose an annotation for this to allow configuration (though as you've noted this would need to be opt-in)

@munnerz do you think there is any reason why implementing an option to allow for this to be configurable (without even thinking about the default) could not be done in #2239 (i.e that we would still need the annotation as well)?