Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great step towards making sure the status state changes are consistent & understandable.
I've flagged a few issues to address.
validate the challenge multiple times (see {{retrying-challenges}}). | ||
Likewise, client requests for retries do not cause a state change. | ||
If validation is successful, the challenge moves to the "valid" | ||
state; if there is an error, the challenge moves to the "invalid" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there is an error, and the challenge moves to the "invalid" state, how does the client request retry? That seems like the primary reason you would want to retry and there isn't any information about how the server should handle failures during processing except to change out of processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think "invalid" is a final decision. If the client wants another try, they'd need to create a new order.
Section 8.2 says that "the server SHOULD retry the query after some time".
Being unable to retry individual challenges might mean that some users will have a hard time acquiring a certificate. But then they should just fix their setup and respond to the challenge only when it will work.
I.e. I agree with the above change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, what @joernheissler said. I had also been thinking of "invalid" as a final state. ISTM if you want to signal to the client that something has gone wrong and a retry is needed, there are basically three options:
- Keep it in "processing" and add to the subproblems array
- Move it to "invalid" and add to the subproblems array
- Move it to a new "retry" state and add to the subproblems array
(1) is what we do now, and in case you can't tell by my phrasing here, I don't think we need any more signal than adding to the subproblems array. It seems like the only real argument for having a non-"processing" state is if find it distasteful to have "processing" with "error" populated, in which case I would propose we do (3) instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. That sounds reasonable.
draft-ietf-acme-acme.md
Outdated
the challenges listed in the authorization transitions to the | ||
"valid" state, then the authorization also changes to the "valid" | ||
state. If there is an error while the authorization is still | ||
pending (e.g., the authorization times out), then the authorizatoin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does "the authorization times out" mean? I thought perhaps expiry but you mention that distinctly later in this paragraph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*authorizatoin->authorization
draft-ietf-acme-acme.md
Outdated
pending (e.g., the authorization times out), then the authorizatoin | ||
transitions to the "invalid" state. Once the authorization is in | ||
the valid state, it can expire ("invalid"), be deactivated by the | ||
client ("deactivated", see {{deactivating-an-authorization}}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're missing a connecting word here between deactivating and revoking.
or, revoked by the server ("revoked").
draft-ietf-acme-acme.md
Outdated
| | | ||
| | | ||
Client | Server | | ||
deactiv.| revoke | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: If you put the revocation case on the left fork I think you could fit the whole word "deactivated" on the right fork
draft-ietf-acme-acme.md
Outdated
"processing" state after the client submits a request to the order's | ||
"finalize" URL and the CA begins the issuance process for the | ||
certificate. Once the certificate is issued, the order enters the | ||
"valid" state. If a fatal error occurs at any of these stages, the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should skip the word "fatal" here - we never define what a fatal error is versus a normal error. They're all just errors.
draft-ietf-acme-acme.md
Outdated
"finalize" URL and the CA begins the issuance process for the | ||
certificate. Once the certificate is issued, the order enters the | ||
"valid" state. If a fatal error occurs at any of these stages, the | ||
order moves to the "invalid" state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This paragraph (& the flow chart) are also missing three ways that an order can become invalid:
- One of its authorizations becomes invalid because it was deactivated
- One of its authorizations becomes invalid because it expired
- The order expires
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of its authorizations becomes invalid because it was deactivated
Or the authorization was revoked. :-)
We should probably say "If any of the authorizations changes to a state other than pending or valid, the order becomes invalid."
draft-ietf-acme-acme.md
Outdated
the challenges listed in the authorization transitions to the | ||
"valid" state, then the authorization also changes to the "valid" | ||
state. If there is an error while the authorization is still | ||
pending (e.g., the authorization times out), then the authorizatoin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*authorizatoin->authorization
draft-ietf-acme-acme.md
Outdated
+---------+---------+ | ||
| | | ||
V V | ||
invalid <----------- valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's treat expired as its own state, and note that it doesn't show up in the "status" field: the URL just starts to 404.
Also, there should be an arrow from pending to expired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't require 404 when an authz expires:
If the client fails to complete the required actions before the "expires" time, then the server SHOULD change the status of the order to "invalid" and MAY delete the order resource.
So we shouldn't add any text here that would imply that 404 is necessary.
|
||
Order objects are created in the "pending" state. Once all of the | ||
authorizations listed in the order object are in the "valid" state, | ||
the order transitions to the "ready" state. The order moves to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "ready" state makes some sense but is a significant addition. I'm inclined to leave it out of this change, to more clearly separate "additional documentation" from "protocol changes".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny you should say that, because "expires" is also a new state :) This change is not large in spec terms -- this PR is all it needs. It doesn't seem like it should be a huge change for implementations either.
draft-ietf-acme-acme.md
Outdated
"finalize" URL and the CA begins the issuance process for the | ||
certificate. Once the certificate is issued, the order enters the | ||
"valid" state. If a fatal error occurs at any of these stages, the | ||
order moves to the "invalid" state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of its authorizations becomes invalid because it was deactivated
Or the authorization was revoked. :-)
We should probably say "If any of the authorizations changes to a state other than pending or valid, the order becomes invalid."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking better after the last few rounds of feedback were applied.
One oversight that still remains is in the "Applying for Certificate Issuance" section.
There is a section about how the status of the order post-finalization tells the client what to do. It covers "invalid", "pending", "processing" and "valid" - I think it needs to be updated with "ready". Similarly the existing text for "processing" doesn't match this new state model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small pieces of feedback
draft-ietf-acme-acme.md
Outdated
is in the process of generating the certificate. Retry after the time given | ||
in the "Retry-After" header field of the response, if any. | ||
* "ready": The server agrees that the requirements have been | ||
fulfilled, and is awaiting on finalization. Submit a finalization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "and is awaiting on finalization" -> "and is awaiting finalization".
draft-ietf-acme-acme.md
Outdated
fulfilled, and is awaiting on finalization. Submit a finalization | ||
request. | ||
|
||
* "processing": The certificate is being issued. Retry after the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Retry" in this case might need to be disambiguated slightly, what about "Poll the order again after the ... " ?
I'm fine accepting the "ready" state based on the above, although Let's Encrypt won't have it implemented by v2 launch time. I think that's fine. One tiny bit of further clarification: I was thinking of "expired" as a meta-state that is not necessarily reflected in the status field. I think we can just add a sentence saying something like "objects in an expired state may simply return 404." |
This PR attempts to add a lot more clarity to the "status" field on objects and how it evolves.
Fixes #391
Fixes #397 (by showing why challenges need separate status)
Slightly beyond the scope of this PR, it might also be good to: