Use upstream golang/crypto functionality for ACME External Account Binding by irbekrm · Pull Request #3850 · cert-manager/cert-manager

irbekrm · 2021-04-06T07:59:54Z

[Update] I will split this PR into multiple ones to make it easier to code review.

See #3220 for full context.

Up till now we used a fork of golang/crypto with added support for ACME External Account Binding and fetching of ACME preferred certificate chains from @meyskens GitHub account.

This PR:

uses upstream golang/crypto functionality for ACME External Account Binding instead of our own
uses golang/crypto fork from cert-manager account instead of @meyskens account

Notes:

I have forked the latest golang/crypto to cert-manager org and cherry-picked @meyskens commit that implements fetching of alternative certificate chains on top of that. We can use this fork till the same functionality gets implemented upstream
The difference between the upstream implementation of ACME EAB and our own that we have used up till now is that in upstream the MAC algorithm value is hardcoded to HS256. This means that the issuer.spec.acme.externalAccountBinding.keyAlgorithm that has been 'required' so far becomes useless. This PR makes the field optional and the validating webhook now returns a warning if it is set. It is now possible (we test for that) to 1) deploy with issuer.spec.acme.externalAccountBinding.keyAlgorithm field set 2) deploy without the field 3) deploy with, then remove.
I have removed a bunch of legacy functions for issuer generation from test/e2e/util/util.go as it partially duplicated the functionality in test/test/unit/gen/issuer.go and added some more specific issuer generation functions to test/unit/gen/issuer.go.
Can we assume that no-one has been importing the legacy functions from test/e2e/util/util.go? I did not find external imports on pkg.go.dev
In order for the webhook to be able to return a warning I had to add it to the signature of most validating functions. I assume that this is a functionality that might potentially become useful? (i.e perhaps the warning about empty hostname for self-signed issuer that @SgtCoDFish worked on could have been implemented as a warning)
Would it make sense to eventually remove issuer.spec.acme.externalAccountBinding.keyAlgorithm? (In which case that should probabaly be added to the warning message).

This is what the warning looks like:

fixes #3822

Deprecate Issuer.spec.acme.externalAccountBinding.keyAlgorithm field. EAB MAC algorithm is now hardcoded to HS256.

jetstack-bot · 2021-04-06T08:00:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [irbekrm]

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

It is hardcoded to HS256 in golang.org/x/crypto Also, we now use a fork of golang.org/x/crypto in cert-manager org. Signed-off-by: irbekrm <irbekrm@gmail.com>

Use test/util/gen instead Signed-off-by: irbekrm <irbekrm@gmail.com>

Signed-off-by: irbekrm <irbekrm@gmail.com>

…ield is set Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm · 2021-04-07T15:24:00Z

/milestone v1.4

Signed-off-by: irbekrm <irbekrm@gmail.com>

SgtCoDFish

I've not run this locally yet: I think this all looks good, but honestly there's so much going on I could easily have missed several things. You could make the case for splitting this into several different PRs:

Use cert-manager/crypto instead of meyskens/crypto
Add support for warnings being returned during validation
Deprecate the keyAlgorithm field and return a warning when it's set
Remove legacy functions for issuer generation from test/e2e/util/util.go

I think 4 stands out to me as being well worth splitting out into a separate PR. I think my preference would be to make 1 its own standalone PR too since it stands out to me as being such an important thing (because the fork makes me really, really nervous).

SgtCoDFish · 2021-04-08T14:31:34Z

pkg/internal/api/validation/registry.go

-type ValidateFunc func(req *admissionv1.AdmissionRequest, obj runtime.Object) field.ErrorList
-type ValidateUpdateFunc func(req *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) field.ErrorList
+type ValidateFunc func(req *admissionv1.AdmissionRequest, obj runtime.Object) (field.ErrorList, []string)
+type ValidateUpdateFunc func(req *admissionv1.AdmissionRequest, oldObj, obj runtime.Object) (field.ErrorList, []string)


it might be helpful to document in a comment on these types what the new []string return value is for, so that it's obvious at a glance. Likewise in the definitions of func (r *Registry) Validate and func (r *Registry) ValidateUpdate below and other instances of those types of function.

Further to that, we have field.ErrorList which has the notion of a path, helping to point people at where exactly an error is.

It seems a shame to not have that kind of rich information for warnings. Maybe even something like this could work:

type WarningList field.ErrorList

Thanks- I agree that splitting makes sense!
Would it makes sense if it was:

Removes legacy functions (so that it's more straightforward to write e2e tests for 2)

Use cert-manager/crypto instead of meyskens/crypto + deprecates the keyAlgorithm- this would allow us to fork upstream golang/crypto & cherry-pick alternative cert chains (current verson from meyskens has an older version of golang/crypto with our custom functionality for ACME EAB) and once we use upstream for ACME EAB we have to get rid of keyAlgorithm at least partially.

Implements warnings

That sounds spot on 👍

SgtCoDFish · 2021-04-08T15:16:04Z

test/e2e/suite/issuers/acme/certificate/http01.go

 	validations := f.Helper().ValidationSetForUnsupportedFeatureSet(unsupportedFeatures)

 	BeforeEach(func() {
-		acmeIssuer := util.NewCertManagerACMEIssuer(issuerName, f.Config.Addons.ACMEServer.URL, testingACMEEmail, testingACMEPrivateKey)


am I correct these changes are related to:

I have removed a bunch of legacy functions for issuer generation ...

if so, I can see why that's a valuable change but this is already a pretty huge PR and these changes to use gen here feel quite distant from the goal of this specific PR; they should maybe be in their own PR, to make everything easier to review?

or have I missed how these changes are linked to this PR?

I put it here because I was adding some e2e tests for issuers and without removing the legacy I would have to either use & update the legacy functions for issuer generation or have two different ways of doing this in one file.
I will create a separate PR with these changes.

#3873 PRs in just the removal of legacy functions.

irbekrm · 2021-04-09T11:44:54Z

/retest

irbekrm · 2021-04-09T11:45:54Z

/test pull-cert-manager-e2e-v1-20

irbekrm · 2021-04-09T11:55:24Z

/test pull-cert-manager-bazel

irbekrm · 2021-04-09T13:50:07Z

/test pull-cert-manager-bazel

irbekrm · 2021-04-09T14:50:00Z

Closing this as I am splitting it into 3 separate PRs.

jetstack-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 6, 2021

jetstack-bot requested a review from meyskens April 6, 2021 08:00

irbekrm changed the title ~~WIP Use upstream golang/crypto for ACME External Account Binding~~ [WIP] Use upstream golang/crypto for ACME External Account Binding Apr 6, 2021

irbekrm mentioned this pull request Apr 7, 2021

Updates README to explain why we are using this fork cert-manager/crypto#1

Merged

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2021

irbekrm added 5 commits April 7, 2021 14:51

Issuer's ACME EAB algorithm can no longer be set

7f5fb08

It is hardcoded to HS256 in golang.org/x/crypto Also, we now use a fork of golang.org/x/crypto in cert-manager org. Signed-off-by: irbekrm <irbekrm@gmail.com>

Remove legacy e2e util functions for Issuer creation

acde6d9

Use test/util/gen instead Signed-off-by: irbekrm <irbekrm@gmail.com>

Add e2e test cases for different ACME EAB configurations

57f012e

Signed-off-by: irbekrm <irbekrm@gmail.com>

Validating webhook can now return warnings on create and update

0577555

Signed-off-by: irbekrm <irbekrm@gmail.com>

Return a warning if issuer.spec.externalAccountBinding.keyAlgorithm f…

16f5141

…ield is set Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm force-pushed the upstream_acme_eab branch from a0acaf9 to 16f5141 Compare April 7, 2021 14:14

jetstack-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2021

irbekrm changed the title ~~[WIP] Use upstream golang/crypto for ACME External Account Binding~~ Use upstream golang/crypto for ACME External Account Binding Apr 7, 2021

jetstack-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 7, 2021

jetstack-bot added this to the v1.4 milestone Apr 7, 2021

Update comment for replace directive in go.mod

b4385f7

Signed-off-by: irbekrm <irbekrm@gmail.com>

irbekrm requested review from JoshVanL and wallrj April 7, 2021 15:53

irbekrm changed the title ~~Use upstream golang/crypto for ACME External Account Binding~~ Use upstream golang/crypto functionality for ACME External Account Binding Apr 8, 2021

SgtCoDFish reviewed Apr 8, 2021

View reviewed changes

irbekrm mentioned this pull request Apr 8, 2021

Remove legacy e2e util functions for Issuer creation #3873

Merged

irbekrm mentioned this pull request Apr 9, 2021

Use upstream golang/crypto for ACME EAB + move crypto fork to cert-manager org #3877

Merged

irbekrm closed this Apr 9, 2021

irbekrm mentioned this pull request Apr 29, 2021

Webhook warnings #3936

Merged

maelvls mentioned this pull request Mar 6, 2022

Retry on conflict for the end-to-end test "CA Injector for api services should update data when the certificate changes" #4925

Merged

Conversation

irbekrm commented Apr 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jetstack-bot commented Apr 6, 2021

Uh oh!

irbekrm commented Apr 7, 2021

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

SgtCoDFish Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

SgtCoDFish Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

irbekrm Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

SgtCoDFish Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

SgtCoDFish Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

irbekrm Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

irbekrm Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

irbekrm commented Apr 9, 2021

Uh oh!

irbekrm commented Apr 9, 2021

Uh oh!

irbekrm commented Apr 9, 2021

Uh oh!

irbekrm commented Apr 9, 2021

Uh oh!

irbekrm commented Apr 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

irbekrm commented Apr 6, 2021 •

edited

Loading