Consider not forcing www canonicalization for API requests, due to endless stream of errant API requests now happening #1139
Comments
|
@peeringdb/pc should be hotfixed IMO |
|
+1 in principle, I got bitten by that issue (curl not following the redirect) but I don't know if that'd break anything wrt auth indeed |
@vegu has indicated off GitHub that changing the ordering of API throttling to be ahead of canonicalization is likely a non-trivial change. He also indicated that changing the canonicalization to only apply to non-API requests does not appear problematic from a coding standpoint. I recommend this change - don't canonicalize API requests - ASAP move forward unless there is an objection to be considered. |
|
@peeringdb/pc @ccaputo is this something that needs a point release or should we schedule it for the next schedule release? |
|
To play devil's advocate, since those aren't working now and not having Eventually someone will notice and correct their script. :) |
|
I like this.
…On Mon, Apr 4, 2022 at 11:47 AM Matt Griswold ***@***.***> wrote:
To play devil's advocate, since those aren't working now and not having
www. will break caching, wouldn't it make more sense to just drop those
requests after the first the redirect?
Eventually someone will notice and correct their script. :)
—
Reply to this email directly, view it on GitHub
<#1139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFA2YQXE3GNJMIK2XLIYV4LVDMFIPANCNFSM5SCQ64VA>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
|
@grizz: I'm intrigued. By drop do you mean no response whatsoever, just close TCP, or a 4xx response or ??? Would you maintain state in some manner, like for a given IP having already been told to redirect in the last 5 mins, if they ignore just drop?
@leovegoda: Not sure. Let's see where the above goes before figuring that out. |
I was originally thinking just close TCP, but we could probably return a 4XX as well, as long as it's caught early in the pipeline (which it would be on the redirect handler).
Yes, that's what I was thinking. Give a redirect, any other requests from the same IP for X time period will just get ignored. |
TBD how nginx would handle the closed TCP request, but something that slows the requests like that could, would be nice. I wonder if there is a hack possible (or already in existence) to signal nginx to be similarly harsh with the client.
I love the idea. Let's see if anyone else has thoughts on this. |
I'm in favour of this approach! If we do not enforce the change to www for all requests and "just" implement workarounds. We are no closer to the goal than |
|
@peeringdb/pc can we make sure @grizz's solution goes into the next release? The endless 301's continue unabated. |
|
Can we reach out to the users of these requests (if they're using user or org api keys) and ask them to switch? |
This isn't really practical. We don't have auth-id logging enabled on production to know, but I highly doubt these are using api keys. In the last log rotation day (ie. a full day, now gzipped) there were over 1,900 unique source IP addresses making API queries and being redirected. Of these, 195 different source IP addresses made the same query 100 times or more. Of that, 116 different source IP addresses made the same query 1,000 times or more. |
|
A quick check indicates about 30% of the HTTP requests to the API servers result in 301 redirects. It would be good to make progress on this. |
|
I learned today that this problem triggered a bug in one user's code that resulted in PeeringDB getting far more requests than previous, from the user. It would be good to make progress on this. |
|
Actually, I think it would make more sense to do this in AWS or a thin responder that's not tied or touching PeeringDB at all. With the added advantage that we don't need to wait for a release. Since they're spamming and not following the redirects, instead of closing the connection, it would be nice to send a redirect and then tarpit the connection. Thoughts? |
|
Potentially interesting re AWS. Following up with you via email. |
|
I think we need to revisit this. The forcing of canonical for /api/ requests continues to waste developer time (#1206). There is an apparent bug in the python requests module in which the followup query after a 301 redirect, fails to include (or mangles?) the request headers necessary for authentication. Thus the PeeringDB server responds as if it is an unauthenticated request, while the developer thinks they are authenticating. Chaos ensues. Totally opaque. |
|
+1 @grizz what are our options here? |
Turns out this isn't a bug in the python requests module (code here: https://github.com/psf/requests/blob/177dd90f/requests/sessions.py#L284-L296), but rather "by-design" to prevent leakage of credentials in the face of a malicious 301 redirect. Other software packages may perform this same safety credential dropping upon redirect. BTW, another PeeringDB client developer hit this problem today. It is opaque because they get an HTTP 200 and some useful data after the redirect, without realizing it is unauthenticated and thus possibly missing User-only fields. |
arnoldnipper
modified the milestones:
1 Decide,
2 Consensus Reached,
3a Needs Implementation discussion
|
So, I hit this issue today (see #1220 for a bit more color) The symptom was that the API worked via the redirect, but the auth header was stripped off for my protection via python-requests as @ccaputo describes. This had the effect of the api appearing to work, but note this would now be an anonymous query subjecting my client to the lower rate-limit now in effect. I would have preferred either that there was no redirect for API access, or a hard 4xx error forcing me to fix my client code. |
|
What would keep it out?
…On Sat, Apr 16, 2022 at 13:46 Chris Caputo ***@***.***> wrote:
@peeringdb/pc <https://github.com/orgs/peeringdb/teams/pc> can we make
sure @grizz <https://github.com/grizz>'s solution goes into the next
release? The endless 301's continue unabated.
—
Reply to this email directly, view it on GitHub
<#1139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFA2YQTBAAYETC34KMZHJODVFL4HXANCNFSM5SCQ64VA>
.
You are receiving this because you are on a team that was mentioned.Message
ID: ***@***.***>
|
#916 implemented www.peeringdb.com canonicalization.
While I understand there are benefits for browser-originated non-API requests, I am not sure there is a benefit to forcing this canonicalization on API requests, and yet a major downside has come up. We now have a continuous stream of API requests in which the client is ignoring the 301 redirect. These are burning CPU and log space, in addition to obscuring other issues when observing logs, since they outnumber everything else. A sample from one server instance, with first three octets of IPv4 addresses obscured:
Short of null routing or ACLing the requests, I don't know how stop them or contact them (without substantial work), and so I fear we may have years of this happening.
We may want to consider not forcing this canonicalization of API requests, if it doesn't break anything to do so, and helps restore clients to functionality.
Current and upcoming API throttling tools (ex #1126) may not help this issue, depending on whether the canonicalization happens before or after API throttling. If nothing else, we may need to have canonicalization after API throttling.
The text was updated successfully, but these errors were encountered: