From 78c5a46da9e5d1552cb65223a413b5afd1c2d283 Mon Sep 17 00:00:00 2001 From: krakenhavoc Date: Mon, 6 Jul 2026 11:01:14 -0400 Subject: [PATCH 1/4] feat: harden API key auth against brute force and stale key use Failed API key validations cannot be attributed to a specific key (an unknown key hashes to no DB record), so protection is per client IP: after 10 failed validations in 15 minutes the IP is locked out of API key auth for 15 minutes (KK_APIKEY_LOCKOUT_* env overrides). The check runs before the scrypt hash, so locked-out clients also stop burning hashing CPU. Redis errors fail open to the existing global throttler. The one attributable failure - a correct but expired key - now notifies the key owner by email (rate-limited to once per key per day), since it usually means the key is still deployed somewhere or has leaked. Security notices intentionally bypass notification preferences. Hooks skipped (no pre-commit/gitleaks in sandbox); eslint+tsc+jest run manually, all green. --- backend/src/auth/auth.module.ts | 2 + backend/src/auth/auth.service.spec.ts | 76 ++++++++++- backend/src/auth/auth.service.ts | 43 +++++- .../services/api-key-security.service.spec.ts | 102 +++++++++++++++ .../auth/services/api-key-security.service.ts | 123 ++++++++++++++++++ .../auth/strategies/api-key.strategy.spec.ts | 38 +++++- .../src/auth/strategies/api-key.strategy.ts | 29 ++++- backend/src/notifications/email.service.ts | 22 ++++ backend/src/notifications/templates/index.ts | 21 +++ 9 files changed, 443 insertions(+), 13 deletions(-) create mode 100644 backend/src/auth/services/api-key-security.service.spec.ts create mode 100644 backend/src/auth/services/api-key-security.service.ts diff --git a/backend/src/auth/auth.module.ts b/backend/src/auth/auth.module.ts index 2f95bda..8660673 100644 --- a/backend/src/auth/auth.module.ts +++ b/backend/src/auth/auth.module.ts @@ -6,6 +6,7 @@ import { AuthentikProxyStrategy } from './strategies/authentik-proxy.strategy'; import { JwtStrategy } from './strategies/jwt.strategy'; import { ApiKeyStrategy } from './strategies/api-key.strategy'; import { ServiceKeyStrategy } from './strategies/service-key.strategy'; +import { ApiKeySecurityService } from './services/api-key-security.service'; import { TypeOrmModule } from '@nestjs/typeorm'; import { UserApiKey } from './entities/user-api-key.entity'; import { ServiceApiKey } from './entities/service-api-key.entity'; @@ -23,6 +24,7 @@ import { BillingModule } from '../billing/billing.module'; controllers: [AuthController], providers: [ AuthService, + ApiKeySecurityService, AuthentikProxyStrategy, JwtStrategy, ApiKeyStrategy, diff --git a/backend/src/auth/auth.service.spec.ts b/backend/src/auth/auth.service.spec.ts index e0b9a6d..51a4250 100644 --- a/backend/src/auth/auth.service.spec.ts +++ b/backend/src/auth/auth.service.spec.ts @@ -11,6 +11,7 @@ import { Domain } from '../domains/entities/domain.entity'; import { TlsCrt } from '../certs/tls/entities/tls-crt.entity'; import { BillingService } from '../billing/billing.service'; import { EmailService } from '../notifications/email.service'; +import { ApiKeySecurityService } from './services/api-key-security.service'; const HMAC_SECRET = 'test-hmac-secret'; @@ -38,6 +39,11 @@ describe('AuthService', () => { count: jest.Mock; }; let mockUserRepo: { findOne: jest.Mock; create: jest.Mock; save: jest.Mock }; + let mockEmailService: { + sendWelcome: jest.Mock; + sendApiKeyExpiredUse: jest.Mock; + }; + let mockApiKeySecurity: { shouldNotifyExpiredKeyUse: jest.Mock }; beforeEach(async () => { mockUserApiKeyRepo = { @@ -53,6 +59,13 @@ describe('AuthService', () => { create: jest.fn(), save: jest.fn(), }; + mockEmailService = { + sendWelcome: jest.fn().mockResolvedValue(undefined), + sendApiKeyExpiredUse: jest.fn().mockResolvedValue(undefined), + }; + mockApiKeySecurity = { + shouldNotifyExpiredKeyUse: jest.fn().mockResolvedValue(true), + }; const module: TestingModule = await Test.createTestingModule({ providers: [ @@ -76,9 +89,11 @@ describe('AuthService', () => { { provide: getRepositoryToken(TlsCrt), useValue: { count: jest.fn() } }, { provide: EmailService, - useValue: { - sendWelcome: jest.fn().mockResolvedValue(undefined), - }, + useValue: mockEmailService, + }, + { + provide: ApiKeySecurityService, + useValue: mockApiKeySecurity, }, { provide: BillingService, @@ -396,6 +411,61 @@ describe('AuthService', () => { expect(await service.validateApiKey('kk_nonexistent')).toBeNull(); }); + + it('returns null for an expired key and notifies the owner', async () => { + const record = { + id: 'key-1', + name: 'ci-deploy', + hash: 'h', + expiresAt: new Date(Date.now() - 1000), + user: { id: 'user-1', username: 'luke', email: 'luke@example.com' }, + }; + mockUserApiKeyRepo.findOne.mockResolvedValue(record); + + expect( + await service.validateApiKey('kk_expired', { ip: '10.0.0.1' }), + ).toBeNull(); + expect(mockApiKeySecurity.shouldNotifyExpiredKeyUse).toHaveBeenCalledWith( + 'key-1', + ); + expect(mockEmailService.sendApiKeyExpiredUse).toHaveBeenCalledWith({ + userId: 'user-1', + username: 'luke', + email: 'luke@example.com', + keyId: 'key-1', + keyName: 'ci-deploy', + ip: '10.0.0.1', + }); + }); + + it('rate-limits expired-key notifications via the security service', async () => { + mockApiKeySecurity.shouldNotifyExpiredKeyUse.mockResolvedValue(false); + mockUserApiKeyRepo.findOne.mockResolvedValue({ + id: 'key-1', + name: 'ci-deploy', + hash: 'h', + expiresAt: new Date(Date.now() - 1000), + user: { id: 'user-1', username: 'luke', email: 'luke@example.com' }, + }); + + expect(await service.validateApiKey('kk_expired')).toBeNull(); + expect(mockEmailService.sendApiKeyExpiredUse).not.toHaveBeenCalled(); + }); + + it('still returns null when the expired-key notification fails', async () => { + mockApiKeySecurity.shouldNotifyExpiredKeyUse.mockRejectedValue( + new Error('redis down'), + ); + mockUserApiKeyRepo.findOne.mockResolvedValue({ + id: 'key-1', + name: 'ci-deploy', + hash: 'h', + expiresAt: new Date(Date.now() - 1000), + user: { id: 'user-1', username: 'luke', email: 'luke@example.com' }, + }); + + expect(await service.validateApiKey('kk_expired')).toBeNull(); + }); }); // --------------------------------------------------------------------------- diff --git a/backend/src/auth/auth.service.ts b/backend/src/auth/auth.service.ts index bb35c5c..2ac3586 100644 --- a/backend/src/auth/auth.service.ts +++ b/backend/src/auth/auth.service.ts @@ -19,6 +19,7 @@ import { UpdateProfileDto } from './dto/update-profile.dto'; import { BillingService } from '../billing/billing.service'; import { PLAN_LIMITS } from '../billing/constants/plan-limits'; import { EmailService } from '../notifications/email.service'; +import { ApiKeySecurityService } from './services/api-key-security.service'; import type { ApiKey, AuthCallbackResponse, @@ -45,6 +46,7 @@ export class AuthService implements OnModuleInit { private readonly tlsCrtRepo: Repository, private readonly billingService: BillingService, private readonly emailService: EmailService, + private readonly apiKeySecurity: ApiKeySecurityService, ) {} async onModuleInit() { @@ -321,18 +323,55 @@ export class AuthService implements OnModuleInit { * Validates an API key by hashing and looking up in the database. * * Returns the UserApiKey record with user relation if valid, null otherwise. + * Use of a correct-but-expired key triggers an owner notification (the only + * failure mode attributable to a specific key), rate-limited per key. */ - async validateApiKey(rawKey: string) { + async validateApiKey(rawKey: string, meta?: { ip?: string }) { const hash = this.hashKey(rawKey); const record = await this.userApiKeyRepo.findOne({ where: { hash }, relations: ['user'], }); if (!record) return null; - if (record.expiresAt && record.expiresAt < new Date()) return null; + if (record.expiresAt && record.expiresAt < new Date()) { + await this.notifyExpiredKeyUse(record, meta?.ip); + return null; + } return record; } + /** + * Notifies a key owner that their expired key was presented for auth — + * a signal the key is still deployed somewhere (or leaked). Never throws. + */ + private async notifyExpiredKeyUse( + record: UserApiKey, + ip?: string, + ): Promise { + try { + if (!record.user) return; + if (!(await this.apiKeySecurity.shouldNotifyExpiredKeyUse(record.id))) { + return; + } + this.logger.warn( + `Expired API key ${record.id} ("${record.name}") presented for user ${record.user.id}${ip ? ` from ${ip}` : ''}`, + ); + await this.emailService.sendApiKeyExpiredUse({ + userId: record.user.id, + username: record.user.username, + email: record.user.email, + keyId: record.id, + keyName: record.name, + ip, + }); + } catch (err) { + this.logger.error( + 'Failed to send expired-key-use notification', + err instanceof Error ? err.stack : err, + ); + } + } + // --- Profile Management --- async getFullProfile(userId: string): Promise { diff --git a/backend/src/auth/services/api-key-security.service.spec.ts b/backend/src/auth/services/api-key-security.service.spec.ts new file mode 100644 index 0000000..90986b0 --- /dev/null +++ b/backend/src/auth/services/api-key-security.service.spec.ts @@ -0,0 +1,102 @@ +import { ConfigService } from '@nestjs/config'; +import { ApiKeySecurityService } from './api-key-security.service'; + +describe('ApiKeySecurityService', () => { + let service: ApiKeySecurityService; + let mockRedis: Record; + + const configService = { + get: jest.fn((key: string, def?: string) => def), + } as unknown as ConfigService; + + beforeEach(() => { + service = new ApiKeySecurityService(configService); + mockRedis = { + exists: jest.fn().mockResolvedValue(0), + incr: jest.fn().mockResolvedValue(1), + expire: jest.fn().mockResolvedValue(1), + set: jest.fn().mockResolvedValue('OK'), + disconnect: jest.fn(), + }; + // Replace the lazy-connecting real client with a mock. + (service as any).redis.disconnect(); + (service as any).redis = mockRedis; + }); + + describe('isLockedOut', () => { + it('returns false when no lock exists', async () => { + expect(await service.isLockedOut('1.2.3.4')).toBe(false); + expect(mockRedis.exists).toHaveBeenCalledWith('lock:1.2.3.4'); + }); + + it('returns true when a lock exists', async () => { + mockRedis.exists.mockResolvedValue(1); + expect(await service.isLockedOut('1.2.3.4')).toBe(true); + }); + + it('returns false for empty IP without touching Redis', async () => { + expect(await service.isLockedOut('')).toBe(false); + expect(mockRedis.exists).not.toHaveBeenCalled(); + }); + + it('fails open on Redis errors', async () => { + mockRedis.exists.mockRejectedValue(new Error('down')); + expect(await service.isLockedOut('1.2.3.4')).toBe(false); + }); + }); + + describe('recordFailure', () => { + it('increments the counter and sets the window on first failure', async () => { + await service.recordFailure('1.2.3.4'); + expect(mockRedis.incr).toHaveBeenCalledWith('fail:1.2.3.4'); + expect(mockRedis.expire).toHaveBeenCalledWith('fail:1.2.3.4', 900); + expect(mockRedis.set).not.toHaveBeenCalled(); + }); + + it('does not re-arm the window on subsequent failures', async () => { + mockRedis.incr.mockResolvedValue(5); + await service.recordFailure('1.2.3.4'); + expect(mockRedis.expire).not.toHaveBeenCalled(); + }); + + it('activates a lockout at the threshold', async () => { + mockRedis.incr.mockResolvedValue(10); + await service.recordFailure('1.2.3.4'); + expect(mockRedis.set).toHaveBeenCalledWith( + 'lock:1.2.3.4', + '1', + 'EX', + 900, + 'NX', + ); + }); + + it('swallows Redis errors (fail open)', async () => { + mockRedis.incr.mockRejectedValue(new Error('down')); + await expect(service.recordFailure('1.2.3.4')).resolves.toBeUndefined(); + }); + }); + + describe('shouldNotifyExpiredKeyUse', () => { + it('returns true on first use within the dedup window', async () => { + expect(await service.shouldNotifyExpiredKeyUse('key-1')).toBe(true); + expect(mockRedis.set).toHaveBeenCalledWith( + 'expired-notice:key-1', + '1', + 'EX', + 86_400, + 'NX', + ); + }); + + it('returns false when already notified', async () => { + mockRedis.set.mockResolvedValue(null); + expect(await service.shouldNotifyExpiredKeyUse('key-1')).toBe(false); + }); + + it('fails closed (no notification) on Redis errors', async () => { + mockRedis.set.mockRejectedValue(new Error('down')); + expect(await service.shouldNotifyExpiredKeyUse('key-1')).toBe(false); + }); + }); +}); diff --git a/backend/src/auth/services/api-key-security.service.ts b/backend/src/auth/services/api-key-security.service.ts new file mode 100644 index 0000000..601fa22 --- /dev/null +++ b/backend/src/auth/services/api-key-security.service.ts @@ -0,0 +1,123 @@ +import { Injectable, Logger, OnModuleDestroy } from '@nestjs/common'; +import { ConfigService } from '@nestjs/config'; +import Redis from 'ioredis'; + +/** + * Brute-force protection for API key authentication. + * + * Failed API key validations cannot be attributed to a specific key (an + * unknown key hashes to nothing in the database), so failures are counted + * per client IP. When an IP crosses the failure threshold inside the + * counting window it is locked out of API key auth for the lockout period. + * + * The lockout check runs *before* the expensive scrypt hash, so a locked-out + * client also stops consuming hashing CPU. + * + * All Redis errors fail open: brute-force protection degrades to the global + * IP throttler rather than blocking legitimate authentication. + */ +@Injectable() +export class ApiKeySecurityService implements OnModuleDestroy { + private readonly logger = new Logger(ApiKeySecurityService.name); + private readonly redis: Redis; + private readonly failureThreshold: number; + private readonly windowSeconds: number; + private readonly lockoutSeconds: number; + private redisWarned = false; + + constructor(config: ConfigService) { + this.failureThreshold = parseInt( + config.get('KK_APIKEY_LOCKOUT_THRESHOLD', '10'), + ); + this.windowSeconds = parseInt( + config.get('KK_APIKEY_LOCKOUT_WINDOW_SEC', '900'), + ); + this.lockoutSeconds = parseInt( + config.get('KK_APIKEY_LOCKOUT_DURATION_SEC', '900'), + ); + + this.redis = new Redis({ + host: config.get('KK_BULLMQ_HOST', 'localhost'), + port: parseInt(config.get('KK_BULLMQ_PORT', '6379')), + password: config.get('KK_BULLMQ_PASSWORD', '') || undefined, + keyPrefix: 'apikey-sec:', + lazyConnect: true, + maxRetriesPerRequest: 1, + enableOfflineQueue: false, + }); + // Without a listener ioredis turns connection errors into uncaught + // exceptions. Log the first one; individual commands fail open below. + this.redis.on('error', (err) => { + if (!this.redisWarned) { + this.redisWarned = true; + this.logger.warn( + `Redis unavailable for API key lockout (failing open): ${err.message}`, + ); + } + }); + } + + onModuleDestroy() { + this.redis.disconnect(); + } + + /** True when this IP is currently locked out of API key authentication. */ + async isLockedOut(ip: string): Promise { + if (!ip) return false; + try { + return (await this.redis.exists(`lock:${ip}`)) === 1; + } catch { + return false; // fail open + } + } + + /** + * Records a failed API key validation for this IP and activates a lockout + * once the threshold is crossed. + */ + async recordFailure(ip: string): Promise { + if (!ip) return; + try { + const failKey = `fail:${ip}`; + const count = await this.redis.incr(failKey); + if (count === 1) { + await this.redis.expire(failKey, this.windowSeconds); + } + if (count >= this.failureThreshold) { + const activated = await this.redis.set( + `lock:${ip}`, + '1', + 'EX', + this.lockoutSeconds, + 'NX', + ); + if (activated === 'OK') { + this.logger.warn( + `API key lockout activated for ${ip}: ${count} failed validations within ${this.windowSeconds}s (locked for ${this.lockoutSeconds}s)`, + ); + } + } + } catch { + // fail open — the error listener above already logged connectivity loss + } + } + + /** + * Rate-limits "expired key used" owner notifications to one per key per day. + * Returns true when the caller should send the notification. + */ + async shouldNotifyExpiredKeyUse(keyId: string): Promise { + try { + const claimed = await this.redis.set( + `expired-notice:${keyId}`, + '1', + 'EX', + 86_400, + 'NX', + ); + return claimed === 'OK'; + } catch { + return false; // fail closed for notifications: no Redis, no dedup, no spam + } + } +} diff --git a/backend/src/auth/strategies/api-key.strategy.spec.ts b/backend/src/auth/strategies/api-key.strategy.spec.ts index 67b0211..ad2573e 100644 --- a/backend/src/auth/strategies/api-key.strategy.spec.ts +++ b/backend/src/auth/strategies/api-key.strategy.spec.ts @@ -1,15 +1,20 @@ -import { UnauthorizedException } from '@nestjs/common'; +import { HttpException, UnauthorizedException } from '@nestjs/common'; import { ApiKeyStrategy } from './api-key.strategy'; describe('ApiKeyStrategy', () => { let strategy: ApiKeyStrategy; let mockAuthService: Record; + let mockSecurity: Record; beforeEach(() => { mockAuthService = { validateApiKey: jest.fn(), }; - strategy = new ApiKeyStrategy(mockAuthService as any); + mockSecurity = { + isLockedOut: jest.fn().mockResolvedValue(false), + recordFailure: jest.fn().mockResolvedValue(undefined), + }; + strategy = new ApiKeyStrategy(mockAuthService as any, mockSecurity as any); }); describe('validate', () => { @@ -18,29 +23,54 @@ describe('ApiKeyStrategy', () => { mockAuthService.validateApiKey.mockResolvedValue(record); const req = { + ip: '10.0.0.1', headers: { authorization: 'Bearer kk_abc123' }, } as any; const result = await strategy.validate(req); expect(result).toEqual({ userId: 'user-1', apiKeyId: 'key-1' }); - expect(mockAuthService.validateApiKey).toHaveBeenCalledWith('kk_abc123'); + expect(mockAuthService.validateApiKey).toHaveBeenCalledWith('kk_abc123', { + ip: '10.0.0.1', + }); + expect(mockSecurity.recordFailure).not.toHaveBeenCalled(); }); - it('throws UnauthorizedException when API key is invalid', async () => { + it('throws UnauthorizedException and records failure when API key is invalid', async () => { mockAuthService.validateApiKey.mockResolvedValue(null); const req = { + ip: '10.0.0.1', headers: { authorization: 'Bearer kk_invalid' }, } as any; await expect(strategy.validate(req)).rejects.toThrow( UnauthorizedException, ); + expect(mockSecurity.recordFailure).toHaveBeenCalledWith('10.0.0.1'); + }); + + it('rejects locked-out IPs with 429 before any validation work', async () => { + mockSecurity.isLockedOut.mockResolvedValue(true); + + const req = { + ip: '10.0.0.9', + headers: { authorization: 'Bearer kk_whatever' }, + } as any; + + const err = await strategy.validate(req).then( + () => null, + (e: unknown) => e, + ); + expect(err).toBeInstanceOf(HttpException); + expect((err as HttpException).getStatus()).toBe(429); + expect(mockAuthService.validateApiKey).not.toHaveBeenCalled(); + expect(mockSecurity.recordFailure).not.toHaveBeenCalled(); }); it('returns null when no authorization header', async () => { const req = { headers: {} } as any; expect(await strategy.validate(req)).toBeNull(); + expect(mockSecurity.isLockedOut).not.toHaveBeenCalled(); }); it('returns null when authorization is not a kk_ token', async () => { diff --git a/backend/src/auth/strategies/api-key.strategy.ts b/backend/src/auth/strategies/api-key.strategy.ts index 580dc74..08cdee8 100644 --- a/backend/src/auth/strategies/api-key.strategy.ts +++ b/backend/src/auth/strategies/api-key.strategy.ts @@ -1,12 +1,21 @@ -import { Injectable, UnauthorizedException } from '@nestjs/common'; +import { + HttpException, + HttpStatus, + Injectable, + UnauthorizedException, +} from '@nestjs/common'; import { PassportStrategy } from '@nestjs/passport'; import { Strategy } from 'passport-custom'; import { Request } from 'express'; import { AuthService } from '../auth.service'; +import { ApiKeySecurityService } from '../services/api-key-security.service'; @Injectable() export class ApiKeyStrategy extends PassportStrategy(Strategy, 'api-key') { - constructor(private readonly authService: AuthService) { + constructor( + private readonly authService: AuthService, + private readonly apiKeySecurity: ApiKeySecurityService, + ) { super(); } @@ -20,9 +29,21 @@ export class ApiKeyStrategy extends PassportStrategy(Strategy, 'api-key') { return null; } + // Brute-force lockout check runs before any hashing work. + const ip = req.ip ?? ''; + if (await this.apiKeySecurity.isLockedOut(ip)) { + throw new HttpException( + 'Too many failed API key attempts. Try again later.', + HttpStatus.TOO_MANY_REQUESTS, + ); + } + const apiKey = authHeader.split(' ')[1]; - const record = await this.authService.validateApiKey(apiKey); - if (!record) throw new UnauthorizedException('Invalid API key'); + const record = await this.authService.validateApiKey(apiKey, { ip }); + if (!record) { + await this.apiKeySecurity.recordFailure(ip); + throw new UnauthorizedException('Invalid API key'); + } return { userId: record.user.id, diff --git a/backend/src/notifications/email.service.ts b/backend/src/notifications/email.service.ts index a251b0c..c75e33f 100644 --- a/backend/src/notifications/email.service.ts +++ b/backend/src/notifications/email.service.ts @@ -15,6 +15,7 @@ import { autoRenewalPausedTemplate, welcomeTemplate, activationReminderTemplate, + apiKeyExpiredUseTemplate, } from './templates'; import { User } from '../users/entities/user.entity'; import { NotificationType } from '@krakenkey/shared'; @@ -66,6 +67,15 @@ export interface ActivationReminderContext { email: string; } +export interface ApiKeyExpiredUseContext { + userId?: string; + username: string; + email: string; + keyId: string; + keyName: string; + ip?: string; +} + @Injectable() export class EmailService { private readonly logger = new Logger(EmailService.name); @@ -248,4 +258,16 @@ export class EmailService { planLimitReachedTemplate(ctx), ); } + + /** + * Security notification — intentionally not gated by notification + * preferences: owners should always learn their expired key is in use. + */ + async sendApiKeyExpiredUse(ctx: ApiKeyExpiredUseContext): Promise { + await this.send( + ctx.email, + `Security notice: expired API key "${ctx.keyName}" was used`, + apiKeyExpiredUseTemplate(ctx), + ); + } } diff --git a/backend/src/notifications/templates/index.ts b/backend/src/notifications/templates/index.ts index f934687..b1cab47 100644 --- a/backend/src/notifications/templates/index.ts +++ b/backend/src/notifications/templates/index.ts @@ -4,6 +4,7 @@ import type { PlanLimitReachedContext, WelcomeContext, ActivationReminderContext, + ApiKeyExpiredUseContext, } from '../email.service'; function escapeHtml(str: string): string { @@ -222,3 +223,23 @@ export function autoRenewalPausedTemplate(ctx: { username: string }): string { ].join(''), ); } + +export function apiKeyExpiredUseTemplate(ctx: ApiKeyExpiredUseContext): string { + return layout( + 'Expired API key was used', + [ + p( + `Hi ${ctx.username}, an expired API key on your account was just presented for authentication. The request was rejected, but this usually means the key is still configured somewhere — or has leaked.`, + ), + detail('Key name', ctx.keyName), + detail('Key ID', ctx.keyId), + ctx.ip ? detail('Source IP', ctx.ip) : '', + p( + 'If this was one of your own systems, update it to use a current key. If you do not recognize this activity, delete the key and review your account.', + ), + ``, + ].join(''), + ); +} From b9bc780b3fd8dbc7c05e06d37a90151715d5806d Mon Sep 17 00:00:00 2001 From: krakenhavoc Date: Mon, 6 Jul 2026 11:01:36 -0400 Subject: [PATCH 2/4] feat: idempotent cert issuance and correct retry semantics POST /certs/tls is now idempotent within a 15-minute window: requests are keyed by sha256(CSR) per user in Redis (reusing the BullMQ connection), duplicates return the original cert id/status, and an in-flight duplicate gets 409. Redis errors fail open. Failed or revoked certs do not block a fresh attempt. Issuance job failures are now classified: permanent errors (bad CSR, ACME policy rejections, CA rate limits) abort remaining retries via UnrecoverableError instead of burning attempts, while transient errors still retry with backoff. The cert is marked failed and the owner emailed once - when no retry will follow - instead of on every attempt, which previously sent up to three failure emails and flapped the status mid-retry. The failed-cert email no longer promises automatic retries. The dns-01 keyAuthorization is validated to be a 43-char base64url digest before it is handed to the DNS provider - defense in depth so a misbehaving ACME directory cannot inject arbitrary TXT content. Hooks skipped (no pre-commit/gitleaks in sandbox); eslint+tsc+jest run manually, all green. --- .../tls-crt-issuer.processor.spec.ts | 114 ++++++++++- .../processors/tls-crt-issuer.processor.ts | 95 ++++++--- .../strategies/acme-issuer.strategy.spec.ts | 20 +- .../tls/strategies/acme-issuer.strategy.ts | 9 + backend/src/certs/tls/tls.controller.ts | 6 + backend/src/certs/tls/tls.service.spec.ts | 107 +++++++++++ backend/src/certs/tls/tls.service.ts | 181 ++++++++++++++++-- backend/src/notifications/templates/index.ts | 2 +- 8 files changed, 485 insertions(+), 49 deletions(-) diff --git a/backend/src/certs/tls/processors/tls-crt-issuer.processor.spec.ts b/backend/src/certs/tls/processors/tls-crt-issuer.processor.spec.ts index d412ed1..511fdea 100644 --- a/backend/src/certs/tls/processors/tls-crt-issuer.processor.spec.ts +++ b/backend/src/certs/tls/processors/tls-crt-issuer.processor.spec.ts @@ -26,6 +26,7 @@ describe('CertIssuerConsumer', () => { }; beforeEach(() => { + jest.clearAllMocks(); // the metrics/email mocks are module-scoped mockTlsService = { findOneInternal: jest.fn().mockResolvedValue(mockCsrRecord), updateInternal: jest.fn().mockResolvedValue({}), @@ -203,9 +204,120 @@ describe('CertIssuerConsumer', () => { data: { certId: 1 }, } as any; + await expect(processor.process(job)).rejects.toThrow('string error'); + expect(mockTlsService.updateInternal).toHaveBeenCalledWith( + 1, + { crtPem: null, chainPem: null }, + 'failed', + ); + }); + }); + + describe('retry semantics', () => { + const userRecord = { + ...mockCsrRecord, + user: { id: 'user-1', username: 'luke', email: 'luke@example.com' }, + }; + + it('keeps in-progress status and stays quiet when a transient error will retry', async () => { + mockTlsService.findOneInternal.mockResolvedValue(userRecord); + mockAcme.issue.mockRejectedValue(new Error('DNS propagation timeout')); + + const job = { + name: 'tlsCertIssuance', + data: { certId: 1 }, + attemptsMade: 0, + opts: { attempts: 3 }, + } as any; + await expect(processor.process(job)).rejects.toThrow( - 'Unknown error processing certificate', + 'DNS propagation timeout', + ); + // No failure bookkeeping mid-retry: + expect(mockTlsService.updateInternal).not.toHaveBeenCalledWith( + 1, + expect.anything(), + 'failed', + ); + expect(mockEmailService.sendCertFailed).not.toHaveBeenCalled(); + expect( + (mockMetricsService.certIssuanceTotal.inc as jest.Mock).mock.calls, + ).not.toContainEqual([{ status: 'failed' }]); + }); + + it('marks failed and emails the owner once on the final transient attempt', async () => { + mockTlsService.findOneInternal.mockResolvedValue(userRecord); + mockAcme.issue.mockRejectedValue(new Error('DNS propagation timeout')); + + const job = { + name: 'tlsCertIssuance', + data: { certId: 1 }, + attemptsMade: 2, + opts: { attempts: 3 }, + } as any; + + const err = await processor.process(job).then( + () => null, + (e: unknown) => e, + ); + expect((err as Error).message).toBe('DNS propagation timeout'); + expect((err as Error).name).not.toBe('UnrecoverableError'); + expect(mockTlsService.updateInternal).toHaveBeenCalledWith( + 1, + { crtPem: null, chainPem: null }, + 'failed', + ); + expect(mockEmailService.sendCertFailed).toHaveBeenCalledTimes(1); + }); + + it('aborts retries via UnrecoverableError for permanent ACME rejections', async () => { + mockTlsService.findOneInternal.mockResolvedValue(userRecord); + mockAcme.issue.mockRejectedValue( + new Error( + 'Error creating new order :: too many certificates already issued for "example.com"', + ), + ); + + const job = { + name: 'tlsCertIssuance', + data: { certId: 1 }, + attemptsMade: 0, + opts: { attempts: 3 }, + } as any; + + const err = await processor.process(job).then( + () => null, + (e: unknown) => e, + ); + expect((err as Error).name).toBe('UnrecoverableError'); + expect(mockTlsService.updateInternal).toHaveBeenCalledWith( + 1, + { crtPem: null, chainPem: null }, + 'failed', + ); + expect(mockEmailService.sendCertFailed).toHaveBeenCalledTimes(1); + }); + + it('treats invalid CSR as permanent even with retries remaining', async () => { + mockTlsService.findOneInternal.mockResolvedValue({ + ...userRecord, + rawCsr: 'invalid-csr-no-pem', + }); + + const job = { + name: 'tlsCertIssuance', + data: { certId: 1 }, + attemptsMade: 0, + opts: { attempts: 3 }, + } as any; + + const err = await processor.process(job).then( + () => null, + (e: unknown) => e, ); + expect((err as Error).name).toBe('UnrecoverableError'); + expect((err as Error).message).toMatch(/CSR appears to be invalid/); + expect(mockEmailService.sendCertFailed).toHaveBeenCalledTimes(1); }); }); }); diff --git a/backend/src/certs/tls/processors/tls-crt-issuer.processor.ts b/backend/src/certs/tls/processors/tls-crt-issuer.processor.ts index 58370a9..014c69d 100644 --- a/backend/src/certs/tls/processors/tls-crt-issuer.processor.ts +++ b/backend/src/certs/tls/processors/tls-crt-issuer.processor.ts @@ -1,5 +1,5 @@ import { Processor, WorkerHost } from '@nestjs/bullmq'; -import { Job } from 'bullmq'; +import { Job, UnrecoverableError } from 'bullmq'; import { TlsService } from '../tls.service'; import { TlsCrt } from '../entities/tls-crt.entity'; import { InternalUpdateTlsCrtDto } from '../dto/update-tls-crt.dto'; @@ -17,13 +17,42 @@ import { EmailService } from '../../../notifications/email.service'; import type { CertEmailContext } from '../../../notifications/email.service'; import { User } from '../../../users/entities/user.entity'; +/** + * Error detail patterns that indicate a permanent failure — retrying cannot + * succeed (or, for CA rate limits, cannot succeed within the seconds-scale + * retry backoff). Anything not matched is treated as transient and retried. + * + * acme-client surfaces the ACME problem document's `detail` string as the + * Error message, so these match detail phrasing, not problem-type URNs. + */ +const PERMANENT_FAILURE_PATTERNS: RegExp[] = [ + // Our own pre-flight validation + /CSR appears to be invalid/i, + /Unexpected ACME keyAuthorization format/i, + /Unable to produce key authorization/i, + // ACME policy/authorization rejections (Let's Encrypt detail phrasing) + /refuses to issue/i, + /Cannot issue for/i, + /policy forbids issuing/i, + /CAA record/i, + /account.+deactivated/i, + // CA rate limits last hours-to-days; the 5-20s job backoff cannot outwait + // them, so fail fast and let the user retry deliberately later. + /too many certificates/i, + /rate ?limited/i, +]; + /** * Background job processor for certificate issuance and renewal. * * Handles both 'tlsCertIssuance' (new certificates) and 'tlsCertRenewal' jobs. * Processes ACME DNS-01 challenges using the configured DNS provider. * - * Job retries are configured in tls.service.ts (3 attempts, exponential backoff). + * Job retries are configured in tls.service.ts (3 attempts, exponential + * backoff). Only transient errors (DNS propagation, network, ACME 5xx) are + * retried; permanent errors abort retries via UnrecoverableError. The cert + * is marked failed and the user emailed once — when no retry will follow — + * instead of on every attempt. */ @Processor('tlsCertIssuance') export class CertIssuerConsumer extends WorkerHost { @@ -72,21 +101,16 @@ export class CertIssuerConsumer extends WorkerHost { csrRecord.parsedCsr?.extensions?.[0]?.altNames?.[0]?.value ?? `cert #${certId}`; - // Validate CSR format before attempting ACME - const raw = csrRecord.rawCsr ?? ''; - this.logger.debug(`Validating CSR format for cert #${certId}`); - if (!raw.includes('-----BEGIN') || !raw.includes('-----END')) { - await this.tlsService.updateInternal( - csrRecord.id, - { crtPem: null, chainPem: null }, - CertStatus.FAILED, - ); - throw new Error( - 'CSR appears to be invalid or empty (missing PEM delimiters)', - ); - } - try { + // Validate CSR format before attempting ACME + const raw = csrRecord.rawCsr ?? ''; + this.logger.debug(`Validating CSR format for cert #${certId}`); + if (!raw.includes('-----BEGIN') || !raw.includes('-----END')) { + throw new Error( + 'CSR appears to be invalid or empty (missing PEM delimiters)', + ); + } + const statusDuringProcess = isRenewal ? CertStatus.RENEWING : CertStatus.ISSUING; @@ -157,8 +181,28 @@ export class CertIssuerConsumer extends WorkerHost { return { success: true }; } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + const permanent = this.isPermanentFailure(message); + const attemptsAllowed = job.opts?.attempts ?? 1; + // attemptsMade is incremented after the attempt finishes, so during + // processing it equals the number of *previous* attempts. + const attemptNumber = (job.attemptsMade ?? 0) + 1; + const willRetry = !permanent && attemptNumber < attemptsAllowed; + + this.logger.error( + `Error ${isRenewal ? 'renewing' : 'issuing'} certificate #${certId} ` + + `(attempt ${attemptNumber}/${attemptsAllowed}, ` + + `${permanent ? 'permanent' : 'transient'}${willRetry ? ', will retry' : ''}): ${message}`, + ); + + if (willRetry) { + // Keep the in-progress status; BullMQ retries with backoff. Failure + // bookkeeping and the owner email happen only on the final attempt. + throw err instanceof Error ? err : new Error(message); + } + + // No retry will follow: record the failure and notify the owner once. this.metricsService.certIssuanceTotal.inc({ status: 'failed' }); - // Mark as failed and let BullMQ retry the job await this.tlsService.updateInternal( csrRecord.id, { crtPem: null, chainPem: null }, @@ -171,18 +215,19 @@ export class CertIssuerConsumer extends WorkerHost { email: csrRecord.user.email, certId, commonName, - errorMessage: err instanceof Error ? err.message : String(err), + errorMessage: message, }); } - if (err instanceof Error) { - this.logger.error( - `Error ${isRenewal ? 'renewing' : 'issuing'} certificate #${certId}: ${err.message}`, - ); - throw err; + if (permanent) { + // UnrecoverableError stops BullMQ from consuming remaining attempts. + throw new UnrecoverableError(message); } - this.logger.error(`Unknown error processing certificate #${certId}`, err); - throw new Error('Unknown error processing certificate'); + throw err instanceof Error ? err : new Error(message); } } + + private isPermanentFailure(message: string): boolean { + return PERMANENT_FAILURE_PATTERNS.some((p) => p.test(message)); + } } diff --git a/backend/src/certs/tls/strategies/acme-issuer.strategy.spec.ts b/backend/src/certs/tls/strategies/acme-issuer.strategy.spec.ts index 7179cda..64803a0 100644 --- a/backend/src/certs/tls/strategies/acme-issuer.strategy.spec.ts +++ b/backend/src/certs/tls/strategies/acme-issuer.strategy.spec.ts @@ -47,6 +47,9 @@ jest.mock('dns', () => ({ }, })); +// dns-01 keyAuthorization values are 43-char base64url SHA-256 digests +const KEY_AUTHZ = 'AAAAAAAAAAAAAAAAAAAAb2c3d4e5f6g7h8i9j0K-_23'; + describe('AcmeIssuerStrategy', () => { let strategy: AcmeIssuerStrategy; let mockDnsProvider: jest.Mocked; @@ -233,7 +236,7 @@ describe('AcmeIssuerStrategy', () => { .mockReturnValue({ commonName: 'example.com', altNames: [] }); mockClient.createOrder.mockResolvedValue(mockOrder); mockClient.getAuthorizations.mockResolvedValue([mockAuthz]); - mockClient.getChallengeKeyAuthorization.mockResolvedValue('key-authz'); + mockClient.getChallengeKeyAuthorization.mockResolvedValue(KEY_AUTHZ); mockClient.completeChallenge.mockResolvedValue({}); mockClient.waitForValidStatus.mockResolvedValue({}); mockClient.finalizeOrder.mockResolvedValue({}); @@ -244,7 +247,7 @@ describe('AcmeIssuerStrategy', () => { mockClient.getCertificate.mockResolvedValue(FAKE_CERT_PEM); // DNS resolves immediately - mockResolveTxt.mockResolvedValue([['key-authz']]); + mockResolveTxt.mockResolvedValue([[KEY_AUTHZ]]); }); it('issues a certificate through the full ACME flow', async () => { @@ -259,7 +262,7 @@ describe('AcmeIssuerStrategy', () => { }); expect(mockDnsProvider.createRecord).toHaveBeenCalledWith( '_acme-challenge.example.com', - 'key-authz', + KEY_AUTHZ, ); expect(mockClient.completeChallenge).toHaveBeenCalledWith( mockAuthz.challenges[0], @@ -300,6 +303,17 @@ describe('AcmeIssuerStrategy', () => { ); }); + it('refuses to publish a malformed keyAuthorization to DNS', async () => { + mockClient.getChallengeKeyAuthorization.mockResolvedValue( + 'not-a-digest; DROP TXT', + ); + + await expect( + strategy.issue(FAKE_CSR_PEM, mockDnsProvider), + ).rejects.toThrow(/Unexpected ACME keyAuthorization format/); + expect(mockDnsProvider.createRecord).not.toHaveBeenCalled(); + }); + it('throws when no DNS-01 challenge is found', async () => { mockClient.getAuthorizations.mockResolvedValue([ { diff --git a/backend/src/certs/tls/strategies/acme-issuer.strategy.ts b/backend/src/certs/tls/strategies/acme-issuer.strategy.ts index f2ad6cc..ac15f78 100644 --- a/backend/src/certs/tls/strategies/acme-issuer.strategy.ts +++ b/backend/src/certs/tls/strategies/acme-issuer.strategy.ts @@ -114,6 +114,15 @@ export class AcmeIssuerStrategy implements CertIssuerStrategy { // 1. The keyAuthorization is retrieved via getChallengeKeyAuthorization const keyAuthorization = await client.getChallengeKeyAuthorization(challenge); + // For dns-01 this is always a base64url-encoded SHA-256 digest + // (43 chars, RFC 8555 §8.4). Refuse to publish anything else to DNS — + // defense in depth against a misbehaving ACME directory injecting + // arbitrary content into our customers' TXT records. + if (!/^[A-Za-z0-9_-]{43}$/.test(keyAuthorization)) { + throw new Error( + `Unexpected ACME keyAuthorization format for ${domain}; refusing to publish DNS record`, + ); + } const recordName = `_acme-challenge.${domain}`; // 2. Create DNS Record diff --git a/backend/src/certs/tls/tls.controller.ts b/backend/src/certs/tls/tls.controller.ts index fe0e6e0..d5d982b 100644 --- a/backend/src/certs/tls/tls.controller.ts +++ b/backend/src/certs/tls/tls.controller.ts @@ -53,6 +53,12 @@ export class TlsController { status: 403, description: 'Viewers cannot request certificates', }) + @ApiResponse({ + status: 409, + description: + 'An identical certificate request (same CSR) is already being processed. ' + + 'Duplicate requests within 15 minutes return the original certificate instead of creating a new one.', + }) @Roles('owner', 'admin', 'member') @RateLimitCategoryDecorator(RateLimitCategory.EXPENSIVE) create( diff --git a/backend/src/certs/tls/tls.service.spec.ts b/backend/src/certs/tls/tls.service.spec.ts index a975ab4..7f14d53 100644 --- a/backend/src/certs/tls/tls.service.spec.ts +++ b/backend/src/certs/tls/tls.service.spec.ts @@ -295,6 +295,113 @@ describe('TlsService', () => { }); }); + // ─── create idempotency ─────────────────────────────────────────────── + describe('create idempotency', () => { + const createDto = { csrPem: 'valid-csr-pem' }; + let mockRedis: { set: jest.Mock; get: jest.Mock; del: jest.Mock }; + + beforeEach(() => { + mockRedis = { + set: jest.fn().mockResolvedValue('OK'), + get: jest.fn().mockResolvedValue(null), + del: jest.fn().mockResolvedValue(1), + }; + mockQueue.client = Promise.resolve(mockRedis); + + jest.spyOn(csrUtilService, 'validateAndParse').mockResolvedValue({ + raw: 'pem-data', + parsed: {} as ParsedCsr, + domains: ['example.com'], + publicKeyLength: 2048, + }); + jest + .spyOn(domainsService, 'findAllVerified') + .mockResolvedValue([ + { id: 1, hostname: 'example.com', isVerified: true, userId } as any, + ]); + jest.spyOn(csrUtilService, 'isAuthorized').mockReturnValue(undefined); + }); + + it('claims the idempotency key and stores the cert id on success', async () => { + const result = await service.create(userId, createDto); + + expect(result).toEqual({ id: 1, status: 'pending' }); + // First set: NX claim with pending marker; second set: cert id. + expect(mockRedis.set).toHaveBeenNthCalledWith( + 1, + expect.stringMatching(new RegExp(`^tls:idem:${userId}:[0-9a-f]{64}$`)), + '__pending__', + 'EX', + 900, + 'NX', + ); + expect(mockRedis.set).toHaveBeenNthCalledWith( + 2, + expect.stringMatching(new RegExp(`^tls:idem:${userId}:[0-9a-f]{64}$`)), + '1', + 'EX', + 900, + ); + }); + + it('replays the original response for a duplicate request', async () => { + mockRedis.set.mockResolvedValue(null); // claim fails + mockRedis.get.mockResolvedValue('42'); + mockRepository.findOneBy.mockResolvedValue({ id: 42, status: 'issuing' }); + + const result = await service.create(userId, createDto); + + expect(result).toEqual({ id: 42, status: 'issuing' }); + expect(mockRepository.save).not.toHaveBeenCalled(); + expect(mockQueue.add).not.toHaveBeenCalled(); + }); + + it('returns 409 when an identical request is still in flight', async () => { + mockRedis.set.mockResolvedValue(null); + mockRedis.get.mockResolvedValue('__pending__'); + + await expect(service.create(userId, createDto)).rejects.toMatchObject({ + status: 409, + }); + expect(mockRepository.save).not.toHaveBeenCalled(); + }); + + it('creates a fresh cert when the cached cert failed', async () => { + mockRedis.set + .mockResolvedValueOnce(null) // initial claim fails + .mockResolvedValueOnce('OK') // re-claim after stale key removal + .mockResolvedValue('OK'); // stores new cert id + mockRedis.get.mockResolvedValue('42'); + mockRepository.findOneBy.mockResolvedValue({ id: 42, status: 'failed' }); + + const result = await service.create(userId, createDto); + + expect(result).toEqual({ id: 1, status: 'pending' }); + expect(mockRedis.del).toHaveBeenCalled(); + expect(mockRepository.save).toHaveBeenCalled(); + }); + + it('releases the claim when creation fails', async () => { + mockQueue.add.mockRejectedValue(new Error('queue down')); + + await expect(service.create(userId, createDto)).rejects.toThrow( + 'queue down', + ); + expect(mockRedis.del).toHaveBeenCalledWith( + expect.stringMatching(new RegExp(`^tls:idem:${userId}:`)), + ); + }); + + it('proceeds without idempotency when Redis is unavailable', async () => { + mockRedis.set.mockRejectedValue(new Error('redis down')); + + const result = await service.create(userId, createDto); + + expect(result).toEqual({ id: 1, status: 'pending' }); + expect(mockRepository.save).toHaveBeenCalled(); + }); + }); + // ─── findOne ────────────────────────────────────────────────────────── describe('findOne', () => { it('returns cert when found', async () => { diff --git a/backend/src/certs/tls/tls.service.ts b/backend/src/certs/tls/tls.service.ts index c6dfbef..56c7f0b 100644 --- a/backend/src/certs/tls/tls.service.ts +++ b/backend/src/certs/tls/tls.service.ts @@ -2,10 +2,12 @@ import { Injectable, NotFoundException, BadRequestException, + ConflictException, InternalServerErrorException, HttpException, Logger, } from '@nestjs/common'; +import { createHash } from 'crypto'; import { CreateTlsCrtDto } from './dto/create-tls-crt.dto'; import { UpdateTlsCrtDto, @@ -46,6 +48,24 @@ import type { SubscriptionPlan } from '@krakenkey/shared'; * - failed: ACME challenge or issuance failed * - renewing: Certificate renewal in progress */ +/** How long a create request stays idempotent (seconds). */ +const IDEMPOTENCY_TTL_SECONDS = 15 * 60; +/** Placeholder stored while the original request is still being processed. */ +const IDEMPOTENCY_PENDING = '__pending__'; + +/** Minimal Redis surface used for idempotency (satisfied by BullMQ's client). */ +interface IdempotencyStore { + set( + key: string, + value: string, + ex: 'EX', + ttl: number, + nx?: 'NX', + ): Promise<'OK' | null>; + get(key: string): Promise; + del(key: string): Promise; +} + @Injectable() export class TlsService { private readonly logger = new Logger(TlsService.name); @@ -109,27 +129,150 @@ export class TlsService { this.csrUtilService.isAuthorized(csr.domains, allowedDomainNames); // This throws BadRequestException if any domain is unauthorized - // Plan-based limit checks - await this.enforceCertLimits(userId); + // Idempotency: a retried/duplicated request with the same CSR within the + // idempotency window returns the original cert instead of creating another. + const idemKey = this.idempotencyKey(userId, csr.raw); + const store = await this.getIdempotencyStore(); + if (store) { + const replay = await this.claimOrReplay(store, idemKey, userId); + if (replay) return replay; + } - const savedCsr = await this.TlsCrtRepository.save({ - rawCsr: csr.raw, - parsedCsr: csr.parsed, - status: CertStatus.PENDING, - userId, - }); - // Queue background job for ACME issuance - const jobPayload: TlsCertJobPayload = { certId: savedCsr.id }; - await this.tlsCertQueue.add('tlsCertIssuance', jobPayload, { - attempts: 3, - backoff: { type: 'exponential', delay: 5000 }, - }); + try { + // Plan-based limit checks + await this.enforceCertLimits(userId); + + const savedCsr = await this.TlsCrtRepository.save({ + rawCsr: csr.raw, + parsedCsr: csr.parsed, + status: CertStatus.PENDING, + userId, + }); + // Queue background job for ACME issuance + const jobPayload: TlsCertJobPayload = { certId: savedCsr.id }; + await this.tlsCertQueue.add('tlsCertIssuance', jobPayload, { + attempts: 3, + backoff: { type: 'exponential', delay: 5000 }, + }); - const response: CreateTlsCertResponse = { - id: savedCsr.id, - status: savedCsr.status, - }; - return response; + if (store) { + // Replace the pending marker with the cert id for replay lookups. + await store + .set(idemKey, String(savedCsr.id), 'EX', IDEMPOTENCY_TTL_SECONDS) + .catch((err: unknown) => + this.logger.warn( + `Failed to store idempotency result for ${idemKey}: ${err instanceof Error ? err.message : String(err)}`, + ), + ); + } + + const response: CreateTlsCertResponse = { + id: savedCsr.id, + status: savedCsr.status, + }; + return response; + } catch (err) { + // Release the idempotency claim so a corrected retry isn't blocked. + if (store) { + await store + .del(idemKey) + .catch(() => undefined /* key expires via TTL anyway */); + } + throw err; + } + } + + /** + * Attempts to claim the idempotency key for this request. + * + * Returns null when the claim succeeded (caller proceeds with creation). + * Returns the original create response when this is a replay of a recent + * identical request. Throws 409 when the original request is still in flight. + */ + private async claimOrReplay( + store: IdempotencyStore, + idemKey: string, + userId: string, + ): Promise { + try { + const claimed = await store.set( + idemKey, + IDEMPOTENCY_PENDING, + 'EX', + IDEMPOTENCY_TTL_SECONDS, + 'NX', + ); + if (claimed === 'OK') return null; + + const existing = await store.get(idemKey); + if (existing === IDEMPOTENCY_PENDING) { + throw new ConflictException( + 'An identical certificate request is already being processed. Retry shortly to get its result.', + ); + } + + if (existing) { + const cert = await this.TlsCrtRepository.findOneBy({ + id: Number(existing), + userId, + }); + // Replay only certs that are still progressing or issued; a failed, + // revoked, or deleted cert should not block a fresh attempt. + if ( + cert && + cert.status !== CertStatus.FAILED && + cert.status !== CertStatus.REVOKED + ) { + this.logger.log( + `Idempotent replay of cert #${cert.id} for duplicate create request (user ${userId})`, + ); + return { id: cert.id, status: cert.status }; + } + await store.del(idemKey); + const reclaimed = await store.set( + idemKey, + IDEMPOTENCY_PENDING, + 'EX', + IDEMPOTENCY_TTL_SECONDS, + 'NX', + ); + if (reclaimed !== 'OK') { + throw new ConflictException( + 'An identical certificate request is already being processed. Retry shortly to get its result.', + ); + } + } + return null; + } catch (err) { + if (err instanceof HttpException) throw err; + // Redis trouble must not block issuance — fail open without idempotency. + this.logger.warn( + `Idempotency check unavailable, proceeding without it: ${err instanceof Error ? err.message : String(err)}`, + ); + return null; + } + } + + private idempotencyKey(userId: string, rawCsr: string): string { + const csrHash = createHash('sha256') + .update(rawCsr.replace(/\s+/g, '')) + .digest('hex'); + return `tls:idem:${userId}:${csrHash}`; + } + + /** + * Reuses the BullMQ queue's Redis connection for idempotency bookkeeping. + * Returns null (disabling idempotency) when the connection is unavailable. + */ + private async getIdempotencyStore(): Promise { + try { + return (await this.tlsCertQueue.client) as unknown as IdempotencyStore; + } catch (err) { + this.logger.warn( + `Redis unavailable for idempotency: ${err instanceof Error ? err.message : String(err)}`, + ); + return null; + } } async findOne(id: number, userId: string) { diff --git a/backend/src/notifications/templates/index.ts b/backend/src/notifications/templates/index.ts index b1cab47..a38f5b9 100644 --- a/backend/src/notifications/templates/index.ts +++ b/backend/src/notifications/templates/index.ts @@ -99,7 +99,7 @@ export function certFailedTemplate(ctx: CertEmailContext): string { detail('Common Name', ctx.commonName), ctx.errorMessage ? detail('Error', ctx.errorMessage) : '', p( - 'The system will retry automatically. If this persists, please check your domain configuration.', + 'Automatic retries are exhausted. Please check your domain and DNS configuration, then retry the request from the dashboard.', ), ].join(''), ); From f26a1aa8102371d60de68e5db506c850bfc0f708 Mon Sep 17 00:00:00 2001 From: krakenhavoc Date: Mon, 6 Jul 2026 12:22:32 -0400 Subject: [PATCH 3/4] fix: boot without a Stripe key instead of crashing Stripe's constructor throws on an empty key, so a missing KK_STRIPE_SECRET_KEY prevented the entire app from starting - certs, auth, everything - even though only billing needs it. Local dev has no Stripe secret (LOCAL_DEV.md promises dummy defaults suffice), making local boot impossible. Warn and fall back to a placeholder key: billing calls fail with auth errors until configured, nothing else is affected. Same convention as the KK_HMAC_SECRET handling in AuthService. Found while manually testing the P1 hardening branch; unit-tested, hooks run separately (sandbox lacks pre-commit). --- backend/src/billing/billing.service.spec.ts | 14 ++++++++++++++ backend/src/billing/billing.service.ts | 16 ++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/backend/src/billing/billing.service.spec.ts b/backend/src/billing/billing.service.spec.ts index c12c1e0..4de7111 100644 --- a/backend/src/billing/billing.service.spec.ts +++ b/backend/src/billing/billing.service.spec.ts @@ -116,6 +116,20 @@ describe('BillingService', () => { expect(service).toBeDefined(); }); + it('boots without a Stripe key instead of crashing the app', () => { + // Local dev has no Stripe secret; construction must not throw. + expect( + () => + new BillingService( + {} as any, + {} as any, + {} as any, + {} as any, + { get: jest.fn(() => '') } as any, + ), + ).not.toThrow(); + }); + describe('resolveUserTier', () => { it('returns plan for active subscription', async () => { mockRepository.findOne.mockResolvedValue(mockSubscription); diff --git a/backend/src/billing/billing.service.ts b/backend/src/billing/billing.service.ts index c8db9ae..613617a 100644 --- a/backend/src/billing/billing.service.ts +++ b/backend/src/billing/billing.service.ts @@ -48,9 +48,21 @@ export class BillingService { private readonly dissolutionQueue: Queue, private readonly configService: ConfigService, ) { - this.stripe = new Stripe( - this.configService.get('KK_STRIPE_SECRET_KEY', ''), + // Stripe's constructor throws on an empty key, which would prevent the + // whole app from booting (certs, auth, everything) just because billing + // is unconfigured. Local dev runs without Stripe, so fall back to a + // placeholder and warn — billing calls will fail, nothing else will. + // Same convention as the KK_HMAC_SECRET handling in AuthService. + const stripeKey = this.configService.get( + 'KK_STRIPE_SECRET_KEY', + '', ); + if (!stripeKey) { + this.logger.warn( + 'KK_STRIPE_SECRET_KEY is not set — billing operations will fail until it is configured', + ); + } + this.stripe = new Stripe(stripeKey || 'sk_placeholder_billing_disabled'); this.webhookSecret = this.configService.get( 'KK_STRIPE_WEBHOOK_SECRET', From d3368473023e25bef7863add196a6289688ce7c1 Mon Sep 17 00:00:00 2001 From: krakenhavoc Date: Mon, 6 Jul 2026 12:51:59 -0400 Subject: [PATCH 4/4] fix: resolve real client IP behind Cloudflare for IP-keyed limits Production traffic is client -> Cloudflare -> Traefik -> app, but Express was configured with trust proxy: 1 (one hop), so req.ip resolved to a Cloudflare edge IP. Everything keyed on client IP - the tier-aware throttler's anonymous bucket and the new API-key lockout - was bucketing unrelated users behind shared edge IPs while letting a real attacker dilute counters across many edges. Trust an explicit proxy list instead: loopback/linklocal/uniquelocal (Traefik in docker/private nets) plus Cloudflare's published ranges (www.cloudflare.com/ips, fetched 2026-07-06), overridable via KK_TRUSTED_PROXIES. With an address list, a direct-to-origin caller is itself untrusted, so spoofed X-Forwarded-For headers are never consulted - a hop count of 2 would have allowed that spoof. Also stop keying the throttler on req.ips[0] (leftmost XFF entry = client-forgeable); req.ip is the trusted-chain resolution. Hooks skipped (no pre-commit in sandbox); tsc+eslint+jest green (458). --- backend/src/config/trusted-proxies.spec.ts | 27 +++++++ backend/src/config/trusted-proxies.ts | 77 +++++++++++++++++++ backend/src/main.ts | 12 ++- .../guards/tier-aware-throttler.guard.spec.ts | 6 +- .../guards/tier-aware-throttler.guard.ts | 7 +- 5 files changed, 123 insertions(+), 6 deletions(-) create mode 100644 backend/src/config/trusted-proxies.spec.ts create mode 100644 backend/src/config/trusted-proxies.ts diff --git a/backend/src/config/trusted-proxies.spec.ts b/backend/src/config/trusted-proxies.spec.ts new file mode 100644 index 0000000..df3448a --- /dev/null +++ b/backend/src/config/trusted-proxies.spec.ts @@ -0,0 +1,27 @@ +import { getTrustedProxies } from './trusted-proxies'; + +describe('getTrustedProxies', () => { + it('returns the built-in defaults when the env override is unset', () => { + const list = getTrustedProxies(undefined); + // Local/private proxies (Traefik) must always be present… + expect(list).toEqual( + expect.arrayContaining(['loopback', 'linklocal', 'uniquelocal']), + ); + // …alongside Cloudflare edges (spot-check well-known ranges). + expect(list).toEqual( + expect.arrayContaining(['104.16.0.0/13', '2606:4700::/32']), + ); + // And never a bare hop count. + expect(list.every((e) => typeof e === 'string')).toBe(true); + }); + + it('treats an empty env override as unset', () => { + expect(getTrustedProxies('')).toEqual(getTrustedProxies(undefined)); + expect(getTrustedProxies(' , ')).toEqual(getTrustedProxies(undefined)); + }); + + it('replaces the defaults with the env override when provided', () => { + const list = getTrustedProxies('loopback, 10.0.0.0/8'); + expect(list).toEqual(['loopback', '10.0.0.0/8']); + }); +}); diff --git a/backend/src/config/trusted-proxies.ts b/backend/src/config/trusted-proxies.ts new file mode 100644 index 0000000..1e83395 --- /dev/null +++ b/backend/src/config/trusted-proxies.ts @@ -0,0 +1,77 @@ +/** + * Trusted proxy configuration for Express's `trust proxy` setting. + * + * Why not `trust proxy: 1` (hop count): production traffic is + * client → Cloudflare → Traefik → app, i.e. two proxy hops. Trusting one + * hop resolves `req.ip` to a Cloudflare *edge* IP, so everything keyed on + * client IP (tier throttler, API-key lockout) buckets unrelated users + * together and lets an attacker dilute counters across edge IPs. Trusting + * a fixed hop count of two is also wrong: anyone who reaches the origin + * directly (bypassing Cloudflare) could then spoof X-Forwarded-For. + * + * Trusting an explicit address list solves both: `req.ip` becomes the + * first address (walking socket → XFF right-to-left) that is NOT a known + * proxy. A direct-to-origin caller's own address is untrusted, so their + * spoofed XFF entries are never consulted. + * + * Cloudflare ranges below are from https://www.cloudflare.com/ips + * (fetched 2026-07-06; they change rarely). Override the entire list with + * KK_TRUSTED_PROXIES (comma-separated CIDRs/addresses/keywords) if they + * rotate or the topology changes. + */ + +/** https://www.cloudflare.com/ips-v4 */ +const CLOUDFLARE_IPV4 = [ + '173.245.48.0/20', + '103.21.244.0/22', + '103.22.200.0/22', + '103.31.4.0/22', + '141.101.64.0/18', + '108.162.192.0/18', + '190.93.240.0/20', + '188.114.96.0/20', + '197.234.240.0/22', + '198.41.128.0/17', + '162.158.0.0/15', + '104.16.0.0/13', + '104.24.0.0/14', + '172.64.0.0/13', + '131.0.72.0/22', +]; + +/** https://www.cloudflare.com/ips-v6 */ +const CLOUDFLARE_IPV6 = [ + '2400:cb00::/32', + '2606:4700::/32', + '2803:f800::/32', + '2405:b500::/32', + '2405:8100::/32', + '2a06:98c0::/29', + '2c0f:f248::/32', +]; + +/** + * Default trust list: local/private addresses cover Traefik (docker + * networks in dev, private subnets in prod) plus Cloudflare's edges. + * 'loopback', 'linklocal' and 'uniquelocal' are express/proxy-addr + * keywords (uniquelocal = RFC 1918 + fc00::/7). + */ +const DEFAULT_TRUSTED_PROXIES = [ + 'loopback', + 'linklocal', + 'uniquelocal', + ...CLOUDFLARE_IPV4, + ...CLOUDFLARE_IPV6, +]; + +/** + * Resolves the trusted proxy list, honouring the KK_TRUSTED_PROXIES + * override (comma-separated). Empty/unset env → built-in defaults. + */ +export function getTrustedProxies(env?: string): string[] { + const fromEnv = (env ?? '') + .split(',') + .map((s) => s.trim()) + .filter(Boolean); + return fromEnv.length > 0 ? fromEnv : DEFAULT_TRUSTED_PROXIES; +} diff --git a/backend/src/main.ts b/backend/src/main.ts index ec3d47d..4accbce 100644 --- a/backend/src/main.ts +++ b/backend/src/main.ts @@ -9,6 +9,7 @@ import { Request, Response, NextFunction } from 'express'; import helmet from 'helmet'; import cookieParser from 'cookie-parser'; import { createSwaggerConfig } from './config/swagger.config'; +import { getTrustedProxies } from './config/trusted-proxies'; async function bootstrap() { const app = await NestFactory.create(AppModule, { rawBody: true }); @@ -16,9 +17,16 @@ async function bootstrap() { app.use(helmet()); app.use(cookieParser()); - // Trust the first proxy hop so req.ip returns the real client IP + // Trust the known proxy chain (Traefik + Cloudflare) so req.ip resolves + // to the real client address. A hop count would either expose a + // Cloudflare edge IP as the "client" (breaking IP-keyed throttling and + // API-key lockout) or allow XFF spoofing for direct-to-origin requests. + // See src/config/trusted-proxies.ts; override with KK_TRUSTED_PROXIES. const expressApp = app.getHttpAdapter().getInstance(); - expressApp.set('trust proxy', 1); + expressApp.set( + 'trust proxy', + getTrustedProxies(process.env.KK_TRUSTED_PROXIES), + ); // Always generate the OpenAPI document so /swagger-json is available for the // public docs site (Scalar viewer at /docs/api). diff --git a/backend/src/throttler/guards/tier-aware-throttler.guard.spec.ts b/backend/src/throttler/guards/tier-aware-throttler.guard.spec.ts index 09dc178..f437dfa 100644 --- a/backend/src/throttler/guards/tier-aware-throttler.guard.spec.ts +++ b/backend/src/throttler/guards/tier-aware-throttler.guard.spec.ts @@ -82,12 +82,14 @@ describe('TierAwareThrottlerGuard', () => { expect(tracker).toBe('5.6.7.8'); }); - it('should use first forwarded IP when ips array exists', async () => { + it('ignores the spoofable ips array and keys on req.ip', async () => { + // req.ips[0] is the leftmost X-Forwarded-For entry, which a client + // can forge; req.ip resolves through the trusted proxy chain. const req = { headers: {}, ips: ['10.0.0.1', '10.0.0.2'], ip: '1.2.3.4' }; const tracker = await (guard as any).getTracker(req); - expect(tracker).toBe('10.0.0.1'); + expect(tracker).toBe('1.2.3.4'); }); it('should fall back to IP on malformed JWT', async () => { diff --git a/backend/src/throttler/guards/tier-aware-throttler.guard.ts b/backend/src/throttler/guards/tier-aware-throttler.guard.ts index 85b2dca..951ff88 100644 --- a/backend/src/throttler/guards/tier-aware-throttler.guard.ts +++ b/backend/src/throttler/guards/tier-aware-throttler.guard.ts @@ -45,8 +45,11 @@ export class TierAwareThrottlerGuard extends ThrottlerGuard { return `user:${userId}`; } - // Fall back to client IP - return req.ips?.length ? req.ips[0] : req.ip; + // Fall back to client IP. req.ip already resolves the real client + // through the trusted proxy chain (see src/config/trusted-proxies.ts); + // req.ips[0] would trust the leftmost X-Forwarded-For entry, which a + // client can forge, so it must not be used for rate-limit keying. + return req.ip; } /**