From 0277e9e895e239d603fb1435d31514b574102c49 Mon Sep 17 00:00:00 2001 From: Brent Van Geertruy Date: Mon, 23 Apr 2018 17:40:09 +0200 Subject: [PATCH] SIAP-101: Add bruteforce protection on login --- .env.example | 3 + .travis.yml | 1 + docker-compose.yml | 5 ++ package.json | 5 +- src/app.ts | 9 +-- src/config/errors.config.ts | 3 + src/config/security.config.ts | 63 +++++++++++++++++++ src/controllers/auth.controller.ts | 16 +++-- src/lib/memory-store.ts | 17 +++++ src/middleware/bruteforce.middleware.ts | 16 +++++ src/models/request.model.ts | 7 +++ src/routes/v1/auth.routes.ts | 3 + tests/_helpers/mockdata/data.ts | 3 +- tests/_helpers/mockdata/memory-store.data.ts | 14 +++++ tests/integration/auth.route.test.ts | 1 + tests/lib/utils.test.ts | 2 +- .../middleware/bruteforce.middleware.test.ts | 50 +++++++++++++++ tests/test.config.ts | 1 + yarn.lock | 39 +++++++++++- 19 files changed, 240 insertions(+), 18 deletions(-) create mode 100644 src/config/security.config.ts create mode 100644 src/lib/memory-store.ts create mode 100644 src/middleware/bruteforce.middleware.ts create mode 100644 tests/_helpers/mockdata/memory-store.data.ts create mode 100644 tests/middleware/bruteforce.middleware.test.ts diff --git a/.env.example b/.env.example index e262526..df0a84b 100644 --- a/.env.example +++ b/.env.example @@ -5,6 +5,9 @@ DATABASE_URL= INITIAL_SEED_USERNAME= INITIAL_SEED_PASSWORD= +## Memorystore +REDISCLOUD_URL= + ## Mailing MANDRILL_API_KEY= SYSTEM_EMAIL= diff --git a/.travis.yml b/.travis.yml index e6a77c3..274e4fa 100644 --- a/.travis.yml +++ b/.travis.yml @@ -2,6 +2,7 @@ language: node_js services: - postgresql + - redis-server before_script: - psql -c "CREATE DATABASE silverback_test;" -U postgres diff --git a/docker-compose.yml b/docker-compose.yml index 0876ab4..cb4cb30 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,5 +1,10 @@ version: '3.3' services: + redis: + image: redis:latest + container_name: redis_silverback + ports: + - "6379:6379" postgres: image: sameersbn/postgresql:latest container_name: pgsql_silverback diff --git a/package.json b/package.json index a976e23..c505847 100644 --- a/package.json +++ b/package.json @@ -32,7 +32,8 @@ "lodash": "~4.17.5", "mandrill-api": "~1.0.45", "pg": "~7.4.1", - "tree-house": "~3.1.0", + "redis": "~2.8.0", + "tree-house": "~3.2.0", "tree-house-authentication": "~2.0.5", "tree-house-errors": "^1.0.3", "uuid": "~3.2.1", @@ -42,6 +43,7 @@ "@types/cors": "~2.8.3", "@types/dotenv-safe": "~4.0.1", "@types/express": "~4.11.0", + "@types/express-brute": "~0.0.36", "@types/faker": "~4.1.2", "@types/helmet": "~0.0.37", "@types/http-status": "~0.2.30", @@ -50,6 +52,7 @@ "@types/knex": "~0.14.11", "@types/lodash": "~4.14.105", "@types/mandrill-api": "~1.0.30", + "@types/redis": "~2.8.6", "@types/supertest": "~2.0.4", "@types/uuid": "~3.4.3", "@types/winston": "~2.3.9", diff --git a/src/app.ts b/src/app.ts index ce3b1fa..4eea1d2 100644 --- a/src/app.ts +++ b/src/app.ts @@ -6,7 +6,7 @@ import { responder } from './lib/responder'; // Create express instance const app: express.Application = express(); -// Basic security setup +treehouse.setBodyParser(app, '*'); treehouse.setBasicSecurity(app, '*', { cors: { methods: ['GET', 'PUT', 'POST', 'DELETE', 'PATCH'], @@ -14,11 +14,8 @@ treehouse.setBasicSecurity(app, '*', { }, }); -treehouse.setBodyParser(app, '*'); -// treehouse.setRateLimiter(app, '*'); // TODO: Fix proper settings - -// Display all versions -app.get('/', (_req, res) => res.json(appConfig.VERSIONS)); +app.set('trust proxy', 1); // Heroku proxy stuff +app.get('/', (_req, res) => res.json(appConfig.VERSIONS));// Display all versions // Load routes (versioned routes go in the routes/ directory) for (const x in appConfig.VERSIONS) { diff --git a/src/config/errors.config.ts b/src/config/errors.config.ts index 3dc2082..345aefd 100644 --- a/src/config/errors.config.ts +++ b/src/config/errors.config.ts @@ -1,9 +1,12 @@ import { errors as defaults } from 'tree-house-errors'; +// tslint:disable:max-line-length export const errors = Object.assign({}, defaults, { USER_INACTIVE: { code: 'USER_INACTIVE', message: 'Activate user account before login' }, USER_DUPLICATE: { code: 'USER_DUPLICATE', message: 'A user with this email already exists' }, USER_NOT_FOUND: { code: 'USER_NOT_FOUND', message: 'User not found' }, MISSING_HEADERS: { code: 'MISSING_HEADERS', message: 'Not all required headers are provided' }, NO_PERMISSION: { code: 'NO_PERMISSION', message:'You do not have the proper permissions to execute this operation' }, + TOO_MANY_REQUESTS: { code: 'TOO_MANY_REQUESTS', message: 'You\'ve made too many failed attempts in a short period of time, please try again later' }, }); + // tslint:enable:max-line-length diff --git a/src/config/security.config.ts b/src/config/security.config.ts new file mode 100644 index 0000000..66c87a9 --- /dev/null +++ b/src/config/security.config.ts @@ -0,0 +1,63 @@ +import * as httpStatus from 'http-status'; +import { RateLimiterOptions } from 'tree-house'; +import { InternalServerError, ApiError } from 'tree-house-errors'; +import { getRedisClient } from '../lib/memory-store'; +import { logger } from '../lib/logger'; +import { errors } from './errors.config'; + + +/** + * Handle a rejected request due to too many requests for example + */ +const failCallback = (req, _res, next, timeTooWait) => { + logger.info(`User with username ${req.body.username} has tried to login too many times. Reset time: ${new Date(timeTooWait).toISOString()}`); + next(new ApiError(httpStatus.TOO_MANY_REQUESTS, errors.TOO_MANY_REQUESTS)); +}; + + +/** + * Handle a store error that occured with the persistent memory store (Redis) + */ +const handleStoreError = (error) => { + logger.error(error); + throw new InternalServerError(errors.INTERNAL_ERROR, { message: error.message }); +}; + + +/** + * No more than 1000 attempts per day per IP + */ +export const globalBruteConfig: RateLimiterOptions = { + handleStoreError, + failCallback, + freeRetries: 1000, + attachResetToRequest: false, + refreshTimeoutOnRequest: false, + minWait: 25 * 60 * 60 * 1000, // 1 day 1 hour (should never reach this wait time) + maxWait: 25 * 60 * 60 * 1000, // 1 day 1 hour (should never reach this wait time) + lifetime: 24 * 60 * 60, // 1 day (seconds not milliseconds) + redis: process.env.NODE_ENV === 'development' ? undefined : { client: getRedisClient() }, // Use our existing Redis client (in staging/production) +}; + + +/** + * Start slowing requests after 5 failed attempts + */ +export const userBruteConfig: RateLimiterOptions = { + handleStoreError, + failCallback, + freeRetries: 5, + minWait: 5 * 60 * 1000, // 5 minutes + maxWait: 60 * 60 * 1000, // 1 hour, + redis: process.env.NODE_ENV === 'development' ? undefined : { client: getRedisClient() }, // Use our existing Redis client (in staging/production) +}; + + +/** + * Check for same key per request (username) + */ +export const userBruteMiddlewareConfig = { + failCallback, + ignoreIP: false, + key: (req, _res, next) => next(req.body.username), // Call per username per ip +}; diff --git a/src/controllers/auth.controller.ts b/src/controllers/auth.controller.ts index 30788b6..5c79bda 100644 --- a/src/controllers/auth.controller.ts +++ b/src/controllers/auth.controller.ts @@ -5,18 +5,22 @@ import { responder } from '../lib/responder'; import { authSerializer } from '../serializers/auth.serializer'; import { extractJwt } from '../lib/utils'; import { JwtPayload } from '../middleware/permission.middleware'; -import { AuthRequest } from '../models/request.model'; +import { AuthRequest, BruteRequest } from '../models/request.model'; import * as authService from '../services/auth.service'; /** * Return all users */ -export async function login(req: Request, res: Response): Promise { +export async function login(req: BruteRequest, res: Response): Promise { const data = await authService.login(req.body); - responder.success(res, { - status: httpStatus.OK, - payload: data, - serializer: authSerializer, + + // Reset brute force protection and return response + req.brute.reset(() => { + responder.success(res, { + status: httpStatus.OK, + payload: data, + serializer: authSerializer, + }); }); } diff --git a/src/lib/memory-store.ts b/src/lib/memory-store.ts new file mode 100644 index 0000000..3938937 --- /dev/null +++ b/src/lib/memory-store.ts @@ -0,0 +1,17 @@ +import * as redis from 'redis'; + +let redisClient; + +const options: redis.ClientOpts = { + url: process.env.REDISCLOUD_URL, +}; + + +/** + * Make sure we return the same instance + * Don't create the instance on startup (not needed in all environments) + */ +export function getRedisClient() { + if (!redisClient) redisClient = redis.createClient(options); + return redisClient; +} diff --git a/src/middleware/bruteforce.middleware.ts b/src/middleware/bruteforce.middleware.ts new file mode 100644 index 0000000..4b70554 --- /dev/null +++ b/src/middleware/bruteforce.middleware.ts @@ -0,0 +1,16 @@ +import { getRateLimiter } from 'tree-house'; +import { RequestHandler } from 'express'; +import { globalBruteConfig, userBruteConfig, userBruteMiddlewareConfig } from '../config/security.config'; + + +/** + * No more than 1000 login attempts per day per IP + */ +export const setGlobalBruteforce: RequestHandler = getRateLimiter(globalBruteConfig).prevent; + + +/** + * Start slowing requests after 5 failed attempts to do something for the same user + */ +export const setUserBruteForce: RequestHandler = getRateLimiter(userBruteConfig) + .getMiddleware(userBruteMiddlewareConfig); diff --git a/src/models/request.model.ts b/src/models/request.model.ts index 93ae784..a69c4d6 100644 --- a/src/models/request.model.ts +++ b/src/models/request.model.ts @@ -6,3 +6,10 @@ export interface AuthRequest extends Request { user: User; }; } + +export interface BruteRequest extends Request { + brute: { + reset: (fn: Function) => {}; + }; +} + diff --git a/src/routes/v1/auth.routes.ts b/src/routes/v1/auth.routes.ts index 32877f1..ab83caa 100644 --- a/src/routes/v1/auth.routes.ts +++ b/src/routes/v1/auth.routes.ts @@ -2,11 +2,14 @@ import { Router } from 'express'; import { handleAsyncFn, validateSchema } from 'tree-house'; import { authSchema } from '../../schemes/auth.schema'; +import { setGlobalBruteforce, setUserBruteForce } from '../../middleware/bruteforce.middleware'; import { hasPermission } from '../../middleware/permission.middleware'; import * as controller from '../../controllers/auth.controller'; export const routes: Router = Router({ mergeParams: true }) .post('/auth/login', + setGlobalBruteforce, + setUserBruteForce, validateSchema(authSchema.login), handleAsyncFn(controller.login)) diff --git a/tests/_helpers/mockdata/data.ts b/tests/_helpers/mockdata/data.ts index 58c7f40..fb01312 100644 --- a/tests/_helpers/mockdata/data.ts +++ b/tests/_helpers/mockdata/data.ts @@ -1,12 +1,13 @@ import { tableNames } from '../../../src/constants'; import { db } from '../../../src/lib/db'; - +import { clearMemoryStore } from './memory-store.data'; /** * Clear all databases */ export function clearAll() { return Promise.all([ + clearMemoryStore(), db(tableNames.USERS).del(), db(tableNames.CODES).del(), db(tableNames.CODETYPES).del(), diff --git a/tests/_helpers/mockdata/memory-store.data.ts b/tests/_helpers/mockdata/memory-store.data.ts new file mode 100644 index 0000000..7b5770a --- /dev/null +++ b/tests/_helpers/mockdata/memory-store.data.ts @@ -0,0 +1,14 @@ +import { getRedisClient } from '../../../src/lib/memory-store'; + + +/** + * Clear Redis memory store + */ +export async function clearMemoryStore() { + return new Promise((resolve, reject) => { + getRedisClient().flushdb((error, succeeded) => { + if (error) reject(error); + resolve(succeeded); + }); + }); +} diff --git a/tests/integration/auth.route.test.ts b/tests/integration/auth.route.test.ts index 179febc..9114ebe 100644 --- a/tests/integration/auth.route.test.ts +++ b/tests/integration/auth.route.test.ts @@ -25,6 +25,7 @@ describe('/auth', () => { }); describe('POST /login', () => { + // TODO: Test if brute force protection gets reset after successful attempt! it('Should succesfully login a user with correct credentials', async () => { const { body, status } = await request(app) .post(`${prefix}/auth/login`) diff --git a/tests/lib/utils.test.ts b/tests/lib/utils.test.ts index 9e3e0dd..566ad51 100644 --- a/tests/lib/utils.test.ts +++ b/tests/lib/utils.test.ts @@ -1,5 +1,5 @@ -import { UnauthorizedError } from 'tree-house-errors'; import * as httpMocks from 'node-mocks-http'; +import { UnauthorizedError } from 'tree-house-errors'; import { errors } from '../../src/config/errors.config'; import { roles } from '../../src/config/roles.config'; import { User } from '../../src/models/user.model'; diff --git a/tests/middleware/bruteforce.middleware.test.ts b/tests/middleware/bruteforce.middleware.test.ts new file mode 100644 index 0000000..a2f849c --- /dev/null +++ b/tests/middleware/bruteforce.middleware.test.ts @@ -0,0 +1,50 @@ +import * as httpStatus from 'http-status'; +import * as request from 'supertest'; +import * as express from 'express'; +import { setBodyParser } from 'tree-house'; +import { setUserBruteForce } from '../../src/middleware/bruteforce.middleware'; +import { userBruteConfig } from '../../src/config/security.config'; +import { errors } from '../../src/config/errors.config'; +import { responder } from '../../src/lib/responder'; +import { clearMemoryStore } from '../_helpers/mockdata/memory-store.data'; + +describe('bruteforce middleware', () => { + let app; + + beforeEach(async () => { + app = express(); + setBodyParser(app, '*'); + + await clearMemoryStore(); + }); + + it('Should start blocking requests with the same ip and username after number of retries', async () => { + app.use('/test', setUserBruteForce, (_req, res) => res.status(httpStatus.OK).send('Welcome')); + app.use((error, _req, res, _next) => responder.error(res, error)); + + const numberOfCalls = userBruteConfig.freeRetries + 1; + + // Successful calls + for (const call of Array(numberOfCalls)) { + const { status } = await request(app) + .post('/test') + .send({ username: 'test@icapps.com' }); + expect(status).toEqual(httpStatus.OK); + } + + // Blocked call + const { status, body } = await request(app) + .post('/test') + .send({ username: 'test@icapps.com' }); + + expect(status).toEqual(httpStatus.TOO_MANY_REQUESTS); + expect(body.errors[0].code).toEqual(errors.TOO_MANY_REQUESTS.code); + expect(body.errors[0].title).toEqual(errors.TOO_MANY_REQUESTS.message); + + // Allow call with other username + const { status: status2, body: body2 } = await request(app) + .post('/test') + .send({ username: 'test2@icapps.com' }); + expect(status2).toEqual(httpStatus.OK); + }); +}); diff --git a/tests/test.config.ts b/tests/test.config.ts index 76238d7..68b5614 100644 --- a/tests/test.config.ts +++ b/tests/test.config.ts @@ -11,6 +11,7 @@ export const environment = { MIN_VERSION_IOS: '1.0.0', LATEST_VERSION_IOS: '2.0.2', MANDRILL_API_KEY: 'myKey', + REDISCLOUD_URL: 'redis://0.0.0.0:6379', }; Object.keys(environment).forEach((key) => { diff --git a/yarn.lock b/yarn.lock index a9021c9..f010247 100644 --- a/yarn.lock +++ b/yarn.lock @@ -65,6 +65,12 @@ version "1.2.0" resolved "https://registry.npmjs.org/@types/events/-/events-1.2.0.tgz#81a6731ce4df43619e5c8c945383b3e62a89ea86" +"@types/express-brute@~0.0.36": + version "0.0.36" + resolved "https://registry.npmjs.org/@types/express-brute/-/express-brute-0.0.36.tgz#018b06f5068a407de03527ca310c2b5f75c38c6e" + dependencies: + "@types/express" "*" + "@types/express-serve-static-core@*": version "4.11.1" resolved "https://registry.npmjs.org/@types/express-serve-static-core/-/express-serve-static-core-4.11.1.tgz#f6f7212382d59b19d696677bcaa48a37280f5d45" @@ -128,6 +134,13 @@ version "9.6.5" resolved "https://registry.npmjs.org/@types/node/-/node-9.6.5.tgz#ee700810fdf49ac1c399fc5980b7559b3e5a381d" +"@types/redis@~2.8.6": + version "2.8.6" + resolved "https://registry.npmjs.org/@types/redis/-/redis-2.8.6.tgz#3674d07a13ad76bccda4c37dc3909e4e95757e7e" + dependencies: + "@types/events" "*" + "@types/node" "*" + "@types/serve-static@*": version "1.13.1" resolved "https://registry.npmjs.org/@types/serve-static/-/serve-static-1.13.1.tgz#1d2801fa635d274cd97d4ec07e26b21b44127492" @@ -1353,6 +1366,10 @@ dotenv@^5.0.0, dotenv@^5.0.1: version "5.0.1" resolved "https://registry.npmjs.org/dotenv/-/dotenv-5.0.1.tgz#a5317459bd3d79ab88cff6e44057a6a3fbb1fcef" +double-ended-queue@^2.1.0-0: + version "2.1.0-0" + resolved "https://registry.npmjs.org/double-ended-queue/-/double-ended-queue-2.1.0-0.tgz#103d3527fd31528f40188130c841efdd78264e5c" + duplexer3@^0.1.4: version "0.1.4" resolved "https://registry.npmjs.org/duplexer3/-/duplexer3-0.1.4.tgz#ee01dd1cac0ed3cbc7fdbea37dc0a8f1ce002ce2" @@ -4751,10 +4768,26 @@ rechoir@^0.6.2: dependencies: resolve "^1.1.6" +redis-commands@^1.2.0: + version "1.3.5" + resolved "https://registry.npmjs.org/redis-commands/-/redis-commands-1.3.5.tgz#4495889414f1e886261180b1442e7295602d83a2" + +redis-parser@^2.6.0: + version "2.6.0" + resolved "https://registry.npmjs.org/redis-parser/-/redis-parser-2.6.0.tgz#52ed09dacac108f1a631c07e9b69941e7a19504b" + redis@~0.10.0: version "0.10.3" resolved "https://registry.npmjs.org/redis/-/redis-0.10.3.tgz#8927fe2110ee39617bcf3fd37b89d8e123911bb6" +redis@~2.8.0: + version "2.8.0" + resolved "https://registry.npmjs.org/redis/-/redis-2.8.0.tgz#202288e3f58c49f6079d97af7a10e1303ae14b02" + dependencies: + double-ended-queue "^2.1.0-0" + redis-commands "^1.2.0" + redis-parser "^2.6.0" + referrer-policy@1.1.0: version "1.1.0" resolved "https://registry.npmjs.org/referrer-policy/-/referrer-policy-1.1.0.tgz#35774eb735bf50fb6c078e83334b472350207d79" @@ -5622,9 +5655,9 @@ tree-house-errors@^1.0.3: joi "~13.1.2" uuid "~3.2.1" -tree-house@~3.1.0: - version "3.1.0" - resolved "https://registry.npmjs.org/tree-house/-/tree-house-3.1.0.tgz#e4e5517c23b98171c542d0496dcb659494ebaca4" +tree-house@~3.2.0: + version "3.2.0" + resolved "https://registry.npmjs.org/tree-house/-/tree-house-3.2.0.tgz#6d0c26b96548ed046a82fdcfe8ffb3fb04a3ae7f" dependencies: body-parser "~1.18.2" cors "~2.8.4"