-
Notifications
You must be signed in to change notification settings - Fork 368
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
server: rate limiter v1 #1708
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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, | ||
Comment on lines
+10
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a env var to entirely enable/disable ratelimit? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. any reason for not using try/catch await? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no other reason than using the same style as the |
||
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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes with a fallback to |
||
return `account-${getAccount(res)}`; | ||
} catch (e) { | ||
if (req.user) { | ||
return `user-${req.user.id}`; | ||
} | ||
return `ip-${req.ip}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @khaliqgant why were we using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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({ | ||
|
@@ -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)); | ||
|
||
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)); | ||
|
||
} | ||
|
||
// Webapp routes (session auth). | ||
|
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 other word, an attacker/bad script would only be block for a few seconds now.
There was a problem hiding this comment.
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