fix(federation/auth-guard): remediate CRIT-1/CRIT-2 + HIGH-1..4 review findings
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
ci/woodpecker/push/ci Pipeline was successful

- 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 <noreply@anthropic.com>
This commit is contained in:
Jarvis
2026-04-23 22:45:35 -05:00
parent b01c9b3bb0
commit 0af3e218a1
3 changed files with 146 additions and 32 deletions

View File

@@ -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<boolean> {
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<string, unknown>,
};
@@ -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;