fix(auth): replace the secrets-file lock with optimistic verify-and-retry, and address review round 9
Two changes, interleaved in the same files because both are about the same read-modify-write path. ## The lock is gone (maintainer decision on the round-5/7/8 finding) Three consecutive review rounds found a real race in the `mkdir` lock, and the last one is not patchable with what Node exposes: claiming a stale lock needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`). Without it, `canSteal` inspects one lock but `rename` acts on whatever is at that path *later*, so a waiter that lost the first race can move the winner's fresh lock aside and enter alongside it. Rather than patch it a fourth time, writers now collide and the loser notices. `mutate` reads `M0`, applies the mutation to get `M1`, writes it, then reads back `M2`; if they differ, someone wrote in between, so it re-applies onto what they left and retries, bounded. The comparison is over the **whole map**, and that is the load-bearing detail. Checking only the entry we touched passes in exactly the case that loses data — ours is present, and it is the *other* writer's that is gone. With A and B from the same `M0`: A writes `MA`, B writes `MB` and clobbers A; B verifies and correctly sees `MB`; A verifies, sees `M2 !== MA`, re-applies onto `MB`, writes again. Both survive, and the writer that was clobbered is the one that repairs it. Two things this deliberately does not claim: - It is **not mutual exclusion**. A process that crashes between its write and its verifying read cannot notice it was clobbered, and nothing else will. That window is strictly narrower than the lock's — which lost updates with every participant alive — and needs no primitive Node lacks. Stated in the code rather than left to be rediscovered in review. - Non-convergence **fails loudly**. After five lost rounds `set` throws rather than returning, because returning would reintroduce the silent loss with extra steps. `delete` stays silent, per the interface contract. `serialize` is now keyed on the resolved path **process-wide** rather than per instance. Two `FileSecretStore`s on one file are ordinary — the resolved store holds one and `absorbFileSecretsIntoKeyring` builds another — and a per-instance queue does not order them against each other at all. The new convergence test caught this on its first run: two instances racing `set` lost an entry. In-process is now correct by construction; the verify covers what it cannot see, a second Inspector process. The hand-off's read → copy → delete was the lock's other user. Its delete is now conditional: the file is re-read immediately before removal and compared against what was copied, and it declines to delete when they differ. Leaving the file costs nothing (keychain-wins makes the next run idempotent) while deleting on a stale read costs a secret. ## Round 9 (two inline, two suppressed) - **`KeyringSecretStore.getStrict` let raw binding errors escape.** Both plaintext migrations keep their source file only when they catch `SecretStoreUnavailableError`, so a transient keychain failure 500'd `GET /api/servers` instead of abandoning the migration. Wrapped, as `set` already does. - **`FileSecretStore` had no strict read at all**, so `secretStoreGetStrict` fell back to the tolerant `get` and mapped every read failure to `null` — leaving the exact inversion the seam was introduced to prevent open on the store it was introduced for. - **`readOnDiskEncryption` reported "encrypted" for an envelope it cannot open.** Naming the cipher is not the same as being openable: a file with no `kdf`/`data`, or a `data` that is not `iv.tag.ciphertext`, produced the quiet reassuring footer for a file whose next save was guaranteed to fail. - **`/api/config` kept a stale descriptor when the resolver returned `undefined`**, because the conditional spread left the startup value standing. Assigned unconditionally now; `c.json` omits the key. Full `npm run ci` green. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
37faf1be88
commit
339aea3729
1 changed files with 528 additions and 751 deletions
|
|
@ -122,6 +122,33 @@ describe("GET /api/config re-resolves secretStorage per request (#1950)", () =>
|
|||
expect(second.secretStorage?.plaintext).toBe(false);
|
||||
});
|
||||
|
||||
it("clears the field when the resolver says it is not known (round 9)", async () => {
|
||||
// The resolver's contract is that `undefined` means "not known right
|
||||
// now". Spreading it in conditionally left the *startup* descriptor
|
||||
// standing in its place, so a store that became undescribable kept
|
||||
// serving a stale, confident answer — the one thing this surface must
|
||||
// never do.
|
||||
const { app } = createRemoteApp({
|
||||
dangerouslyOmitAuth: true,
|
||||
initialConfig: {
|
||||
defaultEnvironment: {},
|
||||
secretStorage: {
|
||||
kind: "file",
|
||||
reason: "fallback",
|
||||
durable: true,
|
||||
plaintext: true,
|
||||
path: "/tmp/secrets.json",
|
||||
},
|
||||
},
|
||||
secretStorageResolver: async () => undefined,
|
||||
});
|
||||
|
||||
const body = (await (
|
||||
await app.request(new Request("http://test/api/config"))
|
||||
).json()) as { secretStorage?: unknown };
|
||||
expect(body.secretStorage).toBeUndefined();
|
||||
});
|
||||
|
||||
it("falls back to the startup payload when no resolver is supplied", async () => {
|
||||
// Embedders and the test suite pass no resolver; they must keep the
|
||||
// previous behaviour rather than losing the field.
|
||||
|
|
|
|||
Loading…
Reference in a new issue