From 71c7b8502632ba8a0a305fa8425eb0f1b61f42a4 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 23 Apr 2026 22:45:35 -0500 Subject: [PATCH] fix(federation/auth-guard): remediate CRIT-1/CRIT-2 + HIGH-1..4 review findings - CRIT-1: Validate cert subjectUserId against grant.subjectUserId from DB; use authoritative DB value in FederationContext - CRIT-2: Add @Inject(GrantsService) decorator (tsx/esbuild requirement) - HIGH-1: Validate UTF8String TLV tag, length, and bounds in OID parser - HIGH-2: Collapse all 403 wire messages to a generic string to prevent grant enumeration; keep internal logger detail - HIGH-3: Assert federation wire envelope shape in all guard tests - HIGH-4: Regression test for subjectUserId cert/DB mismatch Co-Authored-By: Claude Sonnet 4.6 --- apps/gateway/src/federation/oid.util.ts | 32 +++++- .../__tests__/federation-auth.guard.spec.ts | 103 ++++++++++++++++-- .../server/federation-auth.guard.ts | 43 ++++---- 3 files changed, 146 insertions(+), 32 deletions(-) diff --git a/apps/gateway/src/federation/oid.util.ts b/apps/gateway/src/federation/oid.util.ts index da9cee4..3cf9e5a 100644 --- a/apps/gateway/src/federation/oid.util.ts +++ b/apps/gateway/src/federation/oid.util.ts @@ -50,10 +50,40 @@ const decoder = new TextDecoder(); /** * Decode an extension value encoded as ASN.1 UTF8String TLV * (tag 0x0C + 1-byte length + UTF-8 bytes). - * Slices off the 2-byte TLV header and decodes the remainder as UTF-8. + * Validates tag, length byte, and buffer bounds before decoding. + * Throws a descriptive Error on malformed input; caller wraps in try/catch. */ function decodeUtf8StringTlv(value: ArrayBuffer): string { const bytes = new Uint8Array(value); + + // Need at least tag + length bytes + if (bytes.length < 2) { + throw new Error(`UTF8String TLV too short: expected at least 2 bytes, got ${bytes.length}`); + } + + // Tag byte must be 0x0C (ASN.1 UTF8String) + if (bytes[0] !== 0x0c) { + throw new Error( + `UTF8String TLV tag mismatch: expected 0x0C, got 0x${bytes[0]!.toString(16).toUpperCase()}`, + ); + } + + // Only single-byte length form is supported (values 0–127); long form not needed + // for OID strings of this length. + const declaredLength = bytes[1]!; + if (declaredLength > 127) { + throw new Error( + `UTF8String TLV uses long-form length (0x${declaredLength.toString(16).toUpperCase()}), which is not supported`, + ); + } + + // Declared length must match actual remaining bytes + if (declaredLength !== bytes.length - 2) { + throw new Error( + `UTF8String TLV length mismatch: declared ${declaredLength}, actual ${bytes.length - 2}`, + ); + } + // Skip: tag (1 byte) + length (1 byte) return decoder.decode(bytes.slice(2)); } diff --git a/apps/gateway/src/federation/server/__tests__/federation-auth.guard.spec.ts b/apps/gateway/src/federation/server/__tests__/federation-auth.guard.spec.ts index a8454e9..a289d9f 100644 --- a/apps/gateway/src/federation/server/__tests__/federation-auth.guard.spec.ts +++ b/apps/gateway/src/federation/server/__tests__/federation-auth.guard.spec.ts @@ -162,7 +162,7 @@ describe('FederationAuthGuard', () => { // ── 401: No TLS socket ──────────────────────────────────────────────────── it('returns 401 when there is no TLS socket (plain HTTP connection)', async () => { - const { ctx, statusMock } = makeContext({ + const { ctx, statusMock, sendMock } = makeContext({ certPem: certPem, hasTlsSocket: false, }); @@ -172,18 +172,28 @@ describe('FederationAuthGuard', () => { expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(401); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }), + }), + ); }); // ── 401: Cert not presented ─────────────────────────────────────────────── it('returns 401 when the peer did not present a certificate', async () => { - const { ctx, statusMock } = makeContext({ certPem: null }); + const { ctx, statusMock, sendMock } = makeContext({ certPem: null }); const guard = new FederationAuthGuard(makeGrantsService()); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(401); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }), + }), + ); }); // ── 401: Cert parse failure ─────────────────────────────────────────────── @@ -191,13 +201,18 @@ describe('FederationAuthGuard', () => { it('returns 401 when the certificate DER bytes are corrupt', async () => { // Build context with a cert that has garbage DER bytes const corruptPem = '-----BEGIN CERTIFICATE-----\naW52YWxpZA==\n-----END CERTIFICATE-----'; - const { ctx, statusMock } = makeContext({ certPem: corruptPem }); + const { ctx, statusMock, sendMock } = makeContext({ certPem: corruptPem }); const guard = new FederationAuthGuard(makeGrantsService()); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(401); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }), + }), + ); }); // ── 401: Missing grantId OID ───────────────────────────────────────────── @@ -206,13 +221,18 @@ describe('FederationAuthGuard', () => { // makeSelfSignedCert produces a cert without any Mosaic OIDs const { makeSelfSignedCert } = await import('../../__tests__/helpers/test-cert.js'); const plainCert = await makeSelfSignedCert(); - const { ctx, statusMock } = makeContext({ certPem: plainCert }); + const { ctx, statusMock, sendMock } = makeContext({ certPem: plainCert }); const guard = new FederationAuthGuard(makeGrantsService()); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(401); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }), + }), + ); }); // ── 401: Missing subjectUserId OID ─────────────────────────────────────── @@ -257,13 +277,18 @@ describe('FederationAuthGuard', () => { ], }); - const { ctx, statusMock } = makeContext({ certPem: cert.toString('pem') }); + const { ctx, statusMock, sendMock } = makeContext({ certPem: cert.toString('pem') }); const guard = new FederationAuthGuard(makeGrantsService()); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(401); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'unauthorized', message: expect.any(String) }), + }), + ); }); // ── 403: Grant not found ───────────────────────────────────────────────── @@ -275,13 +300,18 @@ describe('FederationAuthGuard', () => { .mockRejectedValue(new NotFoundException(`Grant ${GRANT_ID} not found`)), }); - const { ctx, statusMock } = makeContext({ certPem }); + const { ctx, statusMock, sendMock } = makeContext({ certPem }); const guard = new FederationAuthGuard(grantsService); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(403); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }), + }), + ); }); // ── 403: Grant in `pending` status ─────────────────────────────────────── @@ -291,13 +321,18 @@ describe('FederationAuthGuard', () => { getGrantWithPeer: vi.fn().mockResolvedValue(makeGrantWithPeer({ status: 'pending' })), }); - const { ctx, statusMock } = makeContext({ certPem }); + const { ctx, statusMock, sendMock } = makeContext({ certPem }); const guard = new FederationAuthGuard(grantsService); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(403); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }), + }), + ); }); // ── 403: Grant in `revoked` status ─────────────────────────────────────── @@ -309,13 +344,18 @@ describe('FederationAuthGuard', () => { .mockResolvedValue(makeGrantWithPeer({ status: 'revoked', revokedAt: new Date() })), }); - const { ctx, statusMock } = makeContext({ certPem }); + const { ctx, statusMock, sendMock } = makeContext({ certPem }); const guard = new FederationAuthGuard(grantsService); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(403); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }), + }), + ); }); // ── 403: Grant in `expired` status ─────────────────────────────────────── @@ -325,13 +365,18 @@ describe('FederationAuthGuard', () => { getGrantWithPeer: vi.fn().mockResolvedValue(makeGrantWithPeer({ status: 'expired' })), }); - const { ctx, statusMock } = makeContext({ certPem }); + const { ctx, statusMock, sendMock } = makeContext({ certPem }); const guard = new FederationAuthGuard(grantsService); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(403); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }), + }), + ); }); // ── 403: Cert serial mismatch ───────────────────────────────────────────── @@ -360,13 +405,51 @@ describe('FederationAuthGuard', () => { }); // Context presents cert with serial '01' but DB has 'DEADBEEF' - const { ctx, statusMock } = makeContext({ certPem, certSerialHex: '01' }); + const { ctx, statusMock, sendMock } = makeContext({ certPem, certSerialHex: '01' }); const guard = new FederationAuthGuard(grantsService); const result = await guard.canActivate(ctx); expect(result).toBe(false); expect(statusMock).toHaveBeenCalledWith(403); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }), + }), + ); + }); + + // ── 403: subjectUserId cert/DB mismatch (CRIT-1 regression test) ───────── + + it('returns 403 when the cert subjectUserId does not match the DB grant subjectUserId', async () => { + // Build a cert that claims an attacker's subjectUserId + const attackerSubjectUserId = 'attacker-user-id'; + const attackerCertPem = await makeMosaicIssuedCert({ + grantId: GRANT_ID, + subjectUserId: attackerSubjectUserId, + }); + + // DB returns a grant with the legitimate USER_ID + const grantsService = makeGrantsService({ + getGrantWithPeer: vi.fn().mockResolvedValue(makeGrantWithPeer({ subjectUserId: USER_ID })), + }); + + // Cert presents attacker-user-id but DB has USER_ID — should be rejected + const { ctx, statusMock, sendMock } = makeContext({ + certPem: attackerCertPem, + certSerialHex: CERT_SERIAL_HEX, + }); + + const guard = new FederationAuthGuard(grantsService); + const result = await guard.canActivate(ctx); + + expect(result).toBe(false); + expect(statusMock).toHaveBeenCalledWith(403); + expect(sendMock).toHaveBeenCalledWith( + expect.objectContaining({ + error: expect.objectContaining({ code: 'forbidden', message: 'Federation access denied' }), + }), + ); }); // ── Happy path ──────────────────────────────────────────────────────────── diff --git a/apps/gateway/src/federation/server/federation-auth.guard.ts b/apps/gateway/src/federation/server/federation-auth.guard.ts index feb04fe..96bb334 100644 --- a/apps/gateway/src/federation/server/federation-auth.guard.ts +++ b/apps/gateway/src/federation/server/federation-auth.guard.ts @@ -39,7 +39,13 @@ * TLS layer (Node.js `rejectUnauthorized: true` with the CA cert pinned). */ -import { type CanActivate, type ExecutionContext, Injectable, Logger } from '@nestjs/common'; +import { + type CanActivate, + type ExecutionContext, + Inject, + Injectable, + Logger, +} from '@nestjs/common'; import type { FastifyReply, FastifyRequest } from 'fastify'; import * as tls from 'node:tls'; import { X509Certificate } from '@peculiar/x509'; @@ -74,7 +80,7 @@ function sendFederationError( export class FederationAuthGuard implements CanActivate { private readonly logger = new Logger(FederationAuthGuard.name); - constructor(private readonly grantsService: GrantsService) {} + constructor(@Inject(GrantsService) private readonly grantsService: GrantsService) {} async canActivate(context: ExecutionContext): Promise { const http = context.switchToHttp(); @@ -146,16 +152,20 @@ export class FederationAuthGuard implements CanActivate { } catch { // getGrantWithPeer throws NotFoundException when not found this.logger.warn(`Grant not found: ${grantId}`); - return sendFederationError(reply, new FederationForbiddenError(`Grant ${grantId} not found`)); + return sendFederationError(reply, new FederationForbiddenError('Federation access denied')); } // ── Step 5: Assert grant is active ────────────────────────────────────── if (grant.status !== 'active') { this.logger.warn(`Grant ${grantId} is not active — status=${grant.status}`); - return sendFederationError( - reply, - new FederationForbiddenError(`Grant ${grantId} is not active (status: ${grant.status})`), - ); + return sendFederationError(reply, new FederationForbiddenError('Federation access denied')); + } + + // ── Step 5b: Validate cert-extracted subjectUserId against DB (CRIT-1) ── + // The cert claim is untrusted input; the DB row is authoritative. + if (subjectUserId !== grant.subjectUserId) { + this.logger.warn(`subjectUserId mismatch for grant ${grantId}`); + return sendFederationError(reply, new FederationForbiddenError('Federation access denied')); } // ── Step 6: Defense-in-depth — cert serial must match registered peer ─── @@ -168,12 +178,7 @@ export class FederationAuthGuard implements CanActivate { if (!grant.peer.certSerial) { // Peer row exists but has no stored serial — something is wrong with enrollment this.logger.error(`Peer ${grant.peerId} has no stored certSerial — enrollment incomplete`); - return sendFederationError( - reply, - new FederationForbiddenError( - 'Peer registration incomplete — no certificate serial on record', - ), - ); + return sendFederationError(reply, new FederationForbiddenError('Federation access denied')); } // Normalize both to uppercase for comparison (Node.js serialNumber is @@ -184,18 +189,14 @@ export class FederationAuthGuard implements CanActivate { `Cert serial mismatch for grant ${grantId}: ` + `inbound=${inboundSerial} registered=${grant.peer.certSerial}`, ); - return sendFederationError( - reply, - new FederationForbiddenError( - 'Client certificate serial does not match registered peer certificate', - ), - ); + return sendFederationError(reply, new FederationForbiddenError('Federation access denied')); } // ── Step 7: Attach FederationContext to request ────────────────────────── + // Use grant.subjectUserId from DB (authoritative) — not the cert-extracted value. const federationContext: FederationContext = { grantId, - subjectUserId, + subjectUserId: grant.subjectUserId, peerId: grant.peerId, scope: grant.scope as Record, }; @@ -203,7 +204,7 @@ export class FederationAuthGuard implements CanActivate { request.federationContext = federationContext; this.logger.debug( - `Federation auth OK — grantId=${grantId} peerId=${grant.peerId} subjectUserId=${subjectUserId}`, + `Federation auth OK — grantId=${grantId} peerId=${grant.peerId} subjectUserId=${grant.subjectUserId}`, ); return true;