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.

Sign up for GitHub

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

Jump to bottom

x/crypto/acme: nonce error returned because of rate limit hit #18428

Closed
jmhodges opened this issue Dec 25, 2016 · 7 comments
Closed

x/crypto/acme: nonce error returned because of rate limit hit #18428

jmhodges opened this issue Dec 25, 2016 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations)

Comments

@jmhodges
Copy link
Contributor

I'm consistently getting back acme: nonce not found on a cert of mine, because fetchNonce is doing a few HEADs to https://acme-v01.api.letsencrypt.org/acme/new-authz and getting back a 429 rate limit hit back.

While the real problem is the rate limit being reached, it was hidden by the nonce not found error that can only be detected by calling err.Error() on it.

Let's Encrypt could be kinder to us, but it would be beneficial for x/crypto/acme, if it needs a nonce, to hit an endpoint that is guaranteed to be relatively safe from rate-limiting. Perhaps, the /directory path?

It'd also be cool for crypto/acme to be storing nonces it's gathered from previous normal API calls and using those, instead of calling out again to fetchNonce for every new API call.

It'd also be nice for crypto/acme to return a specific error when it hits one of the various rate limits.
@jmhodges jmhodges mentioned this issue Dec 25, 2016
Closed
@jmhodges
Copy link
Contributor Author

I made letsencrypt/boulder#2450 for Let's Encrypt's side of this, but it seems like a fixable thing from crypto/acme's side.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/34623 mentions this issue.

@jmhodges
Copy link
Contributor Author

jmhodges commented Dec 25, 2016 •
edited
Loading

(Just as FYI: the 429s from Let's Encrypt are a new, currently undocumented request-per-IP-per-endpoint-per-second rate limit that's set fairly low if you're parallelizing new-authz calls for a cert. This doesn't change crypto/acme's decision making much, though.)

jmhodges added a commit to jmhodges/crypto that referenced this issue Dec 26, 2016
@jmhodges
x/crypto/acme: use a HEAD-safe URL for nonce fetch
1d4de5f 
This is a minimal fix to the HEAD rate limit problem discussed in
golang/go#18428. We send the HEADs for fetching nonces to a URL that we
know will be safer to hit than the POST-only resources where Let's
Encrypt is interpreting HEAD requests as POSTs and incrementing rate
limits as such. The safe URL chosen is the directory URL of the ACME
server.

A larger fix might use nonces gathered from previous API calls instead
of making a second API call for each API call made. This also does not
address the rate limit error being hidden by the nonce error as
discussed in golang/go#18428.

Updates golang/go#18428.

Change-Id: I378c64759bd5315aa96b4daff107d2741d742349
@rsc
Copy link
Contributor

rsc commented Jan 4, 2017

/cc @bradfitz

@rsc rsc added this to the Soon milestone Jan 4, 2017
@bradfitz
Copy link
Contributor

bradfitz commented Jan 4, 2017

/cc @x1ddos

@titanous titanous added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 7, 2017
@x1ddos
Copy link

x1ddos commented Feb 8, 2017

Working on it now, on all issues (collecting, better err, etc.)

There could be an issue with collecting nonces from previous responses as Let's Encrypt keeps only a limited num. of issued nonces: https://github.com/letsencrypt/boulder/blob/affa0e92cd1bae31dcdfb6ae41c13a8b82e499b7/nonce/nonce.go#L18, so I'll make it a buffer with at most N collected nonces.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/36514 mentions this issue.

@golang golang locked and limited conversation to collaborators Feb 9, 2018
@bradfitz bradfitz added the Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) label May 17, 2018
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
acme: reduce the number of network round trips
e9db3c3 
Before this change, every JWS-signed request was preceded
by a HEAD request to fetch a fresh nonce.

The Client is now able to collect nonce values
from server responses and use them for future requests.
Additionally, this change also makes sure the client propagates
any error encountered during a fresh nonce fetch.

Fixes golang/go#18428.

Change-Id: I33d21b450351cf4d98e72ee6c8fa654e9554bf92
Reviewed-on: https://go-review.googlesource.com/36514
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
acme: reduce the number of network round trips
c947424 
Before this change, every JWS-signed request was preceded
by a HEAD request to fetch a fresh nonce.

The Client is now able to collect nonce values
from server responses and use them for future requests.
Additionally, this change also makes sure the client propagates
any error encountered during a fresh nonce fetch.

Fixes golang/go#18428.

Change-Id: I33d21b450351cf4d98e72ee6c8fa654e9554bf92
Reviewed-on: https://go-review.googlesource.com/36514
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@rsc rsc unassigned x1ddos Jun 23, 2022
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
acme: reduce the number of network round trips
c42b335 
Before this change, every JWS-signed request was preceded
by a HEAD request to fetch a fresh nonce.

The Client is now able to collect nonce values
from server responses and use them for future requests.
Additionally, this change also makes sure the client propagates
any error encountered during a fresh nonce fetch.

Fixes golang/go#18428.

Change-Id: I33d21b450351cf4d98e72ee6c8fa654e9554bf92
Reviewed-on: https://go-review.googlesource.com/36514
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
acme: reduce the number of network round trips
74b66ea 
Before this change, every JWS-signed request was preceded
by a HEAD request to fetch a fresh nonce.

The Client is now able to collect nonce values
from server responses and use them for future requests.
Additionally, this change also makes sure the client propagates
any error encountered during a fresh nonce fetch.

Fixes golang/go#18428.

Change-Id: I33d21b450351cf4d98e72ee6c8fa654e9554bf92
Reviewed-on: https://go-review.googlesource.com/36514
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
desdeel2d0m added a commit to desdeel2d0m/crypto that referenced this issue Jul 1, 2024
@desdeel2d0m
acme: reduce the number of network round trips
de9180e 
Before this change, every JWS-signed request was preceded
by a HEAD request to fetch a fresh nonce.

The Client is now able to collect nonce values
from server responses and use them for future requests.
Additionally, this change also makes sure the client propagates
any error encountered during a fresh nonce fetch.

Fixes golang/go#18428.

Change-Id: I33d21b450351cf4d98e72ee6c8fa654e9554bf92
Reviewed-on: https://go-review.googlesource.com/36514
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@jmhodges @bradfitz @titanous @x1ddos @rsc @gopherbot