Implement account deactivation. #111
Conversation
calavera
force-pushed
the
deactivate_account
branch
2 times, most recently
from
d2c0e55
to
451eca2
Compare
|
@calavera Thanks for the PR! I'll give it a 🔍 in the next day or so. |
There was a problem hiding this comment.
One small request, otherwise looks great. Thanks @calavera ! I appreciate the PR 🌮
This change assumes that any further request that requires to check the account will return an unauthorized error. That might be something to ammend in the ACME spec.
This is what Boulder returns too 👍
wfe/wfe.go
Outdated
| if prob != nil { | ||
| wfe.sendError(prob, response) | ||
| return | ||
| if updateAcctReq.Status == "deactivated" { |
There was a problem hiding this comment.
I think it would be worth returning a probs.Malformed instance if the user POST's an update with Status != "deactivated" to be a little bit more strict here.
The way you have it implemented now will ignore an invalid status in an update which isn't unreasonable, but I'd like to shake out as many client errors in Pebble as possible.
It looks like Boulder is generous and does similar: it only checks that the update status != the existing status: https://github.com/letsencrypt/boulder/blob/699c7e4c4448bf70cd2410504684cc60737cd504/wfe/wfe.go#L1138-L1153 which is fine because ultimately the SA ignores the update status and explicitly uses the deactivated status in sa.DeactivateRegistration: https://github.com/letsencrypt/boulder/blob/699c7e4c4448bf70cd2410504684cc60737cd504/sa/sa.go#L1357
calavera
force-pushed
the
deactivate_account
branch
from
451eca2
to
e281ead
Compare
|
@cpu I've updated this change with your suggestion and rebased the PR to have only one commit. 🙌 |
wfe/wfe.go
Outdated
|
|
||
| // deactivatedStatus is the only valid status accepted by ACME to update Accounts | ||
| // and auhtorizations. | ||
| deactivatedStatus = "deactivated" |
There was a problem hiding this comment.
Could you add this to acme/common.go as a new top level exported status alongside StatusPending, StatusValid, etc? Only the wfe references it presently but I think its best to keep all of the ACME constants in one place where possible.
wfe/wfe.go
Outdated
| case updateAcctReq.Status != newAcct.Status: | ||
| wfe.sendError( | ||
| acme.MalformedProblem(fmt.Sprintf( | ||
| "Invalid account status: %s", updateAcctReq.Status)), response) |
There was a problem hiding this comment.
I think this %s should be %q, otherwise sending { "status": "" } returns "Invalid account status: "
wfe/wfe.go
Outdated
| switch { | ||
| case updateAcctReq.Status == deactivatedStatus: | ||
| newAcct.Status = updateAcctReq.Status | ||
| case updateAcctReq.Status != newAcct.Status: |
There was a problem hiding this comment.
I think this switch case might need to be updatedAcctReq.Status != "" && updatedAcctReq.Status != newAcct.Status.
It looks like as-written this breaks an account update that only changes the contact field, not sending any new status field:
$ pebble-client -email [email protected]
welcome to the pebble shell
Requesting directory from "https://localhost:14000/dir"
Requesting nonce from "https://localhost:14000/nonce-plz"
Registering new account with "https://localhost:14000/sign-me-up"
Requesting nonce from "https://localhost:14000/nonce-plz"
Your account ID is "https://localhost:14000/my-account/4e74148cfb32e323d86667990bc9b9ce5a28b47cfa6b86f76c09a762cec9f523"
Starting REPL environment...
$> Enter a directory endpoint or a URL to POST: https://localhost:14000/my-account/4e74148cfb32e323d86667990bc9b9ce5a28b47cfa6b86f76c09a762cec9f523
$> Enter JSON body, empty line to finish :
{ "contact":["mailto:[email protected]"] }
Requesting nonce from "https://localhost:14000/nonce-plz"
REPL error: Response 400: {
"type": "urn:ietf:params:acme:error:malformedRequest",
"detail": "Invalid account status: ",
"status": 400
}
ACME v11 is not very clear about the problem returned after the account has been deactivated: > Once an account is deactivated, the server MUST NOT accept further > requests authorized by that account's key. This change assumes that any further request that requires to check the account will return an unauthorized error. That might be something to ammend in the ACME spec. - Return a malformed problem if the status is not deactivated or the current account status. Signed-off-by: David Calavera <[email protected]>
calavera
force-pushed
the
deactivate_account
branch
from
e281ead
to
7833c14
Compare
|
@cpu Updated! Thanks again for the reviews 🌞 |
ACME v11 is not very clear about the problem returned after the account
has been deactivated:
This change assumes that any further request that requires to check the
account will return an unauthorized error. That might be something to
ammend in the ACME spec.
Fixes #108
Signed-off-by: David Calavera [email protected]