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

server: rate limiter v1 #1708

Merged
merged 1 commit into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 9 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

70 changes: 70 additions & 0 deletions packages/server/lib/controllers/ratelimit.middleware.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
import type { Request, Response, NextFunction } from 'express';
import { createClient } from 'redis';
import { RateLimiterRedis, RateLimiterMemory, RateLimiterRes } from 'rate-limiter-flexible';
import { getAccount, getRedisUrl } from '@nangohq/shared';
import { logger } from '@nangohq/shared';

const rateLimiter = await (async () => {
const opts = {
keyPrefix: 'middleware',
points: 1200,
duration: 60,
Copy link
Collaborator Author

@TBonnin TBonnin Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I picked a pretty high global limit (20rps). The goal is for now to prevent an "infinite" number of requests to overload the server when users misbehave like it happened with gretel last month.
We can tweak the logic as much as we want later. Let me know what you think

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to change anything, just thinking out loud.
when I set a ratelimiter I always try to put a large window because it allows a few burst but not a constant one, so you don't end up locking the system constantly.

  • in a 60s window you can burst 1200 http call every 60s
  • in a 600s window you can burst once every 600s, but if you reach the limit sooner you are blocked for a long period of time

In other word, an attacker/bad script would only be block for a few seconds now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the right limit is tricky indeed. A larger window with the same limit also means the avg rate per seconds is lower.
I am sure we will be tweaking the logic to handle more scenarii in the future but for now I would sleep better at night knowing that beyond a certain threshold, request will be discarded quickly and not hit the rest of the system, especially the db

Comment on lines +10 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more actionable comment: can you make them env var so we don't need to deploy in case of urgent need?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. I will do that when I actually enable 429s. Now it is just logging

blockDuration: 0
};
const url = getRedisUrl();
if (url) {
const redisClient = await createClient({ url: url, disableOfflineQueue: true }).connect();
redisClient.on('error', (err) => {
logger.error(`Redis (rate-limiter) error: ${err}`);
});
return new RateLimiterRedis({
storeClient: redisClient,
...opts
});
}
return new RateLimiterMemory(opts);
})();

export const rateLimiterMiddleware = (req: Request, res: Response, next: NextFunction) => {
const setXRateLimitHeaders = (rateLimiterRes: RateLimiterRes) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a env var to entirely enable/disable ratelimit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will also do that when actually returning 429s

const resetEpoch = Math.floor(new Date(Date.now() + rateLimiterRes.msBeforeNext).getTime() / 1000);
res.setHeader('X-RateLimit-Limit', rateLimiter.points);
res.setHeader('X-RateLimit-Remaining', rateLimiterRes.remainingPoints);
res.setHeader('X-RateLimit-Reset', resetEpoch);
};
const key = getKey(req, res);
const pointsToConsume = getPointsToConsume(req);
rateLimiter
.consume(key, pointsToConsume)
.then((rateLimiterRes) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason for not using try/catch await?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no other reason than using the same style as the rate-limit-flexible documentation. Also found it easier to read

setXRateLimitHeaders(rateLimiterRes);
next();
})
.catch((rateLimiterRes) => {
res.setHeader('Retry-After', Math.floor(rateLimiterRes.msBeforeNext / 1000));
setXRateLimitHeaders(rateLimiterRes);
logger.info(`Rate limit exceeded for ${key}. Request: ${req.method} ${req.path})`);
next();
// TODO:
// res.status(429).send('Too Many Requests');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a first step, we are just logging and not sending the 429. Just to make sure there is no obvious issue. I will let it run a few hours like this before to start sending 429

});
};

function getKey(req: Request, res: Response): string {
try {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clarification question: for api usage the key will be the account and for web it’ll be the user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes with a fallback to IP for non-authenticated endpoints

return `account-${getAccount(res)}`;
} catch (e) {
if (req.user) {
return `user-${req.user.id}`;
}
return `ip-${req.ip}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if proxy are handled with this. I remember that old version of express would not display the right IP if you had proxy (always happen in cloud env)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure. That's the main reason why I prefer to first just log and not return 429 so I can check with real requests

}
}

function getPointsToConsume(req: Request): number {
if (['/api/v1/signin', '/api/v1/signup', '/api/v1/forgot-password', '/api/v1/reset-password'].includes(req.path)) {
// limiting to 6 requests per period to avoid brute force attacks
return rateLimiter.points / 6;
}
return 1;
}
24 changes: 13 additions & 11 deletions packages/server/lib/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import apiAuthController from './controllers/apiAuth.controller.js';
import appAuthController from './controllers/appAuth.controller.js';
import onboardingController from './controllers/onboarding.controller.js';
import webhookController from './controllers/webhook.controller.js';
import { rateLimiterMiddleware } from './controllers/ratelimit.middleware.js';
import path from 'path';
import { dirname } from './utils/utils.js';
import { WebSocketServer, WebSocket } from 'ws';
Expand Down Expand Up @@ -52,14 +53,15 @@ const app = express();

// Auth
AuthClient.setup(app);
const apiAuth = authMiddleware.secretKeyAuth.bind(authMiddleware);
const apiPublicAuth = authMiddleware.publicKeyAuth.bind(authMiddleware);

const apiAuth = [authMiddleware.secretKeyAuth, rateLimiterMiddleware];
const apiPublicAuth = [authMiddleware.publicKeyAuth, rateLimiterMiddleware];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khaliqgant why were we using bind before?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Legacy, I'm actually not sure why and not sure if it is needed.

const webAuth =
isCloud() || isEnterprise()
? [passport.authenticate('session'), authMiddleware.sessionAuth.bind(authMiddleware)]
? [passport.authenticate('session'), authMiddleware.sessionAuth, rateLimiterMiddleware]
: isBasicAuthEnabled()
? [passport.authenticate('basic', { session: false }), authMiddleware.basicAuth.bind(authMiddleware)]
: [authMiddleware.noAuth.bind(authMiddleware)];
? [passport.authenticate('basic', { session: false }), authMiddleware.basicAuth, rateLimiterMiddleware]
: [authMiddleware.noAuth, rateLimiterMiddleware];

app.use(
express.json({
Expand Down Expand Up @@ -139,12 +141,12 @@ app.route('/proxy/*').all(apiAuth, upload.any(), proxyController.routeCall.bind(

// Webapp routes (no auth).
if (isCloud() || isEnterprise()) {
app.route('/api/v1/signup').post(authController.signup.bind(authController));
app.route('/api/v1/signup/invite').get(authController.invitation.bind(authController));
app.route('/api/v1/logout').post(authController.logout.bind(authController));
app.route('/api/v1/signin').post(passport.authenticate('local'), authController.signin.bind(authController));
app.route('/api/v1/forgot-password').put(authController.forgotPassword.bind(authController));
app.route('/api/v1/reset-password').put(authController.resetPassword.bind(authController));
app.route('/api/v1/signup').post(rateLimiterMiddleware, authController.signup.bind(authController));
Dismissed Show dismissed Hide dismissed
app.route('/api/v1/signup/invite').get(rateLimiterMiddleware, authController.invitation.bind(authController));
app.route('/api/v1/logout').post(rateLimiterMiddleware, authController.logout.bind(authController));
app.route('/api/v1/signin').post(rateLimiterMiddleware, passport.authenticate('local'), authController.signin.bind(authController));
app.route('/api/v1/forgot-password').put(rateLimiterMiddleware, authController.forgotPassword.bind(authController));
app.route('/api/v1/reset-password').put(rateLimiterMiddleware, authController.resetPassword.bind(authController));
Dismissed Show dismissed Hide dismissed
}

// Webapp routes (session auth).
Expand Down
2 changes: 2 additions & 0 deletions packages/server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@
"passport-local": "^1.0.0",
"pg": "^8.8.0",
"posthog-node": "^3.1.3",
"rate-limiter-flexible": "^5.0.0",
"redis": "^4.6.11",
"simple-oauth2": "^5.0.0",
"uuid": "^9.0.0",
"winston": "^3.8.2",
Expand Down