proposal: x/crypto/acme: Add External Account Binding Support

[jdkasten]

RFC 8555's External Account Binding is not currently supported by x/crypto/acme while there are multiple certificate authorities that require the feature. (Sectigo, zerossl, etc)

External Account Binding (EAB) occurs in ACME's "new-account" RPC. Within acme.go this is handled by Register()

The public API for this method would not have to change. Notably, the call takes in an Account struct which is a non-wire compatible version of an ACME Account object. Since the normal wire protocol account object contains the EAB, a simple extension of the Account struct seems appropriate. This new field would contain an "EAB struct" that would be easily configurable by end users. The EAB struct can be similarly serialized into an ACME EAB JWS within the existing Register method.

As far as requirements go, the RFC states that

1. EAB key needs to be a MAC key
2. The key identifier (kid) be ASCII.
3. CA's SHOULD provide the key in base64url-encoded format for compatibility.

I have seen many client implementations assume HMAC SHA256 is used, though this is not guaranteed by the protocol. [1, 2]

The External account binding struct would need to look something like

// ExternalAccountBinding contains the data needed to form a request with
// an external account binding.
// See https://tools.ietf.org/html/rfc8555#section-7.3.4 for more details.
type ExternalAccountBinding struct {
    // KID is the Key ID of the symmetric MAC key that the CA uses to
    // identify an external account from ACME.
    KID string

    // Key is the bytes of a symmetric key that the CA uses to identify
    // the account. The KID should reference the same key that the CA holds.
    Key []byte

    // KeyAlgorithm of the JWS. Only the HMAC algorithms are supported
        // https://tools.ietf.org/html/rfc7518#section-3.1
    KeyAlgorithm string
}

Adapted from @munnerz's existing PR for this functionality.

[FiloSottile]

Looks good to me, thank you. I suspect Account is the right place to put it, but I am not a fan of adding private keys to a type that did not have one before and might get logged. We should probably add a String method to hide the key from formatting.

Given Client has the account key, would it be a better place for the ExternalAccountBinding field?

/cc @rolandshoemaker

[rolandshoemaker]

Account is probably the correct place for it since the EAB is only needed during account creation and putting it in Client may suggest that the user needs to populate it for all subsequent API calls after Client.Register, which could cause them to persist the key longer than they need to.

Agreed we should probably put something in place to prevent logging the field. I realize now that it's not explicitly called out in the RFC but the best practice should be for EAB keys to be one-time use (such that a EAB key can only be used to associate an external account with a single ACME account) but I suspect there will be implementations out there that will allow an EAB key to be used multiple times, so leaking the key would be bad.

[jdkasten]

Thank you for the reviews. I agree that additional precautions should be implemented to help protect the key from being accidentally logged.

[rolandshoemaker]

I think my only other real comment on this would be to replace KeyAlgorithm with a enum instead of a string, since we're only going to be supporting like three things, but ¯_(ツ)_/¯.

[jdkasten]

Assuming this proposal is in a reasonable state, I will aim to build upon the existing work by munnerz@ with the changes discussed here.

[gopherbot]

Change https://golang.org/cl/269279 mentions this issue: acme: Add External Account Binding Support

[tmthrgd]

I realise the CL has just landed so unfortunately not the best timing, but I’m quite confused about why the caller should have to specify the MAC algorithm. The RFC says only that “the CA operating the ACME server needs to provide the ACME client with a MAC key and a key identifier” with no mention of the algorithm.

It seems to me that the algorithm is entirely an internal implantation detail and should not be visible to callers. Furthermore, there’s no compelling reason (that I’m aware of) for callers to want to change this, all of the options are secure, in particular HMAC-SHA256 is a perfectly secure choice here that’s most likely to be compatible across the ecosystem. This really just seems like needless complexity and cryptographic agility that callers will have to understand and deal with (something I do know @FiloSottile has rightly championed against in the past).

The field and constants also provide no documentation about how a caller should pick the algorithm, which at the very least should be rectified. Otherwise I think most choices will be something like: HS256 is the first one so pick that or HS512 is the biggest number so it must be most secure.

Ultimately, is there any real use case where always using HS256 wouldn’t work? If so, how likely are callers to understand and correctly deal with that situation? It seems like that’s the approach used elsewhere and it would both simplify the API, the implementation and ease of use.

[jameshartig]

I wonder if it could default to HMAC-SHA256 (if field was left empty) and then someone would only change it from the default if they explicitly told to.

[FiloSottile]

Ah, yes, I should have caught that. I thought the MAC algorithm was a property of the key, where each key needs to be used with a server-picked algorithm, but if it's not we should just hardcode HMAC-SHA256 and try to shape the ecosystem for the better (as there is definitely no need for agility here).

The API is one weekend old, so informally I can't imagine we will break anyone by dropping it, and formally there is no compatibility promise in x/ repos at v0.0.0. (We still provide extremely strict backwards compatibility, but this feels like a good opportunity for an exception.)

I'll send a CL in a minute. We can always add it back if we're wrong.

[gopherbot]

Change https://golang.org/cl/279453 mentions this issue: acme: hardcode and remove ExternalAccountBinding.Algorithm