BUGFIX: Stricter CertificateRequest CSR webhook validation

[inteon]

Pull Request Motivation

While creating #6056, we realised that the current CertificateRequest CSR webhook validation logic is too permissive:
If CertificateRequest.Usages is empty and the KeyUsages are only set as an extension in the CSR blob, the validation webhook allows this CertificateRequest to be created. However, some solutions (like approver-policy) depend on the Usages always being defined in the Usages array.

This is a quick summary of the current webhook behavior vs the new behavior:

situation	before	after
usages in CertReq & CSR blob	error if !=	error if !=
usages only in CertReq	allow (use CertReq usages)	allow (use CertReq usages)
usages only in CSR blob	allow (use CSR blob usages)	error if CSR blob usages != default usages
no usages defined	allow (use default usages)	allow (use default usages)

Important note: The current webhook behavior differs from our assumptions made in approver-policy & our assumptions made in our Certificate Template logic (https://github.com/cert-manager/cert-manager/blob/master/pkg/util/pki/certificatetemplate.go#L206).

Making the validation webhook stricter is always a breaking change, however I think it is justified because it makes cert-manager more secure (after this change, you can rely purely on the Usages array in the CertReq to make policy decisions). Also, there is no existing implementation that uses the CSR blob as sole source of truth for the usages. (see examples below)
[OK] csi-driver sets Usages only on CertReq: https://github.com/cert-manager/csi-driver/blob/ebe0181784d79b8514bb77a5e516ea978da4c6bd/pkg/requestgen/generator.go#L76-L95
[OK] csi-driver-spiffe sets Usages only on CertReq: https://github.com/cert-manager/csi-driver-spiffe/blob/main/internal/csi/driver/driver.go#L274-L289
[OK] istio-csr sets Usages both on CertReq and CSR blob: https://github.com/cert-manager/istio-csr/blob/main/pkg/server/server.go#L180

Kind

/kind bug

Release Note

⚠️ 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.

[SgtCoDFish]

My 2c after thinking about this and sleeping on it:

I think this sort of thing is a worthwhile change to make and it's reasonable to have it on by default because, as you say, there's a security improvement to be had from having a single source of truth for any given element of a certificate.

There are some caveats to that, though:

1. I don't tend to think that having the CertificateRequest be the single source of truth is the best option. The CSR is a better source of truth in my mind because:
   1. It's signed, and
   2. It's infinitely more customisable than the CertificateRequest - people can put whatever they like in a CSR and it's not a huge change to cert-manager to allow, say, custom extensions or whatever in the future. That means that for future additions I'd think we'd want to add them to the CSR and so it'd be nice to do that here too.
2. That said, I suspect that with tools like approver-policy relying on the Usages today, we can't make the CSR the source of truth for this field and we'd have to wait for v2 (if ever), so maybe 1 is moot
3. I think making a breaking change and then saying "there's no remediation if this breaks you except to change all your workflows" is much more disruptive than adding some sort of flag. Could we add --insecure-allow-csr-usage-definitions or similar to the webhook, so that people who are broken by this can choose to preserve the old behaviour? I don't think - from looking at this change - that that would be a huge burden for us?

[inteon]

@SgtCoDFish I added the DontAllowInsecureCSRUsageDefinition feature gate, which can be used to disable this new behavior (when set to false).
The gate is enabled by default however (default = true).

[SgtCoDFish]

/lgtm
/approve
/hold

Adding a hold because of my suggestion - but I think it's fine to merge as is!

[SgtCoDFish]

suggestion: I don't think this flag really matches my understanding of what we want a "feature gate" to be.

My understanding was that a feature gate was something we were concerned might break or need to change in the future.

I don't really see a huge problem here with this just being a regular CLI flag. I suppose a feature gate makes it easier to remove this in the future, but I don't really see a pressing need to remove this option.

It doesn't really matter though!

[inteon]

I was thinking that we would like to remove this feature flag in an upcoming release if no one complains that this change causes problems for them. That is why I added this as a feature flag.
I think you only want to be less strict in case you have a cert-manager integration that "incorrectly" uses the API.

[inteon]

In the future, I would like to make some other improvements to our validation webhook that might be a bit more difficult to implement with this feature flag (not sure). eg. #6056

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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 [SgtCoDFish,inteon]

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

[inteon]

Thanks @SgtCoDFish
/unhold see #6182 (comment)