diff --git a/apps/server/src/auth/ServerSecretStore.test.ts b/apps/server/src/auth/ServerSecretStore.test.ts index 539b35dc8f..af7bc0774d 100644 --- a/apps/server/src/auth/ServerSecretStore.test.ts +++ b/apps/server/src/auth/ServerSecretStore.test.ts @@ -3,6 +3,7 @@ import { assert, it } from "@effect/vitest"; import * as Cause from "effect/Cause"; import * as Deferred from "effect/Deferred"; import * as Effect from "effect/Effect"; +import * as Fiber from "effect/Fiber"; import * as FileSystem from "effect/FileSystem"; import * as Layer from "effect/Layer"; import * as Option from "effect/Option"; @@ -185,6 +186,57 @@ it.layer(NodeServices.layer)("ServerSecretStore.layer", (it) => { }).pipe(Effect.provide(layerConcurrentCreateSecretStore())), ); + it.effect("never exposes a secret before its bytes are written", () => + Effect.gen(function* () { + const writing = yield* Deferred.make(); + const readDone = yield* Deferred.make(); + const layerPausedWriteFileSystem = Layer.effect( + FileSystem.FileSystem, + Effect.gen(function* () { + const fileSystem = yield* FileSystem.FileSystem; + + return { + ...fileSystem, + open: (path, options) => + fileSystem.open(path, options).pipe( + Effect.map((file) => + Object.assign(Object.create(file) as FileSystem.File, { + writeAll: (buffer: Uint8Array) => + Deferred.succeed(writing, void 0).pipe( + Effect.andThen(Deferred.await(readDone)), + Effect.andThen(file.writeAll(buffer)), + ), + }), + ), + ), + } satisfies FileSystem.FileSystem; + }), + ).pipe(Layer.provide(NodeServices.layer)); + + yield* Effect.gen(function* () { + const secretStore = yield* ServerSecretStore.ServerSecretStore; + const value = Uint8Array.from([1, 2, 3]); + + const creating = yield* Effect.forkChild(secretStore.create("session-signing-key", value)); + yield* Deferred.await(writing); + const midWrite = yield* secretStore.get("session-signing-key"); + yield* Deferred.succeed(readDone, void 0); + yield* Fiber.join(creating); + + assert.isTrue(Option.isNone(midWrite)); + const persisted = Option.getOrThrow(yield* secretStore.get("session-signing-key")); + assert.deepEqual(Array.from(persisted), Array.from(value)); + }).pipe( + Effect.provide( + ServerSecretStore.layer.pipe( + Layer.provide(layerServerConfig()), + Layer.provideMerge(layerPausedWriteFileSystem), + ), + ), + ); + }).pipe(Effect.provide(NodeServices.layer)), + ); + it.effect("uses restrictive permissions for the secret directory and files", () => Effect.gen(function* () { const chmodCalls: Array<{ readonly path: string; readonly mode: number }> = []; diff --git a/apps/server/src/auth/ServerSecretStore.ts b/apps/server/src/auth/ServerSecretStore.ts index 1ee11d3e19..8f20f22b7d 100644 --- a/apps/server/src/auth/ServerSecretStore.ts +++ b/apps/server/src/auth/ServerSecretStore.ts @@ -224,26 +224,42 @@ export const make = Effect.gen(function* () { ); }; + // Writes the complete secret to a temporary file, then hard-links it into place. The link + // fails with AlreadyExists when another creator won, and a reader never sees partial bytes. const create: ServerSecretStore["Service"]["create"] = (name, value) => { const secretPath = resolveSecretPath(name); - return Effect.scoped( - Effect.gen(function* () { - const file = yield* fileSystem.open(secretPath, { - flag: "wx", - mode: 0o600, - }); - yield* file.writeAll(value); - yield* file.sync; - yield* fileSystem.chmod(secretPath, 0o600); - }), - ).pipe( + return crypto.randomUUIDv4.pipe( Effect.mapError( (cause) => - new SecretStorePersistError({ + new SecretStoreTemporaryPathError({ resource: `secret ${name}`, cause, }), ), + Effect.flatMap((uuid) => { + const tempPath = `${secretPath}.${uuid}.tmp`; + return Effect.scoped( + Effect.gen(function* () { + const file = yield* fileSystem.open(tempPath, { + flag: "wx", + mode: 0o600, + }); + yield* file.writeAll(value); + yield* file.sync; + }), + ).pipe( + Effect.andThen(fileSystem.chmod(tempPath, 0o600)), + Effect.andThen(fileSystem.link(tempPath, secretPath)), + Effect.ensuring(fileSystem.remove(tempPath).pipe(Effect.ignore)), + Effect.mapError( + (cause) => + new SecretStorePersistError({ + resource: `secret ${name}`, + cause, + }), + ), + ); + }), ); }; diff --git a/docs/fork-features.md b/docs/fork-features.md index 57cd68ae6c..ae8767f463 100644 --- a/docs/fork-features.md +++ b/docs/fork-features.md @@ -1313,3 +1313,21 @@ Child creation paths are owned by child-threads and shared with thread-people. "keywords": ["realPath", "/private/var", "npm-global"] } ``` + +## secret-store-atomic-create + +```json +{ + "id": "secret-store-atomic-create", + "purpose": "ServerSecretStore.create writes the complete secret to a temporary file and hard-links it into place, so a creator that loses the race never reads a half-written secret. Drop when upstream makes create atomic.", + "issues": ["https://github.com/toolboxmd/chromeria/issues/249"], + "prs": ["https://github.com/toolboxmd/chromeria/pull/261"], + "newFiles": [], + "upstreamFiles": [ + "apps/server/src/auth/ServerSecretStore.test.ts", + "apps/server/src/auth/ServerSecretStore.ts" + ], + "sharedFiles": [], + "keywords": ["getOrCreateRandom", "fileSystem.link", "half-written secret"] +} +``` diff --git a/scripts/fork-upstream-edits.txt b/scripts/fork-upstream-edits.txt index ef88bf33e9..a122467f0d 100644 --- a/scripts/fork-upstream-edits.txt +++ b/scripts/fork-upstream-edits.txt @@ -198,3 +198,5 @@ apps/server/src/orchestration-v2/Adapters/GrokAdapterV2.ts apps/web/src/components/chat/MermaidDiagram.tsx apps/server/src/cli/invocation.test.ts apps/server/src/environment/ServerEnvironment.test.ts +apps/server/src/auth/ServerSecretStore.test.ts +apps/server/src/auth/ServerSecretStore.ts