search

google / certificate-transparency-go

Auditing for TLS certificates (Go code)
https://certificate.transparency.dev
Apache License 2.0
903 stars 232 forks source link

Return 429 Too Many Requests for gRPC error code `ResourceExhausted` from Trillian #1401

Closed roger2hk closed 6 months ago

roger2hk commented 7 months ago

The existing HTTP status code StatusForbidden does not reflect the actual gRPC error code ResourceExhausted from Trillian. 429 Too Many Requests is a better HTTP status code to fit into the translation.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/429

Checklist

roger2hk commented 7 months ago

There is no issue reported. I came across this when I looked at Gunnar A's question in Slack. We merged a similar PR (https://github.com/google/certificate-transparency-go/pull/1313) a while ago, so that is why I find a better CTFE HTTP status code for the rate limit error response from Trillian.

mhutchinson commented 7 months ago

@AlCutter WDYT? The reason I'm hesitant with this is that we've had a lot of problems with rogue client code not backing off when we tell them to. It's been a while since we saw an influx of such bad behaviour. If we change this, it's possible that some of the clients that are currently behaving will start to misbehave.

OTOH, if we returned better status codes then it's more likely that clients will do the right thing.

JulyMoon87 commented 7 months ago

Perhaps find the reason for massive misbehavior first

On Wed, Mar 20, 2024, 9:57 AM Martin Hutchinson @.***> wrote:

@AlCutter https://github.com/AlCutter WDYT? The reason I'm hesitant with this is that we've had a lot of problems with rogue client code not backing off when we tell them to. It's been a while since we saw an influx of such bad behaviour. If we change this, it's possible that some of the clients that are currently behaving will start to misbehave.

OTOH, if we returned better status codes then it's more likely that clients will do the right thing.

— Reply to this email directly, view it on GitHub https://github.com/google/certificate-transparency-go/pull/1401#issuecomment-2009773877, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASY4UXI7OCR62MMH2PLGX3LYZGPWZAVCNFSM6AAAAABE7Q6OV6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMBZG43TGOBXG4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

AlCutter commented 7 months ago

Returning 429 is much more descriptive so ultimately seems like the right thing to do.

If we're worried about breaking clients with the change, perhaps we can pop this behaviour behind a flag, and try an A/B experiment for a bit to get a sense of how it'll affect traffic before deciding whether to make it default-on and remove the flag?

roger2hk commented 6 months ago

According to this thread in ct-policy forum, the error code 429 Too Many Requests is already returned from Cloudflare and DigiCert logs. I believe monitors should have already considered the 429 Too Many Requests in the implementations.

roger2hk commented 6 months ago

According to this thread in ct-policy forum, the error code 429 Too Many Requests is already returned from Cloudflare and DigiCert logs. I believe monitors should have already considered the 429 Too Many Requests in the implementations.

@mhutchinson WDYT?

mhutchinson commented 6 months ago

According to this thread in ct-policy forum, the error code 429 Too Many Requests is already returned from Cloudflare and DigiCert logs. I believe monitors should have already considered the 429 Too Many Requests in the implementations.

@mhutchinson WDYT?

Sure, let's take the chance.