fix(federation): security hardening — OID verification, atomic activation, audit on failure
CRIT-1: Add post-issuance OID verification in CaService.issueCert() — parses
the returned cert with @peculiar/x509 and validates that OIDs 1.3.6.1.4.1.99999.1
(mosaic_grant_id) and 1.3.6.1.4.1.99999.2 (mosaic_subject_user_id) are present
and match the request values. Throws CaServiceError on mismatch or absence.
CRIT-2: Guard grant activation in the redeem() transaction with
WHERE status='pending' (RETURNING to detect no-op). Throw ConflictException
if the grant was already activated. Also add WHERE state='pending' guard on
the federationPeers UPDATE.
HIGH-2: Remove 90-day silent fallback in extractCertNotAfter() — an unparseable
cert now propagates as a 500 error rather than silently setting a wrong expiry.
HIGH-4: Log only the first 8 hex chars of the enrollment token in the issueCert
failure error log — never log the full 64-char token.
HIGH-5: Wrap redeem() body in try/catch; write a best-effort failure audit row
(outside transaction, .catch(() => {}) guarded) on any error path so all
enrollment attempts are audited regardless of outcome.
MED-3: Verify grantId ↔ peerId binding in createToken() before inserting the
token — prevents cross-wiring a grant to an attacker-controlled peer.
Closes #461
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -35,7 +35,7 @@ import * as crypto from 'node:crypto';
|
|||||||
import * as fs from 'node:fs';
|
import * as fs from 'node:fs';
|
||||||
import * as https from 'node:https';
|
import * as https from 'node:https';
|
||||||
import { SignJWT, importJWK } from 'jose';
|
import { SignJWT, importJWK } from 'jose';
|
||||||
import { Pkcs10CertificateRequest } from '@peculiar/x509';
|
import { Pkcs10CertificateRequest, X509Certificate } from '@peculiar/x509';
|
||||||
import type { IssueCertRequestDto } from './ca.dto.js';
|
import type { IssueCertRequestDto } from './ca.dto.js';
|
||||||
import { IssuedCertDto } from './ca.dto.js';
|
import { IssuedCertDto } from './ca.dto.js';
|
||||||
|
|
||||||
@@ -624,6 +624,51 @@ export class CaService {
|
|||||||
|
|
||||||
const serialNumber = extractSerial(response.crt);
|
const serialNumber = extractSerial(response.crt);
|
||||||
|
|
||||||
|
// CRIT-1: Verify the issued certificate contains both Mosaic OID extensions
|
||||||
|
// with the correct values. Step-CA's federation.tpl encodes each as an ASN.1
|
||||||
|
// UTF8String TLV: tag 0x0C + 1-byte length + UUID bytes. We skip 2 bytes
|
||||||
|
// (tag + length) to extract the raw UUID string.
|
||||||
|
const issuedCert = new X509Certificate(response.crt);
|
||||||
|
const decoder = new TextDecoder();
|
||||||
|
|
||||||
|
const grantIdExt = issuedCert.getExtension('1.3.6.1.4.1.99999.1');
|
||||||
|
if (!grantIdExt) {
|
||||||
|
throw new CaServiceError(
|
||||||
|
'Issued certificate is missing required Mosaic OID: mosaic_grant_id',
|
||||||
|
'The Step-CA federation.tpl template did not embed OID 1.3.6.1.4.1.99999.1. Check the provisioner template configuration.',
|
||||||
|
undefined,
|
||||||
|
'OID_MISSING',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
const grantIdInCert = decoder.decode(grantIdExt.value.slice(2));
|
||||||
|
if (grantIdInCert !== req.grantId) {
|
||||||
|
throw new CaServiceError(
|
||||||
|
`Issued certificate mosaic_grant_id mismatch: expected ${req.grantId}, got ${grantIdInCert}`,
|
||||||
|
'The Step-CA issued a certificate with a different grant ID than requested. This may indicate a provisioner misconfiguration or a MITM.',
|
||||||
|
undefined,
|
||||||
|
'OID_MISMATCH',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
const subjectUserIdExt = issuedCert.getExtension('1.3.6.1.4.1.99999.2');
|
||||||
|
if (!subjectUserIdExt) {
|
||||||
|
throw new CaServiceError(
|
||||||
|
'Issued certificate is missing required Mosaic OID: mosaic_subject_user_id',
|
||||||
|
'The Step-CA federation.tpl template did not embed OID 1.3.6.1.4.1.99999.2. Check the provisioner template configuration.',
|
||||||
|
undefined,
|
||||||
|
'OID_MISSING',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
const subjectUserIdInCert = decoder.decode(subjectUserIdExt.value.slice(2));
|
||||||
|
if (subjectUserIdInCert !== req.subjectUserId) {
|
||||||
|
throw new CaServiceError(
|
||||||
|
`Issued certificate mosaic_subject_user_id mismatch: expected ${req.subjectUserId}, got ${subjectUserIdInCert}`,
|
||||||
|
'The Step-CA issued a certificate with a different subject user ID than requested. This may indicate a provisioner misconfiguration or a MITM.',
|
||||||
|
undefined,
|
||||||
|
'OID_MISMATCH',
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
this.logger.log(`Certificate issued — serial=${serialNumber} grantId=${req.grantId}`);
|
this.logger.log(`Certificate issued — serial=${serialNumber} grantId=${req.grantId}`);
|
||||||
|
|
||||||
const result = new IssuedCertDto();
|
const result = new IssuedCertDto();
|
||||||
|
|||||||
@@ -14,6 +14,7 @@
|
|||||||
|
|
||||||
import {
|
import {
|
||||||
BadRequestException,
|
BadRequestException,
|
||||||
|
ConflictException,
|
||||||
GoneException,
|
GoneException,
|
||||||
Inject,
|
Inject,
|
||||||
Injectable,
|
Injectable,
|
||||||
@@ -66,6 +67,21 @@ export class EnrollmentService {
|
|||||||
*/
|
*/
|
||||||
async createToken(dto: CreateEnrollmentTokenDto): Promise<EnrollmentTokenResult> {
|
async createToken(dto: CreateEnrollmentTokenDto): Promise<EnrollmentTokenResult> {
|
||||||
const ttl = Math.min(dto.ttlSeconds, 900);
|
const ttl = Math.min(dto.ttlSeconds, 900);
|
||||||
|
|
||||||
|
// MED-3: Verify the grantId ↔ peerId binding — prevents attacker from
|
||||||
|
// cross-wiring grants to attacker-controlled peers.
|
||||||
|
const [grant] = await this.db
|
||||||
|
.select({ peerId: federationGrants.peerId })
|
||||||
|
.from(federationGrants)
|
||||||
|
.where(eq(federationGrants.id, dto.grantId))
|
||||||
|
.limit(1);
|
||||||
|
if (!grant) {
|
||||||
|
throw new NotFoundException(`Grant ${dto.grantId} not found`);
|
||||||
|
}
|
||||||
|
if (grant.peerId !== dto.peerId) {
|
||||||
|
throw new BadRequestException(`peerId does not match the grant's registered peer`);
|
||||||
|
}
|
||||||
|
|
||||||
const token = crypto.randomBytes(32).toString('hex');
|
const token = crypto.randomBytes(32).toString('hex');
|
||||||
const expiresAt = new Date(Date.now() + ttl * 1000);
|
const expiresAt = new Date(Date.now() + ttl * 1000);
|
||||||
|
|
||||||
@@ -99,132 +115,167 @@ export class EnrollmentService {
|
|||||||
* 8. Return { certPem, certChainPem }
|
* 8. Return { certPem, certChainPem }
|
||||||
*/
|
*/
|
||||||
async redeem(token: string, csrPem: string): Promise<RedeemResult> {
|
async redeem(token: string, csrPem: string): Promise<RedeemResult> {
|
||||||
// 1. Fetch token row
|
// HIGH-5: Track outcome so we can write a failure audit row on any error.
|
||||||
const [row] = await this.db
|
let outcome: 'allowed' | 'denied' = 'denied';
|
||||||
.select()
|
// row may be undefined if the token is not found — used defensively in catch.
|
||||||
.from(federationEnrollmentTokens)
|
let row: typeof federationEnrollmentTokens.$inferSelect | undefined;
|
||||||
.where(eq(federationEnrollmentTokens.token, token))
|
|
||||||
.limit(1);
|
|
||||||
|
|
||||||
if (!row) {
|
|
||||||
throw new NotFoundException('Enrollment token not found');
|
|
||||||
}
|
|
||||||
|
|
||||||
// 2. Already used?
|
|
||||||
if (row.usedAt !== null) {
|
|
||||||
throw new GoneException('Enrollment token has already been used');
|
|
||||||
}
|
|
||||||
|
|
||||||
// 3. Expired?
|
|
||||||
if (row.expiresAt < new Date()) {
|
|
||||||
throw new GoneException('Enrollment token has expired');
|
|
||||||
}
|
|
||||||
|
|
||||||
// 4. Load grant and verify it is still pending
|
|
||||||
let grant;
|
|
||||||
try {
|
try {
|
||||||
grant = await this.grantsService.getGrant(row.grantId);
|
// 1. Fetch token row
|
||||||
|
const [fetchedRow] = await this.db
|
||||||
|
.select()
|
||||||
|
.from(federationEnrollmentTokens)
|
||||||
|
.where(eq(federationEnrollmentTokens.token, token))
|
||||||
|
.limit(1);
|
||||||
|
|
||||||
|
if (!fetchedRow) {
|
||||||
|
throw new NotFoundException('Enrollment token not found');
|
||||||
|
}
|
||||||
|
row = fetchedRow;
|
||||||
|
|
||||||
|
// 2. Already used?
|
||||||
|
if (row.usedAt !== null) {
|
||||||
|
throw new GoneException('Enrollment token has already been used');
|
||||||
|
}
|
||||||
|
|
||||||
|
// 3. Expired?
|
||||||
|
if (row.expiresAt < new Date()) {
|
||||||
|
throw new GoneException('Enrollment token has expired');
|
||||||
|
}
|
||||||
|
|
||||||
|
// 4. Load grant and verify it is still pending
|
||||||
|
let grant;
|
||||||
|
try {
|
||||||
|
grant = await this.grantsService.getGrant(row.grantId);
|
||||||
|
} catch (err) {
|
||||||
|
if (err instanceof FederationScopeError) {
|
||||||
|
throw new BadRequestException(err.message);
|
||||||
|
}
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (grant.status !== 'pending') {
|
||||||
|
throw new GoneException(
|
||||||
|
`Grant ${row.grantId} is no longer pending (status: ${grant.status})`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// 5. Atomically claim the token BEFORE cert issuance to prevent double-minting.
|
||||||
|
// WHERE used_at IS NULL ensures only one concurrent request wins.
|
||||||
|
// Using .returning() works on both node-postgres and PGlite without rowCount inspection.
|
||||||
|
const claimed = await this.db
|
||||||
|
.update(federationEnrollmentTokens)
|
||||||
|
.set({ usedAt: sql`NOW()` })
|
||||||
|
.where(
|
||||||
|
and(
|
||||||
|
eq(federationEnrollmentTokens.token, token),
|
||||||
|
isNull(federationEnrollmentTokens.usedAt),
|
||||||
|
),
|
||||||
|
)
|
||||||
|
.returning({ token: federationEnrollmentTokens.token });
|
||||||
|
|
||||||
|
if (claimed.length === 0) {
|
||||||
|
throw new GoneException('Enrollment token has already been used (concurrent request)');
|
||||||
|
}
|
||||||
|
|
||||||
|
// 6. Issue certificate via CaService (network call — outside any transaction).
|
||||||
|
// If this throws, the token is already consumed. The grant stays pending.
|
||||||
|
// Admin must revoke the grant and create a new one.
|
||||||
|
let issued;
|
||||||
|
try {
|
||||||
|
issued = await this.caService.issueCert({
|
||||||
|
csrPem,
|
||||||
|
grantId: row.grantId,
|
||||||
|
subjectUserId: grant.subjectUserId,
|
||||||
|
ttlSeconds: 300,
|
||||||
|
});
|
||||||
|
} catch (err) {
|
||||||
|
// HIGH-4: Log only the first 8 hex chars of the token for correlation — never log the full token.
|
||||||
|
this.logger.error(
|
||||||
|
`issueCert failed after token ${token.slice(0, 8)}... was claimed — grant ${row.grantId} is stranded pending`,
|
||||||
|
err instanceof Error ? err.stack : String(err),
|
||||||
|
);
|
||||||
|
if (err instanceof FederationScopeError) {
|
||||||
|
throw new BadRequestException((err as Error).message);
|
||||||
|
}
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
|
|
||||||
|
// 7. Atomically activate grant, update peer record, and write audit log.
|
||||||
|
const certNotAfter = this.extractCertNotAfter(issued.certPem);
|
||||||
|
await this.db.transaction(async (tx) => {
|
||||||
|
// CRIT-2: Guard activation with WHERE status='pending' to prevent double-activation.
|
||||||
|
const [activated] = await tx
|
||||||
|
.update(federationGrants)
|
||||||
|
.set({ status: 'active' })
|
||||||
|
.where(and(eq(federationGrants.id, row!.grantId), eq(federationGrants.status, 'pending')))
|
||||||
|
.returning({ id: federationGrants.id });
|
||||||
|
if (!activated) {
|
||||||
|
throw new ConflictException(
|
||||||
|
`Grant ${row!.grantId} is no longer pending — cannot activate`,
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
// CRIT-2: Guard peer update with WHERE state='pending'.
|
||||||
|
await tx
|
||||||
|
.update(federationPeers)
|
||||||
|
.set({
|
||||||
|
certPem: issued.certPem,
|
||||||
|
certSerial: issued.serialNumber,
|
||||||
|
certNotAfter,
|
||||||
|
state: 'active',
|
||||||
|
})
|
||||||
|
.where(and(eq(federationPeers.id, row!.peerId), eq(federationPeers.state, 'pending')));
|
||||||
|
|
||||||
|
await tx.insert(federationAuditLog).values({
|
||||||
|
requestId: crypto.randomUUID(),
|
||||||
|
peerId: row!.peerId,
|
||||||
|
grantId: row!.grantId,
|
||||||
|
verb: 'enrollment',
|
||||||
|
resource: 'federation_grant',
|
||||||
|
statusCode: 200,
|
||||||
|
outcome: 'allowed',
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
this.logger.log(
|
||||||
|
`Enrollment complete — peerId=${row.peerId} grantId=${row.grantId} serial=${issued.serialNumber}`,
|
||||||
|
);
|
||||||
|
|
||||||
|
outcome = 'allowed';
|
||||||
|
|
||||||
|
// 8. Return cert material
|
||||||
|
return {
|
||||||
|
certPem: issued.certPem,
|
||||||
|
certChainPem: issued.certChainPem,
|
||||||
|
};
|
||||||
} catch (err) {
|
} catch (err) {
|
||||||
if (err instanceof FederationScopeError) {
|
// HIGH-5: Best-effort audit write on failure — do not let this throw.
|
||||||
throw new BadRequestException(err.message);
|
if (outcome === 'denied') {
|
||||||
|
await this.db
|
||||||
|
.insert(federationAuditLog)
|
||||||
|
.values({
|
||||||
|
requestId: crypto.randomUUID(),
|
||||||
|
peerId: row?.peerId ?? 'unknown',
|
||||||
|
grantId: row?.grantId ?? 'unknown',
|
||||||
|
verb: 'enrollment',
|
||||||
|
resource: 'federation_grant',
|
||||||
|
statusCode:
|
||||||
|
err instanceof GoneException ? 410 : err instanceof NotFoundException ? 404 : 500,
|
||||||
|
outcome: 'denied',
|
||||||
|
})
|
||||||
|
.catch(() => {});
|
||||||
}
|
}
|
||||||
throw err;
|
throw err;
|
||||||
}
|
}
|
||||||
|
|
||||||
if (grant.status !== 'pending') {
|
|
||||||
throw new GoneException(
|
|
||||||
`Grant ${row.grantId} is no longer pending (status: ${grant.status})`,
|
|
||||||
);
|
|
||||||
}
|
|
||||||
|
|
||||||
// 5. Atomically claim the token BEFORE cert issuance to prevent double-minting.
|
|
||||||
// WHERE used_at IS NULL ensures only one concurrent request wins.
|
|
||||||
// Using .returning() works on both node-postgres and PGlite without rowCount inspection.
|
|
||||||
const claimed = await this.db
|
|
||||||
.update(federationEnrollmentTokens)
|
|
||||||
.set({ usedAt: sql`NOW()` })
|
|
||||||
.where(
|
|
||||||
and(eq(federationEnrollmentTokens.token, token), isNull(federationEnrollmentTokens.usedAt)),
|
|
||||||
)
|
|
||||||
.returning({ token: federationEnrollmentTokens.token });
|
|
||||||
|
|
||||||
if (claimed.length === 0) {
|
|
||||||
throw new GoneException('Enrollment token has already been used (concurrent request)');
|
|
||||||
}
|
|
||||||
|
|
||||||
// 6. Issue certificate via CaService (network call — outside any transaction).
|
|
||||||
// If this throws, the token is already consumed. The grant stays pending.
|
|
||||||
// Admin must revoke the grant and create a new one.
|
|
||||||
let issued;
|
|
||||||
try {
|
|
||||||
issued = await this.caService.issueCert({
|
|
||||||
csrPem,
|
|
||||||
grantId: row.grantId,
|
|
||||||
subjectUserId: grant.subjectUserId,
|
|
||||||
ttlSeconds: 300,
|
|
||||||
});
|
|
||||||
} catch (err) {
|
|
||||||
this.logger.error(
|
|
||||||
`issueCert failed after token ${token} was claimed — grant ${row.grantId} is stranded pending`,
|
|
||||||
err instanceof Error ? err.stack : String(err),
|
|
||||||
);
|
|
||||||
if (err instanceof FederationScopeError) {
|
|
||||||
throw new BadRequestException((err as Error).message);
|
|
||||||
}
|
|
||||||
throw err;
|
|
||||||
}
|
|
||||||
|
|
||||||
// 7. Atomically activate grant, update peer record, and write audit log.
|
|
||||||
const certNotAfter = this.extractCertNotAfter(issued.certPem);
|
|
||||||
await this.db.transaction(async (tx) => {
|
|
||||||
await tx
|
|
||||||
.update(federationGrants)
|
|
||||||
.set({ status: 'active' })
|
|
||||||
.where(eq(federationGrants.id, row.grantId));
|
|
||||||
|
|
||||||
await tx
|
|
||||||
.update(federationPeers)
|
|
||||||
.set({
|
|
||||||
certPem: issued.certPem,
|
|
||||||
certSerial: issued.serialNumber,
|
|
||||||
certNotAfter,
|
|
||||||
state: 'active',
|
|
||||||
})
|
|
||||||
.where(eq(federationPeers.id, row.peerId));
|
|
||||||
|
|
||||||
await tx.insert(federationAuditLog).values({
|
|
||||||
requestId: crypto.randomUUID(),
|
|
||||||
peerId: row.peerId,
|
|
||||||
grantId: row.grantId,
|
|
||||||
verb: 'enrollment',
|
|
||||||
resource: 'federation_grant',
|
|
||||||
statusCode: 200,
|
|
||||||
outcome: 'allowed',
|
|
||||||
});
|
|
||||||
});
|
|
||||||
|
|
||||||
this.logger.log(
|
|
||||||
`Enrollment complete — peerId=${row.peerId} grantId=${row.grantId} serial=${issued.serialNumber}`,
|
|
||||||
);
|
|
||||||
|
|
||||||
// 8. Return cert material
|
|
||||||
return {
|
|
||||||
certPem: issued.certPem,
|
|
||||||
certChainPem: issued.certChainPem,
|
|
||||||
};
|
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Extract the notAfter date from a PEM certificate.
|
* Extract the notAfter date from a PEM certificate.
|
||||||
* Falls back to 90 days from now if parsing fails.
|
* HIGH-2: No silent fallback — a cert that cannot be parsed should fail loud.
|
||||||
*/
|
*/
|
||||||
private extractCertNotAfter(certPem: string): Date {
|
private extractCertNotAfter(certPem: string): Date {
|
||||||
try {
|
const cert = new X509Certificate(certPem);
|
||||||
const cert = new X509Certificate(certPem);
|
return new Date(cert.validTo);
|
||||||
return new Date(cert.validTo);
|
|
||||||
} catch {
|
|
||||||
// Fallback: 90 days from now
|
|
||||||
return new Date(Date.now() + 90 * 24 * 60 * 60 * 1000);
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user