Behavior for deactivated accounts

[felixfontein]

While refactoring some code, I noticed the following:

• when doing a POST-as-GET for an existing, but deactivated account, an error is returned (403, "urn:ietf:params:acme:error:unauthorized");
• when doing a newAccount call for the account private key, the account object is returned with "status" == "deactivated".

According to the current draft, "Once an account is deactivated, the server MUST NOT accept further requests authorized by that account's key.", I would argue that the behavior for newAccount is wrong.

[felixfontein]

I just tested that with Boulder (Let's Encrypt staging v2 endpoint), it has the same behavior.

[cpu]

@felixfontein Interesting observation! Thanks for opening an issue.

According to the current draft, "Once an account is deactivated, the server MUST NOT accept further requests authorized by that account's key.", I would argue that the behavior for newAccount is wrong.

Hmmm. It seems like 7.3.6 is at odds with 7.3.1 which says:

If the server receives a newAccount request signed with a key for which it already has an account registered with the provided account key, then it MUST return a response with a 200 (OK) status code and provide the URL of that account in the Location header field. The body of this response represents the account object as it existed on the server before this request

In an ideal world 7.3.1 would have mentioned deactivated accounts but that ship has sailed ⛵ It seems reasonable to assume that 7.3.6 trumps 7.3.1 since otherwise account contact information would remain accessible after deactivation which seems problematic. I think returning an unauthorized error with a descriptive message ("An account with the provided public key exists but is deactivated") is probably the best path forward.

@felixfontein Do you feel game to write a PR for ☝️ or should I? :-)

I just tested that with Boulder (Let's Encrypt staging v2 endpoint), it has the same behavior.

Yup, I'll raise the issue today in our sprint planning meeting and see what folks think about updating Boulder to return an unauthorized problem for this case.

According to the current draft, "Once an account is deactivated, the server MUST NOT accept further requests authorized by that account's key.", I would argue that the behavior for newAccount is wrong.

Hmmm. It seems like 7.3.6 is at odds with 7.3.1 which says:

If the server receives a newAccount request signed with a key for which it already has an account registered with the provided account key, then it MUST return a response with a 200 (OK) status code and provide the URL of that account in the Location header field. The body of this response represents the account object as it existed on the server before this request

Hmm, that's indeed contradictory.

In an ideal world 7.3.1 would have mentioned deactivated accounts but that ship has sailed boat It seems reasonable to assume that 7.3.6 trumps 7.3.1 since otherwise account contact information would remain accessible after deactivation which seems problematic.

The single word "non-deactivated" inserted at the correct position would solve this. But I can see why you'd prefer no more changes.

I think returning an unauthorized error with a descriptive message ("An account with the provided public key exists but is deactivated") is probably the best path forward.

Sounds good! I'd prefer a new error code for this (so that this can be detected programmatically), but that would be an even bigger change.

@felixfontein Do you feel game to write a PR for point_up or should I? :-)

I'll try! Then you can do the reviewing / cleanup ;)

I just tested that with Boulder (Let's Encrypt staging v2 endpoint), it has the same behavior.

Yup, I'll raise the issue today in our sprint planning meeting and see what folks think about updating Boulder to return an unauthorized problem for this case.

I'd be interested in the outcome of this discussion.

[felixfontein]

Implemented in #180.

[cpu]

Implemented in #180.

Thanks @felixfontein ! I will get 👀 on that.

I'd be interested in the outcome of this discussion.

Everyone concurs with your assessment :-) We'll update Boulder to return a problem for this case as well.

Sounds good! I'd prefer a new error code for this (so that this can be detected programmatically), but that would be an even bigger change.

We're open to this. In discussion we proposed using an error under our own namespace. That might be done as a separate follow-up item because it will require some IANA work to register the error type & the WFE2 is currently always using the IETF namespace: https://github.com/letsencrypt/boulder/blob/9e39680e3f78c410e2d780a7badfe200a31698eb/wfe2/wfe.go#L886-L893

[cpu]

We'll update Boulder to return a problem for this case as well.

I opened letsencrypt/boulder#3971 for this

[jvanasco]

I am very happy Boulder and Pebble both use the same exact error string for this situation. Is there a chance both projects could have a comment in the source to ensure this stays in sync?

[felixfontein]

Hmm, I'm not sure whether it is a good idea that they return the same string. I would rather prefer a dedicated error code for this situation, which would be shared by other ACME servers as well, instead of having to look at error detail strings from different ACME servers (which could vary wildly). My client currently searches for deactivated in the detail string, but that's really a hack I'd like to remove...

[jvanasco]

I absolutely prefer the dedicated error string as well, but as cpu mentioned above... that is a larger effort. If Boulder and Pebble keep this message in sync, that would be relatively reliable until a custom namespace is used or the rfc is updated.