From d95ae74aed0dbdd6c77a0a2bc31d5b3a7f73b9f5 Mon Sep 17 00:00:00 2001 From: Danijel Martinek Date: Fri, 10 Jul 2026 18:13:38 +0200 Subject: [PATCH] fix(core-audit): keyed 128-bit pseudonyms + salted DSR certificate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pseudonymize() used an unkeyed sha256 over 'salt:id' truncated to 64 bits, and the DSR deletion certificate hashed the raw subjectId with no salt at all (audit finding A13). Both now use HMAC-SHA256 keyed by AUDIT_PSEUDONYM_SALT, truncated to 128 bits. Rotation semantics are documented on pseudonymize(): a key rotation changes future pseudonyms only — stored rows keep old tokens and erasure still matches by real actorId — and the certificate change likewise affects new certificates only. Co-Authored-By: Claude Fable 5 --- .../core-audit/src/payload-audit-log.test.ts | 20 ++++++++++--- packages/core-audit/src/pseudonymize.test.ts | 21 +++++++++++-- packages/core-audit/src/pseudonymize.ts | 30 +++++++++++++------ packages/core-dsr/src/payload-data-delete.ts | 16 ++++++++-- 4 files changed, 68 insertions(+), 19 deletions(-) diff --git a/packages/core-audit/src/payload-audit-log.test.ts b/packages/core-audit/src/payload-audit-log.test.ts index e37e5f4..a128bea 100644 --- a/packages/core-audit/src/payload-audit-log.test.ts +++ b/packages/core-audit/src/payload-audit-log.test.ts @@ -25,7 +25,10 @@ describe("PayloadAuditLog.record", () => { await log.record(sample); expect(mockCreate).toHaveBeenCalledOnce(); - const call = mockCreate.mock.calls[0]![0] as { collection: string; data: Record }; + const call = mockCreate.mock.calls[0]![0] as { + collection: string; + data: Record; + }; expect(call.collection).toBe("audit-logs"); expect(call.data.actorId).toBe("user_1"); expect(call.data.action).toBe("UPDATE"); @@ -101,14 +104,21 @@ describe("PayloadAuditLog.eraseSubject", () => { // update called for each doc expect(mockUpdate).toHaveBeenCalledTimes(2); const updateCalls = mockUpdate.mock.calls as Array< - [{ collection: string; id: string; data: Record; overrideAccess: boolean }] + [ + { + collection: string; + id: string; + data: Record; + overrideAccess: boolean; + }, + ] >; expect(updateCalls[0]![0].id).toBe("doc_a"); expect(updateCalls[1]![0].id).toBe("doc_b"); // both updates replace actorId with the same pseudonym const pseudonym = updateCalls[0]![0].data["actorId"] as string; - expect(pseudonym).toMatch(/^erased-[0-9a-f]{16}$/); + expect(pseudonym).toMatch(/^erased-[0-9a-f]{32}$/); expect(updateCalls[1]![0].data["actorId"]).toBe(pseudonym); // overrideAccess bypasses the append-only rule @@ -118,7 +128,9 @@ describe("PayloadAuditLog.eraseSubject", () => { it("mode='pseudonymize' with no matching docs does not call update", async () => { const mockFind = vi.fn().mockResolvedValue({ docs: [] }); const mockUpdate = vi.fn(); - const mockGetPayload = vi.fn().mockResolvedValue({ find: mockFind, update: mockUpdate }); + const mockGetPayload = vi + .fn() + .mockResolvedValue({ find: mockFind, update: mockUpdate }); const log = new PayloadAuditLog({} as never, mockGetPayload); await log.eraseSubject("unknown_user", "pseudonymize"); diff --git a/packages/core-audit/src/pseudonymize.test.ts b/packages/core-audit/src/pseudonymize.test.ts index 66d737c..97d4c93 100644 --- a/packages/core-audit/src/pseudonymize.test.ts +++ b/packages/core-audit/src/pseudonymize.test.ts @@ -21,13 +21,28 @@ describe("pseudonymize", () => { expect(result).toMatch(/^erased-/); }); - it("produces exactly 16 hex chars after the prefix", () => { + it("produces exactly 32 hex chars (128 bits) after the prefix (A13)", () => { const result = pseudonymize("user_42"); const hex = result.slice("erased-".length); - expect(hex).toHaveLength(16); + expect(hex).toHaveLength(32); expect(hex).toMatch(/^[0-9a-f]+$/); }); + it("matches HMAC-SHA256(key, actorId) - keyed, not a bare hash (A13)", async () => { + const { createHmac, createHash } = await import("node:crypto"); + const expected = createHmac("sha256", "test-salt-1") + .update("user_42") + .digest("hex") + .slice(0, 32); + expect(pseudonymize("user_42")).toBe(`erased-` + expected); + // and it must NOT be the legacy unkeyed sha256("salt:id") scheme + const legacy = createHash("sha256") + .update("test-salt-1:user_42") + .digest("hex") + .slice(0, 32); + expect(pseudonymize("user_42")).not.toBe(`erased-` + legacy); + }); + it("is deterministic — same salt + actorId always yields the same token", () => { const a = pseudonymize("user_42"); const b = pseudonymize("user_42"); @@ -53,6 +68,6 @@ describe("pseudonymize", () => { delete process.env["AUDIT_PSEUDONYM_SALT"]; // Should not throw; just use the fallback. const result = pseudonymize("user_1"); - expect(result).toMatch(/^erased-[0-9a-f]{16}$/); + expect(result).toMatch(/^erased-[0-9a-f]{32}$/); }); }); diff --git a/packages/core-audit/src/pseudonymize.ts b/packages/core-audit/src/pseudonymize.ts index 4439cb1..76db4d9 100644 --- a/packages/core-audit/src/pseudonymize.ts +++ b/packages/core-audit/src/pseudonymize.ts @@ -1,22 +1,34 @@ -import { createHash } from "node:crypto"; +import { createHmac } from "node:crypto"; /** * Produces a stable, irreversible token for a GDPR-erased actorId. * - * Format: `erased-` + * Format: `erased-` — + * 128 bits of a KEYED digest (audit finding A13). The previous scheme was an + * unkeyed `sha256("salt:actorId")` truncated to 64 bits, which invited both + * brute-force reversal of small id spaces and birthday collisions. * - * The salt is read from `AUDIT_PSEUDONYM_SALT` env at call time so that + * The HMAC key is read from `AUDIT_PSEUDONYM_SALT` at call time so that * production binding can pre-validate the var at boot (see `bindAudit`) * while tests can override it per-test via `process.env`. * - * Fallback salt is intentionally weak and labelled so that any token + * Rotation expectations (documented, by design): + * - Rotating the key changes pseudonyms produced FROM THEN ON only. Audit + * rows already pseudonymized keep tokens derived from the previous key; + * nothing re-keys stored rows, so a subject's pre- and post-rotation + * tokens no longer correlate. That linkage break is acceptable — the + * token's only job is severing PII linkage, not long-term correlation. + * - Re-erasing a subject after rotation still works: erasure matches rows + * by the REAL actorId, not by a previous pseudonym. + * - Rotate by replacing the env value (e.g. `openssl rand -hex 32`); keep + * retired keys only if you have an explicit need to re-correlate old rows. + * + * Fallback key is intentionally weak and labelled so that any token * produced with it is recognisable as a dev/test artefact. */ export function pseudonymize(actorId: string): string { - const salt = + const key = process.env["AUDIT_PSEUDONYM_SALT"] ?? "dev-fallback-salt-replace-in-prod"; - const hash = createHash("sha256") - .update(`${salt}:${actorId}`) - .digest("hex"); - return `erased-${hash.slice(0, 16)}`; + const digest = createHmac("sha256", key).update(actorId).digest("hex"); + return `erased-${digest.slice(0, 32)}`; } diff --git a/packages/core-dsr/src/payload-data-delete.ts b/packages/core-dsr/src/payload-data-delete.ts index eed93b8..1b3c243 100644 --- a/packages/core-dsr/src/payload-data-delete.ts +++ b/packages/core-dsr/src/payload-data-delete.ts @@ -1,7 +1,6 @@ import { getPayload as _getPayload } from "payload"; import type { SanitizedConfig } from "payload"; -import { randomUUID } from "node:crypto"; -import { createHash } from "node:crypto"; +import { randomUUID, createHmac } from "node:crypto"; import type { AuditLogProtocol } from "@repo/core-shared/di"; import { RETENTION_TOMBSTONE_FIELD, @@ -321,9 +320,20 @@ export class PayloadDataDelete implements IDataDelete { correlationId: string, affected: DeletionAffected[], ): DeletionCertificate { + // Salted, keyed pseudonym (audit finding A13): the certificate used to + // hash the raw subjectId with NO salt (truncated to 64 bits), letting a + // certificate holder brute-force small id spaces offline. Now + // HMAC-SHA256 keyed by the operator secret AUDIT_PSEUDONYM_SALT (the + // same secret the audit-log pseudonymizer uses), truncated to 128 bits. + // NOTE: this changes tokens on FUTURE certificates only — certificates + // issued under the old scheme keep their historical value, and rotating + // the key likewise affects only certificates issued afterwards. + const certKey = + process.env["AUDIT_PSEUDONYM_SALT"] ?? + "dev-fallback-salt-replace-in-prod"; const certSubjectId = mode === "cascade-hard" - ? `erased-${createHash("sha256").update(subjectId).digest("hex").slice(0, 16)}` + ? `erased-${createHmac("sha256", certKey).update(subjectId).digest("hex").slice(0, 32)}` : subjectId; return {