diff --git a/services/secrets-broker/secret b/services/secrets-broker/secret index 17dfdfa..1f1424a 100755 --- a/services/secrets-broker/secret +++ b/services/secrets-broker/secret @@ -21,6 +21,7 @@ Commands: """ import argparse import base64 +import fcntl import glob import hashlib import json @@ -68,7 +69,27 @@ def load_env(): return env +BW_LOCK = Path(os.environ.get("SECRET_LOCK", "/tmp/.secrets-broker-bw.lock")) + + def bw(args, env, session=None, stdin=None, check=True): + """Run one `bw` command. Serialisation is at the COMMAND level — see `main`. + + ⚠ `bw` is NOT concurrency-safe against a shared session. Measured 2026-09-15: + four parallel `secret get` calls for distinct items returned **empty strings + with exit code 0** — 0 of 4 succeeded here, 1 of 4 in svos-dev's run that + reported it. No error, no timeout, no diagnostic. Sequentially all four are + correct. That is the worst possible failure shape for a credential tool: a + caller treating "empty optional secret" as "not configured" degrades silently + and never knows (it cost SVOS its ability to page the operator, and nothing + said so). + + The lock makes concurrent callers queue instead of race, so callers no longer + have to know. It is held for the whole `bw` call — vault reads are ~17s, so + parallelism was never buying much anyway. Paired with the empty-value guard in + `cmd_get`: the lock prevents the race, the guard makes any future silent-empty + loud rather than invisible. + """ cmd = [BW] + args + (["--session", session] if session else []) r = subprocess.run(cmd, env=env, input=stdin, capture_output=True) if check and r.returncode != 0: @@ -97,7 +118,18 @@ def encode(obj, env): def find(env, s, name): - for it in json.loads(bw(["list", "items", "--search", name], env, s).stdout.decode() or "[]"): + raw = bw(["list", "items", "--search", name], env, s).stdout.decode().strip() + # ⚠ Empty stdout is a FAILED listing, not an empty result set. The old code said + # `or "[]"`, which turned a broken read into a confident "no such secret" — the + # same silent-wrong-answer shape as the empty-value bug below. bw returns nothing + # here when its session has been invalidated underneath it. + if not raw: + raise BwError( + "`bw list items` returned NOTHING (not an empty list). The vault session " + "was probably invalidated mid-call — most often by a concurrent `secret` " + "invocation. Retry sequentially." + ) + for it in json.loads(raw): if it.get("name") == name: return it return None @@ -212,6 +244,15 @@ def cmd_get(a): return print(f["value"]) die(f"no field '{a.field}' on {a.name}") data = read_secret(env, s, a.name) + # ⚠ An EMPTY value with exit 0 is the silent-failure surface — see bw() above. + # A stored secret is never legitimately zero-length, so refuse rather than hand + # a caller "" that it will read as "not configured". + if not data: + die(f"{a.name} resolved to an EMPTY value. A stored secret is never empty, so " + "this is a failed read, not an unset secret. Most likely cause: a " + "concurrent `secret` call — bw is not safe under parallel invocation and " + "returns empty with success. Retry sequentially; if it persists the item " + "has no note/field/attachment body.") if a.file: Path(a.file).write_bytes(data) os.chmod(a.file, 0o600) @@ -320,10 +361,32 @@ def main(): pb = sub.add_parser("backfill", help="scan THIS box's local secret files and upsert each") pb.add_argument("--dry-run", action="store_true"); pb.set_defaults(fn=cmd_backfill) a = p.parse_args() + # ⚠ SERIALISE THE WHOLE COMMAND, not individual `bw` calls. + # + # Every invocation runs `bw unlock` to mint a session (see `session`), and + # concurrent unlocks against the shared BITWARDENCLI_APPDATA_DIR invalidate each + # other. The damage then surfaces DOWNSTREAM — `bw list items` returns empty + # stdout, or an item read returns an empty body — so a per-call lock cannot help: + # by the time the read runs, the session it holds is already dead. + # + # Measured 2026-09-15: four parallel `secret get` calls for distinct items + # returned EMPTY STRINGS WITH EXIT 0. Zero of four succeeded here; one of four in + # svos-dev's run that reported it. No error, no timeout, no diagnostic — and a + # caller that treats an empty optional secret as "not configured" then degrades + # silently and never knows. It cost SVOS its ability to page the operator. + # + # Vault reads are ~17s each, so parallelism was never buying much. Correctness + # first: callers queue, and no caller has to know any of this. + BW_LOCK.touch(exist_ok=True) + lk = open(BW_LOCK, "a") + fcntl.flock(lk, fcntl.LOCK_EX) try: a.fn(a) except BwError as e: die(str(e)) + finally: + fcntl.flock(lk, fcntl.LOCK_UN) + lk.close() if __name__ == "__main__":