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

wfe: Handle empty JSON to /acme/acct like POST-as-GET #7844

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jprenken
Copy link
Contributor

ACME clients can make an empty request to /acme/acct for their reg object. wfe.Account handled a truly empty request correctly, but misrouted empty JSON objects to updateAccount. This only happened to work because UpdateRegistration takes an entire Registration and returns it even if unchanged.

This preserves ACMEv1 backwards compatibility when the updateAccount flow is simplified.

ACME clients can make an empty request to /acme/acct for their reg object. wfe.Account handles a truly empty request correctly, but has routed empty JSON objects to updateAccount, even though an update is not appropriate. This only happened to work because UpdateRegistration's impl takes an entire Registration and returns it even if unchanged.

This preserves ACMEv1 backwards compatibility when UpdateRegistration is simplified.
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rephrase the PR description to more fully describe this behavior. Something like:

Early drafts of the ACME spec said that clients should retrieve their existing account information by POSTing the empty JSON object {} to their account URL. This instruction was removed in the final version of ACME, replaced by the concept of POST-as-GET which uses a wholly empty body to accomplish the same goal. However, Boulder has continued to incidentally support this behavior: when we receive an empty JSON object, our updateAccount code in the RA applies their desired diff (none) on top of their current account, writes it back to the database, and returns the updated account object... which hasn't actually changed. This behavior is also half-tested by TestEmptyAccount, but that test is actually testing that the MockRA implements the same behavior as the real RA; it's not truly testing the WFE's behavior.

This PR changes the WFE to explicitly treat receiving the empty JSON object as a request to retrieve the account data unchanged, rather than implicitly relying on internal details of the RA's account-update logic.

Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a prerequisite for #7827, right? Would be good to link that in the description for ease of understanding ("when the updateAccount flow is simplified" was only enough for me to find it because I have the context paged in right now, but pull requests are used in the future to understand why changes were made).

Otherwise looks good. Nice to break out this improvement on its own. Oh, which reminds me - do we already have a test for the "{}" case?

Comment on lines +1419 to +1422
// If the body was not either completely empty or an empty JSON object, then
// this is an account update request. Treating the empty JSON object like a
// POST-as-GET is a holdover from ACMEv1.
if string(body) != "" && string(body) != "{}" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight quibble: it's still reasonable in ACMEv2 to treat "{}" as an update to zero fields, returning the object. It's true that a more recently written client would be more likely to just use "" if it knows its just trying to fetch the account. Perhaps this?

Suggested change
// If the body was not either completely empty or an empty JSON object, then
// this is an account update request. Treating the empty JSON object like a
// POST-as-GET is a holdover from ACMEv1.
if string(body) != "" && string(body) != "{}" {
// An empty string means POST-as-GET (i.e. no update). A body of "{}" means
// an update of zero fields, returning the unchanged object. This was the
// recommended way to fetch the account object in ACMEv1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants