Extended key usages into CSR #3211

meyskens · 2020-08-24T08:25:08Z

What this PR does / why we need it:

This encodes the ExtKeyUsages into the CSR so supported issuers can pick those up and assign them.

Which issue this PR fixes:

Special notes for your reviewer:
~~From my research is isn't possible to define the "old" key usages into the CSR. Prove me wrong is your mission ;)~~
I read more code in the evening, it works :D

Release note:

Add key usages into the CSR body

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

jetstack-bot · 2020-08-24T08:25:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: meyskens

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~~ [meyskens]

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

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

hzhou97 · 2020-08-24T15:57:38Z

It looks good to me.
I also have failed the mission, don't see how to add the Key Usages. But I'm confused why x509.CertificateRequest does not have a field for that. And from my research it seems that nobody else has had need for that either? Kinda seems strange.

Since this only fixes half of #3136, I think we shouldn't close the issue and discuss further with the users?

munnerz · 2020-08-24T16:54:01Z

If both the CSR and CertificateRequest specify key usages/extended key usages, which should be authoritative if they are different? As a consumer of the CertificateRequest API this could lead to confusion between different issuer types which may each behave differently.

This change doesn't really change the state of the world here (previously, if a user created their own CSR which does specify usages and those are different to those on the CR, the question would be the same). But by making it default behaviour for the Certificate controller to add this, it certainly becomes a potentially more prominent issue.

As an item for the v1 API, perhaps validation can be tightened in v1 to require that these are both the same if any are encoded into the CSR?

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens · 2020-08-24T18:12:20Z

Digged deeper into specs and added the classic key usages also :)

To come back to @munnerz' comment, I think validation is the best compromise here? But only if both are set?

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

wallrj

Thanks @meyskens

A few comments below.
Is there any danger that this will break existing issuers?
Will LetsEncrypt pay attention to this?
Will Venafi work with this?

It does seem ugly that we will be adding usages both to the CR and to the x509 CSR.
It makes me wonder whether:

CR resource should be immutable.
Perhaps the x509 CSR should be moved from spec.Request > status.Request, signifying that the CSR data is only meant to be populated by cert-manager.

pkg/util/pki/keyusage.go

pkg/util/pki/generate_test.go

pkg/util/pki/keyusage.go

pkg/util/pki/csr_test.go

JoshVanL

Couple of nits from me

pkg/util/pki/csr_test.go

pkg/util/pki/csr.go

…if both are set Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

wallrj

A few more comments and questions.

wallrj · 2020-08-26T09:27:42Z

pkg/internal/apis/certmanager/validation/certificaterequest.go

 		if err != nil {
 			el = append(el, field.Invalid(fldPath.Child("request"), crSpec.Request, fmt.Sprintf("failed to decode csr: %s", err)))
+		} else {
+			// only compare usages if set on CR and in the CSR
+			if len(crSpec.Usages) > 0 && len(csr.Extensions) > 0 && validateCSRContent {


What if there are crSpec usages but not csr.Extensions? That should be a validation error too, right?

How about creating sets.NewString of the CR and CSR usages and then doing a diff between them?
The differences could be added to the error message.

Ignore the comment above. You explained in standup that we want to allow the case where usages are only defined in CR or CSR.
But perhaps we should also make that clear in the validation error message.

And if the usages are only defined in the CSR Request bytes should we add validation to ensure that those are all in the enums set that we validate the CR usages against?

But perhaps we should also make that clear in the validation error message.

Done

And if the usages are only defined in the CSR Request bytes should we add validation to ensure that those are all in the enums set that we validate the CR usages against?

What do you mean? these are validated by it being a valid x509 CSR

I recently updated the RequestMatchesSpec function in #3224 and it makes me wonder if that function should also be updated to handle the case where the usages are only present in the Request bundle.
Not necessarily here, but perhaps in a followup issue / PR.

What do you mean? these are validated by it being a valid x509 CSR

I was thinking of the the x509.go line 705

UnknownExtKeyUsage []asn1.ObjectIdentifier // Encountered extended key usages unknown to this package.

https://github.com/golang/go/blob/47b450997778163dfed6f58cae379d928fc37687/src/crypto/x509/x509.go#L692-L703

pkg/internal/apis/certmanager/validation/certificaterequest.go

test/integration/validation/certificaterequest_test.go

pkg/internal/apis/certmanager/validation/clusterissuer.go

pkg/apis/certmanager/v1/types_certificaterequest.go

wallrj · 2020-08-26T12:01:12Z

/kind feature

pkg/internal/apis/certmanager/validation/certificaterequest.go

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

wallrj · 2020-08-27T14:08:58Z

/lgtm

Add extended key usages into CSR

d15054e

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens added this to the v1.0 milestone Aug 24, 2020

jetstack-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 24, 2020

jetstack-bot requested a review from munnerz August 24, 2020 08:25

jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2020

Add boilerplate

0e17b9d

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

Support key usages

f6610fb

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens changed the title ~~Add extended key usages into CSR~~ Extended key usages into CSR Aug 24, 2020

meyskens added 2 commits August 25, 2020 10:39

Add CSR validation

abb56fb

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

Fix syntax

3bb0431

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

wallrj requested changes Aug 25, 2020

View reviewed changes

pkg/util/pki/keyusage.go Outdated Show resolved Hide resolved

pkg/util/pki/generate_test.go Show resolved Hide resolved

pkg/util/pki/keyusage.go Show resolved Hide resolved

pkg/util/pki/csr_test.go Outdated Show resolved Hide resolved

pkg/util/pki/csr_test.go Show resolved Hide resolved

JoshVanL requested changes Aug 25, 2020

View reviewed changes

pkg/util/pki/csr_test.go Outdated Show resolved Hide resolved

pkg/util/pki/csr.go Outdated Show resolved Hide resolved

Add validation for CR creation only to match Key usages with the CSR …

627b57f

…if both are set Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

Implement feedback

3154be7

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

meyskens requested review from JoshVanL and wallrj August 25, 2020 12:51

meyskens added 2 commits August 25, 2020 14:56

Fix boilerplate

1cd5c38

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

Fix unit tests

60db2d1

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

wallrj requested changes Aug 26, 2020

View reviewed changes

jetstack-bot added kind/feature Categorizes issue or PR as related to a new feature. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Aug 26, 2020

meyskens commented Aug 27, 2020

View reviewed changes

pkg/internal/apis/certmanager/validation/certificaterequest.go Outdated Show resolved Hide resolved

meyskens added 2 commits August 27, 2020 14:42

Implement feedback + improve logic

b319ff8

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

Update CRDs

45d1457

Signed-off-by: Maartje Eyskens <maartje@eyskens.me>

jetstack-bot assigned wallrj Aug 27, 2020

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2020

jetstack-bot merged commit 647035a into cert-manager:master Aug 27, 2020

inteon mentioned this pull request Jun 22, 2023

Improve CertificateRequest's CSR validation code #6056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extended key usages into CSR #3211

Extended key usages into CSR #3211

meyskens commented Aug 24, 2020 •

edited

jetstack-bot commented Aug 24, 2020

hzhou97 commented Aug 24, 2020

munnerz commented Aug 24, 2020

meyskens commented Aug 24, 2020

wallrj left a comment

JoshVanL left a comment

wallrj left a comment

wallrj Aug 26, 2020

wallrj Aug 26, 2020

meyskens Aug 27, 2020

wallrj Aug 27, 2020

wallrj Aug 27, 2020

wallrj commented Aug 26, 2020

wallrj commented Aug 27, 2020

Extended key usages into CSR #3211

Extended key usages into CSR #3211

Conversation

meyskens commented Aug 24, 2020 • edited

jetstack-bot commented Aug 24, 2020

hzhou97 commented Aug 24, 2020

munnerz commented Aug 24, 2020

meyskens commented Aug 24, 2020

wallrj left a comment

Choose a reason for hiding this comment

JoshVanL left a comment

Choose a reason for hiding this comment

wallrj left a comment

Choose a reason for hiding this comment

wallrj Aug 26, 2020

Choose a reason for hiding this comment

wallrj Aug 26, 2020

Choose a reason for hiding this comment

meyskens Aug 27, 2020

Choose a reason for hiding this comment

wallrj Aug 27, 2020

Choose a reason for hiding this comment

wallrj Aug 27, 2020

Choose a reason for hiding this comment

wallrj commented Aug 26, 2020

wallrj commented Aug 27, 2020

meyskens commented Aug 24, 2020 •

edited