Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/crypto/acme: add AccountKeyRollover #42516

Open
dandragona-dev opened this issue Nov 11, 2020 · 8 comments
Open

x/crypto/acme: add AccountKeyRollover #42516

dandragona-dev opened this issue Nov 11, 2020 · 8 comments

Comments

@dandragona-dev
Copy link

@dandragona-dev dandragona-dev commented Nov 11, 2020

RFC8555's Account Key Rollover is not yet supported in the acme package. This is a desirable RFC8555 feature that is supported by Let's Encrypt, and so CAs depending on this library may wish to also implement this feature.

The public API for this could be something like:

// AccountKeyRollover attempts to transition a client's account key to a new key.
// If the new key already belongs to an account registered with the CA then it will return the existing
// account's account URL (AKA the 'kid').
// Otherwise returns "", nil on success, and "", err for other error types.
// On successful key rollovers the client's Key field is updated with 'newKey'.
// https://tools.ietf.org/html/rfc8555#section-7.3.5
func (c *Client) AccountKeyRollover(ctx context.Context, newKey crypto.Signer) (string, error) {}
@gopherbot gopherbot added this to the Proposal milestone Nov 11, 2020
@gopherbot gopherbot added the Proposal label Nov 11, 2020
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals Nov 11, 2020
@rsc rsc changed the title proposal: x/crytpo/acme: Add Account Key Rollover Support proposal: x/crypto/acme: add AccountKeyRollover Dec 2, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Dec 2, 2020

@rsc rsc moved this from Incoming to Active in Proposals Dec 2, 2020
@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Dec 2, 2020

Seems reasonable. I'm not entirely sure there is much value in having the KID as a return value. I think we can probably just return an error and include the existing KID as part of the error message.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 9, 2020

OK, so I am seeing:

func (c *Client) AccountKeyRollover(ctx context.Context, newKey crypto.Signer) error

where there may be a specific KeyAlreadyRegisteredError returned containing the existing KID.
Do I have that right? Thanks.

@dandragona-dev
Copy link
Author

@dandragona-dev dandragona-dev commented Dec 9, 2020

That would work for my needs. I just wanted to make sure we have access to the existing KID for that case.

@rolandshoemaker
Copy link
Member

@rolandshoemaker rolandshoemaker commented Dec 9, 2020

That would work for my needs. I just wanted to make sure we have access to the existing KID for that case.

What is your use case for wanting the KID? I can see why you might want it during registration, but I'm not sure I see the value in having it during key rollover as you already have an account. Are you going to switch to the account using the key you wanted to rollover to?

@dandragona-dev
Copy link
Author

@dandragona-dev dandragona-dev commented Dec 15, 2020

I wanted to be able to provide some more context for the user about the error that occurred, although I do understand your line of reasoning.
Having the KID be returned through an error is perfectly suitable rather than having it in the API though. Since this is a proposal for the API I will leave the final decision to you whether you want to return the KID in the error message or not.

Thanks!

@rsc
Copy link
Contributor

@rsc rsc commented Dec 16, 2020

Based on the discussion above, this #42516 (comment) seems like a likely accept.

@rsc rsc moved this from Active to Likely Accept in Proposals Dec 16, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jan 6, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc moved this from Likely Accept to Accepted in Proposals Jan 6, 2021
@rsc rsc changed the title proposal: x/crypto/acme: add AccountKeyRollover x/crypto/acme: add AccountKeyRollover Jan 6, 2021
@rsc rsc modified the milestones: Proposal, Backlog Jan 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants