diff --git a/docs/typescript_port_compatibility_matrix.md b/docs/typescript_port_compatibility_matrix.md index 6c5b924b..0e846dd0 100644 --- a/docs/typescript_port_compatibility_matrix.md +++ b/docs/typescript_port_compatibility_matrix.md @@ -116,6 +116,18 @@ need a versioned protocol change in both implementations: numbers (or an info string) and a compatibility version, since existing keys must keep deriving as they do. +**Deliberately identical, and wider than it reads: token `label_regex`.** +Both implementations match a token's `label_regex` as a *search*, not a full +match — Python with `re.search` (`src/seedpass/cli/agent.py`), TypeScript with +`RegExp.test` (`js/packages/cli/src/agent.ts`). A token issued for `prod` +therefore also reaches `not-prod-db`. Anchoring the TypeScript side alone +would make one token mean two different things depending on which +implementation holds it, and would silently narrow the scope of every token +already issued, so the semantics stay as they are and are stated instead: in +`--label-regex` help, in `capabilities().tokens.label_regex_semantics`, and +pinned by tests in `js/packages/cli/test/tokens.test.ts`. Operators wanting an +exact match anchor the pattern themselves (`^prod$`). + **Resolved:** TOTP codes for entries with a non-default period/digits used to differ — Python built `pyotp.TOTP(secret)` with library defaults and ignored the entry's recorded values, so an imported 8-digit/45s secret diff --git a/js/packages/cli/src/agent.ts b/js/packages/cli/src/agent.ts index 8b78572a..180ab585 100644 --- a/js/packages/cli/src/agent.ts +++ b/js/packages/cli/src/agent.ts @@ -29,7 +29,7 @@ import { tmpdir } from "node:os"; import { dirname } from "node:path"; import { join } from "node:path"; import process from "node:process"; -import { deriveKeyIndex, sha256Hex, utf8, type Entry } from "@seedpass/core"; +import { deriveKeyIndex, generateFingerprint, sha256Hex, utf8, type Entry } from "@seedpass/core"; import { openVault } from "./vaultFile.js"; import { materializeSecret } from "./secrets.js"; import { entryMetadata } from "./refs.js"; @@ -42,7 +42,7 @@ import { } from "./sinks.js"; import { AuditLog } from "./audit.js"; import { loadConfig, passwordPolicyFromConfig } from "./configFile.js"; -import { INDEX_FILENAME } from "./appDir.js"; +import { INDEX_FILENAME, FINGERPRINT_RE } from "./appDir.js"; export const DEFAULT_TTL_SECONDS = 900; @@ -245,7 +245,24 @@ export class AgentDaemon { return { token }; } - /** Would this token be allowed to act on this entry at all? */ + /** + * Would this token be allowed to act on this entry at all? + * + * `label_regex` is matched with `RegExp.test`, i.e. unanchored: it succeeds + * if the pattern matches ANYWHERE in the label, so a token issued for + * `prod` also reaches `not-prod-db`. That is wider than the operator + * issuing it is likely to assume, and it is deliberate: Python matches with + * `re.search` (cli/agent.py), and a token must not mean two different + * things depending on which implementation is holding it. Anchoring here + * would silently narrow every existing token's scope on upgrade. + * + * The semantics are stated in `--label-regex` help and reported in + * `capabilities()` under `label_regex_semantics`, so a caller can scope + * correctly without reading this code. + * + * A pattern that fails to compile denies rather than throws: an invalid + * constraint must never widen access. + */ private tokenMaySee(token: TokenRecord, entry: { kind: string; label: string }): boolean { if (token.kinds && !token.kinds.includes(entry.kind)) return false; try { @@ -336,6 +353,17 @@ export class AgentDaemon { }; } if (sinkSpec.sink !== "clipboard" && allowed && allowed.length > 0) { + // The allowlist constrains the command WORD only. The token holder + // still chooses every argument, and the child receives the secret in + // its environment, so an allowlisted binary with an output-file or + // network flag hands the secret straight back to the holder. Choose + // allowlist entries as if the holder writes their argv, because they + // do. Containment here is "which binary", never "what it does". + // + // Argument-level containment would mean allowlisting full argv + // templates (["ssh-add", "-"]) rather than command words; that is a + // token-format change and has to land in both implementations at + // once, so it is tracked rather than done here. const [cmd] = parseCommandSpec(sinkSpec.command); if (!allowed.includes(cmd)) { await this.auditLog(fingerprint, "access_denied", { @@ -641,8 +669,26 @@ export class AgentDaemon { const fingerprint = String(msg["fingerprint"] ?? ""); const mnemonic = String(msg["mnemonic"] ?? ""); if (!fingerprint || !mnemonic) return { ok: false, error: "missing fields" }; + // `put` is the only way a fingerprint enters `held`, and every later + // handler joins that string onto a path (the vault, the audit log). + // The CLI validates the format before it gets here, which is exactly + // why the daemon must too: agent.ts's own header states the CLI is + // untrusted, and the browser extension will be the next thing + // speaking this protocol. + if (!FINGERPRINT_RE.test(fingerprint)) { + return { ok: false, error: "fingerprint must be 16 uppercase hex characters" }; + } const ttl = positiveIntField(msg["ttl"], this.defaultTtl); if (ttl === null) return { ok: false, error: "ttl must be a positive integer" }; + // Last of the validations because it is the expensive one: deriving a + // fingerprint runs the BIP-39 KDF, and a malformed ttl should not pay + // for it. A fingerprint that does not belong to this seed is not an + // attack so much as a mix-up, but it files the audit records under + // the wrong profile and unlocks a vault the caller did not name -- + // both cheaper to refuse here than to explain later. + if (generateFingerprint(mnemonic) !== fingerprint) { + return { ok: false, error: "fingerprint does not match the supplied seed" }; + } const expiresAt = Math.floor(Date.now() / 1000 + ttl); // The audit key is derived from the held seed, so the seed must go in // before the record is written — but an audit failure must then undo diff --git a/js/packages/cli/src/appDir.ts b/js/packages/cli/src/appDir.ts index 4e888ed8..9571e32c 100644 --- a/js/packages/cli/src/appDir.ts +++ b/js/packages/cli/src/appDir.ts @@ -79,7 +79,7 @@ export function resolveAppDir(override?: string): string { * containing `..` would otherwise steer a recursive delete out of the * profile directory. */ -const FINGERPRINT_RE = /^[0-9A-F]{16}$/; +export const FINGERPRINT_RE = /^[0-9A-F]{16}$/; export function assertValidFingerprint(fingerprint: string): string { if (!FINGERPRINT_RE.test(fingerprint)) { diff --git a/js/packages/cli/src/capabilities.ts b/js/packages/cli/src/capabilities.ts index 2469dd79..a440c8af 100644 --- a/js/packages/cli/src/capabilities.ts +++ b/js/packages/cli/src/capabilities.ts @@ -57,6 +57,17 @@ export function capabilities(): Record { commands: ["agent token-issue/token-list/token-revoke"], scopes: ["read", "use", "reveal"], constraints: ["kinds", "label_regex", "ttl", "uses", "exec_allowlist"], + // Automation issuing tokens has to know this to scope them correctly: + // label_regex is a SEARCH, not a full match, in both implementations + // (Python uses re.search). `prod` therefore also matches `not-prod-db`. + // Anchor it yourself -- `^prod$` -- to get an exact match. + label_regex_semantics: "search (substring); anchor with ^...$ for exact match", + // Automation that issues `use` tokens needs this to pick allowlist + // entries safely: the check covers the command word, not its arguments. + exec_allowlist_semantics: + "command word only; the token holder supplies all arguments, so an " + + "allowlisted binary that can write a file or open a socket can " + + "return the secret to its caller", token_env_var: "SEEDPASS_TOKEN", enforcement: "session agent (secrets materialized agent-side in token mode)", escalation: diff --git a/js/packages/cli/src/program.ts b/js/packages/cli/src/program.ts index 6c4e08b3..f0f914b8 100644 --- a/js/packages/cli/src/program.ts +++ b/js/packages/cli/src/program.ts @@ -1361,12 +1361,20 @@ export function buildProgram(io: ProgramIo = defaultIo): Command { .option("--name ", "token name", "agent") .option("--scope ", "read/use/reveal (repeatable)", ["read"]) .option("--kind ", "restrict to entry kinds (repeatable)") - .option("--label-regex ", "restrict to matching labels", ".*") + .option( + "--label-regex ", + "restrict to labels the pattern MATCHES ANYWHERE (like Python's " + + "re.search): 'prod' also matches 'not-prod-db'. Anchor it — '^prod$' " + + "— for an exact match", + ".*", + ) .option("--ttl ", "token lifetime", "300") .option("--uses ", "max secret deliveries", "1") .option( "--exec-allowlist ", - "restrict `use` to these commands (without it, a use-scoped holder can read the secret)", + "restrict `use` to these commands (without it, a use-scoped holder can " + + "read the secret). Matches the command WORD only — the holder still " + + "chooses the arguments, so pick binaries that stay safe with any argv", ) .action( async (o: { diff --git a/js/packages/cli/test/agentSecurity.test.ts b/js/packages/cli/test/agentSecurity.test.ts index da27b139..40f01607 100644 --- a/js/packages/cli/test/agentSecurity.test.ts +++ b/js/packages/cli/test/agentSecurity.test.ts @@ -409,6 +409,41 @@ describe("malformed daemon input cannot corrupt held state", () => { expect(JSON.stringify(owner)).not.toContain("abandon"); }); + /** + * `put` is the only way a fingerprint enters `held`, and every later handler + * joins that string onto a path — the vault it opens, the audit log it + * writes. The CLI validates the format before the daemon ever sees it, + * which is precisely why the daemon cannot rely on that: it treats every + * caller as untrusted, and the browser extension will speak this protocol + * next. + */ + it("refuses a fingerprint that is not 16 uppercase hex characters", async () => { + for (const bad of ["../../../etc", "bbbbbbbbbbbbbbbb", "BBBB", "BBBBBBBBBBBBBBBB/x"]) { + const put = await rawRequest({ + op: "put", cap, fingerprint: bad, mnemonic: MNEMONIC, ttl: 60, + }); + expect(put["ok"]).toBe(false); + expect(String(put["error"])).toContain("16 uppercase hex"); + const owner = await rawRequest({ op: "owner-mnemonic", cap, fingerprint: bad }); + expect(owner["ok"]).toBe(false); + expect(JSON.stringify(owner)).not.toContain("abandon"); + } + }); + + it("refuses a well-formed fingerprint that does not belong to the seed", async () => { + // Correct shape, wrong profile: this would unlock a vault the caller did + // not name and file its audit records under the wrong fingerprint. + const put = await rawRequest({ + op: "put", cap, fingerprint: SCRATCH_FP, mnemonic: MNEMONIC, ttl: 60, + }); + expect(put["ok"]).toBe(false); + expect(String(put["error"])).toContain("does not match"); + + const owner = await rawRequest({ op: "owner-mnemonic", cap, fingerprint: SCRATCH_FP }); + expect(owner["ok"]).toBe(false); + expect(JSON.stringify(owner)).not.toContain("abandon"); + }); + it("token-issue refuses a bare-string scope and a non-numeric ttl without crashing", async () => { const strScope = await rawRequest({ op: "token-issue", cap, fingerprint: FINGERPRINT, scopes: "reveal", ttl: 600, uses: 1, diff --git a/js/packages/cli/test/tokens.test.ts b/js/packages/cli/test/tokens.test.ts index e507c69c..dc31dccf 100644 --- a/js/packages/cli/test/tokens.test.ts +++ b/js/packages/cli/test/tokens.test.ts @@ -96,6 +96,82 @@ describe("token issuance", () => { }); }); +/** + * `label_regex` is a search, not a full match — in both implementations. + * Python enforces it with `re.search` (cli/agent.py), so anchoring on the TS + * side would mean the same token grants different access depending on which + * implementation holds it, and would silently narrow every token already + * issued. These tests pin the semantics so the surprise stays documented + * rather than becoming an accidental behavior change later. + */ +describe("label_regex matches anywhere, matching Python's re.search", () => { + it("an unanchored pattern reaches labels that merely contain it", async () => { + await run(asOwner, "entry", "add", "key-value", "not-prod-db", "k", "adjacent-secret"); + await run(asOwner, "entry", "add", "key-value", "prod", "k", "intended-secret"); + + const issued = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "substring-demo", + "--scope", "read", + "--kind", "key_value", + "--label-regex", "prod", + "--ttl", "600", + )).stdout, + ); + const rows = JSON.parse( + (await run(asTokenHolder(issued.token), "entry", "list")).stdout, + ) as { label: string }[]; + // The point of the test: BOTH are visible, not just the exact match. + expect(rows.map((x) => x.label).sort()).toEqual(["not-prod-db", "prod"]); + }); + + it("anchoring the pattern is how an operator gets an exact match", async () => { + const issued = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "anchored-demo", + "--scope", "read", + "--kind", "key_value", + "--label-regex", "^prod$", + "--ttl", "600", + )).stdout, + ); + const rows = JSON.parse( + (await run(asTokenHolder(issued.token), "entry", "list")).stdout, + ) as { label: string }[]; + expect(rows.map((x) => x.label)).toEqual(["prod"]); + }); + + it("reports the semantics in capabilities so automation can scope correctly", async () => { + const caps = JSON.parse((await run({}, "capabilities")).stdout); + expect(caps.tokens.label_regex_semantics).toContain("search"); + expect(caps.tokens.label_regex_semantics).toContain("^...$"); + // The exec allowlist's containment is likewise narrower than it reads: + // it names a binary, not what that binary may be asked to do. + expect(caps.tokens.exec_allowlist_semantics).toContain("command word only"); + }); + + it("an uncompilable pattern denies rather than throwing", async () => { + const issued = JSON.parse( + (await run( + {}, + "agent", "token-issue", + "--name", "bad-regex", + "--scope", "read", + "--label-regex", "([unclosed", + "--ttl", "600", + )).stdout, + ); + const rows = JSON.parse( + (await run(asTokenHolder(issued.token), "entry", "list")).stdout, + ) as { label: string }[]; + expect(rows).toEqual([]); + }); +}); + describe("token-mode access (no mnemonic anywhere)", () => { it("lists only the entries the token may act on, with no secret values", async () => { const r = await run(asTokenHolder(token), "entry", "list");