Fix changed behavior: set critical flag of SANs extension based on subject #6724
Conversation
| @@ -321,11 +321,7 @@ func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.Cert | |||
| var extraExtensions []pkix.Extension | |||
|
|
|||
| if !sans.Empty() { | |||
| // emptyASN1Subject is the ASN.1 DER encoding of an empty Subject, which is | |||
There was a problem hiding this comment.
I moved this logic to the new IsASN1SubjectEmpty function.
…bject Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
23bc695 to
0acde5b
Compare
|
/cherrypick release-1.14 |
|
@inteon: once the present PR merges, I will cherry-pick it on top of release-1.14 in a new PR and assign it to you. In response to this:
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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:
Approvers can indicate their approval by writing |
|
@inteon: new pull request created: #6727 In response to this:
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. |
| @@ -261,6 +261,26 @@ func CertificateTemplateFromCSR(csr *x509.CertificateRequest, validatorMutators | |||
| } | |||
| } | |||
|
|
|||
| // Finally, we fix up the certificate template to ensure that it is valid | |||
| { | |||
| // If the certificate has an empty Subject, we set any SAN extensions to be critical | |||
There was a problem hiding this comment.
A comment explaining why would have been helpful, with a link to the RFC.
There was a problem hiding this comment.
Agreed, should be added and I should've required it in review. Sorry!
In #6404, we introduced a new SAN (OtherName).
This SAN is not directly supported by the golang x509 library.
That is why we had to create our custom parsing logic to still be able to add this SAN.
However, the behavior of the
x509 CSR -> x509 Certconversion logic was also accidentally changed.The following lines blindly copy the SAN extension from the CSR:
cert-manager/pkg/util/pki/certificatetemplate.go
Lines 233 to 239 in 895c10c
We do however not re-evaluate if the critical flag should be set on the extension.
Before #6404, we did re-create the SANs and re-determined if the critical flag has to be set at that point in time.
This PR fixes this issue by adding a new "fix" step that fixes the SAN extensions based on the provided subject.
Long term, I think we should move this x509 logic into a seperate library with a much reduced public API.
Kind
/kind bug
Release Note