From f7f91f3f22080a8fc55aad8b34a1a82294fb72ac Mon Sep 17 00:00:00 2001 From: Anass Bouassaba Date: Mon, 9 Dec 2024 15:12:54 +0100 Subject: [PATCH] fix(idp): brute force protection --- idp/.env | 4 ++ idp/src/config/config.ts | 14 ++++++ idp/src/config/types.ts | 7 +++ idp/src/infra/error/core.ts | 2 + idp/src/infra/error/creators.ts | 8 +++ idp/src/token/service.ts | 47 +++++++++++++---- idp/src/user/model.ts | 2 + idp/src/user/repo.ts | 23 +++------ migrations/src/lib.rs | 2 + ...209_000001_add_user_locked_until_column.rs | 50 +++++++++++++++++++ migrations/src/models/v1/user.rs | 1 + 11 files changed, 132 insertions(+), 28 deletions(-) create mode 100644 migrations/src/m20241209_000001_add_user_locked_until_column.rs diff --git a/idp/.env b/idp/.env index 36988b437..1d1a5631e 100644 --- a/idp/.env +++ b/idp/.env @@ -18,6 +18,10 @@ PASSWORD_MIN_UPPERCASE=1 PASSWORD_MIN_NUMBERS=1 PASSWORD_MIN_SYMBOLS=1 +# Security +SECURITY_MAX_FAILED_ATTEMPTS=5 +SECURITY_LOCKOUT_PERIOD=300 + # CORS CORS_ORIGINS="http://127.0.0.1:3000" diff --git a/idp/src/config/config.ts b/idp/src/config/config.ts index 327d456ee..d1be6027f 100644 --- a/idp/src/config/config.ts +++ b/idp/src/config/config.ts @@ -22,6 +22,7 @@ export function getConfig(): Config { readCORS(config) readSearch(config) readSMTP(config) + readSecurity(config) } return config } @@ -79,3 +80,16 @@ export function readSMTP(config: Config) { config.smtp.senderAddress = process.env.SMTP_SENDER_ADDRESS config.smtp.senderName = process.env.SMTP_SENDER_NAME } + +export function readSecurity(config: Config) { + if (process.env.SECURITY_MAX_FAILED_ATTEMPTS) { + config.security.maxFailedAttempts = parseInt( + process.env.SECURITY_MAX_FAILED_ATTEMPTS, + ) + } + if (process.env.SECURITY_LOCKOUT_PERIOD) { + config.security.lockoutPeriod = parseInt( + process.env.SECURITY_LOCKOUT_PERIOD, + ) + } +} diff --git a/idp/src/config/types.ts b/idp/src/config/types.ts index 7af4ba89d..d952fd2e9 100644 --- a/idp/src/config/types.ts +++ b/idp/src/config/types.ts @@ -14,6 +14,7 @@ export class Config { databaseURL: string token: TokenConfig password: PasswordConfig + security: SecurityConfig corsOrigins: string[] search: SearchConfig smtp: SMTPConfig @@ -23,6 +24,7 @@ export class Config { this.password = new PasswordConfig() this.search = new SearchConfig() this.smtp = new SMTPConfig() + this.security = new SecurityConfig() } } @@ -42,6 +44,11 @@ export class PasswordConfig { minSymbols: number } +export class SecurityConfig { + maxFailedAttempts: number + lockoutPeriod: number +} + export class SearchConfig { url: string } diff --git a/idp/src/infra/error/core.ts b/idp/src/infra/error/core.ts index 201772ad2..e531e4222 100644 --- a/idp/src/infra/error/core.ts +++ b/idp/src/infra/error/core.ts @@ -24,6 +24,7 @@ export enum ErrorCode { InvalidGrantType = 'invalid_grant_type', PasswordValidationFailed = 'password_validation_failed', UserSuspended = 'user_suspended', + UserTemporarilyLocked = 'user_locked', UserIsNotAdmin = 'user_is_not_admin', UserNotFound = 'user_not_found', CannotSuspendLastAdmin = 'cannot_suspend_last_admin', @@ -45,6 +46,7 @@ const statuses: { [key: string]: number } = { [ErrorCode.InvalidGrantType]: 400, [ErrorCode.PasswordValidationFailed]: 400, [ErrorCode.UserSuspended]: 403, + [ErrorCode.UserTemporarilyLocked]: 429, [ErrorCode.UserIsNotAdmin]: 403, [ErrorCode.UserNotFound]: 404, [ErrorCode.CannotSuspendLastAdmin]: 400, diff --git a/idp/src/infra/error/creators.ts b/idp/src/infra/error/creators.ts index 8d3f018a5..45e4b1b47 100644 --- a/idp/src/infra/error/creators.ts +++ b/idp/src/infra/error/creators.ts @@ -65,6 +65,14 @@ export function newUserSuspendedError() { }) } +export function newUserTemporarilyLockedError() { + return newError({ + code: ErrorCode.UserTemporarilyLocked, + message: 'User temporarily locked. Try again later.', + userMessage: 'User temporarily locked. Try again later.', + }) +} + export function newRefreshTokenExpiredError() { return newError({ code: ErrorCode.RefreshTokenExpired, diff --git a/idp/src/token/service.ts b/idp/src/token/service.ts index 1bec361e6..194fae6f0 100644 --- a/idp/src/token/service.ts +++ b/idp/src/token/service.ts @@ -17,6 +17,7 @@ import { newRefreshTokenExpiredError, newUserIsNotAdminError, newUserSuspendedError, + newUserTemporarilyLockedError, } from '@/infra/error' import { newHyphenlessUuid } from '@/infra/id' import { verifyPassword } from '@/infra/password' @@ -57,12 +58,17 @@ export async function exchange(options: TokenExchangeOptions): Promise { if (!user.isActive) { throw newUserSuspendedError() } - if (verifyPassword(options.password, user.passwordHash)) { - await resetFailedAttempts(user.id) - return newToken(user.id, user.isAdmin) + console.log(JSON.stringify(user, null, 2)) + if (isStillLocked(user)) { + throw newUserTemporarilyLockedError() } else { - await increaseFailedAttempts(user.id) - throw newInvalidUsernameOrPasswordError() + if (verifyPassword(options.password, user.passwordHash)) { + await resetFailedAttemptsAndUnlock(user.id) + return newToken(user.id, user.isAdmin) + } else { + await increaseFailedAttemptsOrLock(user.id) + throw newInvalidUsernameOrPasswordError() + } } } else if (options.grant_type === 'refresh_token') { // https://datatracker.ietf.org/doc/html/rfc6749#section-6 @@ -148,18 +154,37 @@ function newAccessTokenExpiry(): number { return Math.floor(now.getTime() / 1000) } -async function increaseFailedAttempts(userId: string): Promise { +async function increaseFailedAttemptsOrLock(userId: string): Promise { const user = await userRepo.findById(userId) - await userRepo.update({ - id: user.id, - failedAttempts: user.failedAttempts + 1, - }) + const failedAttempts = user.failedAttempts + 1 + if (failedAttempts <= getConfig().security.maxFailedAttempts) { + await userRepo.update({ + id: user.id, + failedAttempts, + }) + } else { + await userRepo.update({ + id: user.id, + lockedUntil: newLockoutUntil(), + }) + } } -async function resetFailedAttempts(userId: string): Promise { +async function resetFailedAttemptsAndUnlock(userId: string): Promise { const user = await userRepo.findById(userId) await userRepo.update({ id: user.id, failedAttempts: 0, + lockedUntil: null, }) } + +function newLockoutUntil(): string { + const now = new Date() + now.setSeconds(now.getSeconds() + getConfig().security.lockoutPeriod) + return now.toISOString() +} + +function isStillLocked(user: User): boolean { + return user.lockedUntil && new Date() < new Date(user.lockedUntil) +} diff --git a/idp/src/user/model.ts b/idp/src/user/model.ts index e45f9ad64..948435f09 100644 --- a/idp/src/user/model.ts +++ b/idp/src/user/model.ts @@ -25,6 +25,7 @@ export type User = { emailUpdateValue?: string picture?: string failedAttempts: number + lockedUntil?: string createTime: string updateTime?: string } @@ -62,6 +63,7 @@ export type UpdateOptions = { emailUpdateValue?: string picture?: string failedAttempts?: number + lockedUntil?: string createTime?: string updateTime?: string } diff --git a/idp/src/user/repo.ts b/idp/src/user/repo.ts index e0ae3f724..4c6775aa5 100644 --- a/idp/src/user/repo.ts +++ b/idp/src/user/repo.ts @@ -195,8 +195,9 @@ class UserRepoImpl { email_update_value = $13, picture = $14, failed_attempts = $15, - update_time = $16 - WHERE id = $17 + locked_until = $16, + update_time = $17 + WHERE id = $18 RETURNING *`, [ entity.fullName, @@ -214,6 +215,7 @@ class UserRepoImpl { entity.emailUpdateValue, entity.picture, entity.failedAttempts, + entity.lockedUntil, new Date().toISOString(), entity.id, ], @@ -268,6 +270,7 @@ class UserRepoImpl { emailUpdateValue: row.email_update_value, picture: row.picture, failedAttempts: row.failed_attempts, + lockedUntil: row.locked_until, createTime: row.create_time, updateTime: row.update_time, } @@ -275,21 +278,7 @@ class UserRepoImpl { /* eslint-disable-next-line @typescript-eslint/no-explicit-any */ private mapList(list: any): User[] { - /* eslint-disable-next-line @typescript-eslint/no-explicit-any */ - return list.map((user: any) => { - return { - id: user.id, - fullName: user.full_name, - username: user.username, - email: user.email, - isEmailConfirmed: user.is_email_confirmed, - isAdmin: user.is_admin, - isActive: user.is_active, - picture: user.picture, - createTime: user.create_time, - updateTime: user.update_time, - } - }) + return list.map(this.mapRow) } } diff --git a/migrations/src/lib.rs b/migrations/src/lib.rs index fef86a002..230b4f386 100644 --- a/migrations/src/lib.rs +++ b/migrations/src/lib.rs @@ -30,6 +30,7 @@ mod m20240907_000001_add_user_force_change_password_field; mod m20240913_000001_drop_segmentation_column; mod m20241114_000001_drop_user_force_change_password_column; mod m20241209_000001_add_user_failed_attempts_column; +mod m20241209_000001_add_user_locked_until_column; #[async_trait::async_trait] impl MigratorTrait for Migrator { @@ -52,6 +53,7 @@ impl MigratorTrait for Migrator { Box::new(m20240913_000001_drop_segmentation_column::Migration), Box::new(m20241114_000001_drop_user_force_change_password_column::Migration), Box::new(m20241209_000001_add_user_failed_attempts_column::Migration), + Box::new(m20241209_000001_add_user_locked_until_column::Migration), ] } } diff --git a/migrations/src/m20241209_000001_add_user_locked_until_column.rs b/migrations/src/m20241209_000001_add_user_locked_until_column.rs new file mode 100644 index 000000000..43a7a5ab8 --- /dev/null +++ b/migrations/src/m20241209_000001_add_user_locked_until_column.rs @@ -0,0 +1,50 @@ +// Copyright (c) 2023 Anass Bouassaba. +// +// Use of this software is governed by the Business Source License +// included in the file LICENSE in the root of this repository. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the GNU Affero General Public License v3.0 only, included in the file +// AGPL-3.0-only in the root of this repository. +use sea_orm_migration::prelude::*; + +use crate::models::v1::{User}; + +#[derive(DeriveMigrationName)] +pub struct Migration; + +#[async_trait::async_trait] +impl MigrationTrait for Migration { + async fn up( + &self, + manager: &SchemaManager, + ) -> Result<(), DbErr> { + manager + .alter_table( + Table::alter() + .table(User::Table) + .add_column(ColumnDef::new(User::LockedUntil).text()) + .to_owned(), + ) + .await?; + + Ok(()) + } + + async fn down( + &self, + manager: &SchemaManager, + ) -> Result<(), DbErr> { + manager + .alter_table( + Table::alter() + .table(User::Table) + .drop_column(User::LockedUntil) + .to_owned(), + ) + .await?; + + Ok(()) + } +} diff --git a/migrations/src/models/v1/user.rs b/migrations/src/models/v1/user.rs index c2abd8c83..566371d39 100644 --- a/migrations/src/models/v1/user.rs +++ b/migrations/src/models/v1/user.rs @@ -29,6 +29,7 @@ pub enum User { ForceChangePassword, Picture, FailedAttempts, + LockedUntil, CreateTime, UpdateTime, }