diff --git a/.changeset/settings-crypto-fail-closed.md b/.changeset/settings-crypto-fail-closed.md new file mode 100644 index 0000000000..e530a9f50b --- /dev/null +++ b/.changeset/settings-crypto-fail-closed.md @@ -0,0 +1,49 @@ +--- +"@objectstack/service-settings": patch +--- + +fix(service-settings): refuse to persist a secret through the base64 `NoopCryptoAdapter` — the settings write path now fails closed like the engine's (#8026) + +`SettingsService` constructed without a `cryptoProvider` + `secretStore` fell +back to `NoopCryptoAdapter`, whose `encrypt()` is `'b64:' + base64(plaintext)`. +That is **encoding, not encryption**: trivially reversible, and it leaves +`sys_setting.value_enc` populated — so the row reads as protected to the next +author and to the next audit while being plaintext with extra steps. + +The engine's `Field.secret()` path has always taken the opposite posture: with +no `CryptoProvider` registered it throws rather than store cleartext. The +platform therefore had two credential-encryption paths with **opposite failure +modes**. This aligns the settings side onto the engine's. + +**Not a live leak, and not written as one.** The shipped plugin path wires a +real `LocalCryptoProvider` at `kernel:ready` once an `objectql` engine resolves, +so a default deployment never took the base64 branch. What this closes is the +fail-open *direction* on a path an engine-less deployment can still reach. + +**What changed.** A write of a declared-encrypted key (`encrypted: true` or the +manifest's `type: 'password'`, which means "encrypt this") is now refused with +`SettingsCryptoUnavailableError` when the `sys_secret` path is not wired *and* +the inline `CryptoAdapter` declares no confidentiality. The whole batch is +rejected — a plain sibling key in the same patch is not half-written — and one +operator-actionable line is reported through the deployment logger, deduped per +key, so a caller that swallows the error still leaves a trace. Over REST the +refusal answers `500` on the declared envelope, carrying the fix in the message. + +**What did not change.** + +- `NoopCryptoAdapter` remains exported (public API) and its `decrypt()` is + untouched: existing `b64:` rows stay readable, reportable and migratable. The + refusal is write-only — refusing the reads too would strand exactly the data + worth surfacing. +- Injected adapters (`SettingsServicePluginOptions.crypto`) are unaffected. An + adapter declares itself fit to hold a secret with the new optional + `CryptoAdapter.confidential`; **absent means yes**, so every adapter written + before this flag keeps working, and only the base64 default declares `false`. + The exported `providesConfidentiality(adapter)` is the predicate the write + path uses. +- Clearing an encrypted key (writing `null`) is still allowed: there is no + plaintext to protect, and an operator must always be able to REMOVE a value on + a deployment that cannot store one. +- Validation still runs first, so a caller submitting a bad value on a namespace + that happens to carry a secret still gets the field-level diagnostic they can + act on rather than a deployment fault they cannot. diff --git a/packages/services/service-settings/README.md b/packages/services/service-settings/README.md index 90542988c2..4124b7ea43 100644 --- a/packages/services/service-settings/README.md +++ b/packages/services/service-settings/README.md @@ -71,8 +71,20 @@ behind the same `ICryptoProvider` seam via `cryptoProvider` plugin option. > `InMemoryCryptoProvider` is a deprecated alias for `LocalCryptoProvider` > (the old name wrongly implied an ephemeral key). -The legacy `CryptoAdapter` / `NoopCryptoAdapter` (a base64 wrapper) remains -only as a pre-Phase-3 backward-compat path when no `cryptoProvider` is wired. +The legacy `CryptoAdapter` seam remains as a pre-Phase-3 backward-compat path +when no `cryptoProvider` is wired — but only for an adapter that actually +encrypts. `NoopCryptoAdapter` (a base64 wrapper) no longer takes part in a +**write**: since #8026 the write path **fails closed** and refuses to persist a +declared-encrypted value (`encrypted: true` / `type: 'password'`) when the only +thing available is an adapter that provides no confidentiality, matching the +engine's `Field.secret()` posture. Base64 is encoding, not encryption, and a +populated `value_enc` reads as protected to the next author and the next audit. + +The class stays exported and its `decrypt()` is unchanged, so existing `b64:` +rows remain readable and migratable. An adapter declares itself fit to hold a +secret with `confidential: true` (absent means "yes" — only the base64 default +declares `false`), and `providesConfidentiality(adapter)` is the exported +predicate the write path uses. ## Audit diff --git a/packages/services/service-settings/src/crypto-adapter.ts b/packages/services/service-settings/src/crypto-adapter.ts index 82828e8047..d36a7b2582 100644 --- a/packages/services/service-settings/src/crypto-adapter.ts +++ b/packages/services/service-settings/src/crypto-adapter.ts @@ -18,6 +18,24 @@ export interface CryptoAdapter { * operators can correlate value changes without leaking secrets. */ digest(plaintext: string): string; + /** + * Does `encrypt()` actually provide confidentiality (#8026)? + * + * DECLARED, never inferred: the write path cannot tell an AES envelope from + * a base64 wrapper by looking at the output, so an adapter that does not + * protect its input has to say so — and {@link NoopCryptoAdapter} does. + * `false` makes the settings write path refuse to persist a declared-secret + * value through this adapter, matching the engine's `Field.secret()` + * posture (fail-closed, never store something reversible under a name that + * reads as encryption). + * + * OPTIONAL and defaulting to "yes" on purpose: every adapter written before + * this flag existed is a deliberately-injected real one (the base64 default + * is the only implementation in this repo that is not), so absence must not + * start refusing their writes. Opting IN to the refusal is a one-line + * declaration; opting out of protection is not something silence should buy. + */ + readonly confidential?: boolean; } /** @@ -26,8 +44,30 @@ export interface CryptoAdapter { * * Operators are expected to override this via * `SettingsServicePluginOptions.crypto`. + * + * ## What this adapter can and cannot do since #8026 + * + * It still DECODES: `decrypt()` is unchanged, so a deployment that already + * has `b64:` rows can still read them (and migrate them). What it can no + * longer do is take part in a WRITE: `SettingsService` refuses to persist a + * declared-encrypted value when this is the only path available, because + * `'b64:' + base64(x)` is encoding, not encryption — trivially reversible, + * while producing a `value_enc` that looks protected to the next author and + * to the next audit. The refusal restores parity with the engine's + * `Field.secret()` path, which has always thrown rather than persist a secret + * with no CryptoProvider registered. + * + * The class stays exported (public API) and the read half stays useful; a + * subclass that supplies REAL encryption declares `confidential = true` and + * is accepted by the write path like any other adapter. */ export class NoopCryptoAdapter implements CryptoAdapter { + /** + * #8026 — base64 is encoding, not encryption. This is the declaration the + * write path reads before it agrees to store an `encryptedKeys` value. + */ + readonly confidential: boolean = false; + async encrypt(plaintext: string): Promise { return 'b64:' + Buffer.from(plaintext, 'utf8').toString('base64'); } @@ -48,3 +88,22 @@ export class NoopCryptoAdapter implements CryptoAdapter { return 'fnv32:' + h.toString(16).padStart(8, '0'); } } + +/** + * Is this adapter allowed to hold a declared-secret value at rest (#8026)? + * + * Two arms, in this order: + * + * 1. **The declaration wins.** `confidential === false` refuses, `true` + * accepts. That is how a subclass of {@link NoopCryptoAdapter} supplying + * real encryption opts back in, and how any future development-only + * adapter opts out without this function having to know its name. + * 2. **Undeclared falls back to the class identity**, which catches an + * instance of this module's `NoopCryptoAdapter` whose flag was removed or + * overwritten. Anything else undeclared is assumed real — see the + * `confidential` doc for why silence must not refuse. + */ +export function providesConfidentiality(adapter: CryptoAdapter): boolean { + if (typeof adapter.confidential === 'boolean') return adapter.confidential; + return !(adapter instanceof NoopCryptoAdapter); +} diff --git a/packages/services/service-settings/src/index.ts b/packages/services/service-settings/src/index.ts index 09038292a8..2adc461f34 100644 --- a/packages/services/service-settings/src/index.ts +++ b/packages/services/service-settings/src/index.ts @@ -9,6 +9,11 @@ export { SettingsService } from './settings-service.js'; export { type CryptoAdapter, NoopCryptoAdapter, + // #8026 — the predicate the write path asks before it agrees to hold a + // declared-secret value. Published with the interface it reads: an adapter + // author needs to be able to check their own `confidential` declaration the + // same way the service does, rather than re-deriving the two-arm rule. + providesConfidentiality, } from './crypto-adapter.js'; // Default, KMS-free ICryptoProvider. AES-256-GCM keyed off `OS_SECRET_KEY` // (production) or a persisted dev key; fails loud in production rather than @@ -32,6 +37,10 @@ export { type SettingsRow, type SettingsServiceOptions, envKeyOf, + // #8026 — thrown when a declared-encrypted write has nothing able to encrypt + // it. Exported so an in-process caller can branch on the refusal (there is no + // dedicated wire code for it yet; see the class doc). + SettingsCryptoUnavailableError, SettingsLockedError, SettingsValidationError, UnknownKeyError, diff --git a/packages/services/service-settings/src/manifests/ai.manifest.test.ts b/packages/services/service-settings/src/manifests/ai.manifest.test.ts index 5118f212e4..19376a15f0 100644 --- a/packages/services/service-settings/src/manifests/ai.manifest.test.ts +++ b/packages/services/service-settings/src/manifests/ai.manifest.test.ts @@ -3,8 +3,29 @@ import { describe, it, expect } from 'vitest'; import { SettingsManifestSchema } from '@objectstack/spec/system'; import { SettingsService } from '../settings-service.js'; +import type { CryptoAdapter } from '../crypto-adapter.js'; import { aiSettingsManifest, aiTestActionHandler, aiTestEmbedderActionHandler } from './ai.manifest.js'; +/** + * #8026 — the write-door cases below save a complete `openai` provider config, + * which includes `openai_api_key` (a declared secret). Since the settings write + * path fails CLOSED on a secret it cannot protect, the fixture supplies an + * adapter that DECLARES confidentiality; without it these cases would be + * measuring the crypto gate instead of `temperature`'s declared window. + */ +class ConfidentialTestAdapter implements CryptoAdapter { + readonly confidential = true; + async encrypt(plaintext: string): Promise { + return 'kms:' + Buffer.from(plaintext, 'utf8').toString('base64'); + } + async decrypt(ciphertext: string): Promise { + return Buffer.from(ciphertext.replace(/^kms:/, ''), 'base64').toString('utf8'); + } + digest(): string { + return 'fnv32:deadbeef'; + } +} + describe('aiSettingsManifest', () => { it('parses against SettingsManifestSchema', () => { expect(() => SettingsManifestSchema.parse(aiSettingsManifest)).not.toThrow(); @@ -143,7 +164,7 @@ describe('aiSettingsManifest — temperature declares a window, not a grid (#655 // default provider is `memory`, so the patch carries a real provider (and // its required key) — otherwise the TOUCH/visible contract skips the // specifier entirely and this test would be green for the wrong reason. - const svc = new SettingsService({ env: {} }); + const svc = new SettingsService({ env: {}, crypto: new ConfidentialTestAdapter() }); svc.registerManifest(aiSettingsManifest); await expect( svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: 0.15 }), @@ -157,7 +178,7 @@ describe('aiSettingsManifest — temperature declares a window, not a grid (#655 }); it('write door: the window still binds — out-of-range values are refused in the min/max vocabulary', async () => { - const svc = new SettingsService({ env: {} }); + const svc = new SettingsService({ env: {}, crypto: new ConfidentialTestAdapter() }); svc.registerManifest(aiSettingsManifest); await expect( svc.setMany('ai', { provider: 'openai', openai_api_key: 'sk-test', temperature: 2.5 }), diff --git a/packages/services/service-settings/src/settings-crypto-fail-closed.test.ts b/packages/services/service-settings/src/settings-crypto-fail-closed.test.ts new file mode 100644 index 0000000000..28603dc285 --- /dev/null +++ b/packages/services/service-settings/src/settings-crypto-fail-closed.test.ts @@ -0,0 +1,403 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8026 — **the settings write path fails CLOSED on a declared-encrypted key.** + * + * ## What was wrong + * + * `SettingsService` constructed with no `cryptoProvider` + `secretStore` fell + * back to `NoopCryptoAdapter`, whose `encrypt()` is `'b64:' + base64(plain)`. + * That is encoding, not encryption: trivially reversible, and it leaves + * `sys_setting.value_enc` POPULATED — so the row reads as protected to the next + * author and to the next audit while being plaintext with extra steps. + * + * The engine's `Field.secret()` path has always taken the opposite posture: + * with no CryptoProvider registered it THROWS (`engine.ts` — + * `encryptSecretFields`, "Refusing to store cleartext (fail-closed)"), which is + * what makes a provider-less deployment safe to report on rather than silently + * wrong. Two credential-encryption paths, opposite failure modes; this file + * pins the settings side onto the engine's. + * + * ## Not a live-leak card + * + * The shipped plugin path wires a real `LocalCryptoProvider` + * (`settings-service-plugin.ts`, at `kernel:ready` once an `objectql` engine + * resolves). Nothing here closes a leak in a default deployment; it removes the + * fail-OPEN direction from a path an engine-less deployment can still take. + * + * ## Why the harness is a real engine, and why that matters here + * + * The vacuity trap for a pin like this is a fixture that never reaches the + * refused path at all: "no base64 value is persisted" is trivially green when + * nothing was ever going to write one. So the cases below drive a GENUINELY + * provider-less construction — a real `ObjectQL` over the real `SysSetting` + * schema, `SettingsService` with no `crypto`, no `cryptoProvider` and no + * `secretStore` — and assert the PERSISTED STATE off the driver, not just the + * thrown error. On `origin/main` the same fixture writes a `sys_setting` row + * whose `value_enc` is `b64:cmUtc2VjcmV0LTEyMw==`; that positive reading is what + * makes the refusal assertion non-vacuous (see the PR body for the measurement). + */ + +import { describe, expect, it } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SysSetting } from '@objectstack/platform-objects/system'; +import type { SettingsManifest } from '@objectstack/spec/system'; +import type { IHttpServer, IHttpRequest, IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { SettingsService } from './settings-service.js'; +import { registerSettingsRoutes } from './settings-routes.js'; +import { wrapEngineAsSettingsEngine } from './settings-service-plugin.js'; +import { NoopCryptoAdapter, providesConfidentiality, type CryptoAdapter } from './crypto-adapter.js'; +import { SettingsCryptoUnavailableError } from './settings-service.types.js'; + +const OWNER_PACKAGE = 'com.objectstack.test.settings-crypto-fail-closed'; + +const SECRET = 're-secret-123'; +/** Exactly what `NoopCryptoAdapter.encrypt(SECRET)` produced on `origin/main`. */ +const B64_OF_SECRET = 'b64:' + Buffer.from(SECRET, 'utf8').toString('base64'); + +/** + * One encrypted key per flavour plus a plain control. Minimal on purpose: the + * shipped manifests carry `visible` predicates and cross-field `required` + * rules, and this file is about the crypto gate, not `validatePatch`. + */ +const manifest: SettingsManifest = { + namespace: 'crypto_ns', + version: 1, + label: 'Crypto', + scope: 'global', + readPermission: 'setup.access', + writePermission: 'setup.write', + specifiers: [ + // Flavour A — implicitly encrypted because the TYPE is `password`. NB: this + // is the settings manifest's `password`, meaning "encrypt this"; it is NOT + // the objectql `Field` type and has nothing to do with ADR-0100. + { type: 'password', key: 'api_key', label: 'API key', required: false }, + // Flavour B — an ordinary type carrying an explicit `encrypted: true`. + { type: 'text', key: 'webhook_token', label: 'Webhook token', required: false, encrypted: true }, + // Control — never encrypted; must keep writing on a provider-less service. + { type: 'text', key: 'from_email', label: 'From', required: false }, + ], +}; + +type Store = Map>>; + +/** A driver over plain Maps — enough of `IDataDriver` for the settings write path. */ +function makeMemoryDriver() { + const store: Store = new Map(); + let nextId = 0; + const copy = (r: Record) => ({ ...r }); + const rowsOf = (object: string) => { + let s = store.get(object); + if (!s) { s = new Map(); store.set(object, s); } + return s; + }; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]) => (row[k] ?? null) === (v ?? null)); + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, + async execute() { return null; }, + async find(object: string, ast: any) { + return [...rowsOf(object).values()].filter((r) => matches(r, ast?.where)).map(copy); + }, + async findOne(object: string, ast: any) { + for (const r of rowsOf(object).values()) if (matches(r, ast?.where)) return copy(r); + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `row_${nextId}`; + const row = { ...data, id }; + rowsOf(object).set(id, row); + return copy(row); + }, + async update(object: string, id: string, data: Record) { + const s = rowsOf(object); + const cur = s.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id }; + s.set(id, next); + return copy(next); + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + return id && rowsOf(object).has(id) ? this.update(object, id, data) : this.create(object, data); + }, + async delete(object: string, id: string) { return rowsOf(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + const s = rowsOf(object); + for (const r of rows) s.set(r.id as string, { ...s.get(r.id as string), ...data, id: r.id }); + return rows.length; + }, + async deleteMany(object: string, ast: any) { + const rows = await this.find(object, ast); + for (const r of rows) rowsOf(object).delete(r.id as string); + return rows.length; + }, + async syncSchema() {}, async dropTable() {}, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, rowsOf }; +} + +/** + * A real engine, the real adapter, and a service wired with NOTHING that can + * encrypt — the engine-less-deployment shape the card is about, except that the + * engine IS here so the persisted rows are inspectable. `opts.crypto` is left + * undefined, so the constructor takes the `NoopCryptoAdapter` default: this is + * the fallback under test, reached the way production would reach it. + */ +async function bootProviderless(opts: { crypto?: CryptoAdapter } = {}) { + const engine = new ObjectQL(); + const { driver, rowsOf } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(SysSetting as any, OWNER_PACKAGE); + + const logged: string[] = []; + const svc = new SettingsService({ + env: {}, + engine: wrapEngineAsSettingsEngine(engine as any), + ...(opts.crypto ? { crypto: opts.crypto } : {}), + logger: { error: (m) => { logged.push(m); } }, + }); + svc.registerManifest(manifest); + + const settingRows = () => [...rowsOf('sys_setting').values()]; + const rowFor = (key: string) => settingRows().find((r) => r.key === key); + return { svc, settingRows, rowFor, logged }; +} + +// --------------------------------------------------------------------------- +// 1. The refusal, and the state it leaves behind +// --------------------------------------------------------------------------- + +describe('#8026 — a declared-encrypted write is refused when nothing can encrypt it', () => { + it('rejects with SETTINGS_CRYPTO_UNAVAILABLE and persists NO row at all', async () => { + const { svc, settingRows, rowFor } = await bootProviderless(); + + const err = await svc + .setMany('crypto_ns', { api_key: SECRET }) + .then(() => null, (e) => e); + + expect(err).toBeInstanceOf(SettingsCryptoUnavailableError); + expect(err.code).toBe('SETTINGS_CRYPTO_UNAVAILABLE'); + expect(err.namespace).toBe('crypto_ns'); + expect(err.key).toBe('api_key'); + + // The half that cannot be faked by a thrown error: nothing reached storage. + // On `origin/main` this same fixture leaves one row whose `value_enc` is + // exactly B64_OF_SECRET — which is why this assertion is not vacuous. + expect(rowFor('api_key')).toBeUndefined(); + expect(JSON.stringify(settingRows())).not.toContain(B64_OF_SECRET); + expect(JSON.stringify(settingRows())).not.toContain(SECRET); + + // And the read side agrees the value was never configured. + const read = await svc.get('crypto_ns', 'api_key'); + expect(read.value ?? null).toBeNull(); + }); + + it('refuses flavour B too — `encrypted: true` on an ordinary type', async () => { + const { svc, rowFor } = await bootProviderless(); + await expect(svc.set('crypto_ns', 'webhook_token', 'whsec_live')).rejects.toBeInstanceOf( + SettingsCryptoUnavailableError, + ); + expect(rowFor('webhook_token')).toBeUndefined(); + }); + + it('an explicitly injected NoopCryptoAdapter is refused just the same', async () => { + // Provenance-independent on purpose: the fail-open path must not be + // reachable by one line of caller code. The escape hatch is an adapter that + // DECLARES confidentiality, not one that arrives by a different route. + const { svc, rowFor } = await bootProviderless({ crypto: new NoopCryptoAdapter() }); + await expect(svc.set('crypto_ns', 'api_key', SECRET)).rejects.toBeInstanceOf( + SettingsCryptoUnavailableError, + ); + expect(rowFor('api_key')).toBeUndefined(); + }); + + it('reports one operator-actionable line through the deployment logger', async () => { + // The throw reaches the caller; the LOG is what reaches the operator when + // the caller swallows it. Deduped per key, like the #5204 env reporter. + const { svc, logged } = await bootProviderless(); + await svc.set('crypto_ns', 'api_key', SECRET).catch(() => {}); + await svc.set('crypto_ns', 'api_key', SECRET).catch(() => {}); + + expect(logged).toHaveLength(1); + expect(logged[0]).toContain("Cannot persist encrypted setting 'crypto_ns.api_key'"); + expect(logged[0]).toContain('fail-closed'); + // Never echo the secret itself into the log pipeline. + expect(logged[0]).not.toContain(SECRET); + }); + + it('rejects the WHOLE batch — a plain sibling key is not half-written', async () => { + // The pre-flight arm. Without it the write loop would persist `from_email` + // and then throw on `api_key`, leaving the namespace half-applied. + const { svc, rowFor } = await bootProviderless(); + await expect( + svc.setMany('crypto_ns', { from_email: 'ops@example.com', api_key: SECRET }), + ).rejects.toBeInstanceOf(SettingsCryptoUnavailableError); + expect(rowFor('from_email')).toBeUndefined(); + expect(rowFor('api_key')).toBeUndefined(); + }); +}); + +// --------------------------------------------------------------------------- +// 2. What the refusal must NOT break +// --------------------------------------------------------------------------- + +describe('#8026 — the refusal is scoped to secrets that would be stored unprotected', () => { + it('non-encrypted keys still write on a provider-less service', async () => { + const { svc, rowFor } = await bootProviderless(); + await svc.set('crypto_ns', 'from_email', 'ops@example.com'); + expect(rowFor('from_email')?.value).toBe('ops@example.com'); + expect((await svc.get('crypto_ns', 'from_email')).value).toBe('ops@example.com'); + }); + + it('CLEARING an encrypted key is still allowed — there is no plaintext to protect', async () => { + // A fail-closed path that also refuses `null` would trap an operator on a + // provider-less deployment: unable to store a secret AND unable to remove + // one already stored. + const { svc, rowFor } = await bootProviderless(); + await svc.set('crypto_ns', 'api_key', null); + expect(rowFor('api_key')?.value_enc ?? null).toBeNull(); + expect((await svc.get('crypto_ns', 'api_key')).value ?? null).toBeNull(); + }); + + it('an adapter that DECLARES confidentiality still persists', async () => { + // The injected-adapter seam (`SettingsServicePluginOptions.crypto`) is + // untouched: a KMS-backed adapter is trusted on its declaration. + const kms: CryptoAdapter = { + confidential: true, + encrypt: async (p) => 'kms:' + Buffer.from(p, 'utf8').toString('base64'), + decrypt: async (c) => Buffer.from(c.replace(/^kms:/, ''), 'base64').toString('utf8'), + digest: () => 'fnv32:deadbeef', + }; + const { svc, rowFor } = await bootProviderless({ crypto: kms }); + await svc.set('crypto_ns', 'api_key', SECRET); + expect(String(rowFor('api_key')?.value_enc)).toMatch(/^kms:/); + expect((await svc.get('crypto_ns', 'api_key')).value).toBe(SECRET); + }); + + it('legacy `b64:` rows stay READABLE — the refusal is write-only', async () => { + // The Noop adapter keeps decoding. A deployment that already wrote such + // rows must still be able to read, report and migrate them; refusing the + // read too would strand precisely the data this card wants surfaced. + const noop: CryptoAdapter = new NoopCryptoAdapter(); + expect(await noop.decrypt(B64_OF_SECRET, { namespace: 'crypto_ns', key: 'api_key' })).toBe(SECRET); + + const { svc } = await bootProviderless(); + // Seed the row the OLD code would have written, straight past the gate. + await (svc as any).upsertRow({ + namespace: 'crypto_ns', + key: 'api_key', + scope: 'global', + user_id: null, + value: null, + value_enc: B64_OF_SECRET, + encrypted: true, + }); + expect((await svc.get('crypto_ns', 'api_key')).value).toBe(SECRET); + }); +}); + +// --------------------------------------------------------------------------- +// 3. The declaration the gate reads +// --------------------------------------------------------------------------- + +describe('#8026 — providesConfidentiality', () => { + it('NoopCryptoAdapter declares false; an undeclared adapter is assumed real', () => { + expect(providesConfidentiality(new NoopCryptoAdapter())).toBe(false); + const legacy = { + encrypt: async (p: string) => p, + decrypt: async (c: string) => c, + digest: () => 'fnv32:00000000', + } satisfies CryptoAdapter; + // Silence must not start refusing adapters written before the flag existed; + // they are deliberately-injected real ones. Opting IN is one line. + expect(providesConfidentiality(legacy)).toBe(true); + }); + + it('the declaration wins over the class — a real subclass opts back in', () => { + class RealSubclass extends NoopCryptoAdapter { + override readonly confidential = true; + override async encrypt(plaintext: string): Promise { + return 'kms:' + plaintext; + } + } + expect(providesConfidentiality(new RealSubclass())).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// 4. The wire +// --------------------------------------------------------------------------- + +class MockHttp implements IHttpServer { + routes = new Map(); + private add(method: string, path: string, handler: RouteHandler) { + this.routes.set(`${method} ${path}`, handler); + } + get(path: string, h: RouteHandler) { this.add('GET', path, h); return this as any; } + post(path: string, h: RouteHandler) { this.add('POST', path, h); return this as any; } + put(path: string, h: RouteHandler) { this.add('PUT', path, h); return this as any; } + delete(path: string, h: RouteHandler) { this.add('DELETE', path, h); return this as any; } + patch(path: string, h: RouteHandler) { this.add('PATCH', path, h); return this as any; } + use() { return this as any; } + listen() { return Promise.resolve(); } + close() { return Promise.resolve(); } + getInstance() { return null; } +} + +describe('#8026 — the refusal on the REST boundary', () => { + it('PUT answers the declared envelope: status 500, code INTERNAL_ERROR, actionable message', async () => { + // There is no dedicated wire code yet — `SETTINGS_CRYPTO_UNAVAILABLE` would + // have to be registered in `ERROR_CODE_LEDGER` (`packages/spec`) first, and + // that is out of this card's scope. So the refusal takes the same + // `500 INTERNAL_ERROR` arm every unmapped service error takes; what this + // case pins is that it does so INSIDE the envelope, carrying the operator's + // fix, and that the request wrote nothing. + const { svc, rowFor } = await bootProviderless(); + const http = new MockHttp(); + registerSettingsRoutes(http, svc, { + contextFromRequest: () => ({ enforced: true, permissions: ['setup.access', 'setup.write'] }), + }); + + const state: { status: number; body?: any } = { status: 200 }; + const req = { + params: { namespace: 'crypto_ns' }, + query: {}, + body: { api_key: SECRET }, + headers: {}, + method: 'PUT', + path: '/api/settings/crypto_ns', + } as unknown as IHttpRequest; + const res = { + json: (data: any) => { state.body = data; }, + send: () => {}, + status: (code: number) => { state.status = code; return res; }, + header: () => res, + } as unknown as IHttpResponse; + + await http.routes.get('PUT /api/settings/:namespace')!(req, res); + + expect(state.status).toBe(500); + expect(state.body.success).toBe(false); + expect(state.body.error.code).toBe('INTERNAL_ERROR'); + expect(state.body.error.message).toContain('fail-closed'); + expect(state.body.error.message).toContain('cryptoProvider'); + // Nothing leaked the submitted secret back over the wire, and nothing landed. + expect(JSON.stringify(state.body)).not.toContain(SECRET); + expect(rowFor('api_key')).toBeUndefined(); + }); +}); diff --git a/packages/services/service-settings/src/settings-service.test.ts b/packages/services/service-settings/src/settings-service.test.ts index 0aa45ed325..764dc84c71 100644 --- a/packages/services/service-settings/src/settings-service.test.ts +++ b/packages/services/service-settings/src/settings-service.test.ts @@ -2,8 +2,14 @@ import { describe, expect, it, vi } from 'vitest'; import { SettingsService } from './settings-service.js'; -import { SettingsLockedError, UnknownKeyError, UnknownNamespaceError, envKeyOf } from './settings-service.types.js'; -import { NoopCryptoAdapter } from './crypto-adapter.js'; +import { + SettingsCryptoUnavailableError, + SettingsLockedError, + UnknownKeyError, + UnknownNamespaceError, + envKeyOf, +} from './settings-service.types.js'; +import type { CryptoAdapter } from './crypto-adapter.js'; import { mailSettingsManifest, mailTestActionHandler } from './manifests/mail.manifest.js'; import { aiSettingsManifest } from './manifests/ai.manifest.js'; import { authSettingsManifest } from './manifests/auth.manifest.js'; @@ -89,15 +95,57 @@ describe('SettingsService — resolver precedence', () => { }); }); +/** + * Stands in for a real, injected `CryptoAdapter` (a KMS-backed one in + * production). What matters to the write path is the DECLARATION — an adapter + * that says it protects its input is trusted to, exactly as a KMS adapter is — + * so the wrapping itself is deliberately trivial and reversible in-test. + */ +class ReversibleTestAdapter implements CryptoAdapter { + readonly confidential = true; + async encrypt(plaintext: string): Promise { + return 'kms:' + Buffer.from(plaintext, 'utf8').toString('base64'); + } + async decrypt(ciphertext: string): Promise { + return Buffer.from(ciphertext.replace(/^kms:/, ''), 'base64').toString('utf8'); + } + digest(): string { + return 'fnv32:deadbeef'; + } +} + describe('SettingsService — encryption round-trip', () => { + // #8026 — this case used to inject `new NoopCryptoAdapter()`, which the write + // path now REFUSES (base64 is encoding, not encryption). Re-pointed rather + // than deleted or merely re-spelled: its subject is the injected-adapter seam + // (`opts.crypto` still holds and returns an encrypted value), and a fixture + // pinned to the refused limb would have kept passing only because nothing was + // being stored. The refusal itself is pinned in + // `settings-crypto-fail-closed.test.ts`; what stays here is the positive half. it('persists encrypted=true values via crypto adapter', async () => { - const svc = new SettingsService({ env: {}, crypto: new NoopCryptoAdapter() }); + const svc = new SettingsService({ env: {}, crypto: new ReversibleTestAdapter() }); svc.registerManifest(mailSettingsManifest); await svc.setMany('mail', { provider: 'resend', api_key: 're-secret-123', from_email: 'a@b.com' }); const ns = await svc.getNamespace('mail'); expect(ns.values.api_key.value).toBe('re-secret-123'); expect(ns.values.api_key.source).toBe('global'); }); + + it('an adapter that declares no confidentiality is refused, whatever its class', async () => { + // Not a NoopCryptoAdapter — the gate reads the DECLARATION, so a bespoke + // development adapter opting out is refused on the same terms. + const declaredWeak: CryptoAdapter = { + confidential: false, + encrypt: async (p) => 'weak:' + p, + decrypt: async (c) => c.replace(/^weak:/, ''), + digest: () => 'fnv32:00000000', + }; + const svc = new SettingsService({ env: {}, crypto: declaredWeak }); + svc.registerManifest(mailSettingsManifest); + await expect( + svc.setMany('mail', { provider: 'resend', api_key: 're-secret-123', from_email: 'a@b.com' }), + ).rejects.toBeInstanceOf(SettingsCryptoUnavailableError); + }); }); describe('SettingsService — global scope', () => { @@ -140,6 +188,10 @@ describe('SettingsService — audit sink', () => { const events: any[] = []; const svc = new SettingsService({ env: {}, + // #8026 — the subject is the audit event's masked digest, which only + // exists for a value that was actually persisted; the adapter declares + // confidentiality so the write is allowed to happen at all. + crypto: new ReversibleTestAdapter(), audit: { record: (e) => { events.push(e); @@ -324,7 +376,11 @@ describe('SettingsService — resetNamespace / built-in reset action', () => { describe('SettingsService — save-time validation (required/visible/pattern)', () => { function aiService(): SettingsService { - const svc = new SettingsService({ env: {} }); + // #8026 — same reason as `mailService` below: the provider configs these + // cases save carry a declared secret (`cloudflare_api_key`), which the + // write path will not persist through an adapter that provides no + // confidentiality. The subject here is required/visible validation. + const svc = new SettingsService({ env: {}, crypto: new ReversibleTestAdapter() }); svc.registerManifest(aiSettingsManifest); return svc; } @@ -439,7 +495,12 @@ describe('SettingsService — save-time validation (required/visible/pattern)', */ describe('SettingsService — save-time validation (declared options are enforced)', () => { const mailService = () => { - const svc = new SettingsService({ env: {} }); + // #8026 — these cases write `api_key` (a declared secret) only as part of a + // complete provider config; their subject is the option table. The write + // path now refuses a secret when nothing can encrypt it, so the fixture + // declares an adapter that can, and the option-table assertions stay about + // the option table. + const svc = new SettingsService({ env: {}, crypto: new ReversibleTestAdapter() }); svc.registerManifest(mailSettingsManifest); return svc; }; @@ -488,7 +549,9 @@ describe('SettingsService — save-time validation (declared options are enforce // A workspace that saved `sendgrid` while the option existed still carries // it. Simulated exactly as it happened: write under the OLD table, then // re-register the narrowed manifest (#5094) over the same namespace. - const svc = new SettingsService({ env: {} }); + // (#8026 — `crypto` for the same reason as `mailService`: the seed write + // carries `api_key`.) + const svc = new SettingsService({ env: {}, crypto: new ReversibleTestAdapter() }); svc.registerManifest({ ...mailSettingsManifest, specifiers: mailSettingsManifest.specifiers.map((s: any) => diff --git a/packages/services/service-settings/src/settings-service.ts b/packages/services/service-settings/src/settings-service.ts index 1704c42de7..36df9b856e 100644 --- a/packages/services/service-settings/src/settings-service.ts +++ b/packages/services/service-settings/src/settings-service.ts @@ -15,6 +15,7 @@ import type { import { type CryptoAdapter, NoopCryptoAdapter, + providesConfidentiality, } from './crypto-adapter.js'; import { type SettingsActionHandler, @@ -24,6 +25,7 @@ import { type SettingsRow, type SettingsServiceOptions, envKeyOf, + SettingsCryptoUnavailableError, SettingsForbiddenError, SettingsLockedError, SettingsValidationError, @@ -538,6 +540,13 @@ export class SettingsService { * the value is part of the key and not just the var name. */ private readonly reportedEnvOverrides = new Set(); + /** + * `.` pairs whose fail-closed encryption refusal (#8026) has + * already been reported to the logger. Deduped for the same reason + * {@link reportedEnvOverrides} is: a settings form that retries a save would + * otherwise repeat one operator-actionable line per attempt. + */ + private readonly reportedCryptoRefusals = new Set(); /** In-memory fallback when no engine is wired. */ private readonly memory: SettingsRow[] = []; /** Change subscribers, optionally scoped to a namespace. */ @@ -1245,6 +1254,49 @@ export class SettingsService { // Mutations // --------------------------------------------------------------------- + /** + * Fail-closed gate for `encryptedKeys` writes (#8026). + * + * Refuses when BOTH credential paths are unavailable: no + * `cryptoProvider` + `secretStore` pair (the Phase 3 `sys_secret` path), and + * an inline {@link CryptoAdapter} that declares no confidentiality — i.e. + * the `NoopCryptoAdapter` default, whose `encrypt()` is base64. Anything + * that CAN protect the value is left alone: an injected KMS-backed adapter + * keeps working exactly as before, and so does the `sys_secret` path the + * shipped plugin wires. + * + * ## Boot vs write + * + * The card asked for this to be loud at boot. It cannot be *refused* at + * boot: `SettingsServicePlugin` constructs the service in `init()` and binds + * the real provider later, at `kernel:ready`, so a construction-time throw + * would refuse every shipped deployment — and a namespace with no encrypted + * specifier needs no provider at all. The refusal therefore lands at the + * write, where the fact is finally knowable, and the LOUDNESS lands here: + * one operator-actionable line through the deployment's own logger the first + * time a given key is refused. A caller that swallows the thrown error still + * leaves that line behind. + * + * Reads are deliberately NOT gated: `NoopCryptoAdapter.decrypt` still + * decodes existing `b64:` rows, so a deployment that already wrote some can + * still read them, report them, and migrate them. Refusing those reads too + * would strand exactly the data this refusal exists to stop producing. + */ + private assertEncryptionAvailable(namespace: string, key: string): void { + if (this.cryptoProvider && this.secretStore) return; + if (providesConfidentiality(this.crypto)) return; + + const err = new SettingsCryptoUnavailableError(namespace, key); + const dedupeAt = `${namespace}.${key}`; + if (!this.reportedCryptoRefusals.has(dedupeAt)) { + this.reportedCryptoRefusals.add(dedupeAt); + const message = `[SettingsService] ${err.message}`; + if (this.logger?.error) this.logger.error(message); + else console.error(message); + } + throw err; + } + /** Persist a single key. Throws SettingsLockedError when env-locked. */ async set( namespace: string, @@ -1305,6 +1357,26 @@ export class SettingsService { // validatePatch for the exact semantics. await this.validatePatch(namespace, patch, ctx); + // #8026 — a declared-encrypted key with nothing able to encrypt it fails + // the WHOLE batch, here, rather than part-way down the write loop below. + // The persist site enforces the same rule (that is the load-bearing half); + // this pass is what keeps a refused batch from leaving the namespace + // half-written. + // + // Ordered AFTER `validatePatch` deliberately. Both refuse the whole batch, + // but they address different people: a validation error names something the + // CALLER can fix in the form they are looking at, while this one names a + // deployment the caller cannot reconfigure. Checked first, it would mask + // every field-level diagnostic on a namespace that happens to carry a + // secret. Clearing a key (null/undefined) is exempt throughout: there is no + // plaintext to protect, and an operator must always be able to REMOVE a + // value on a deployment that cannot store one. + for (const [key, submitted] of Object.entries(patch)) { + if (submitted === null || typeof submitted === 'undefined') continue; + if (!reg.encryptedKeys.has(key)) continue; + this.assertEncryptionAvailable(namespace, key); + } + for (const [key, rawValue] of Object.entries(patch)) { const scope = reg.scopes.get(key)!; // global rows are platform-wide (tenant_id=null, user_id=null); @@ -1343,6 +1415,13 @@ export class SettingsService { storedEnc = handle.id; digest = this.cryptoProvider.digest(plain); } else { + // #8026 — the legacy inline-adapter path persists only through an + // adapter that declares real confidentiality. The base64 default + // is refused here rather than silently writing a reversible + // `value_enc` that reads as protected. Kept AT the write (not only + // in the pre-flight above) so no future caller can reach this + // branch and fall open. + this.assertEncryptionAvailable(namespace, key); storedEnc = await this.crypto.encrypt(plain, { namespace, key }); digest = this.crypto.digest(plain); } diff --git a/packages/services/service-settings/src/settings-service.types.ts b/packages/services/service-settings/src/settings-service.types.ts index 7fe763fe7f..a3ca335927 100644 --- a/packages/services/service-settings/src/settings-service.types.ts +++ b/packages/services/service-settings/src/settings-service.types.ts @@ -310,6 +310,47 @@ export class UnknownKeyError extends Error { } } +/** + * Thrown when a write would persist a declared-encrypted value (`encrypted: + * true` or `type: 'password'`) and nothing able to encrypt it is wired (#8026). + * + * ## Why this is a refusal and not a fallback + * + * The path this replaces persisted `'b64:' + base64(plaintext)` through + * `NoopCryptoAdapter` — encoding, not encryption, and worse than plaintext in + * one specific way: `sys_setting.value_enc` comes back populated, so both the + * next author and the next audit read the row as protected. The engine's + * `Field.secret()` path has always thrown here instead, which is what makes a + * provider-less deployment safe to REPORT on rather than silently wrong. This + * error is the settings side taking the same posture. + * + * ## Wire spelling + * + * Not mapped to a dedicated HTTP status/code by `settings-routes.ts`: it + * surfaces on the `500 INTERNAL_ERROR` arm every unmapped service error takes, + * carrying this message. That is deliberate for now — a dedicated + * `SETTINGS_CRYPTO_UNAVAILABLE` on the wire has to be registered in + * `ERROR_CODE_LEDGER` (`packages/spec`) first, or it is the "silent fourth + * state" ADR-0112 forbids, and that registration is out of this card's scope. + * `code` is therefore an IN-PROCESS discriminator today: a plugin calling + * `settings.setMany` branches on it exactly as it does on `SETTINGS_LOCKED`. + */ +export class SettingsCryptoUnavailableError extends Error { + readonly code = 'SETTINGS_CRYPTO_UNAVAILABLE' as const; + constructor( + readonly namespace: string, + readonly key: string, + ) { + super( + `Cannot persist encrypted setting '${namespace}.${key}': no CryptoProvider is wired ` + + 'and the configured CryptoAdapter declares no confidentiality (base64 is encoding, ' + + 'not encryption). Wire SettingsServicePluginOptions.cryptoProvider ' + + '(LocalCryptoProvider in dev, a KMS/Vault provider in production), or inject a real ' + + '`crypto` adapter. Refusing to store a reversible value (fail-closed).', + ); + } +} + /** * [Finding-1] Thrown when an ENFORCED (HTTP-boundary) caller lacks the * capability a manifest declares for the operation — `readPermission` for