diff --git a/.changeset/26534-secret-audit-old-hash.md b/.changeset/26534-secret-audit-old-hash.md new file mode 100644 index 00000000000..e0939326769 --- /dev/null +++ b/.changeset/26534-secret-audit-old-hash.md @@ -0,0 +1,11 @@ +--- +'@objectstack/service-settings': patch +--- + +fix(service-settings): a secret overwrite's audit row records the previous value's keyed digest as `old_hash` (#26534) + +Clause-②: no +Metadata-protocol: none +Ruling-ref: #26534 triage `6436617436`; the server seat 2 ruling, arm (a) + +A secret-valued setting's overwrite used to record `old_hash` null on `sys_setting_audit` / `sys_platform_setting_audit`, though the column declares null only when the key was previously unset. It now records the same keyed digest (`hmac-sha256:…`) that `new_hash` records, taken over the previous value, so an overwrite's `old_hash` equals the previous row's `new_hash` and a rewrite of the same secret reads `old_hash == new_hash`. It stays null when the key was unset or reset, when no keyed digest is available, or when the previous value can no longer be read, and it is never an unkeyed hash of the secret. Only a secret overwrite pays for it: one `sys_secret` read and one decrypt of the previous value, before the rotated-away secret is reaped. diff --git a/packages/services/service-settings/src/settings-audit-old-hash.test.ts b/packages/services/service-settings/src/settings-audit-old-hash.test.ts index 857e84036c3..dc986806d3c 100644 --- a/packages/services/service-settings/src/settings-audit-old-hash.test.ts +++ b/packages/services/service-settings/src/settings-audit-old-hash.test.ts @@ -16,9 +16,11 @@ * expected digests are computed here with `node:crypto`, independently of the * implementation. * - * A secret-valued key is the CONTROL: its keyed fingerprint path - * (`hmac-sha256:…`, `secretAuditDigest`) is untouched, so its rows keep a null - * `old_hash` and a keyed `new_hash`. + * [#26534] A secret-valued key records the SAME keyed digest on both columns + * (`hmac-sha256:…`, `secretAuditDigest`): `old_hash` is the keyed digest of + * the previous value, read back and decrypted inside the write, so it equals + * the previous row's `new_hash`. No row ever carries an unkeyed digest of a + * secret. */ import { createHash } from 'node:crypto'; @@ -182,7 +184,7 @@ class MockHttp implements IHttpServer { } /** The pieces the running server binds: real engine, real objects, the plugin's audit writer, real routes. */ -async function boot() { +async function boot(opts: { cryptoProvider?: LocalCryptoProvider } = {}) { const engine = new ObjectQL(); const { driver, rowsOf } = makeMemoryDriver(); engine.registerDriver(driver, true); @@ -190,11 +192,19 @@ async function boot() { for (const o of [SysSetting, SysPlatformSetting, SysSecret, SysSettingAudit, SysPlatformSettingAudit]) { engine.registry.registerObject(o as any, OWNER_PACKAGE); } + // The plugin's own `sys_secret` store, with its reads recorded: which + // handles a write dereferences is what the cost cases measure. + const store = (new SettingsServicePlugin() as any).buildSecretStore(engine); + const secretReads: string[] = []; + const secretStore = { + ...store, + get: async (id: string) => { secretReads.push(id); return store.get(id); }, + }; const svc = new SettingsService({ env: {}, engine: withEngineRegistry(wrapEngineAsSettingsEngine(engine as any), engine), - cryptoProvider: new LocalCryptoProvider(), - secretStore: (new SettingsServicePlugin() as any).buildSecretStore(engine), + cryptoProvider: opts.cryptoProvider ?? new LocalCryptoProvider(), + secretStore, auditWriter: buildSettingAuditWriter(engine as any), }); for (const m of [TENANT_MANIFEST, GLOBAL_MANIFEST, USER_MANIFEST]) svc.registerManifest(m); @@ -238,7 +248,19 @@ async function boot() { const auditOf = (object: 'sys_setting_audit' | 'sys_platform_setting_audit', key: string) => [...rowsOf(object).values()].filter((r) => r.key === key); - return { put, auditOf, asUser: (id: string) => { userId = id; } }; + /** The `sys_secret` handle the global row of `key` names now. */ + const handleOf = (key: string) => + [...rowsOf('sys_platform_setting').values()].find((r) => r.key === key)?.value_enc as string | undefined; + + return { + put, + auditOf, + asUser: (id: string) => { userId = id; }, + svc, + rowsOf, + secretReads, + handleOf, + }; } describe('#26465 — old_hash / new_hash on the tenant rung (sys_setting_audit)', () => { @@ -330,8 +352,36 @@ describe('#26465 — the global rung (sys_platform_setting_audit) records the sa [sha256Of('smtp2.example.com'), sha256Of('smtp2.example.com')], ]); }); +}); + +/** The unkeyed digests a leak of `value` would carry, for the "never in any row" check. */ +const unkeyedOf = (value: string) => [ + sha256Of(value).slice('sha256:'.length), + createHash('sha256').update(value, 'utf8').digest('hex'), +]; +const expectNoUnkeyedDigest = (rows: Record[], values: string[]) => { + for (const row of rows) { + for (const v of values) { + expect(JSON.stringify(row)).not.toContain(v); + for (const d of unkeyedOf(v)) expect(JSON.stringify(row)).not.toContain(d); + } + } +}; + +describe('#26534 — a secret value records the keyed digest of its previous value as old_hash', () => { + it('first write: old_hash null, keyed new_hash', async () => { + const { put, auditOf } = await boot(); + + await put('hash_global', { api_key: 'pw-alpha' }); + + const rows = auditOf('sys_platform_setting_audit', 'api_key'); + expect(rows).toHaveLength(1); + expect(rows[0]).toMatchObject({ action: 'set', encrypted: true, old_hash: null }); + expect(rows[0].new_hash).toMatch(KEYED_SHAPE); + expectNoUnkeyedDigest(rows, ['pw-alpha']); + }); - it('CONTROL — a secret value keeps its keyed path: keyed new_hash, null old_hash, no unkeyed digest anywhere', async () => { + it('overwrite: old_hash is the previous row\'s keyed new_hash, and no unkeyed digest is anywhere in a row', async () => { const { put, auditOf } = await boot(); await put('hash_global', { api_key: 'pw-alpha' }); @@ -341,11 +391,135 @@ describe('#26465 — the global rung (sys_platform_setting_audit) records the sa expect(rows).toHaveLength(2); for (const row of rows) { expect(row.encrypted).toBe(true); - expect(row.old_hash).toBeNull(); expect(row.new_hash).toMatch(KEYED_SHAPE); } - expect(rows[1].new_hash).not.toBe(rows[0].new_hash); - const unkeyed = [sha256Of('pw-alpha'), sha256Of('pw-beta'), 'sha256:' + createHash('sha256').update('pw-alpha').digest('hex')]; - for (const row of rows) for (const d of unkeyed) expect(JSON.stringify(row)).not.toContain(d.slice('sha256:'.length)); + expect(rows[1].old_hash).toMatch(KEYED_SHAPE); + expect(rows[1].old_hash).toBe(rows[0].new_hash); + expect(rows[1].new_hash).not.toBe(rows[1].old_hash); + expectNoUnkeyedDigest(rows, ['pw-alpha', 'pw-beta']); + }); + + it('same-value rewrite: old_hash equals new_hash, so a rotation that changed nothing is visible', async () => { + const { put, auditOf } = await boot(); + + await put('hash_global', { api_key: 'pw-alpha' }); + await put('hash_global', { api_key: 'pw-alpha' }); + + const rows = auditOf('sys_platform_setting_audit', 'api_key'); + expect(rows).toHaveLength(2); + expect(rows[1].old_hash).toMatch(KEYED_SHAPE); + expect(rows[1].old_hash).toBe(rows[1].new_hash); + expect(rows[1].old_hash).toBe(rows[0].new_hash); + expectNoUnkeyedDigest(rows, ['pw-alpha']); + }); + + it('set → reset → set: the reset carries the previous keyed digest and no new one; the next set starts from unset', async () => { + const { put, auditOf } = await boot(); + + await put('hash_global', { api_key: 'pw-alpha' }); + await put('hash_global', { api_key: null }); + await put('hash_global', { api_key: 'pw-beta' }); + + const rows = auditOf('sys_platform_setting_audit', 'api_key'); + expect(rows.map((r) => r.action)).toEqual(['set', 'reset', 'set']); + expect(rows[1]).toMatchObject({ old_hash: rows[0].new_hash, new_hash: null }); + expect(rows[1].old_hash).toMatch(KEYED_SHAPE); + expect(rows[2].old_hash).toBeNull(); + expect(rows[2].new_hash).toMatch(KEYED_SHAPE); + expectNoUnkeyedDigest(rows, ['pw-alpha', 'pw-beta']); + }); + + it('cost: only a secret overwrite pays one more read of the previous handle, and that secret is still reaped after it', async () => { + /** The reads of the previous handle one overwrite makes, on a fresh boot. */ + const overwriteReads = async (provider?: LocalCryptoProvider) => { + const { put, auditOf, rowsOf, secretReads, handleOf } = await boot({ cryptoProvider: provider }); + // A non-secret write never touches sys_secret. + await put('hash_global', { host: 'smtp.example.com' }); + expect(secretReads).toEqual([]); + // A first write has no previous handle to read: every read is of the + // handle it just wrote (the response's re-resolve). + await put('hash_global', { api_key: 'pw-alpha' }); + const first = handleOf('api_key')!; + expect(first).toMatch(/^sec_/); + expect(secretReads.every((id) => id === first)).toBe(true); + secretReads.length = 0; + await put('hash_global', { api_key: 'pw-beta' }); + // The rotated-away secret is reaped AFTER the old_hash read (#8030). + expect(rowsOf('sys_secret').has(first)).toBe(false); + expect(rowsOf('sys_secret').has(handleOf('api_key')!)).toBe(true); + return { + reads: secretReads.filter((id) => id === first).length, + oldHash: auditOf('sys_platform_setting_audit', 'api_key')[1].old_hash, + }; + }; + // The baseline: with no keyed digest the write skips the old_hash read, + // and what is left is the read the write path already made before this + // rule (`validatePatch` resolves the key's current value). + const unkeyed = new LocalCryptoProvider(); + (unkeyed as any).keyedDigest = undefined; + const baseline = await overwriteReads(unkeyed); + const keyed = await overwriteReads(); + + expect(baseline.oldHash).toBeNull(); + expect(keyed.oldHash).toMatch(KEYED_SHAPE); + expect(keyed.reads - baseline.reads).toBe(1); + }); + + it('no keyed digest available: old_hash null like new_hash, and the write lands', async () => { + const provider = new LocalCryptoProvider(); + (provider as any).keyedDigest = undefined; + const { put, auditOf, svc } = await boot({ cryptoProvider: provider }); + + await put('hash_global', { api_key: 'pw-alpha' }); + await put('hash_global', { api_key: 'pw-beta' }); + + const rows = auditOf('sys_platform_setting_audit', 'api_key'); + expect(rows.map((r) => [r.old_hash, r.new_hash])).toEqual([[null, null], [null, null]]); + expect((await svc.get('hash_global', 'api_key')).value).toBe('pw-beta'); + expectNoUnkeyedDigest(rows, ['pw-alpha', 'pw-beta']); + }); + + it('a previous handle that no longer resolves records old_hash null and the write still lands', async () => { + const { put, auditOf, rowsOf, handleOf, svc } = await boot(); + + await put('hash_global', { api_key: 'pw-alpha' }); + rowsOf('sys_secret').delete(handleOf('api_key')!); + await put('hash_global', { api_key: 'pw-beta' }); + + const rows = auditOf('sys_platform_setting_audit', 'api_key'); + expect(rows).toHaveLength(2); + expect(rows[1].old_hash).toBeNull(); + expect(rows[1].new_hash).toMatch(KEYED_SHAPE); + expect((await svc.get('hash_global', 'api_key')).value).toBe('pw-beta'); + }); + + it('a key whose manifest changes between non-secret and secret never records an unkeyed digest of a secret', async () => { + // Not reachable through PUT: `encrypted` is the manifest's, and only a + // re-registration (a deploy that edits the manifest) changes it. + const { put, auditOf, svc } = await boot(); + const asSecret = (secret: boolean): SettingsManifest => ({ + ...GLOBAL_MANIFEST, + specifiers: [ + secret + ? { type: 'password', key: 'host', label: 'Host', required: false, encrypted: true } + : { type: 'text', key: 'host', label: 'Host', required: false }, + ], + }); + + await put('hash_global', { host: 'plain-value' }); + svc.registerManifest(asSecret(true)); + await put('hash_global', { host: 'secret-value' }); + svc.registerManifest(asSecret(false)); + await put('hash_global', { host: 'plain-again' }); + + const rows = auditOf('sys_platform_setting_audit', 'host'); + expect(rows.map((r) => r.encrypted)).toEqual([false, true, false]); + // non-secret → secret: the previous row held no secret, so no keyed digest of it. + expect(rows[1].old_hash).toBeNull(); + expect(rows[1].new_hash).toMatch(KEYED_SHAPE); + // secret → non-secret: the previous value was a secret, so no unkeyed digest of it. + expect(rows[2].old_hash).toBeNull(); + expect(rows[2].new_hash).toBe(sha256Of('plain-again')); + expectNoUnkeyedDigest(rows, ['secret-value']); }); }); diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index 7425ed93544..148247002ea 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -2447,6 +2447,16 @@ export class SettingsService { }; const { previousEnc, previous } = await this.upsertRow(rowForKey); + // The audit's `old_hash`, taken HERE: after the upsert's probe named the + // previous row, and before the reap below destroys the ciphertext a + // secret's previous handle points at (#26534). Only an audited write + // pays for it, and only a secret overwrite pays a read and a decrypt. + const oldHash = !this.auditWriter + ? null + : isEncrypted + ? await this.previousSecretAuditDigest(namespace, key, previous) + : SettingsService.previousAuditDigest(previous); + // Destroy the ciphertext the row USED to name (#8030) — but only once a // re-read confirms the row really stopped naming it (#8262); "a new // ciphertext was written" is not evidence that the repoint landed. @@ -2497,10 +2507,9 @@ export class SettingsService { // [ADR-0131 D3/D9] The organization whose value changed — the same // owner the settings row carries; none for a global value. ...(organizationId ? { organizationId } : {}), - // #26465: the previous value's digest, off the row the upsert - // already read. A secret write keeps no old fingerprint (its keyed - // digest path is unchanged), see `previousAuditDigest`. - oldHash: isEncrypted ? null : SettingsService.previousAuditDigest(previous), + // #26465 / #26534: the previous value's digest, off the row the + // upsert already read — keyed for a secret, see `oldHash` above. + oldHash, newHash: isNull ? null : digest, encrypted: isEncrypted, requestId: ctx.requestId, @@ -3267,7 +3276,8 @@ export class SettingsService { * * A row that held a SECRET also answers `null`: its value is ciphertext or a * handle, and an unkeyed hash of the decrypted secret is exactly the - * fingerprint the audit trail must never carry (see `secretAuditDigest`). + * fingerprint the audit trail must never carry (see `secretAuditDigest`). A + * secret write takes its keyed `old_hash` from `previousSecretAuditDigest`. */ private static previousAuditDigest(previous: SettingsRow | null): string | null { if (!previous || previous.encrypted) return null; @@ -3275,6 +3285,50 @@ export class SettingsService { return settingAuditDigest(stableStringify(previous.value)); } + /** + * The `old_hash` of a secret write (#26534): the SAME keyed digest + * `new_hash` records ({@link secretAuditDigest}), taken over the value the + * row held before, so an overwrite's `old_hash` is the previous row's + * `new_hash` and a same-value rewrite reads `old_hash === new_hash`. + * + * `null` when the row held no secret (no row, a reset row, or a row written + * while the key was not secret-valued), and `null` when no keyed digest can + * be had — the rule `new_hash` follows. Never a digest of the ciphertext: + * that changes on every re-encryption, so it cannot say whether the value + * changed. Never an unkeyed hash of the plaintext, either. + * + * The provider is checked BEFORE the read, so a write whose `new_hash` would + * be `null` anyway pays no `sys_secret` read and no decrypt. The previous + * plaintext lives only in this frame: it is digested and dropped, never + * logged, returned or stored. A previous value that cannot be read back (its + * handle already gone, or a data key that no longer opens it) records `null`, + * says so on the logger, and never fails the write, the rule both audit + * seams follow. + */ + private async previousSecretAuditDigest( + namespace: string, + key: string, + previous: SettingsRow | null, + ): Promise { + if (!previous || previous.encrypted !== true) return null; + if (SettingsService.handleOf(previous.value_enc) === null) return null; + const provider = this.cryptoProvider; + if (!provider || typeof provider.keyedDigest !== 'function') return null; + let plain: string | null; + try { + plain = await this.decryptStored(previous); + } catch (err) { + const message = + `[SettingsService] ${namespace}.${key}: the audit trail records this overwrite with no ` + + `old_hash because the previous value could not be decrypted (${(err as Error)?.message ?? err}).`; + if (this.logger?.warn) this.logger.warn(message); + else console.warn(message); + return null; + } + if (plain === null) return null; + return this.secretAuditDigest(namespace, key, plain); + } + /** * Insert or update the row at its identity, and report what that identity * held BEFORE: `previousEnc` for the secret reaper (#8030), `previous` for @@ -3496,39 +3550,47 @@ export class SettingsService { else console.error(message); } + /** + * The plaintext an encrypted row's `value_enc` stands for, or `null` when + * it names nothing (no value, or a `sys_secret` handle whose row is gone). + * Throws when the decrypt does; each caller decides what that means. + */ + private async decryptStored(row: SettingsRow): Promise { + if (!row.value_enc) return null; + // Phase 3: when the value_enc looks like a sys_secret handle and + // both the secretStore + cryptoProvider are wired, dereference + // through sys_secret. Otherwise (legacy rows or in-memory tests) + // fall back to inline crypto-adapter decryption. + if ( + this.cryptoProvider && + this.secretStore && + typeof row.value_enc === 'string' && + row.value_enc.startsWith('sec_') + ) { + const secret = await this.secretStore.get(row.value_enc); + if (!secret) return null; + return this.cryptoProvider.decrypt( + { + id: secret.id, + kmsKeyId: secret.kms_key_id, + alg: secret.alg, + version: secret.version, + ciphertext: secret.ciphertext, + }, + { scope: 'settings', namespace: row.namespace, key: row.key }, + ); + } + return this.crypto.decrypt(row.value_enc, { + namespace: row.namespace, + key: row.key, + }); + } + private async materialiseRow(row: SettingsRow): Promise { if (row.encrypted) { - if (!row.value_enc) return null; - let plain: string; + let plain: string | null; try { - // Phase 3: when the value_enc looks like a sys_secret handle and - // both the secretStore + cryptoProvider are wired, dereference - // through sys_secret. Otherwise (legacy rows or in-memory tests) - // fall back to inline crypto-adapter decryption. - if ( - this.cryptoProvider && - this.secretStore && - typeof row.value_enc === 'string' && - row.value_enc.startsWith('sec_') - ) { - const secret = await this.secretStore.get(row.value_enc); - if (!secret) return null; - plain = await this.cryptoProvider.decrypt( - { - id: secret.id, - kmsKeyId: secret.kms_key_id, - alg: secret.alg, - version: secret.version, - ciphertext: secret.ciphertext, - }, - { scope: 'settings', namespace: row.namespace, key: row.key }, - ); - } else { - plain = await this.crypto.decrypt(row.value_enc, { - namespace: row.namespace, - key: row.key, - }); - } + plain = await this.decryptStored(row); } catch (err) { // Decrypt failures are almost always operational: the crypto // provider's data key changed (e.g. InMemoryCryptoProvider @@ -3545,6 +3607,7 @@ export class SettingsService { ); return null; } + if (plain === null) return null; // Try JSON parse so non-string secrets round-trip. try { return JSON.parse(plain);