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

Fix handling of Retry-After header #4543

Closed
mhoffrog opened this issue Mar 7, 2023 · 4 comments
Closed

Fix handling of Retry-After header #4543

mhoffrog opened this issue Mar 7, 2023 · 4 comments

Comments

@mhoffrog
Copy link
Contributor

mhoffrog commented Mar 7, 2023

Description

@Neilpang IMO the following coding is critical - see list of reasons below:

acme.sh/acme.sh

Lines 2232 to 2233 in 799e402

_retryafter=$(echo "$responseHeaders" | grep -i "^Retry-After *:" | cut -d : -f 2 | tr -d ' ' | tr -d '\r')
if [ "$code" = '503' ] || [ "$_retryafter" ]; then

  1. If the Retry-After header is provided by another status than 503 - e.g. by 429 (limit reached), then a retry at this code place will be critical, since e.g. in case of limit "too many requests for the same domain id within last 168 hours(=7 days)" the Retry-After duration will be a couple of days!

  2. The current coding will fail, if the Retry-After value is provided as RFC1123 HTTP-date formatted value.
    To keep it simple at this place we should allow value in seconds only by filtering Retry-After for a numeric value through the regex used for grep.

PR #4544 has been created to, close this issue.

References

LE about 503 vs. 429: https://community.letsencrypt.org/t/new-service-busy-responses-beginning-during-high-load/184174
429 mentioning Retry-After: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/429
Rate Limits: https://www.rfc-editor.org/rfc/rfc8555#section-6.6
Retry-After value formats: https://www.rfc-editor.org/rfc/rfc7231#section-7.1.3
RFC1123 HTTP-date format https://wiki.freepascal.org/RFC_1123_Time_Format

@github-actions
Copy link

github-actions bot commented Mar 7, 2023

Please upgrade to the latest code and try again first. Maybe it's already fixed. acme.sh --upgrade If it's still not working, please provide the log with --debug 2, otherwise, nobody can help you.

@mhoffrog
Copy link
Contributor Author

mhoffrog commented Mar 7, 2023

Please upgrade to the latest code and try again first. Maybe it's already fixed. acme.sh --upgrade If it's still not working, please provide the log with --debug 2, otherwise, nobody can help you.

PR #4544 is supposed to close this issue.

@Th0masL
Copy link

Th0masL commented Mar 16, 2023

I can confirm that there's a problem in the way the retry are currently handled.

I posted a reply to the PR #4530, as I thought the problem was coming from it.

In my case, there was too many identical certs issued, and it was asking me to wait 168 hours ...

[Wed Mar 15 23:08:31 EET 2023] response='{
  "type": "urn:ietf:params:acme:error:rateLimited",
  "detail": "Error creating new order :: too many certificates (5) already issued for this exact set of domains in the last 168 hours: mycert.domain.com, retry after 2023-03-17T01:40:49Z: see https://letsencrypt.org/docs/duplicate-certificate-limit/",
  "status": 429
}'
[Wed Mar 15 23:08:31 EET 2023] It seems the CA server is currently overloaded, let's wait and retry. Sleeping 102737 seconds.

We should probably add a high threshold that bypass the waiting logic.

For example, if the retry header value is more than X (i.e. 600 seconds, or10 minutes) , then don't wait and throw an error.

I don't know anyone that want to wait for 10 minutes to be able to retry to have their cert generated.

@mhoffrog
Copy link
Contributor Author

mhoffrog commented Mar 16, 2023

@Neilpang
Many thanks to @Th0masL confirming my assumption - this was exactly what I expect with the current implementation!
I did not test it that way. Your post completely confirms it!
Fortunately they come up with the Retry-After value formatted as seconds - if they would have come with the http date format that would have had failed anyway, since we do not consider this format yet.

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

No branches or pull requests

2 participants