acme: external account binding support #2392

JoshVanL · 2019-11-21T17:56:05Z

Adds ACME external account binding support

/assign
/hold

jetstack-bot · 2019-11-21T17:56:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoshVanL

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

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

munnerz · 2019-11-26T09:36:58Z

pkg/internal/apis/acme/types_issuer.go

+
+	// Key is a base64 encoded symmetric key that authenticates the account with
+	// an external account.
+	Key string `json:"key,omitempty"`


This feels like a sensitive value, given it is used to authenticate/bind a registration to some external system?

Also, how come this is a base64 encoded string? Does this really mean it is a []byte?

Indeed, makes sense to move to a secret

In theory you can should be able to use raw []byte data for your symmetric MAC key. base64 encoding makes then becomes a requirement

munnerz · 2019-11-26T09:39:06Z

test/e2e/suite/conformance/certificates/acme/acme.go

+
+	provisionerDNS01 := &acmeIssuerProvisioner{
+		eab: eab,
+	}


Does this mean we only test with EAB enabled now? I'd be happy to just see the suite run using e.g. HTTP01 with EAB enabled (I don't think we really need to run the full DNS01 && HTTP01 suite with EAB enabled), but I'd also like to see this run without EAB enabled too (as before).

This should run the acme tests with and without EAB. the passed in eab struct can be nil

third_party/crypto/acme/acme.go

munnerz · 2019-11-26T09:40:36Z

third_party/crypto/acme/acme.go

-	if err != nil {
-		return nil, err
-	}
+func (c *Client) postJWS(ctx context.Context, key crypto.Signer, accountURL, url, nonce string, body interface{}) (*http.Response, error) {


Can you update the comments on every function/method you've changed the signature on to explain what the new parameter is, what it can be set to, and its behaviour if not set?

Also, how come you've changed this to accept a nonce instead of just retrieving one as before?

All in all, we should attempt to keep the diff in third_party as minimal as possible IMO 😄

Yes, this signature shouldn't need to change I think. Some things moved around in dev but was moved back. Forgot about reverting this change

munnerz · 2019-12-04T09:53:15Z

Not sure why Pebble is returning this:

  Warning  Failed     4m31s  cert-manager  Accepting challenge authorization failed: acme: authorization error for efsxk.certmanager.kubernetes.network: 400 urn:ietf:params:acme:error:connection: Get https://efsxk.certmanager.kubernetes.network/.well-known/acme-challenge/TRrVcMxk3V2oH6r6WK3kbMzmNvWopGCiRRtsqJHNe3k: http: server gave HTTP response to HTTPS client

in some of the ACME test cases where nginx is configured to redirect to a HTTPS address. This implies to me that nginx is responding with HTTP content on the HTTPS port 🤔

Retesting to see if this is a flake, and will audit changes to Pebble since the old vs new commit as I've not seen this before!

/retest

munnerz · 2019-12-04T10:06:58Z

After some digging in pebble, I think I have identified the cause and I've opened this issue to track it: letsencrypt/pebble#293

JoshVanL

A couple comments.

We also need to add to the checks for secrets that are referenced by a possible EAB account and have been updated

/unassign
/assign @munnerz

JoshVanL · 2019-12-10T14:41:06Z

pkg/apis/acme/v1alpha2/types_issuer.go

+// server.
+type ACMEExternalAccountBinding struct {
+	// keyID is the ID of the CA key that the External Account is bound to.
+	KeyID string `json:"keyID"`


KeyID can be potentially sensitive information. We should er on the side of caution and make this a secret reference

I think at this point we can keep it as keyID - it's effectively the 'username' parameter here, and the 'secret' value should be sufficient to protect user accounts. It's not really explored in the RFC, and if needed in future, we can add an additional field for a secret ref variant of this, for those who are more sensitive about key IDs.

pkg/internal/apis/acme/fuzzer/fuzzer.go

pkg/internal/apis/acme/types_issuer.go

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

Signed-off-by: James Munnelly <james@munnelly.eu>

JoshVanL · 2019-12-11T14:51:01Z

Looks good to me :)

munnerz · 2019-12-11T14:53:28Z

/lgtm

jetstack-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 21, 2019

jetstack-bot assigned JoshVanL Nov 21, 2019

jetstack-bot requested a review from munnerz November 21, 2019 17:56

JoshVanL force-pushed the acme-external-account-binding branch from 0cbad88 to 6d9473c Compare November 21, 2019 17:58

JoshVanL force-pushed the acme-external-account-binding branch 2 times, most recently from f522658 to 78cbe36 Compare November 23, 2019 17:41

munnerz reviewed Nov 26, 2019

View reviewed changes

munnerz force-pushed the acme-external-account-binding branch from 326d360 to 7ef9782 Compare December 3, 2019 12:00

jetstack-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 3, 2019

munnerz force-pushed the acme-external-account-binding branch 5 times, most recently from 013fee8 to 5d9391e Compare December 3, 2019 17:38

munnerz added this to In progress in v0.13 via automation Dec 3, 2019

munnerz added this to the v0.13 milestone Dec 3, 2019

munnerz force-pushed the acme-external-account-binding branch from bad13ae to 3b33bd3 Compare December 3, 2019 21:49

munnerz mentioned this pull request Dec 4, 2019

Regression when validating HTTP01 with a HTTP->HTTPS redirect letsencrypt/pebble#293

Closed

munnerz force-pushed the acme-external-account-binding branch from 3b33bd3 to 3bb71e5 Compare December 5, 2019 12:19

munnerz moved this from In progress to Review in progress in v0.13 Dec 5, 2019

JoshVanL commented Dec 10, 2019

View reviewed changes

jetstack-bot assigned munnerz and unassigned JoshVanL Dec 10, 2019

jetstack-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 11, 2019

munnerz added 8 commits December 11, 2019 10:37

acme: Add API fields for ExternalAccountBinding

80bc253

Signed-off-by: James Munnelly <james@munnelly.eu>

Use fork of x/crypto with support for EAB

861e0f9

Signed-off-by: James Munnelly <james@munnelly.eu>

Use forked pebble with support for EABs

0f196a5

Signed-off-by: James Munnelly <james@munnelly.eu>

Add end-to-end tests using EAB

4930a0e

Signed-off-by: James Munnelly <james@munnelly.eu>

acme: Add support for external account binding

fd306c5

Signed-off-by: James Munnelly <james@munnelly.eu>

Fix apiext.JSON fuzzer

3f21284

Signed-off-by: James Munnelly <james@munnelly.eu>

Use enum for HMAC algorithm field

5c4e278

Signed-off-by: James Munnelly <james@munnelly.eu>

Update checks.go for external account bindings

ff8c683

Signed-off-by: James Munnelly <james@munnelly.eu>

munnerz force-pushed the acme-external-account-binding branch from 6d6219a to ff8c683 Compare December 11, 2019 12:38

jetstack-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 11, 2019

munnerz mentioned this pull request Dec 11, 2019

webhook: register http handlers for pprof debug endpoints #2450

Merged

jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2019

jetstack-bot merged commit 9d95d2b into cert-manager:master Dec 11, 2019

v0.13 automation moved this from Review in progress to Done Dec 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

acme: external account binding support #2392

acme: external account binding support #2392

JoshVanL commented Nov 21, 2019 •

edited by munnerz

Loading

jetstack-bot commented Nov 21, 2019

munnerz Nov 26, 2019

munnerz Nov 26, 2019

JoshVanL Nov 26, 2019

JoshVanL Nov 26, 2019

munnerz Nov 26, 2019

JoshVanL Nov 26, 2019

munnerz Nov 26, 2019

munnerz Nov 26, 2019

munnerz Nov 26, 2019

JoshVanL Nov 26, 2019

munnerz commented Dec 4, 2019

munnerz commented Dec 4, 2019

JoshVanL left a comment

JoshVanL Dec 10, 2019

munnerz Dec 11, 2019

JoshVanL commented Dec 11, 2019

munnerz commented Dec 11, 2019

acme: external account binding support #2392

acme: external account binding support #2392

Conversation

JoshVanL commented Nov 21, 2019 • edited by munnerz Loading

jetstack-bot commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

munnerz commented Dec 4, 2019

munnerz commented Dec 4, 2019

JoshVanL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshVanL commented Dec 11, 2019

munnerz commented Dec 11, 2019

JoshVanL commented Nov 21, 2019 •

edited by munnerz

Loading