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

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.

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

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 ¯_(ツ)_/¯.

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

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

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.

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.

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.

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