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/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', 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/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/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..a38f5b9 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 { @@ -98,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(''), ); @@ -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(''), + ); +} 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; } /**