Use upstream golang/crypto for ACME EAB + move crypto fork to cert-manager org by irbekrm · Pull Request #3877 · cert-manager/cert-manager

irbekrm · 2021-04-09T14:22:22Z

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.

This PR was originally part of the too large #3850 that I split into smaller PRs. Once/if this gets merged, I would like to create another PR to return a warning from the validating webhook if an issuer with spec.acme.externalAccountBinding.keyAlgorithm field set.

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

/kind cleanup

Closes #3822

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>

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

irbekrm · 2021-04-09T14:23:43Z

/kind cleanup

SgtCoDFish

a few quite minor things which I don't feel strongly about but might be worth considering! looks good, generally 😄

pkg/apis/acme/v1/types_issuer.go

pkg/apis/acme/v1alpha2/types_issuer.go

pkg/apis/acme/v1alpha3/types_issuer.go

pkg/apis/acme/v1beta1/types_issuer.go

SgtCoDFish

/lgtm

wallrj

A couple of comments which you can address or simply answer and /unhold if you disagree.

/lgtm
/hold

go.mod

pkg/issuer/acme/setup.go

wallrj · 2021-04-09T15:18:02Z

test/e2e/suite/issuers/acme/issuer.go

+		})
+		Expect(err).To(HaveOccurred())
+		Expect(err).To(MatchError(wait.ErrWaitTimeout))
+	})


I'm not convinced that we need an E2E test for this. AFAICS it demonstrates that we can include or omit the KeyAlgorithm in our API calls and that the Issuer controller marks the Issuer Ready regardless....but it doesn't really tell us anything about whether the KeyAlgorithm is used.

I was going to suggest adding a unit-test instead, for the Setup function (above) but I see that it's over 200 lines long and has no existing unit-tests, so that might be a big rabbit hole.

Up to you. Remove this if you agree or leave it if you think it adds value.

+1 to Richard's comments - I'm not sure if the e2e test provides much value (and I think a unit test would actually provide more value) - but can appreciate this is a relatively untested area as is 😬

I added that bit to the e2e tests because this PR changes the part that e2e-tests ACME Issuer with ExternalAccountBinding- we are now only testing with an Issuer without the keyAlgorithm field.

Because of that I thought it would be good to have a basic high level e2e test that verifies that 1) an Issuer with that field can be created (gets past webhooks, becomes Ready etc) and 2) the field can be removed- which are the two things users probably would be doing? I could remove it, if it seems like it doesn't have any value.

I could look at unit testing the Setup function. I expect that it will need some refactoring so it might be a separate PR (that could come before this PR).

Seems reasonable to keep in order to verify that it all works fine with and without! 😊

wallrj · 2021-04-09T15:20:38Z

/unassign

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

pkg/issuer/acme/setup.go

test/e2e/suite/issuers/acme/issuer.go

munnerz · 2021-04-09T15:28:18Z

test/e2e/suite/issuers/acme/issuer.go

+		})
+		Expect(err).To(HaveOccurred())
+		Expect(err).To(MatchError(wait.ErrWaitTimeout))
+	})


+1 to Richard's comments - I'm not sure if the e2e test provides much value (and I think a unit test would actually provide more value) - but can appreciate this is a relatively untested area as is 😬

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

irbekrm · 2021-04-12T15:02:00Z

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

wallrj

@irbekrm it looks like you've answered all our comments.

/lgtm
/hold

I added a /hold comment in case you were planning to add unit-tests in this PR.
Unhold if you prefer to get this merged now.

jetstack-bot · 2021-04-13T10:20:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: irbekrm, SgtCoDFish, wallrj

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~~ [SgtCoDFish,irbekrm,wallrj]

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

irbekrm · 2021-04-13T12:16:48Z

I added a /hold comment in case you were planning to add unit-tests in this PR

Thank you @wallrj
If that's ok, I would like to add the tests in a separate PR closely following this one, because the Setup function is currently untested and I expect that we will need to refactor it a little.
This PR is not untested as such and in this case I feel like it might be better to not make this PR gigantic.
I will unhold this.

/hold cancel

irbekrm added 3 commits April 9, 2021 10:28

Issuer's ACME EAB algorithm can no longer be set

09af959

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>

Add e2e test cases for different ACME EAB configurations

2c2b3da

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

Update comment for replace directive in go.mod

697836e

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

irbekrm requested review from SgtCoDFish and wallrj April 9, 2021 14:22

jetstack-bot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Apr 9, 2021

SgtCoDFish reviewed Apr 9, 2021

View reviewed changes

pkg/apis/acme/v1/types_issuer.go Outdated Show resolved Hide resolved

pkg/apis/acme/v1alpha2/types_issuer.go Outdated Show resolved Hide resolved

pkg/apis/acme/v1alpha3/types_issuer.go Outdated Show resolved Hide resolved

pkg/apis/acme/v1beta1/types_issuer.go Outdated Show resolved Hide resolved

SgtCoDFish approved these changes Apr 9, 2021

View reviewed changes

jetstack-bot assigned SgtCoDFish Apr 9, 2021

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 9, 2021

wallrj approved these changes Apr 9, 2021

View reviewed changes

jetstack-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 9, 2021

jetstack-bot assigned wallrj Apr 9, 2021

jetstack-bot unassigned wallrj Apr 9, 2021

munnerz reviewed Apr 9, 2021

View reviewed changes

irbekrm mentioned this pull request Apr 9, 2021

Unit test for ACME Issuer setup #3879

Closed

Standardize deprecation warnings

d213b4b

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

irbekrm force-pushed the move_crypto_fork branch from b32e59f to d213b4b Compare April 12, 2021 08:39

jetstack-bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 12, 2021

irbekrm mentioned this pull request Apr 12, 2021

[release-1.3] Certificate's revision history limit validated by webhook #3886

Merged

wallrj approved these changes Apr 13, 2021

View reviewed changes

jetstack-bot assigned wallrj Apr 13, 2021

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2021

jetstack-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2021

jetstack-bot merged commit b5be5a8 into cert-manager:master Apr 13, 2021

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

irbekrm deleted the move_crypto_fork branch April 13, 2021 12:30

maelvls mentioned this pull request Apr 21, 2021

Move fork github.com/meyskens/crypto to github.com/cert-manager/crypto #3220

Closed

This was referenced Apr 29, 2021

Webhook warnings #3936

Merged

3879 test acme issuer setup #3938

Merged

irbekrm mentioned this pull request Jun 10, 2021

Update EAB to reflect requirement for the MAC algorithm being removed cert-manager/website#598

Merged

maelvls mentioned this pull request Jun 15, 2021

Write the 1.4 release notes cert-manager/website#605

Merged

mrajashree mentioned this pull request Nov 30, 2021

Update cert-manager tag to v1.5.3 aws/eks-anywhere-build-tooling#213

Merged

Conversation

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

Uh oh!

Uh oh!

irbekrm commented Apr 9, 2021

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wallrj Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

munnerz Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

munnerz Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

wallrj commented Apr 9, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

munnerz Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

irbekrm commented Apr 12, 2021

Uh oh!

wallrj left a comment

Choose a reason for hiding this comment

Uh oh!

jetstack-bot commented Apr 13, 2021

Uh oh!

irbekrm commented Apr 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

irbekrm commented Apr 9, 2021 •

edited

Loading

irbekrm Apr 9, 2021 •

edited

Loading