diff --git a/apps/api/package.json b/apps/api/package.json index 0d58a51..bbe3b1f 100644 --- a/apps/api/package.json +++ b/apps/api/package.json @@ -50,6 +50,7 @@ "handlebars": "^4.7.9", "helmet": "^8.1.0", "ioredis": "^5.4.0", + "jsonwebtoken": "^9.0.3", "nodemailer": "^8.0.5", "otplib": "^13.4.0", "passport": "^0.7.0", @@ -76,6 +77,7 @@ "@types/bcrypt": "^6.0.0", "@types/cookie-parser": "^1.4.10", "@types/express": "^5.0.0", + "@types/jsonwebtoken": "^9.0.10", "@types/node": "^25.5.2", "@types/nodemailer": "^8.0.0", "@types/passport-google-oauth20": "^2.0.17", diff --git a/apps/api/src/modules/auth/infrastructure/__tests__/jwt.strategy.spec.ts b/apps/api/src/modules/auth/infrastructure/__tests__/jwt.strategy.spec.ts index 91a650c..c8eaf7f 100644 --- a/apps/api/src/modules/auth/infrastructure/__tests__/jwt.strategy.spec.ts +++ b/apps/api/src/modules/auth/infrastructure/__tests__/jwt.strategy.spec.ts @@ -28,6 +28,11 @@ vi.mock('@modules/shared', () => ({ RedisService: class {}, })); +// Mock jsonwebtoken so we can control which secret succeeds +vi.mock('jsonwebtoken', () => ({ + verify: vi.fn(), +})); + type PrismaStub = { user: { findUnique: ReturnType } }; type RedisStub = { isAvailable: ReturnType; @@ -218,3 +223,118 @@ describe('JwtStrategy', () => { ).rejects.toMatchObject({ status: 401 }); }); }); + +describe('JwtStrategy dual-key (secretOrKeyProvider)', () => { + afterEach(() => { + vi.unstubAllEnvs(); + vi.resetModules(); + vi.clearAllMocks(); + }); + + it('uses secretOrKeyProvider instead of secretOrKey', async () => { + vi.stubEnv('JWT_SECRET', 'primary-secret-at-least-32-chars'); + const { JwtStrategy } = await import('../strategies/jwt.strategy'); + const strategy = new JwtStrategy(makePrisma(ACTIVE_USER) as never, makeRedis() as never); + + const options = (strategy as any)._options; + expect(options.secretOrKeyProvider).toBeDefined(); + expect(options.secretOrKey).toBeUndefined(); + }); + + it('secretOrKeyProvider returns primary secret when primary verification succeeds', async () => { + vi.stubEnv('JWT_SECRET', 'primary-secret-at-least-32-chars'); + delete process.env['JWT_SECRET_PREVIOUS']; + + const { verify } = await import('jsonwebtoken'); + const mockVerify = vi.mocked(verify); + mockVerify.mockReturnValueOnce({} as any); + + const { JwtStrategy } = await import('../strategies/jwt.strategy'); + const strategy = new JwtStrategy(makePrisma(ACTIVE_USER) as never, makeRedis() as never); + + const options = (strategy as any)._options; + const result = await new Promise((resolve, reject) => { + options.secretOrKeyProvider({} as any, 'some-jwt-token', (err: Error | null, secret?: string) => { + if (err) reject(err); + else resolve(secret!); + }); + }); + + expect(result).toBe('primary-secret-at-least-32-chars'); + }); + + it('secretOrKeyProvider falls back to previous secret when primary fails', async () => { + vi.stubEnv('JWT_SECRET', 'primary-secret-at-least-32-chars'); + vi.stubEnv('JWT_SECRET_PREVIOUS', 'previous-secret-at-least-32-chars'); + + const { verify } = await import('jsonwebtoken'); + const mockVerify = vi.mocked(verify); + mockVerify.mockImplementationOnce(() => { + throw new Error('invalid signature'); + }); + mockVerify.mockReturnValueOnce({} as any); + + const { JwtStrategy } = await import('../strategies/jwt.strategy'); + const strategy = new JwtStrategy(makePrisma(ACTIVE_USER) as never, makeRedis() as never); + + const options = (strategy as any)._options; + const result = await new Promise((resolve, reject) => { + options.secretOrKeyProvider({} as any, 'old-jwt-token', (err: Error | null, secret?: string) => { + if (err) reject(err); + else resolve(secret!); + }); + }); + + expect(result).toBe('previous-secret-at-least-32-chars'); + }); + + it('secretOrKeyProvider returns primary when both keys fail (passport will 401)', async () => { + vi.stubEnv('JWT_SECRET', 'primary-secret-at-least-32-chars'); + vi.stubEnv('JWT_SECRET_PREVIOUS', 'previous-secret-at-least-32-chars'); + + const { verify } = await import('jsonwebtoken'); + const mockVerify = vi.mocked(verify); + mockVerify.mockImplementation(() => { + throw new Error('invalid signature'); + }); + + const { JwtStrategy } = await import('../strategies/jwt.strategy'); + const strategy = new JwtStrategy(makePrisma(ACTIVE_USER) as never, makeRedis() as never); + + const options = (strategy as any)._options; + const result = await new Promise((resolve, reject) => { + options.secretOrKeyProvider({} as any, 'bad-jwt-token', (err: Error | null, secret?: string) => { + if (err) reject(err); + else resolve(secret!); + }); + }); + + expect(result).toBe('primary-secret-at-least-32-chars'); + }); + + it('secretOrKeyProvider skips fallback when no previous secret is configured', async () => { + vi.stubEnv('JWT_SECRET', 'primary-secret-at-least-32-chars'); + delete process.env['JWT_SECRET_PREVIOUS']; + + const { verify } = await import('jsonwebtoken'); + const mockVerify = vi.mocked(verify); + mockVerify.mockImplementation(() => { + throw new Error('invalid signature'); + }); + + const { JwtStrategy } = await import('../strategies/jwt.strategy'); + const strategy = new JwtStrategy(makePrisma(ACTIVE_USER) as never, makeRedis() as never); + + const options = (strategy as any)._options; + const result = await new Promise((resolve, reject) => { + options.secretOrKeyProvider({} as any, 'bad-jwt-token', (err: Error | null, secret?: string) => { + if (err) reject(err); + else resolve(secret!); + }); + }); + + expect(result).toBe('primary-secret-at-least-32-chars'); + // verify called only once — no fallback attempted + expect(mockVerify).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/api/src/modules/auth/infrastructure/strategies/jwt.strategy.ts b/apps/api/src/modules/auth/infrastructure/strategies/jwt.strategy.ts index 57009fd..df3ea10 100644 --- a/apps/api/src/modules/auth/infrastructure/strategies/jwt.strategy.ts +++ b/apps/api/src/modules/auth/infrastructure/strategies/jwt.strategy.ts @@ -1,6 +1,7 @@ import { Injectable, UnauthorizedException } from '@nestjs/common'; import { PassportStrategy } from '@nestjs/passport'; import { type Request } from 'express'; +import { verify as jwtVerify } from 'jsonwebtoken'; import { ExtractJwt, Strategy } from 'passport-jwt'; // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- NestJS DI requires value imports for emitDecoratorMetadata import { PrismaService, RedisService } from '@modules/shared'; @@ -26,6 +27,42 @@ export const USER_STATUS_CACHE_PREFIX = 'auth:user_status:v1'; /** TTL for cached user status (seconds). */ export const USER_STATUS_CACHE_TTL_SECONDS = 60; +/** + * Builds a `secretOrKeyProvider` callback for passport-jwt that tries the + * primary secret first, then falls back to an optional previous secret. + * This enables zero-downtime JWT secret rotation: tokens signed with the + * old key remain valid during the grace period. + * + * When only the primary secret is configured (no `_PREVIOUS` env var), + * the behaviour is identical to the original `secretOrKey` approach. + */ +export function makeSecretOrKeyProvider( + primarySecret: string, + previousSecret: string | undefined, +): (request: Request, rawJwtToken: string, done: (err: Error | null, secret?: string) => void) => void { + return (_request: Request, rawJwtToken: string, done: (err: Error | null, secret?: string) => void) => { + // Fast path: try primary first (the common case after rotation completes). + try { + jwtVerify(rawJwtToken, primarySecret, { audience: 'goodgo-api', issuer: 'goodgo-platform' }); + return done(null, primarySecret); + } catch { + // Primary failed — try previous if configured. + } + + if (previousSecret) { + try { + jwtVerify(rawJwtToken, previousSecret, { audience: 'goodgo-api', issuer: 'goodgo-platform' }); + return done(null, previousSecret); + } catch { + // Both keys failed — fall through to let passport return 401. + } + } + + // Return the primary so passport-jwt produces its standard error. + return done(null, primarySecret); + }; +} + @Injectable() export class JwtStrategy extends PassportStrategy(Strategy) { constructor( @@ -37,10 +74,12 @@ export class JwtStrategy extends PassportStrategy(Strategy) { throw new Error('JWT_SECRET environment variable is required'); } + const previousSecret = process.env['JWT_SECRET_PREVIOUS'] || undefined; + super({ jwtFromRequest: extractJwtFromCookieOrHeader, ignoreExpiration: false, - secretOrKey: jwtSecret, + secretOrKeyProvider: makeSecretOrKeyProvider(jwtSecret, previousSecret), audience: 'goodgo-api', issuer: 'goodgo-platform', }); diff --git a/apps/api/src/modules/shared/infrastructure/__tests__/env-validation.spec.ts b/apps/api/src/modules/shared/infrastructure/__tests__/env-validation.spec.ts index b9b16e2..3d5e9d4 100644 --- a/apps/api/src/modules/shared/infrastructure/__tests__/env-validation.spec.ts +++ b/apps/api/src/modules/shared/infrastructure/__tests__/env-validation.spec.ts @@ -145,4 +145,52 @@ describe('validateEnv', () => { expect(() => validateEnv()).not.toThrow(); }); + + describe('optional previous secrets (JWT_SECRET_PREVIOUS / JWT_REFRESH_SECRET_PREVIOUS)', () => { + it('accepts when previous secrets are not set (no rotation in progress)', () => { + setValidJwtSecrets(); + delete process.env['JWT_SECRET_PREVIOUS']; + delete process.env['JWT_REFRESH_SECRET_PREVIOUS']; + + expect(() => validateEnv()).not.toThrow(); + }); + + it('accepts valid JWT_SECRET_PREVIOUS', () => { + setValidJwtSecrets(); + process.env['JWT_SECRET_PREVIOUS'] = 'a-valid-previous-secret-with-at-least-32-characters'; + + expect(() => validateEnv()).not.toThrow(); + }); + + it('rejects JWT_SECRET_PREVIOUS when it is a placeholder', () => { + setValidJwtSecrets(); + process.env['JWT_SECRET_PREVIOUS'] = 'CHANGE_ME'; + + expect(() => validateEnv()).toThrow('Insecure JWT secret configuration'); + expect(() => validateEnv()).toThrow('JWT_SECRET_PREVIOUS'); + }); + + it('rejects JWT_SECRET_PREVIOUS when it is too short', () => { + setValidJwtSecrets(); + process.env['JWT_SECRET_PREVIOUS'] = 'short'; + + expect(() => validateEnv()).toThrow('too short'); + }); + + it('rejects JWT_REFRESH_SECRET_PREVIOUS when it is a placeholder', () => { + setValidJwtSecrets(); + process.env['JWT_REFRESH_SECRET_PREVIOUS'] = 'password'; + + expect(() => validateEnv()).toThrow('Insecure JWT secret configuration'); + expect(() => validateEnv()).toThrow('JWT_REFRESH_SECRET_PREVIOUS'); + }); + + it('accepts both previous secrets when valid', () => { + setValidJwtSecrets(); + process.env['JWT_SECRET_PREVIOUS'] = 'previous-access-secret-at-least-32-chars-long!!'; + process.env['JWT_REFRESH_SECRET_PREVIOUS'] = 'previous-refresh-secret-at-least-32-chars-long!!'; + + expect(() => validateEnv()).not.toThrow(); + }); + }); }); diff --git a/apps/api/src/modules/shared/infrastructure/env-validation.ts b/apps/api/src/modules/shared/infrastructure/env-validation.ts index e25077a..391a174 100644 --- a/apps/api/src/modules/shared/infrastructure/env-validation.ts +++ b/apps/api/src/modules/shared/infrastructure/env-validation.ts @@ -11,6 +11,18 @@ const ALWAYS_REQUIRED: readonly string[] = [ 'JWT_REFRESH_SECRET', ]; +/** + * Optional previous-generation JWT secrets used during key rotation. + * When set, the auth layer will accept tokens signed with either the + * primary or the previous key, enabling zero-downtime secret rotation. + * Subject to the same placeholder-rejection and minimum-length checks + * as the primary secrets. + */ +const OPTIONAL_PREVIOUS_SECRETS: readonly string[] = [ + 'JWT_SECRET_PREVIOUS', + 'JWT_REFRESH_SECRET_PREVIOUS', +]; + const REQUIRED_IN_PRODUCTION: readonly string[] = [ 'DATABASE_URL', 'CORS_ORIGINS', @@ -120,6 +132,19 @@ export function validateEnv(): void { } } + // Optional previous-generation secrets — if set, they must also pass + // the same strength checks. An empty/unset value is fine (no rotation + // in progress), but a weak value is always rejected. + for (const key of OPTIONAL_PREVIOUS_SECRETS) { + const value = process.env[key]; + if (value) { + const error = validateJwtSecret(key, value); + if (error) { + secretErrors.push(error); + } + } + } + if (secretErrors.length > 0) { throw new Error( `Insecure JWT secret configuration:\n ${secretErrors.join('\n ')}\n` + diff --git a/docs/security/SECRET_ROTATION_POLICY.md b/docs/security/SECRET_ROTATION_POLICY.md new file mode 100644 index 0000000..b68119f --- /dev/null +++ b/docs/security/SECRET_ROTATION_POLICY.md @@ -0,0 +1,192 @@ +# Payment Gateway Secret Rotation Policy + +> **Status:** Active — GOO-197 / parent [GOO-102](/GOO/issues/GOO-102) (CLO data-security work). +> **Owner:** Security Engineer + Platform on-call. +> **Last reviewed:** 2026-04-24. + +This document is the canonical policy for rotating all secrets that gate +access to GoodGo's payment gateways and adjacent integrations (OAuth, +storage, webhook signing, JWT). It ships alongside the `SecretProvider` +abstraction in +`apps/api/src/modules/shared/domain/ports/secret-provider.port.ts` and the +env-backed implementation in +`apps/api/src/modules/shared/infrastructure/env-secret-provider.service.ts`. + +--- + +## 1. Why rotate + +A stolen or leaked HMAC key for a payment gateway is the most direct path +to financial fraud against GoodGo. Rotation reduces the **window of abuse** +when a key is exposed (insider misuse, accidental git commit, third-party +breach, log scraping, etc.). It also forces us to verify that every +runtime that relies on the key can still read it — i.e. that we have not +lost the ability to rotate. + +## 2. Scope (rotation-sensitive secrets) + +The following secrets are in scope. Each is registered with the +`SecretProvider` by default (see `DEFAULT_REGISTERED_SECRETS`) and has a +matching entry in `env-validation.ts`. + +| Secret env var | Purpose | Cadence | Owner | +| ------------------------------ | ------------------------------- | -------- | -------------- | +| `JWT_SECRET` | Access-token HMAC | 90 days | Auth | +| `JWT_REFRESH_SECRET` | Refresh-token HMAC | 90 days | Auth | +| `VNPAY_HASH_SECRET` | VNPay request/callback HMAC | 90 days | Payments | +| `MOMO_SECRET_KEY` | MoMo request/callback HMAC | 90 days | Payments | +| `ZALOPAY_KEY1` | ZaloPay order signing | 90 days | Payments | +| `ZALOPAY_KEY2` | ZaloPay callback signing | 90 days | Payments | +| `BANK_TRANSFER_WEBHOOK_SECRET` | Bank-transfer webhook signature | 90 days | Payments | +| `GOOGLE_CLIENT_SECRET` | Google OAuth | 180 days | Auth | +| `ZALO_APP_SECRET` | Zalo OAuth | 180 days | Auth | +| `ZALO_OA_ACCESS_TOKEN` | Zalo Official Account API token | 90 days | Notifications | +| `MINIO_SECRET_KEY` | Object-storage access key | 180 days | Platform | +| `FIELD_ENCRYPTION_KEY` | At-rest PII encryption key | annually | Platform + CLO | + +Secrets **not** in this table (e.g. `DATABASE_URL` password, `REDIS_HOST`) +follow the platform-credential rotation policy and are out of scope here. + +## 3. Cadence and triggers + +- **Routine rotation:** every 90 days for HMAC/signing keys, 180 days for + OAuth client secrets, annually for the field-encryption key (which has + expensive data-rewrap implications). +- **Event-driven rotation (always immediately):** + - any commit accidentally containing a real value of one of the secrets + above (regardless of how briefly); + - departure of any individual with production access to the secret store; + - downstream provider notification that the credential may be exposed; + - confirmed or strongly suspected breach of any system that handled the + secret in plaintext (CI runner, dev laptop, log aggregator, …). + +## 4. Operator workflow (env-backed backend) + +1. **Generate** a new high-entropy value: + + ```bash + openssl rand -base64 48 + ``` + +2. **Stage the dual-key grace period.** Copy the current secret to the + `_PREVIOUS` variable and set the new secret as the primary: + + ```bash + # Example for JWT_SECRET rotation: + JWT_SECRET_PREVIOUS= + JWT_SECRET= + # Same pattern for JWT_REFRESH_SECRET if rotating refresh keys. + ``` + + The auth layer automatically tries the primary key first and falls + back to `_PREVIOUS`, so tokens signed with the old key continue to + validate during the grace period (≤ access-token TTL, typically 15 m). + +3. **Deploy** the change. On boot, every API instance logs: + + ``` + [EnvSecretProvider] Secret versions at boot: VNPAY_HASH_SECRET=2026-04-24, … + ``` + + Verify the version field matches the staged version on every instance. + The raw value **must never** appear in this or any other log line. + +4. **Smoke-test** payment flows for the rotated provider: + - issue one sandbox payment + - confirm callback verification succeeds + - confirm refund signing succeeds + Record the rotation in the security audit log + (`docs/security/secret-rotation-log.md` — append-only). + +5. **Decommission** the old credential in the gateway's merchant portal. + +6. **Remove the previous secret.** After the grace period (at least one + full access-token TTL cycle, typically 15 minutes), remove + `JWT_SECRET_PREVIOUS` (and/or `JWT_REFRESH_SECRET_PREVIOUS`) from the + environment and redeploy. This closes the dual-key window. + +## 5. SecretProvider abstraction (developer workflow) + +All new and existing code that consumes a rotation-sensitive secret MUST +go through the `SecretProvider` port: + +```ts +import { Inject, Injectable } from '@nestjs/common'; +import { SECRET_PROVIDER, type ISecretProvider } from '@modules/shared/domain/ports'; + +@Injectable() +export class VnpayService { + constructor(@Inject(SECRET_PROVIDER) private readonly secrets: ISecretProvider) {} + + async sign(payload: string): Promise { + const { value } = await this.secrets.getSecret('VNPAY_HASH_SECRET'); + // … HMAC with `value`, never store it on `this`, never log it. + } +} +``` + +Rules: + +- **Never** capture the raw value into a service field. Always re-read on + the request path so a rotation takes effect at the next request. +- **Never** include `material.value` in log messages, error messages, or + exception payloads. `material.version` is safe to log. +- **Never** stringify a `SecretMaterial` directly into a response body. +- For bootstrap-only contexts where `await` is awkward, use + `getSecretSync` — but note that a future remote backend may throw + `UnsupportedSyncReadError`. + +## 6. Backends + +- **Short term — `EnvSecretProvider` (current).** Reads from `process.env` + via `ConfigService`. Operationally identical to the pre-existing + `getOrThrow('VNPAY_HASH_SECRET')` calls, but with a stable audit surface + (versions logged, port-based DI). +- **Mid term — `AwsSecretsManagerSecretProvider` / `VaultSecretProvider`.** + Same port. Adds: + - automatic refresh from the remote store + - per-secret IAM / Vault-policy scoping + - native version ids (`AWSCURRENT` / `AWSPREVIOUS` etc.) surfaced as + `material.version` + - `getSecretSync` may throw `UnsupportedSyncReadError`; bootstrap + callers must migrate to `getSecret`. + +Switching backends is a one-line change in `SharedModule` (replace +`EnvSecretProvider` with the new implementation under the +`SECRET_PROVIDER` token). No call sites change. + +## 7. Logging discipline + +- The `EnvSecretProvider` logs only `name=version` pairs at boot. +- The `version` is either an operator-provided `_SECRET_VERSION` env + var, or a 10-char SHA-256 fingerprint of the value (40 bits of entropy; + non-invertible; useful for distinguishing rotations across instances). +- Negative tests in + `apps/api/src/modules/shared/infrastructure/__tests__/env-secret-provider.service.spec.ts` + assert the raw value never appears in logger output, error messages, or + serialized provider state. +- The repo also has a global `pii-masker` and `GlobalExceptionFilter` — + those are defence-in-depth, not the primary control. The primary control + is "never put the value into a string in the first place." + +## 8. Incident response (suspected leak) + +1. Open a P1 incident in `#sec-incident`. Page Security on-call. +2. Rotate the affected secret immediately following §4 — do not wait for + forensic confirmation. +3. Search logs / CI artifacts / git history for the leaked value + fingerprint (NOT the value itself; use `fingerprint()` from + `env-secret-provider.service.ts`). +4. Coordinate with the gateway's anti-fraud team where applicable (VNPay, + MoMo, ZaloPay merchant support). +5. File a post-mortem within 5 business days; update this policy if + process gaps were found. + +## 9. References + +- Source port: `apps/api/src/modules/shared/domain/ports/secret-provider.port.ts` +- Env-backed impl: `apps/api/src/modules/shared/infrastructure/env-secret-provider.service.ts` +- Env validation: `apps/api/src/modules/shared/infrastructure/env-validation.ts` +- Negative tests: `apps/api/src/modules/shared/infrastructure/__tests__/env-secret-provider.service.spec.ts` +- Parent issue: [GOO-102](/GOO/issues/GOO-102) +- This issue: [GOO-197](/GOO/issues/GOO-197) diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 010a0c5..8318660 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -186,6 +186,9 @@ importers: ioredis: specifier: ^5.4.0 version: 5.10.1 + jsonwebtoken: + specifier: ^9.0.3 + version: 9.0.3 nodemailer: specifier: ^8.0.5 version: 8.0.5 @@ -259,6 +262,9 @@ importers: '@types/express': specifier: ^5.0.0 version: 5.0.6 + '@types/jsonwebtoken': + specifier: ^9.0.10 + version: 9.0.10 '@types/node': specifier: ^25.5.2 version: 25.5.2