markhuot / craftql

A drop-in GraphQL server for Craft CMS
Other
320 stars 53 forks source link

FR: rate limiting #112

Open timkelty opened 6 years ago

timkelty commented 6 years ago

Would be great if we could tie our tokens/permissions to rate limiting as well (n queries/second, etc)

markhuot commented 6 years ago

So that some user groups could access more often and others less often? Hrm… right now rate limiting isn't exposed to the UI, would you be okay with this per-group limiting being done in the PHP config as well?

timkelty commented 6 years ago

Totally fine if this were a PHP-only config setting (but I'll outline below how I'm thinking it would be added to CP)

Wouldn't it make more sense to tie this to tokens, rather than user group permissions, though?

FWIW, here's my very specific, but perhaps not uncommon real-world scenario:

How I'm thinking about it, in the Token Scopes settings page for a given token, you could specify your limits there. Roughly, something like:

screen_shot_2018-06-27_at_11_26_45_pm

I suppose it may be desirable to have different limits for queries/mutations, but all I'm looking for right now is just straight up request limiting.

markhuot commented 6 years ago

Yup, this makes perfect sense. FYI, on dev-master there's a feature that hasn't made it in to the docs yet that allows you to limit queries by depth and by complexity: https://github.com/markhuot/craftql/commit/306778448e9d5ab1984512b5e9295c336b336314.

Some details on the numbers are here: http://webonyx.github.io/graphql-php/security/

I don't know that this solves your immediate issue but it may help in the short term.

timkelty commented 6 years ago

nice! I'll check it out

markhuot commented 6 years ago

I'm working on adding this in to the Token "Settings" page. I have query depth and complexity in there relatively easily.

I'm struggling with query rate limiting though because right now I'm not logging any queries. In an effort to keep things fast I don't really want to log queries for each request (which I'd have to do in order to count against a quota).

Do you feel that depth and complexity is enough of a limit or is rate a necessary limit?

--

If you wanted to test this out, it's on the dev-security branch.

timkelty commented 6 years ago

@markhuot In my case, It's really the quantity that is the problem, so I'm not sure if depth/complexity would help…but I wouldn't want to bog down everything just to support this.

I wonder if this could be a good use-case for a JWT body? Going to talk through this…might not make sense :):

So – rather than logging queries, you could send something likeremainingRequests in the JWT body. When that is sent back to CraftQL in subsequent queries, CraftQL can decrement the value, and pass along the updated JWT with the response.

markhuot commented 6 years ago

In my case, It's really the quantity that is the problem, so I'm not sure if depth/complexity would help…but I wouldn't want to bog down everything just to support this.

That makes sense and I have a feeling you're not the only one. Rate limiting seems like the easiest/safest way to limit access for a lot of people.

I wonder if this could be a good use-case for a JWT body? Going to talk through this…might not make sense :):

I like it and I thought of it too, my only concern is that then you'd need to send the updated token each time, otherwise you could keep sending the 100 queries left token and never decrement. Let me think on this a bit more though because I like the idea of it, just need to figure out the right implementation.

timkelty commented 6 years ago

Coincidentally, Phil Sturgeon posted this recently, which I haven't read but is on my list: https://blog.apisyouwonthate.com/what-is-api-rate-limiting-all-about-1819a390ab06

markhuot commented 6 years ago

Yea, I may just bite the bullet and enable logging per token. There are a bunch of cool side effects of that decision too, like being able to show a log of queries in the CP, analyze what queries took the longest/shortest, what user sent the most queries, etc…

timkelty commented 6 years ago

@markhuot That's true, you could always make it optional if someone was really trying to tweak perf and didn't need those features.

Also some interesting stuff here: https://www.nginx.com/blog/authenticating-api-clients-jwt-nginx-plus/ "Leveraging JWT Claims for Logging and Rate Limiting"

andreas83 commented 4 years ago

one and a half year late someone on the internet (me) still thinking about what a wonderful feature this might be