Add Retry-After header when responding to Order and Authorization object #380
Conversation
|
Thank you very much for your feedback. |
aarongable
left a comment
There was a problem hiding this comment.
Just two small comments, otherwise looks good to me!
wfe/wfe.go
Outdated
| if (second > 0){ | ||
| if (rand.Intn(2) == 0){ | ||
| response.Header().Add("Retry-After", strconv.Itoa(second)) | ||
| }else{ |
There was a problem hiding this comment.
Make sure you run gofmt to fix the spacing around these curly braces.
wfe/wfe.go
Outdated
| // Check for the existence of a challenge which state is processing | ||
| processingChallenge := false | ||
| for _, c := range authz.Challenges { | ||
| if (c.Status == acme.StatusProcessing) { | ||
| processingChallenge = true | ||
| break | ||
| } | ||
| } | ||
| // if exist, then add header | ||
| if processingChallenge { | ||
| addRetryAfterHeader(response, wfe.retryAfterAuthz) | ||
| } |
There was a problem hiding this comment.
nit: inline the call to add the header, so you don't need the extra variable:
| // Check for the existence of a challenge which state is processing | |
| processingChallenge := false | |
| for _, c := range authz.Challenges { | |
| if (c.Status == acme.StatusProcessing) { | |
| processingChallenge = true | |
| break | |
| } | |
| } | |
| // if exist, then add header | |
| if processingChallenge { | |
| addRetryAfterHeader(response, wfe.retryAfterAuthz) | |
| } | |
| // Check for the existence of a challenge which state is processing, and add the | |
| // Retry-After header if there is one. | |
| processingChallenge := false | |
| for _, c := range authz.Challenges { | |
| if (c.Status == acme.StatusProcessing) { | |
| addRetryAfterHeader(resonse, wfe.retryAfterAuthz) | |
| break | |
| } | |
| } |
|
I finished editing it! |
wfe/wfe.go
Outdated
| // Check for the existence of a challenge which state is processing, and add the | ||
| // Retry-After header if there is one. | ||
| for _, c := range authz.Challenges { | ||
| if c.Status == acme.StatusProcessing { |
There was a problem hiding this comment.
As mentioned in this comment, the processing state for challenges is not currently implemented in Pebble.
Consequently, the Retry-After header is never sent for challenges.
I think we'd need to set the state to processing right as the challenge is queued up for validation in wfe.updateChallenge. Do we want to do this in a separate PR or here?
There was a problem hiding this comment.
@alexzorin
There was an error in my understanding. My apologies.
I now understand that processing status is not currently used for challenge in pebble.
Regarding the unused processing, I felt that perhaps it would be appropriate to proceed with implementation in another PR.
wfe/wfe.go
Outdated
| // Check for the existence of a challenge which state is processing, and add the | ||
| // Retry-After header if there is one. | ||
| for _, c := range authz.Challenges { | ||
| if c.Status == acme.StatusProcessing { |
There was a problem hiding this comment.
I think it's likely that we need to do a c.RLock() and c.RUnlock() around the access to c.Status, or it may be a race.
There was a problem hiding this comment.
@alexzorin
Thank you for your comment. I will attempt to fix this problem.
There was a problem hiding this comment.
This problem was fixed by commit 6f709f9
|
@alexzorin |
|
Sorry for disappearing. I'd still like to fix #382 soon, to enable this PR. |
|
Thank you for implementing it! |
For #380 (comment). Keeping track of when a challenge is in the `processing` state means we will be able to then include a `Retry-After` response header for authorizations, as described in: > To check on the status of an authorization, the client sends a POST- as-GET request to the authorization URL, and the server responds with the current authorization object. In responding to poll requests while the validation is still in progress, the server MUST return a 200 (OK) response and MAY include a Retry-After header field to suggest a polling interval to the client. One extra side effect here is that clients will no longer be able to cause Pebble to queue up a `pending` challenge multiple times for processing by a VA. This seems like a good thing anyway.
|
@alexzorin |
alexzorin
left a comment
There was a problem hiding this comment.
This LGTM and appears to work well, thank you!
The PR will still require another review from @aarongable to merge.
Address the issue #83
Fixed to add Retry-After header when responding to requests for Order and Authorization objects.
Although it is a static, The value of Retry-After is defined in the configuration file.
I would appreciate it if you could review this PR.