DuendeSoftware / IdentityServer

The most flexible and standards-compliant OpenID Connect and OAuth 2.x framework for ASP.NET Core
https://duendesoftware.com/products/identityserver
Other
1.49k stars 344 forks source link

Consider adding support for per client rate limiting #1591

Open AndersAbel opened 2 months ago

AndersAbel commented 2 months ago

We currently have callbacks on the authorize and token endpoint validators that can be used to implement custom validation logic. But for a rate limiting scenario these runs too late - when they are called we've already done a lot of expensive storage calls.

Consider adding a new rate limiting feature that is shared across all endpoints. The rate limiting should include a context that includes source IP address (or maybe the full request object?) and extracted parameters - at least clientId and possibly a principal or the sub. Each endpoint would call the rate limiting as soon as the client id has been read from the request.

josephdecock commented 2 months ago

Custom validation gets a sophisticated object model and the values in that model can be trusted. The rate limiting would have to run earlier and so wouldn't have that.

Instead, we would just give the raw parameters as inputs. But what would be gained by that? Would the abstraction be nicer than doing this with asp.net core middleware? I guess I see the benefit of not needing the custom middleware to look at routes.

brockallen commented 2 months ago

Perhaps a general purpose pre-endpoint hook? IOW, we add something that gets called and knows which endpoint before the actual EP is invoked. This would allow IdentityServer endpoints to be pre-processed without the customer having to write general middleware that checks specific paths with the knowledge of IS EP paths?

brockallen commented 2 months ago

In the past when this type of thing has been requested, we suggested using a decorator pattern on our ITokenRequestValidator to pre-process the requests. Perhaps this is the better angle on this issue -- IOW, make pre-processing these couple of validators easier?

AndersAbel commented 1 month ago

@josephdecock The benefit of handing over the raw (unvalidated) parameters is that they could reuse our existing code to extract the parameters. Especially for a form body it is a performance penalty to read and parse the body in a custom middleware, just to let our middleware do it right after.

@brockallen Yes, some kind of pre-endpoint hook - but with access to raw parameters. And also the client IP address.

brockallen commented 1 month ago

And also the client IP address.

Inject IHttpRequestAccessor?

runegri commented 1 month ago

If you implement this, it would be very useful to support other parameters than just the ClientID. So a way to inspect the request would be nice.