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.

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

Proposal: Custom rate limiting rules #1232

Open
MarvinJWendt opened this issue Dec 11, 2022 · 12 comments
Open

Proposal: Custom rate limiting rules #1232

MarvinJWendt opened this issue Dec 11, 2022 · 12 comments

Comments

@MarvinJWendt
Copy link
Contributor

MarvinJWendt commented Dec 11, 2022

Preliminary: I know that rate limiting has been proposed before (in #387), but due to the lack of a description, I would like to submit a justified proposal herewith.

Description

I propose the option to implement basic rate-limiting to specific API endpoints. Most, if not all apps, would benefit from rate-limiting user and record creation. For this proposal, I want to create an imaginary app that is like Instagram.

Attack vector

Servers / apps will, sooner or later, be attacked.

  1. A common attack vector is to spam the creation of posts (like pictures on our imaginary Instagram clone). At the current state, PocketBase (in its bare form) would allow the creation of thousands of posts per second. This is obviously nothing that we would want in our app.
  2. Another attack vector is dumping the whole database. An attacker could query all posts and increase the "page" in a loop. This would cause intense load on the servers memory and the attacker would have every post locally (which might be millions on big apps).
  3. The probably most dangerous attack would be to create thousands of users. Those are hard to delete, as it's not tracked which IP created the user. A simple script is currently capable to create users in a loop. Such an attack could hardly be prevented in PocketBases current form.

Current solution

The current solution requires the use of a reverse-proxy. This would be feasible, but the programmer would have to create individual rules for each route (which are currently not marked as stable, so the rules might need to be updated frequently). With larger apps this is hardly possible to keep synchronous and would require the usage of lots of wildcards (posts creation might have a different rate-limit than post-view for example). Also, some people do not have a reverse-proxy or are not capable to set one up.

Proposed solution

I propose that an option is added to each collection, how rate-limiting should be handled. This option could be embedded in the existing API rules. A simple implementation could look like this:

Note
Text in square brackets could be an input field as dropdown or raw input

Allow [user/IP/IP||user] [number] [creations/updates/views/lists] per [number] [second/minute/hour] 

Note
The limiting metric IP||User should count request on the same IP or the same user, to prevent users to use proxy lists.
When anonymous users have access to the collection endpoint, it should fallback to IP||session_id

As a quick mockup, this could look like:
image
image

Maybe this could be integrated in the API Rule syntax somehow, but I'll leave that up to you :)

This could also be extended, so that unauthorized users, or users which fit a requirement (like a role), have different rate-limits.


Please let me know what you think, in my opinion this would be a very useful security feature!

@MarvinJWendt MarvinJWendt changed the title Proposal: Basic rate limiting Proposal: Custom rate limiting rules Dec 11, 2022
@ganigeorgiev
Copy link
Member

ganigeorgiev commented Dec 12, 2022

While I agree that this would be a nice to have feature, for now it remains "unplanned" because there are way too many other issues with higher priority that represents a blocker for some use cases (multi-match and @request.data.* rel filter enhancements, private/protected files, single and combined field indexes, raw sql or collection views for aggregations, etc.).

I'm not even sure that I'll accept a PR if someone decide to work on this. I want to keep PocketBase as simple as possible.
For the past month I'm lucky enough to be able to work full-time on PocketBase, but I'm well aware that this will not last forever and I have to think a little more on the eventual maintenance burden that will come with every new feature (not to mention that I'm already slightly behind the original planned schedule with my other open source project - Presentator #183).

Maybe the better approach in the long run will be to prioritize the Admin UI extendability (#898) to allow users to create plugins that eventually may get integrated/embedded in the core.

I'll try to reorganize and update the roadmap after the upcoming few releases.

@MarvinJWendt
Copy link
Contributor Author

MarvinJWendt commented Dec 12, 2022

for now it remains "unplanned" because there are way too many other issues with higher priority

Sure, I was just posting this so that we can track possible future development here.

I want to keep PocketBase as simple as possible.

At this point, I would even say that inbuilt rate-limiting would keep PocketBase more simple. Running production apps will always require to rate limit the creation of users and stuff like posts. Which means, that every PocketBase instance has to run behind a reverse proxy with a lot of custom rules to different API endpoints, to be production ready. This makes a PocketBase deployment much harder, than just deploying the single binary on a rather cheap VPS / Droplet.

For the past month I'm lucky enough to be able to work full-time on PocketBase, but I'm well aware that this will not last forever

I hope the funding will increase! Really love the project and your commitment. You totally deserve it!

Maybe the better approach in the long run will be to prioritize the Admin UI extendability (#898) to allow users to create plugins that eventually may get integrated/embedded in the core.

I am very excited for this! But I think rate-limiting might be too major, to be completely written by community plugins.

and I have to think a little more on the eventual maintenance burden that will come with every new feature

I am not sure how much maintenance this would take. If I am not mistaken, it can be split up into:

  1. UI
  2. Saving the settings (possibly in the already existing API rules?)
  3. Dynamically adding the appropriate echo middleware to the route.

As the rate-limiting middleware comes with echo directly, I think the maintenance on the rate-limiting itself would be small? The biggest part would probably be adding the middleware dynamically and saving it?

@MarvinJWendt
Copy link
Contributor Author

MarvinJWendt commented Dec 12, 2022

A far-fetched idea would be to make it possible to add middlewares in EventHooks (if that's possible to implement).

A rate limiting route could then be implemented like this, when PocketBase is used as a framework:

userCreateLimiter := middleware.RateLimiter(middleware.NewRateLimiterMemoryStore(100))
app.OnRecordBeforeCreateRequest().Add(func(e *core.RecordCreateEvent) error {
	if e.Record.Collection().Name == "users" {
		return e.Use(userCreateLimiter)
	}

	// ...

	return nil
})

@ganigeorgiev
Copy link
Member

ganigeorgiev commented Dec 12, 2022

If you want to use the default echo rate-limiter middleware you can register it to the e.Router instance in the OnBeforeServe event hook:

app.OnBeforeServe().Add(func(e *core.ServeEvent) error {
	e.Router.Use(middleware.RateLimiterWithConfig(yourConfig))

	return nil
})

You can specify to which routes to apply by using the Skipper field of the middleware config.

We can't register middlewares inside other event hooks because they are triggered more than once. Additionally the echo Use and Pre middleware registration functions are not concurrent safe and may cause some issues if a middleware is registered after the server start (I haven't really tested it).

In any case, as mentioned previously for me this personally is a very low priority.
I understand the eventual benefits of it but most users in normal situations don't really need it and in some cases using an in-memory rate-limiter may actually cause more harm than good since it will increase the load on the server and I don't want to rush the implementation.

@MarvinJWendt
Copy link
Contributor Author

MarvinJWendt commented Dec 12, 2022

I made my own custom middleware, and I think that should be enough for now. If anyone is interested, here is the implementation:

Middleware

// RateLimitCollectionOperations is a middleware that rate limits HTTP requests on collection operations.
// Possible operations:
// - list
// - view
// - create
// - update
// - delete
func RateLimitCollectionOperations(collection string, operations []string, limiter echo.MiddlewareFunc) echo.MiddlewareFunc {
	return func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			contains := func(s string, list []string) bool {
				for _, v := range list {
					if v == s {
						return true
					}
				}
				return false
			}

			collectionEndpoint := fmt.Sprintf("/api/collections/%s/records", collection)

			if contains("list", operations) && c.Request().URL.Path == collectionEndpoint && c.Request().Method == http.MethodGet {
				return limiter(next)(c)
			} else if contains("view", operations) && strings.HasPrefix(c.Request().URL.Path, collectionEndpoint) && c.Request().Method == http.MethodGet {
				return limiter(next)(c)
			} else if contains("create", operations) && strings.HasPrefix(c.Request().URL.Path, collectionEndpoint) && c.Request().Method == http.MethodPost {
				return limiter(next)(c)
			} else if contains("update", operations) && strings.HasPrefix(c.Request().URL.Path, collectionEndpoint) && c.Request().Method == http.MethodPatch {
				return limiter(next)(c)
			} else if contains("delete", operations) && strings.HasPrefix(c.Request().URL.Path, collectionEndpoint) && c.Request().Method == http.MethodDelete {
				return limiter(next)(c)
			}

			return next(c)
		}
	}
}

Usage

func main() {
	app := pocketbase.New()

	limiter := middleware.RateLimiter(middleware.NewRateLimiterMemoryStore(2))
	app.OnBeforeServe().Add(func(e *core.ServeEvent) error {
		// Rate limit list and view operations on the "posts" collection
		e.Router.Use(RateLimitCollectionOperations("posts", []string{"list", "view"}, limiter))
		return nil
	})

	app.Start()
}

Warning
I have not tested every case, but for now it seems to work fine. I'll update the solution when I find a bug.
Also, the code could be refactored, but I wanted to keep it a single function for sharing. Feel free to extract the contains method, etc...

@greendesertsnow
Copy link

We need a login abuse limiter as well

@MarvinJWendt
Copy link
Contributor Author

MarvinJWendt commented Dec 21, 2022

We need a login abuse limiter as well

I think the most common approach for login and registration abuse prevention is implementing a captcha. There are ones that can be configured to only prompt the user, if suspicious activities are detected. Even with basic rate limiting, logins could still be brute forced with proxy lists.

For basic rate limiting registrations, you can use the middleware I posted earlier (with a create action on the users collection). With some adaptions, this could also be used for login rate limiting, but like I said, this is usually not enough for those "critical public endpoints" and I highly recommend a captcha after a certain amount of failed logins.

@ninakylle02

This comment was marked as spam.

@GewoonJaap
Copy link

GewoonJaap commented Dec 24, 2022

This would be a great addition to PocketBase! Max requests per ip/user per action would be amazing!

@adam-ah
Copy link

adam-ah commented May 26, 2023

While this feature is needed in production, I'm not sure it belongs to Pocketbase itself.
A reverse proxy, such as NGINX, is better suited for geo / rate limiting.

https://www.nginx.com/blog/rate-limiting-nginx/
https://gist.github.com/ipmb/472da2a9071dd87e24d3

@MarvinJWendt
Copy link
Contributor Author

@adam-ah Many highly scalable applications run behind a reverse proxy, of course. But PocketBase is an all-in-one solution. Therefore I think it should not be necessary to put a reverse-proxy in between. Also, some routes need a different rate-limit. When setting up a reverse-proxy you would have to know each API route and limit it individually, which is quite a time-consuming task. Especially when apps get new features, you would have to evaluate each time if the reverse-proxy config needs to be adjusted.

@pchasco
Copy link

pchasco commented Sep 28, 2023

I think the right question to ask is "Is a rate limiter needed?" If the answer is "yes," then in the spirit of PocketBase's tagline, "Open Source backend for your next SaaS and Mobile app in 1 file," then rate limiting should be included in PocketBase.

Personally, I think rate limiting is an important feature, as all services exposed to the internet of any interest at all will suffer some attack. It is certainly as important as some of the nice-to-have features already slated to be implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants