diff --git a/apps/server/src/auth/ServerSecretStore.test.ts b/apps/server/src/auth/ServerSecretStore.test.ts index 08a33e2fde3c5..fa216177f494a 100644 --- a/apps/server/src/auth/ServerSecretStore.test.ts +++ b/apps/server/src/auth/ServerSecretStore.test.ts @@ -113,6 +113,55 @@ const layerConcurrentCreateSecretStore = () => Layer.provideMerge(layerConcurrentReadMissFileSystem), ); +// Both callers read the empty secret before either repairs it, and the second removal of the +// secret file waits until the first caller has returned its key. +const layerConcurrentEmptyRepairFileSystem = (firstReturned: Deferred.Deferred) => + Layer.effect( + FileSystem.FileSystem, + Effect.gen(function* () { + const fileSystem = yield* FileSystem.FileSystem; + const readCountRef = yield* Ref.make(0); + const removeCountRef = yield* Ref.make(0); + const readBarrier = yield* Deferred.make(); + const isSecretPath = (path: string) => /[\\/]session-signing-key\.bin$/.test(path); + + return { + ...fileSystem, + readFile: (path) => + isSecretPath(path) + ? Ref.updateAndGet(readCountRef, (count) => count + 1).pipe( + Effect.flatMap((count) => + count > 2 + ? fileSystem.readFile(path) + : Effect.gen(function* () { + if (count === 2) { + yield* Deferred.succeed(readBarrier, void 0); + } + yield* Deferred.await(readBarrier); + return yield* fileSystem.readFile(path); + }), + ), + ) + : fileSystem.readFile(path), + remove: (path, options) => + isSecretPath(path) + ? Ref.updateAndGet(removeCountRef, (count) => count + 1).pipe( + Effect.flatMap((count) => + count > 1 ? Deferred.await(firstReturned) : Effect.void, + ), + Effect.flatMap(() => fileSystem.remove(path, options)), + ) + : fileSystem.remove(path, options), + } satisfies FileSystem.FileSystem; + }), + ).pipe(Layer.provide(Layer.mergeAll(NodeServices.layer, NodeCrypto.layer))); + +const layerConcurrentEmptyRepairSecretStore = (firstReturned: Deferred.Deferred) => + ServerSecretStore.layer.pipe( + Layer.provide(layerServerConfig()), + Layer.provideMerge(layerConcurrentEmptyRepairFileSystem(firstReturned)), + ); + it.layer(Layer.mergeAll(NodeServices.layer, NodeCrypto.layer))("ServerSecretStore.layer", (it) => { it.effect("reuses an existing secret instead of regenerating it", () => Effect.gen(function* () { @@ -125,6 +174,40 @@ it.layer(Layer.mergeAll(NodeServices.layer, NodeCrypto.layer))("ServerSecretStor }).pipe(Effect.provide(layerServerSecretStore())), ); + it.effect("replaces an empty persisted secret with a fresh random one", () => + Effect.gen(function* () { + const secretStore = yield* ServerSecretStore.ServerSecretStore; + yield* secretStore.set("session-signing-key", new Uint8Array()); + + const secret = yield* secretStore.getOrCreateRandom("session-signing-key", 32); + const persisted = yield* secretStore.get("session-signing-key"); + + assert.equal(secret.byteLength, 32); + assert.deepEqual(Array.from(Option.getOrThrow(persisted)), Array.from(secret)); + }).pipe(Effect.provide(layerServerSecretStore())), + ); + + it.effect("keeps the repaired secret when concurrent callers find it empty", () => + Effect.gen(function* () { + const firstReturned = yield* Deferred.make(); + yield* Effect.gen(function* () { + const secretStore = yield* ServerSecretStore.ServerSecretStore; + yield* secretStore.set("session-signing-key", new Uint8Array()); + const getOrCreate = secretStore + .getOrCreateRandom("session-signing-key", 32) + .pipe(Effect.tap(() => Deferred.succeed(firstReturned, void 0))); + + const [first, second] = yield* Effect.all([getOrCreate, getOrCreate], { + concurrency: "unbounded", + }); + const persisted = Option.getOrThrow(yield* secretStore.get("session-signing-key")); + + assert.deepEqual(Array.from(first), Array.from(persisted)); + assert.deepEqual(Array.from(second), Array.from(persisted)); + }).pipe(Effect.provide(layerConcurrentEmptyRepairSecretStore(firstReturned))); + }), + ); + it.effect("returns the persisted secret when concurrent creators race", () => Effect.gen(function* () { const secretStore = yield* ServerSecretStore.ServerSecretStore; diff --git a/apps/server/src/auth/ServerSecretStore.ts b/apps/server/src/auth/ServerSecretStore.ts index e950acb38a71d..cabded3d0c7b0 100644 --- a/apps/server/src/auth/ServerSecretStore.ts +++ b/apps/server/src/auth/ServerSecretStore.ts @@ -8,6 +8,7 @@ import * as Path from "effect/Path"; import * as Predicate from "effect/Predicate"; import * as PlatformError from "effect/PlatformError"; import * as Schema from "effect/Schema"; +import * as Semaphore from "effect/Semaphore"; import * as ServerConfig from "../config.ts"; @@ -169,6 +170,9 @@ export const make = Effect.gen(function* () { ); const resolveSecretPath = (name: string) => path.join(serverConfig.secretsDir, `${name}.bin`); + const emptySecretRepair = yield* Semaphore.make(1); + const isMissingOrNonEmpty = (secret: Option.Option) => + Option.isNone(secret) || secret.value.byteLength > 0; const get: ServerSecretStore["Service"]["get"] = (name) => fileSystem.readFile(resolveSecretPath(name)).pipe( @@ -263,6 +267,19 @@ export const make = Effect.gen(function* () { const getOrCreateRandom: ServerSecretStore["Service"]["getOrCreateRandom"] = (name, bytes) => get(name).pipe( + // Older builds could persist a 0-byte secret, which is never a usable key. Re-read under + // the permit, and keep it until an interrupted removal settles, so a caller that saw the + // empty file cannot remove a key another caller created. + Effect.filterOrElse(isMissingOrNonEmpty, () => + emptySecretRepair.withPermits(1)( + get(name).pipe( + Effect.filterOrElse(isMissingOrNonEmpty, () => + remove(name).pipe(Effect.as(Option.none())), + ), + Effect.uninterruptible, + ), + ), + ), Effect.flatMap( Option.match({ onSome: Effect.succeed,