diff --git a/.abcd/config/reading-presets.json b/.abcd/config/reading-presets.json index 7047d76da..7ad520d81 100644 --- a/.abcd/config/reading-presets.json +++ b/.abcd/config/reading-presets.json @@ -60,10 +60,10 @@ "test" ], "window": { - "tokens_est": 1360000, - "measured_tokens_est": 1344909, - "measured_bytes": 5177902, - "measured_at": "db30f1a10992df69d1253260a88065884a21937d" + "tokens_est": 1370000, + "measured_tokens_est": 1346832, + "measured_bytes": 5185304, + "measured_at": "a887092f79d847680e09a6f0a7a447bfb6f01749" } }, "entailment": { diff --git a/.abcd/development/brief/04-surfaces/01-ahoy.md b/.abcd/development/brief/04-surfaces/01-ahoy.md index 621253ec6..f14392299 100644 --- a/.abcd/development/brief/04-surfaces/01-ahoy.md +++ b/.abcd/development/brief/04-surfaces/01-ahoy.md @@ -26,6 +26,7 @@ repo whose stamp says it is current. | Verb | Bucket | Status | |---|---|---| | `connect` | — | shipped | +| `credential` | — | shipped | | `doctor` | — | shipped | | `install` | — | shipped | | `remote apply` | gate | shipped | @@ -122,10 +123,10 @@ authenticated identity: abcd never holds a token. The setup takes the provider's name, its base URL, its first allowlist (every model it may serve) and where its key lives. It verifies the provider with one -call to the first model listed and, only when that call succeeds, writes the key -and then the provider block, both under `~/.abcd/`: the key into the owner-only -`credentials.json`, the block (base URL, the key's name, the models) into -`config.json`. A failed verification writes nothing. Nothing reaches the +call to the first model listed and, only when that call succeeds, keeps the key +in the home chosen through the credential store's walkthrough and then writes +the provider block (base URL, the key's name, the models) into +`~/.abcd/config.json`. A failed verification writes nothing. Nothing reaches the repository or the harness's settings. Every fault the configuration read would refuse (a denylisted or malformed model, a base URL that is plain HTTP to another machine, a provider already configured, a key name already holding a @@ -140,13 +141,55 @@ the same reason the walkthrough is this sub-verb, which the person runs with the key piped in, rather than a question the install pass asks: declining is not running it, and changes nothing. -Of the three homes a key may live in, the setup builds the abcd-only one. The -environment-variable-or-external-tool home and the platform keychain arrive with -the credential store (itd-2609221017023290); asked for either, the setup refuses -naming it. A fourth answer, no key, sets up a local server that takes none. +The key lives in one of the credential store's three homes (below), and a +fourth answer, no key, sets up a local server that takes none. No delegating verb sends a step to a configured provider until provider dispatch lands (spc-2609251028149555), and both the board and the setup say so. +### The credential store and its walkthrough + +Every external credential abcd holds goes through one store +(`internal/core/credential`, adr-2609221017021499): configuration names a +credential, and the value lives in the home the person chose for it, once, in +the credential walkthrough at `ahoy`. Without a name, the walkthrough lists +every credential an adapter reads (the site setup's hosting token, each +configured provider's key) with whether it is set and in which home, never the +value; with a name and no home, it explains what the credential unlocks and +what works without it, then the three homes, the keychain recommended in the +prose above them and never marked as an option. Given a home, it runs: the +reading adapter's own verification call (the provider's one short exchange, +the hosting provider's account read) with the value, and only when that +succeeds, the write. The provider setup runs the same walkthrough for a new +provider's key. + +The three homes: + +- `external` — a setup outside abcd: an environment variable, or a dotted + field of a tool's JSON configuration file under the home directory. The + store keeps only the pointer, in + `~/.abcd/credential-homes.json`, and follows it on every read. A file + pointer is refused, naming the link, when any directory between the home and + the tool's file is a symlink, wherever the link leads; the + environment-variable pointer stays open. +- `abcd` — the owner-only `~/.abcd/credentials.json`, which holds the value. +- `keychain` — the platform keychain under the service name `abcd` (the + Keychain through `/usr/bin/security` on macOS, the secret service through + `/usr/bin/secret-tool` on Linux), the value handed over on stdin, never in an + argument; `credential-homes.json` records only that the name lives there. A + platform with neither tool refuses this home and names the other two. + +One reader, `credential.Store(home).Resolve(name)`, serves every adapter; a +name no home holds is a refusal naming the walkthrough, and the caller makes no +call. A test walks the production tree for any other read (a store file named, +a keychain command run, a secret-shaped environment variable read). One write, +`credential.Set`, is reached only through the walkthrough: it refuses the abcd +home when `~/.abcd` lies inside a git working tree, since that home alone keeps +a value there, a name another home already holds, and a different +value for a name already kept, and the secret scanner reads the index's bytes +before they are written, refusing any finding. A value is read from stdin only, +and never printed, logged or written to a record; a call's record names the +credential it used. + ## What abcd manages — repos and `~/.abcd/` abcd manages exactly one kind of folder, a **repository**, and keeps one @@ -186,13 +229,17 @@ user-scope directory for machine-local state. load-limits the load check's per-machine limits (stray-minutes, extreme-load), read-only; abcd never creates it (itd-2609231434459890) - credentials.json external credentials by name (a hosting token for - setting up a site, a provider's key), mode 0600; - only the provider setup writes it, one new name at - a time, never replacing a stored value, holding - .credentials.json.lock beside it across the read - and the write. The interim source the credential - store replaces (itd-2609221017023290) + credentials.json the credential store's abcd home: external + credentials by name (a hosting token, a provider's + key), mode 0600; only the walkthrough writes it, + one new name at a time, never replacing a stored + value, holding .credentials.json.lock beside it + across the read and the write (itd-2609221017023290) + credential-homes.json the credential store's index: which names live in + the keychain, and the pointer for each in the + external home; never a value, scanned before it is + written, mode 0600, .credential-homes.json.lock + beside it rules.json the machine's rule conventions, the user layer between the bundled domains and each repo's .abcd/rules.json, read-only; abcd never creates it @@ -722,7 +769,7 @@ _Generated from the command tree; a drift test fails `go test` when this appendi ### `abcd ahoy` -Sub-verbs: `abcd ahoy connect`, `abcd ahoy doctor`, `abcd ahoy install`, `abcd ahoy remote`, `abcd ahoy uninstall`. +Sub-verbs: `abcd ahoy connect`, `abcd ahoy credential`, `abcd ahoy doctor`, `abcd ahoy install`, `abcd ahoy remote`, `abcd ahoy uninstall`. | Flag | Type | |---|---| @@ -738,10 +785,24 @@ Sub-verbs: none. | Flag | Type | |---|---| | `--base-url` | string | +| `--env` | string | +| `--field` | string | +| `--file` | string | | `--home` | string | | `--key` | string | | `--model` | stringArray | +### `abcd ahoy credential` + +Sub-verbs: none. + +| Flag | Type | +|---|---| +| `--env` | string | +| `--field` | string | +| `--file` | string | +| `--home` | string | + ### `abcd ahoy doctor` Sub-verbs: none. diff --git a/.abcd/development/brief/04-surfaces/22-site.md b/.abcd/development/brief/04-surfaces/22-site.md index 2435eb47d..77ead0808 100644 --- a/.abcd/development/brief/04-surfaces/22-site.md +++ b/.abcd/development/brief/04-surfaces/22-site.md @@ -75,15 +75,17 @@ any secret step. **The host.** With a hosting credential on this machine, the provider adapter creates the host, routes the custom domain to it and reports the live address. -Without one, the stage stops and says what remains: store the credential and -re-run, or create the host in the provider's console. +Without one, the stage stops and says what remains: store the credential through the credential +walkthrough at `ahoy`, which verifies it with the provider's own account read +before it keeps it, and re-run, or create the host in the +provider's console. Both remote stages write only after a confirmation that names each change, and an unanswered run declines them. The deploy environment's secrets are never set by abcd, because the value would pass through it: the verb reads which secret names are present and prints the exact command for each one that is not. -The credential is read by name from the machine and never written into the -repository or the report. A second run over an unchanged repository and host +The credential is read by name through the credential store and never written +into the repository or the report, which names only the credential's name. A second run over an unchanged repository and host writes nothing and says so. One provider ships, behind an adapter seam ([`05-internals/02-adapters.md`](../05-internals/02-adapters.md#hosting-providers)). diff --git a/.abcd/development/brief/04-surfaces/23-reading.md b/.abcd/development/brief/04-surfaces/23-reading.md index f181b6200..57eb47e33 100644 --- a/.abcd/development/brief/04-surfaces/23-reading.md +++ b/.abcd/development/brief/04-surfaces/23-reading.md @@ -134,7 +134,11 @@ copy there when the pair was sent elsewhere. A run assembled to any other direct ingest, and bare `abcd reading` does not list it among the staged runs either, because that listing reads the same one directory. A named directory is for a run whose artefacts are being inspected or archived; a run meant to come back through -ingest lets the default run directory name itself. +ingest lets the default run directory name itself. The assembly says which it +made before the reading is commissioned: The result carries `ingestable`, +false for a run written to a named directory and for a dry run, and a run +written to a named directory renders an `ingest:` line naming the run directory +the ingest reads. An output directory the include table can reach is refused when it is named, because writing a run where the table reaches it commits the next run's diff --git a/.abcd/development/brief/05-internals/02-adapters.md b/.abcd/development/brief/05-internals/02-adapters.md index d994abc32..85211ce27 100644 --- a/.abcd/development/brief/05-internals/02-adapters.md +++ b/.abcd/development/brief/05-internals/02-adapters.md @@ -66,8 +66,12 @@ judged by the caller's output contract, the one the host sub-agent's payload is judged by. The request is the host's brief in the protocol's two roles: the agent's prompt as the system message, the verb's request as the user message. -The key is resolved by name through `internal/core/credential`, the one reader; -the adapter reads no file and no store of its own. The one environment it +The key is resolved by name through the credential store +(`internal/core/credential`, `Store(home).Resolve`), the one reader, from +whichever of its three homes the person chose; the adapter reads no file and no +store of its own, and a key that resolves to nothing refuses before any call, +naming `abcd ahoy credential `. A call's record names the credential it +used, never the key. The one environment it honours is the HTTP stack's: the standard proxy variables (`HTTPS_PROXY`, `NO_PROXY`) and the platform's trust roots. An https call through a proxy is a tunnel, so the key and the brief stay inside TLS, and a call to this machine is @@ -125,10 +129,18 @@ domain to it and reports the address, after a read that writes nothing. One provider ships: an assets-only Cloudflare Worker, the host abcd's own site uses. A second is one implementation of the interface and one entry in the site package's provider list; the verb does not change. The credential is -resolved by name through `internal/core/credential`, whose interim source is -`~/.abcd/credentials.json` until the credential store (itd-2609221017023290) -replaces it; the connected adapter holds it, and it is scrubbed from every host -message before one can reach an error. +resolved by name through the credential store (`internal/core/credential`), +the one reader; the connected adapter holds it, and it is scrubbed from every +host message before one can reach an error. The provider's `Verify`, the same +account read the host stage begins with, is the verification call the +credential walkthrough makes before it stores a token. + +**Every external credential goes through one store** (adr-2609221017021499): +configuration names a credential, and its value lives in the home the person +chose once at `abcd ahoy credential`, the external setup, the abcd-only file or +the platform keychain ([`04-surfaces/01-ahoy.md`](../04-surfaces/01-ahoy.md)). +An adapter that reads a secret any other way is a defect, and a test walks the +production tree for one. ## Lifeboat source readers diff --git a/.abcd/development/brief/05-internals/03-configuration.md b/.abcd/development/brief/05-internals/03-configuration.md index 4d4393e5e..10f4144e7 100644 --- a/.abcd/development/brief/05-internals/03-configuration.md +++ b/.abcd/development/brief/05-internals/03-configuration.md @@ -217,6 +217,10 @@ The transcript corpus is a **sibling** user-scope store rather than a sub-tree o the registry, at `~/.abcd/transcripts//`, holding redacted records and a staging area for raw transcripts awaiting redaction ([adr-2609091248201071](../../decisions/adrs/2609091248201071-the-transcript-corpus-is-a-sibling-store-that-creates-itself.md)). +Every machine-scoped store keyed on the root commit takes the full object name +as its ``: Forty hex digits under SHA-1, sixty-four under SHA-256, the +form `gitutil.RootCommit` returns and `gitutil.IsFullSHA` admits as a path +segment. An abbreviated key is a different directory, and no verb reads it. One package owns its layout: `internal/core/history` declares both the user-scope default and the opt-in per-repo location, and every resolver goes through it. The rule is a convention with nothing behind it, and it already has one exception — @@ -352,13 +356,13 @@ keys, each read beneath the repo-scope `.abcd/config.json`, and its one write the provider block `ahoy connect` adds; every other config read resolves the repo-scope `.abcd/config.json` alone), the load check's two limits in `load-limits` (read-only and never created, itd-2609231434459890), the external -credentials adapters resolve by name in `credentials.json` (refused unless it is -a regular file this uid owns at mode 0600 that names each credential once, a -repeated key or a case twin included; `ahoy connect` adds one name at a time and -never replaces a stored value, holding the file's lock across the read and the -write as the provider block's write holds `config.json`'s, so concurrent setups -lose nothing — the interim source the credential store, itd-2609221017023290, -replaces), the +credentials adapters resolve by name through the credential store, whose abcd +home is `credentials.json` and whose index is `credential-homes.json` (each +refused unless it is a regular file this uid owns at mode 0600 that names each +credential once, a repeated key or a case twin included; the walkthrough adds +one name at a time and never replaces a stored value, holding the file's lock +across the read and the write as the provider block's write holds +`config.json`'s, so concurrent setups lose nothing), the machine's rule conventions in `rules.json` (the user layer of the rules loader, read-only and never created, itd-117 — see [the rules layers](#the-rules-layers--bundled-user-repo) below), user-scope memory for personal cross-project knowledge (a later @@ -373,11 +377,20 @@ the two are one list and must agree. **A symlinked `~/.abcd` hosts nothing abcd trusts.** Every file in the user scope whose contents abcd acts on — `rules.json`, `trusted-roots`, `local-transcript-roots`, `path-entry`, `cache-attestation`, `config.json`, -`oracle-routing.json`, `statusline.json`, `load-limits` and `credentials.json` — -is refused when `~/.abcd`, or a directory below it on the way to the file, is a +`oracle-routing.json`, `statusline.json`, `load-limits`, `credentials.json` and +`credential-homes.json` — is refused when `~/.abcd`, or a directory below it on the way to the file, is a symlink: the rule the rules loader states for `rules.json`, applied by one check (`fsutil.HomeScopeLink`, read through `fsutil.ReadHomeDeclaration`) so it cannot -drift per file. A symlinked `~/.abcd` holding no such file reads as absent and +drift per file. The rule holds against a race as well as a layout: a reader or +writer opens `~/.abcd` and each level below it relative to the descriptor of the +level above (`fsutil.OpenHomeScope`, or `fsutil.EnsureHomeScope` to create the +missing levels), confirms each descriptor is the real directory its judgement +saw, and reaches the file only through that descriptor, so a process swapping +`~/.abcd` for a link between the check and the use is refused rather than +followed (iss-2609281310017733). The file's own guards are judged on that +descriptor too: the credential store's mode 0600 is judged on the fstat of the +file that is opened (`fsutil.ReadHomeDeclarationDenying`), never on its path, +so a store swapped for a group-readable file after any check is refused. A symlinked `~/.abcd` holding no such file reads as absent and costs nothing. A file that is there behind the link is refused the way its reader refuses any declaration that is not the caller's word: the rules load fails, a declaration is ignored with a note, the path entry and the cache attestation @@ -386,15 +399,23 @@ into those files — the credential and the provider block `ahoy connect` adds, path entry, the routing table and the status-line setting `ahoy install` writes, and the path entry and cache attestation `hooks/bootstrap.sh` writes — refuses the link rather than writing through it, naming it and the repair: replace the -link with a real directory. The hook shims refuse a `path-entry` behind the link +link with a real directory. The path entry's removal on uninstall goes through +the same descriptor, so it removes nothing behind the link. A credential +setup refuses a symlinked `~/.abcd` in every home, the keychain and external +homes included, before it creates anything: the index, the value and both +locks are reached through the one walk that created and judged `~/.abcd`, +with the index's lock taken there and the abcd home's lock nested inside it. +An external home's pointer at a file under a symlinked directory (a +`~/.config` linked into a dotfiles repository) is refused the same way, +naming the link, and the file is read through the descriptor walk. The hook shims refuse a `path-entry` behind the link too, before they read it. The home directory itself may be a link; only `~/.abcd` and what lies under it are judged. The stores are not declarations: `transcripts/`, `voyage/`, `lab/`, `inbox/` and `runs/` refuse a symlinked level through their own create-then-prove seam (`fsutil.EnsureRealDir`), and the `sources/` corpus is the caller's to place. The `history/` registry applies both: it is neither read nor written behind a symlinked `~/.abcd` or -`~/.abcd/history`, and it is created through the same create-then-prove seam -(iss-2609281129171021). `ahoy install` skips the registration with a note naming +`~/.abcd/history`, and it is created, locked, read and written through +`fsutil.EnsureHomeScope`'s descriptor (iss-2609281129171021). `ahoy install` skips the registration with a note naming the link and the repair, and the detector reports it as a diagnostic rather than a gap install would try and fail to close. diff --git a/.abcd/development/intents/drafts/itd-2609091014076309-session-and-agent-worktrees-live-in-a-machine-scoped-store-t.md b/.abcd/development/intents/drafts/itd-2609091014076309-session-and-agent-worktrees-live-in-a-machine-scoped-store-t.md index 65313c172..a33bf672e 100644 --- a/.abcd/development/intents/drafts/itd-2609091014076309-session-and-agent-worktrees-live-in-a-machine-scoped-store-t.md +++ b/.abcd/development/intents/drafts/itd-2609091014076309-session-and-agent-worktrees-live-in-a-machine-scoped-store-t.md @@ -94,6 +94,7 @@ We expect a machine-scoped, root-SHA-keyed store with a list verb and a reclaim - Whether the plugin surface should carry `add` as a host-run step, so a harness's own checkout-isolation feature lands in the store rather than wherever the harness defaults to. The prose stays host-agnostic either way. - Whether the lane is `0o700` like the transcript store. A worktree holds the same bytes as the checkout, so the checkout's own mode is the nearer precedent. - Whether prune treats a squash-merged branch as merged (see Scope Conditions); the conservative default is stated there and the spec may widen it. +- Whether the listing names a hand-laid lane keyed on an abbreviated root commit beside the full-SHA lane of the same repository. Lanes laid by hand before the verb exists sit under the eight-digit short form, and an autonomous run once split one store's log across the two spellings (iss-2609240646458365); a listing that reads only the full key leaves the short one unseen. ## Audit Notes diff --git a/.abcd/development/intents/planned/itd-2609221017023290-abcd-keeps-every-external-credential-the-same-way-one.md b/.abcd/development/intents/shipped/itd-2609221017023290-abcd-keeps-every-external-credential-the-same-way-one.md similarity index 97% rename from .abcd/development/intents/planned/itd-2609221017023290-abcd-keeps-every-external-credential-the-same-way-one.md rename to .abcd/development/intents/shipped/itd-2609221017023290-abcd-keeps-every-external-credential-the-same-way-one.md index 29d2eb45e..f8dd4eac6 100644 --- a/.abcd/development/intents/planned/itd-2609221017023290-abcd-keeps-every-external-credential-the-same-way-one.md +++ b/.abcd/development/intents/shipped/itd-2609221017023290-abcd-keeps-every-external-credential-the-same-way-one.md @@ -69,7 +69,8 @@ _None open._ ## Audit Notes -_Empty. Populated by intent-auditor when intent moves to shipped/._ + +Fidelity review OWED (receipt rcp-ebf7d171b544). ## Grounds diff --git a/.abcd/development/release/surface.json b/.abcd/development/release/surface.json index 7cd5bba2b..e61375635 100644 --- a/.abcd/development/release/surface.json +++ b/.abcd/development/release/surface.json @@ -76,7 +76,7 @@ { "path": "abcd ahoy connect", "hidden": false, - "sentence": "Verify a model provider with one call, then configure it: Writes its block and its key under ~/.abcd/; refuses a key typed at a terminal.", + "sentence": "Verify a model provider with one call, then configure it: Writes its block under ~/.abcd/ and its key to the home chosen; refuses a key typed at a terminal.", "flags": [ { "name": "base-url", @@ -85,6 +85,27 @@ "required": false, "hidden": false }, + { + "name": "env", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "field", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "file", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, { "name": "home", "shorthand": "", @@ -108,6 +129,41 @@ } ] }, + { + "path": "abcd ahoy credential", + "hidden": false, + "sentence": "List the credentials abcd reads, explain one, or verify and store it: Writes the chosen home only with --home; refuses a value the adapter's call fails.", + "flags": [ + { + "name": "env", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "field", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "file", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + }, + { + "name": "home", + "shorthand": "", + "type": "string", + "required": false, + "hidden": false + } + ] + }, { "path": "abcd ahoy doctor", "hidden": false, diff --git a/.abcd/development/specs/open/spc-2609221017544877-abcd-keeps-every-external-credential-the-same-way-one.md b/.abcd/development/specs/closed/spc-2609221017544877-abcd-keeps-every-external-credential-the-same-way-one.md similarity index 100% rename from .abcd/development/specs/open/spc-2609221017544877-abcd-keeps-every-external-credential-the-same-way-one.md rename to .abcd/development/specs/closed/spc-2609221017544877-abcd-keeps-every-external-credential-the-same-way-one.md diff --git a/.abcd/work/issues/open/iss-2609231016278107-itd-162-ac-1-ac-2-diverged-the-adopt-phase-s-commit-gate.md b/.abcd/work/issues/open/iss-2609231016278107-itd-162-ac-1-ac-2-diverged-the-adopt-phase-s-commit-gate.md index b5c9b8ced..4671e39c6 100644 --- a/.abcd/work/issues/open/iss-2609231016278107-itd-162-ac-1-ac-2-diverged-the-adopt-phase-s-commit-gate.md +++ b/.abcd/work/issues/open/iss-2609231016278107-itd-162-ac-1-ac-2-diverged-the-adopt-phase-s-commit-gate.md @@ -9,6 +9,8 @@ found_during: "autonomous run 2026-09-23 fidelity audit" origin: researcher-authored production_mode: hand-written found_at: "commands/prepare-this-repo.md" +deferred_after: "v0.11.0" +deferral_reason: "ruling owed to the product thinker (run A rulings-owed list, theme D, narrowing a shipped promise): Should the adopt phase offer a secrets and absolute-path commit gate from the binary as itd-162 ac-2 promised, or does the record amend ac-2 to say the private name guard replaced that gate deliberately?" --- itd-162 ac-1/ac-2 diverged: the adopt phase's commit-gate step used to offer a secrets + absolute-path gate as a pre-commit framework config (commands/prepare-this-repo.md at 328a6755^1 lines 171-174, template under a machine-local ~ path), and the intent promised that asset would resolve from the record or the binary instead. The delivery (PR #555) substitutes a different asset: step 5 now scaffolds abcd's private name guard (internal/core/ahoy/defaults/pre-commit, via ahoy install), and the embedded hook carries no secrets scan and no absolute-path gate, so an adopted repository no longer gets the gate the step existed to offer. spc-54 conflated the two ('the pre-commit config the Phase-3 line reaches for is thus already available embedded'). Either the adopt phase should offer a secrets/absolute-path commit gate from the binary or the record should say that gate was dropped deliberately diff --git a/.abcd/work/issues/open/iss-2609281654467661-the-macos-keychain-home-s-write-and-read-have-never-run.md b/.abcd/work/issues/open/iss-2609281654467661-the-macos-keychain-home-s-write-and-read-have-never-run.md new file mode 100644 index 000000000..eacdebc7b --- /dev/null +++ b/.abcd/work/issues/open/iss-2609281654467661-the-macos-keychain-home-s-write-and-read-have-never-run.md @@ -0,0 +1,16 @@ +--- +schema_version: 1 +id: "iss-2609281654467661" +slug: "the-macos-keychain-home-s-write-and-read-have-never-run" +severity: "minor" +category: "security" +source: "review-followup" +found_during: "autonomous run A resumed 2026-09-25: review-cred" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/credential/keychain.go" +deferred_after: "v0.11.0" +deferral_reason: "Deferred out loud by autonomous run A (lane fix2-cred, 2026-09-28): the check is a real-machine round trip against the macOS security tool, which the run must not touch, so it is owed to the technical facilitator as a human act outside the tree (rulings-owed section K). Until it runs, the fake keychain is the only proof of the -X in, -w out round trip." +--- + +The macOS keychain home's write and read have never run against the real security tool. Set stores a value through 'security -i' with the value hex-encoded behind -X on one stdin line (internal/core/credential/keychain.go), and Resolve reads it back with find-generic-password -w; a MaxValueBytes value (4096 bytes) makes that line about 8 KiB. Only the test binary's fake keychain (store_test.go TestMain) has proven the -X in, -w out round trip. If security's non-terminal line reader splits or truncates a long line, the read-back check after the write refuses loudly and echoes nothing, but a wrong item stays in the keychain for the person to remove by hand. One real-machine round trip is owed: a value of MaxValueBytes set through the keychain home, read back byte for byte, then the item removed. Surfaced by the review of the credential store lane (review-cred finding 5, unverified). diff --git a/.abcd/work/issues/open/iss-2609290300313698-the-credential-index-can-split-under-a-same-uid-swap-of-abcd-between-set-s-walk-and-readindex.md b/.abcd/work/issues/open/iss-2609290300313698-the-credential-index-can-split-under-a-same-uid-swap-of-abcd-between-set-s-walk-and-readindex.md new file mode 100644 index 000000000..5dedaabaa --- /dev/null +++ b/.abcd/work/issues/open/iss-2609290300313698-the-credential-index-can-split-under-a-same-uid-swap-of-abcd-between-set-s-walk-and-readindex.md @@ -0,0 +1,16 @@ +--- +schema_version: 1 +id: "iss-2609290300313698" +slug: "the-credential-index-can-split-under-a-same-uid-swap-of-abcd-between-set-s-walk-and-readindex" +severity: "minor" +category: "security" +source: "review-followup" +found_during: "autonomous run 2026-09-23" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/credential/store.go" +deferred_after: "v0.11.1" +deferral_reason: "Deferred out loud by autonomous run A (re-merge of integ/land-14, 2026-09-29), on review-integ14's recommendation: names, homes and pointers only, never a value, and the same uid can edit either index directly, so it is not a gate before the cut. The fix is an exported in-root declaration read, ReadHomeDeclarationDenying over the held *os.Root, used by readIndex under the index lock, with SetMachine taking Set's root for symmetry. capture defer refuses a minor record, so these two fields were set by hand (the iss-2609281654467661 precedent)." +--- + +The credential index can split between two ~/.abcd directories under a same-uid swap of one real directory for another between Set's walk and the index's re-walk. Set opens ~/.abcd once by descriptor and writes the index through that held root (store.go setIndex, WriteFileAtomicInRoot), but readIndex under the index lock re-walks ~/.abcd by path (fsutil.ReadHomeDeclarationDenying). If ~/.abcd is renamed aside and a different real directory swapped in between the two walks, the index read comes from the new directory while the write goes to the held one: the held directory's index becomes the new one's entries plus the new name (its own entries lost), and the live ~/.abcd does not record the new name. Names, homes and pointers only, never a value: SetMachine re-walks for its read and its write, so the value and its store stay together in the live directory. Same uid, who can edit either file directly. Surfaced by the security review of integ/land-14 (review-integ14 item 4, LOW (b)). diff --git a/.abcd/work/issues/open/iss-2608221254560250-docs-reference-terminology-md-memory-row-describes-retrieval.md b/.abcd/work/issues/resolved/iss-2608221254560250-docs-reference-terminology-md-memory-row-describes-retrieval.md similarity index 56% rename from .abcd/work/issues/open/iss-2608221254560250-docs-reference-terminology-md-memory-row-describes-retrieval.md rename to .abcd/work/issues/resolved/iss-2608221254560250-docs-reference-terminology-md-memory-row-describes-retrieval.md index 6316b2120..8cfcd2327 100644 --- a/.abcd/work/issues/open/iss-2608221254560250-docs-reference-terminology-md-memory-row-describes-retrieval.md +++ b/.abcd/work/issues/resolved/iss-2608221254560250-docs-reference-terminology-md-memory-row-describes-retrieval.md @@ -7,6 +7,14 @@ category: "observation" source: "user-observation" found_during: "context-window SOTA investigation" found_at: "docs/reference/terminology.md" +resolution: "the terminology Memory row describes the shipped retrieval: memory ask's word-overlap ranking over classes, domain and summary, top five with citations" +impact: fix +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- -docs/reference/terminology.md Memory row describes retrieval as 'recall-matched and budget-bracketed', but no retrieval engine exists: the recall: frontmatter field is parsed and round-tripped yet never consumed, and the budget brackets live only in itd-39 (draft). The row overstates shipped behaviour. \ No newline at end of file +docs/reference/terminology.md Memory row describes retrieval as 'recall-matched and budget-bracketed', but no retrieval engine exists: the recall: frontmatter field is parsed and round-tripped yet never consumed, and the budget brackets live only in itd-39 (draft). The row overstates shipped behaviour. + +## Grounds + +- pursued: the row now claims only shipped behaviour; a retrieval claim with no code behind it would show it wrong diff --git a/.abcd/work/issues/open/iss-2608301244458074-internal-core-frontmatter-has-no-entry-in-the-internal-readm.md b/.abcd/work/issues/resolved/iss-2608301244458074-internal-core-frontmatter-has-no-entry-in-the-internal-readm.md similarity index 70% rename from .abcd/work/issues/open/iss-2608301244458074-internal-core-frontmatter-has-no-entry-in-the-internal-readm.md rename to .abcd/work/issues/resolved/iss-2608301244458074-internal-core-frontmatter-has-no-entry-in-the-internal-readm.md index 5111bc31b..e6b135bc2 100644 --- a/.abcd/work/issues/open/iss-2608301244458074-internal-core-frontmatter-has-no-entry-in-the-internal-readm.md +++ b/.abcd/work/issues/resolved/iss-2608301244458074-internal-core-frontmatter-has-no-entry-in-the-internal-readm.md @@ -7,6 +7,10 @@ category: "documentation" source: "user-observation" found_during: "itd-179-round-3-builder" found_at: "internal/README.md" +resolution: "internal/README.md carries a core/frontmatter entry naming Unquote, QuoteScalar and EmptinessOf" +impact: internal +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- internal/core/frontmatter has no entry in the internal README package map @@ -19,3 +23,7 @@ map. The gap matters slightly more after round 3 than before it, because that package is now the canonical home of `Unquote` — the decoder consolidated out of `capture.unquote` and `lint.unescapeScalar` — so a reader looking for the one true scalar decoder has no map entry pointing there. + +## Grounds + +- pursued: a reader looking for the one scalar decoder finds it from the package map; a second decoder appearing outside the package would show it wrong diff --git a/.abcd/work/issues/open/iss-2609061503374089-the-plugin-provisioned-binary-v0-7-1-plugin-root-e3696dc524e.md b/.abcd/work/issues/resolved/iss-2609061503374089-the-plugin-provisioned-binary-v0-7-1-plugin-root-e3696dc524e.md similarity index 56% rename from .abcd/work/issues/open/iss-2609061503374089-the-plugin-provisioned-binary-v0-7-1-plugin-root-e3696dc524e.md rename to .abcd/work/issues/resolved/iss-2609061503374089-the-plugin-provisioned-binary-v0-7-1-plugin-root-e3696dc524e.md index 4f84a873e..69cc75fda 100644 --- a/.abcd/work/issues/open/iss-2609061503374089-the-plugin-provisioned-binary-v0-7-1-plugin-root-e3696dc524e.md +++ b/.abcd/work/issues/resolved/iss-2609061503374089-the-plugin-provisioned-binary-v0-7-1-plugin-root-e3696dc524e.md @@ -9,6 +9,14 @@ found_during: "2026-09-06 use in a managed repo" origin: researcher-authored production_mode: hand-written found_at: "commands/decide.md" +resolution: "commands/decide.md states the verb needs abcd 0.8.0 or later and routes an older binary's refusal to the release check in that binary's own spelling; the intent page says research/notes/ is not scaffolded, and ahoy install already writes identity.json. The record's second remedy, that the ahoy status should report the gap, is dropped rather than built: the payload lag that caused the gap is gone at 0.11.0, so the documented minimum version is the whole fix on the docs' merit" +impact: fix +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- The plugin-provisioned binary (v0.7.1, plugin root e3696dc524e3) has no decide verb, but the plugin ships the /abcd:decide skill, which tells a plugin user to run '/abcd decide' and reports an unknown-command refusal. Observed on 2026-09-06 in a managed repo when minting its first ADR; the source-checkout build has the verb. Either the plugin payload lags the skill it documents, or the skill should state the minimum binary version and the ahoy status should report the gap. Same session also found the intent skill referring to .abcd/config/identity.json and to research/notes/, neither of which ahoy install scaffolds in a fresh managed repo. + +## Grounds + +- pursued: a plugin user on an older binary is told why decide is unknown and how to update; a user still meeting a bare unknown-command with no route would show it wrong diff --git a/.abcd/work/issues/open/iss-2609091648476051-the-assemble-help-teaches-an-invocation-that-can-never-be-ingested.md b/.abcd/work/issues/resolved/iss-2609091648476051-the-assemble-help-teaches-an-invocation-that-can-never-be-ingested.md similarity index 74% rename from .abcd/work/issues/open/iss-2609091648476051-the-assemble-help-teaches-an-invocation-that-can-never-be-ingested.md rename to .abcd/work/issues/resolved/iss-2609091648476051-the-assemble-help-teaches-an-invocation-that-can-never-be-ingested.md index abe289a43..6082e3d95 100644 --- a/.abcd/work/issues/open/iss-2609091648476051-the-assemble-help-teaches-an-invocation-that-can-never-be-ingested.md +++ b/.abcd/work/issues/resolved/iss-2609091648476051-the-assemble-help-teaches-an-invocation-that-can-never-be-ingested.md @@ -9,6 +9,14 @@ found_during: "release-gate" origin: researcher-authored production_mode: hand-written found_at: "commands/reading.md" +resolution: "reading assemble reports ingestable and renders an ingest: line for a run written outside the run directory; the help example and the plugin page assemble into the default run directory and name --out as an inspection copy" +impact: fix +resolved_by: + commit: "400625adc0d0c908b96a23a6c86fb97c7b3ed5e0" --- The reading assemble verb accepts an output directory, and both its terminal help and its plugin page use one in the worked example, writing the run somewhere other than the default run directory. A run assembled that way can never be ingested: the ingest verb resolves a run's manifest only from the default run directory under its run id, so a manifest parked anywhere else is invisible to it and the run cannot be proven. Nothing says so at assembly time, and nothing says so in the example. The shipped help is therefore teaching the one invocation that leads to a dead end, and it teaches it in the place a reader is most likely to copy from. The cost lands late: the assembly succeeds, the reading is commissioned and returned, and the failure appears at the ingest, by which point the run's output exists and has nowhere to go. Fix direction: either make the example use the default location and document the flag as a dry-run and inspection aid rather than a run location, or teach the ingest to resolve a manifest from a named directory so the flag means what the example implies. Detector: an assembly written outside the default run directory either reports at assembly time that it cannot be ingested, or is ingestable, and the worked example in the help and on the plugin page is one a reader can follow to a committed run. + +## Grounds + +- pursued: an assembly written with --out now says at assembly time it cannot be ingested, and the worked examples reach a committed run; a named-directory run reading ingestable, or an example still using --out, would show it wrong diff --git a/.abcd/work/issues/open/iss-2609120452369809-the-record-dispatch-documentation-says-iss-n-while-the-minte.md b/.abcd/work/issues/resolved/iss-2609120452369809-the-record-dispatch-documentation-says-iss-n-while-the-minte.md similarity index 87% rename from .abcd/work/issues/open/iss-2609120452369809-the-record-dispatch-documentation-says-iss-n-while-the-minte.md rename to .abcd/work/issues/resolved/iss-2609120452369809-the-record-dispatch-documentation-says-iss-n-while-the-minte.md index 4c72599ad..81f562acf 100644 --- a/.abcd/work/issues/open/iss-2609120452369809-the-record-dispatch-documentation-says-iss-n-while-the-minte.md +++ b/.abcd/work/issues/resolved/iss-2609120452369809-the-record-dispatch-documentation-says-iss-n-while-the-minte.md @@ -9,6 +9,10 @@ found_during: "peer session report from a downstream repo, 2026-09-12" origin: researcher-authored production_mode: hand-written found_at: "commands/abcd.md" +resolution: "commands/abcd.md and the root help state both id shapes resolve (short ordinal and sixteen-digit stamp) and nothing renumbers; placeholders keep their form" +impact: fix +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- Reported from a downstream repository using abcd: `abcd capture ""` mints @@ -58,3 +62,7 @@ from after it, and both resolve. - **Given** the surfaces that spell an id, **when** the sweep runs, **then** placeholders keep their `` form and claims about the shape are corrected. + +## Grounds + +- pursued: a reader of the dispatch surface learns both shapes resolve; a surface claiming ids are short ordinals would show it wrong diff --git a/.abcd/work/issues/open/iss-2609151150180399-the-abcd-intent-skill-text-grounds-section-shows-the-ready-g.md b/.abcd/work/issues/resolved/iss-2609151150180399-the-abcd-intent-skill-text-grounds-section-shows-the-ready-g.md similarity index 73% rename from .abcd/work/issues/open/iss-2609151150180399-the-abcd-intent-skill-text-grounds-section-shows-the-ready-g.md rename to .abcd/work/issues/resolved/iss-2609151150180399-the-abcd-intent-skill-text-grounds-section-shows-the-ready-g.md index f5e66fe7d..389cb8f44 100644 --- a/.abcd/work/issues/open/iss-2609151150180399-the-abcd-intent-skill-text-grounds-section-shows-the-ready-g.md +++ b/.abcd/work/issues/resolved/iss-2609151150180399-the-abcd-intent-skill-text-grounds-section-shows-the-ready-g.md @@ -9,6 +9,14 @@ found_during: "peer session report 2026-09-15 (a teaching-repo session planning origin: researcher-authored production_mode: hand-written found_at: "commands/intent.md" +resolution: "commands/intent.md says grounds.redacted is omitted when zero, matching the omitempty convention every write verb's redacted count follows" +impact: fix +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- The /abcd:intent skill text (Grounds section) shows the ready --grounds --json envelope carrying "redacted": 0 and asks the host to report grounds.redacted whenever it is non-zero, but GroundsResult.Redacted (internal/core/intent/grounds.go) is tagged omitempty, so the key is absent whenever the count is zero. A host following the page finds no key at all on every ordinary write (five of five in the reporting session, v0.8.0 plugin binary) and cannot tell an omitted zero from a missing field. Either the skill text says the key is omitted when zero, or the envelope carries it always; the envelope's own convention for counts elsewhere (json_empty_collections_test) should decide which. + +## Grounds + +- pursued: a host reading the page expects no key on an unredacted write; an envelope that emits redacted: 0 would show it wrong diff --git a/.abcd/work/issues/open/iss-2609190338070038-the-capture-surface-page-documents-the-two-status-folders-re.md b/.abcd/work/issues/resolved/iss-2609190338070038-the-capture-surface-page-documents-the-two-status-folders-re.md similarity index 77% rename from .abcd/work/issues/open/iss-2609190338070038-the-capture-surface-page-documents-the-two-status-folders-re.md rename to .abcd/work/issues/resolved/iss-2609190338070038-the-capture-surface-page-documents-the-two-status-folders-re.md index 2540e8d49..deabf2cfc 100644 --- a/.abcd/work/issues/open/iss-2609190338070038-the-capture-surface-page-documents-the-two-status-folders-re.md +++ b/.abcd/work/issues/resolved/iss-2609190338070038-the-capture-surface-page-documents-the-two-status-folders-re.md @@ -9,6 +9,14 @@ found_during: "Gropius autonomous sweep, session gropiusllm-66, relayed to abcd- origin: researcher-authored production_mode: hand-written found_at: "commands/capture.md" +resolution: "commands/capture.md states the resolve-on-the-carrying-branch convention for managed repositories beside the two-status-folders refusal" +impact: fix +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- The capture surface page documents the two-status-folders refusal but not the convention that avoids it: a record is resolved on the branch that carries it. commands/capture.md explains why an id in open/ and resolved/ at once is refused and how the merge artefact arises, and stops there; the rule that prevents it (never re-add a record to the default branch after a branch was cut from it; resolve it on the branch that carries it) lives only in this repository's AGENTS.md, which a managed repository does not inherit. In the Gropius sweep of 2026-09-19 (session gropiusllm-66, forty lanes) a record captured on an unmerged branch could not be resolved from another branch without producing exactly that duplicate once both landed, so lanes merged each other's branches to resolve. Wanted, either: one paragraph on the capture page stating the convention for a managed repository, or a resolve that tolerates a record absent from this checkout when told where it is (the session's --expect-on-branch), which is the ledger-visibility capability itd-2609091416295622 already weighs. This record is the documentation half. + +## Grounds + +- pursued: a managed-repository reader of the capture page finds the convention that avoids the duplicate; a lane resolving a record from a second branch after reading the page would show it wrong diff --git a/.abcd/work/issues/open/iss-2609240519413467-the-disembark-plugin-page-s-argument-hint-advertises.md b/.abcd/work/issues/resolved/iss-2609240519413467-the-disembark-plugin-page-s-argument-hint-advertises.md similarity index 69% rename from .abcd/work/issues/open/iss-2609240519413467-the-disembark-plugin-page-s-argument-hint-advertises.md rename to .abcd/work/issues/resolved/iss-2609240519413467-the-disembark-plugin-page-s-argument-hint-advertises.md index db78554b9..dbc29f217 100644 --- a/.abcd/work/issues/open/iss-2609240519413467-the-disembark-plugin-page-s-argument-hint-advertises.md +++ b/.abcd/work/issues/resolved/iss-2609240519413467-the-disembark-plugin-page-s-argument-hint-advertises.md @@ -9,6 +9,14 @@ found_during: "v0.10.0 release gate: brief-surface crosscheck" origin: researcher-authored production_mode: hand-written found_at: "commands/disembark.md" +resolution: "the disembark argument-hint lists pack, plan, probe and the five further sub-verbs with the operands the CLI takes" +impact: fix +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- The disembark plugin page's argument-hint advertises ' ', a form the binary rejects with 'unknown command' because packing needs the explicit pack sub-verb; the hint also omits pack and five shipped sub-verbs (coverage, graveyard, press-release, principles, review) and presents plan/probe as required where the CLI makes the repo optional. Found by checker a01 at fa744b41 (finding x-011). + +## Grounds + +- pursued: every form the hint advertises is one the binary accepts; a hint form the CLI rejects would show it wrong diff --git a/.abcd/work/issues/open/iss-2609240519418856-the-help-for-abcd-docs-cite-confirm-receipt-cite-go-165.md b/.abcd/work/issues/resolved/iss-2609240519418856-the-help-for-abcd-docs-cite-confirm-receipt-cite-go-165.md similarity index 66% rename from .abcd/work/issues/open/iss-2609240519418856-the-help-for-abcd-docs-cite-confirm-receipt-cite-go-165.md rename to .abcd/work/issues/resolved/iss-2609240519418856-the-help-for-abcd-docs-cite-confirm-receipt-cite-go-165.md index 3ac64a704..064596cfd 100644 --- a/.abcd/work/issues/open/iss-2609240519418856-the-help-for-abcd-docs-cite-confirm-receipt-cite-go-165.md +++ b/.abcd/work/issues/resolved/iss-2609240519418856-the-help-for-abcd-docs-cite-confirm-receipt-cite-go-165.md @@ -9,6 +9,14 @@ found_during: "v0.10.0 release gate: brief-surface crosscheck" origin: researcher-authored production_mode: hand-written found_at: "internal/surface/cli/cite.go" +resolution: "the docs cite confirm --receipt help describes the receipt schema instead of a checklist page that does not exist, mirrored in the generated reference" +impact: fix +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- The help for 'abcd docs cite confirm --receipt' (cite.go:165, mirrored in docs/reference/cli/commands.md:437) describes the receipt file as 'the format the generated checklist page emits', presenting a generated checklist page as an existing producer; no such generator exists in the tree (internal/core/cite/confirm.go names it as later). Found by checker a09 at fa744b41 (finding x-035). + +## Grounds + +- pursued: the help names only producers that exist; a help line naming an unbuilt generator would show it wrong diff --git a/.abcd/work/issues/open/iss-2609240519427388-commands-prepare-this-repo-md-228-231-promises-that-no.md b/.abcd/work/issues/resolved/iss-2609240519427388-commands-prepare-this-repo-md-228-231-promises-that-no.md similarity index 61% rename from .abcd/work/issues/open/iss-2609240519427388-commands-prepare-this-repo-md-228-231-promises-that-no.md rename to .abcd/work/issues/resolved/iss-2609240519427388-commands-prepare-this-repo-md-228-231-promises-that-no.md index da0d6b86d..bc4c444ba 100644 --- a/.abcd/work/issues/open/iss-2609240519427388-commands-prepare-this-repo-md-228-231-promises-that-no.md +++ b/.abcd/work/issues/resolved/iss-2609240519427388-commands-prepare-this-repo-md-228-231-promises-that-no.md @@ -9,6 +9,14 @@ found_during: "v0.10.0 release gate: brief-surface crosscheck" origin: researcher-authored production_mode: hand-written found_at: "commands/prepare-this-repo.md" +resolution: "commands/prepare-this-repo.md already says the config ahoy install seeds, .abcd/docs-lint.json among them, is the repository's own and is committed; only hand-copied lint config is barred" +impact: fix +resolved_by: + commit: "d0c1899262284daae9413703dbf4c203b992995f" --- commands/prepare-this-repo.md:228-231 promises that no lint-config JSON is committed, but step 5 runs ahoy install, which writes .abcd/docs-lint.json into the target repository, so an adopter commits abcd-authored content the page said would not be there. Found by the v0.10.0 brief-surface crosscheck at fa744b41 (finding x-050). + +## Grounds + +- pursued: the page no longer promises an absence ahoy install contradicts; a page barring the seeded docs-lint.json would show it wrong diff --git a/.abcd/work/issues/open/iss-2609240646458365-store-root-sha-key-length-is-undocumented.md b/.abcd/work/issues/resolved/iss-2609240646458365-store-root-sha-key-length-is-undocumented.md similarity index 76% rename from .abcd/work/issues/open/iss-2609240646458365-store-root-sha-key-length-is-undocumented.md rename to .abcd/work/issues/resolved/iss-2609240646458365-store-root-sha-key-length-is-undocumented.md index f674366c1..a0002bc84 100644 --- a/.abcd/work/issues/open/iss-2609240646458365-store-root-sha-key-length-is-undocumented.md +++ b/.abcd/work/issues/resolved/iss-2609240646458365-store-root-sha-key-length-is-undocumented.md @@ -9,6 +9,14 @@ found_during: "autonomous run 2026-09-23" origin: researcher-authored production_mode: hand-written found_at: "AGENTS.md" +resolution: "AGENTS.md and the configuration chapter state is the full object name; the worktree-store draft already said so and gains an open question on listing a short-key lane" +impact: internal +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- The machine-scoped stores are documented as keyed on the repository's root commit (`~/.abcd///`), and no page says the key is the full forty-character sha. The verbs that create a store (the run store, the history store, the transcript store) key on the full sha. The worktree store has no verb yet and is laid by hand, and its lanes sit under the eight-character short form; AGENTS.md says it is keyed "the way the history, transcript and voyage stores already are", which only a reader who checks those stores reads as the full sha. Autonomous run A's run file named the run store by the short form too, so the orchestrator appended its first events by hand to `~/.abcd/runs//` while `abcd implement` wrote to the full-sha directory, and the two logs were joined by moving the short directory and leaving a symlink in its place. Wanted: AGENTS.md, the brief's configuration chapter and the worktree-store draft itd-2609091014076309 say the key is the full sha, and whatever lists the stores names a short-form directory beside a full one. + +## Grounds + +- pursued: a reader keying a store by hand uses the full sha; a verb keying a store on an abbreviation would show it wrong diff --git a/.abcd/work/issues/open/iss-2609251645376219-docs-reference-cli-readme-md-says-abcd-help-documents-itself.md b/.abcd/work/issues/resolved/iss-2609251645376219-docs-reference-cli-readme-md-says-abcd-help-documents-itself.md similarity index 61% rename from .abcd/work/issues/open/iss-2609251645376219-docs-reference-cli-readme-md-says-abcd-help-documents-itself.md rename to .abcd/work/issues/resolved/iss-2609251645376219-docs-reference-cli-readme-md-says-abcd-help-documents-itself.md index da906be1f..7e66d5623 100644 --- a/.abcd/work/issues/open/iss-2609251645376219-docs-reference-cli-readme-md-says-abcd-help-documents-itself.md +++ b/.abcd/work/issues/resolved/iss-2609251645376219-docs-reference-cli-readme-md-says-abcd-help-documents-itself.md @@ -8,6 +8,14 @@ source: "user-observation" found_during: "autonomous run A resumed 2026-09-25" origin: researcher-authored production_mode: hand-written +resolution: "docs/reference/cli/README.md says abcd --help lists the verbs a person types and abcd --help --agent adds the agents-and-hosts verbs" +impact: fix +resolved_by: + commit: "07af2eb092552739a5360c39e5659457e7b3c734" --- docs/reference/cli/README.md says abcd --help documents itself, but the default list now omits the agents-and-hosts verbs and nothing in the prose names --help --agent, so a person who finds abcd version in commands.md does not see it under abcd --help and is not told the list expands (review-helpgroups 1). + +## Grounds + +- pursued: a person who finds an agent verb in commands.md learns how to list it; a hidden verb the README does not account for would show it wrong diff --git a/.abcd/work/issues/open/iss-2609281310017733-home-scope-link-check-is-by-path-a-same-uid-race-can-swap-a.md b/.abcd/work/issues/resolved/iss-2609281310017733-home-scope-link-check-is-by-path-a-same-uid-race-can-swap-a.md similarity index 63% rename from .abcd/work/issues/open/iss-2609281310017733-home-scope-link-check-is-by-path-a-same-uid-race-can-swap-a.md rename to .abcd/work/issues/resolved/iss-2609281310017733-home-scope-link-check-is-by-path-a-same-uid-race-can-swap-a.md index 6c5707c01..a315730e8 100644 --- a/.abcd/work/issues/open/iss-2609281310017733-home-scope-link-check-is-by-path-a-same-uid-race-can-swap-a.md +++ b/.abcd/work/issues/resolved/iss-2609281310017733-home-scope-link-check-is-by-path-a-same-uid-race-can-swap-a.md @@ -9,6 +9,14 @@ found_during: "autonomous run A resumed 2026-09-25" origin: researcher-authored production_mode: hand-written found_at: "internal/fsutil/home.go" +resolution: "Every reader and writer of a ~/.abcd file now reaches it through the descriptor of the directory that was judged: fsutil.OpenHomeScope and EnsureHomeScope walk each level below home relative to the one above, refuse a symlink by name and confirm with os.SameFile that the opened directory is the one the Lstat vetted. ReadHomeDeclaration, statusline.ReadSettingsFile, credential.SetMachine, oracle writeProviderBlock, ahoy writePathEntry, writeMachineRouting, the status-line setting write and the history registry go through it. stdlib only: syscall.Openat is linux-only, so os.Root plus the identity check stands in for openat with O_NOFOLLOW, with the same guarantee." +impact: fix +resolved_by: + commit: "3688bbe55" --- The symlinked ~/.abcd rule is enforced by path, not by descriptor, so a same-uid race can still slip a link in between the check and the use. fsutil.HomeScopeLink Lstats each directory below home (internal/fsutil/home.go), and the reader then opens the file by its full path (ReadHomeDeclaration, then ReadDeclaration's own Lstat, open and SameFile), while the writers check and then MkdirAll, lock or create by path (credential.SetMachine, oracle writeProviderBlock, ahoy writePathEntry, writeMachineRouting, wireStatusLine, and the history registry's EnsureRealDirAll after historyRoot). A process running as the same uid that swaps ~/.abcd for a symlink after the Lstat reads or writes through the link. It is the same residual the rules loader's Lstat carried, and it needs the caller's own uid, so it widens nothing an attacker at that uid could not do directly; it is recorded so the rule's guarantee is stated at the strength it actually has. os.Root would not close it: it follows a symlink that stays inside the root, and a dotfiles ~/.abcd usually points inside home. The airtight form uses only the standard library (syscall on darwin and linux, no new dependency): open home, then syscall.Openat(homefd, ".abcd", O_DIRECTORY|O_NOFOLLOW), then Openat(dirfd, leaf, O_NOFOLLOW) (O_CREAT with O_EXCL or O_NOFOLLOW for writers, Mkdirat for a missing level), and judge and read or write through those descriptors. Left open by the fix round that routed every reader and writer through HomeScopeLink (branch fix/drain-symlinked-home); not built there. + +## Grounds + +- pursued: a ~/.abcd swapped for a symlink between its judgement and its use is refused and nothing is read from or written behind the link; the race tests in fsutil, credential, oracle and ahoy stage the swap through the vetting hook and would show it wrong by finding the dotfiles copy read or a file in the link target diff --git a/.abcd/work/issues/resolved/iss-2609290259108077-a-home-that-is-itself-a-symlink-into-a-checkout-escapes-the-credential-working-tree-check.md b/.abcd/work/issues/resolved/iss-2609290259108077-a-home-that-is-itself-a-symlink-into-a-checkout-escapes-the-credential-working-tree-check.md new file mode 100644 index 000000000..b063e5e22 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2609290259108077-a-home-that-is-itself-a-symlink-into-a-checkout-escapes-the-credential-working-tree-check.md @@ -0,0 +1,22 @@ +--- +schema_version: 1 +id: "iss-2609290259108077" +slug: "a-home-that-is-itself-a-symlink-into-a-checkout-escapes-the-credential-working-tree-check" +severity: "minor" +category: "security" +source: "review-followup" +found_during: "autonomous run 2026-09-23" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/credential/store.go" +resolution: "Fixed by 16b7e58da: workingTreeAbove judges the place by its lexical path and again with the home replaced by where it resolves, so a home that is itself a symlink into a checkout is seen as lying inside it; the abcd home is refused there with nothing written, and a pointer under that home is not read. The home is never refused for being a link." +impact: fix +resolved_by: + commit: "16b7e58da" +--- + +The credential store's working-tree check misses a home directory that is itself a symlink into a git checkout. workingTreeAbove (internal/core/credential/store.go) climbs the lexical path of ~/.abcd, so when HOME is a link such as ~ -> /home, the target's parents (/.git) are never examined; Set with the abcd home then writes credentials.json, a value, inside the checkout, where a commit could carry it. The dotfiles layout AGENTS.md names (a symlinked ~/.abcd) is refused already; this is the account's own home layout, same uid. Judging the tree on the home's resolved path (filepath.EvalSymlinks) as well as its lexical path closes it without refusing a home for being a link. Surfaced by the security review of integ/land-14 (review-integ14 LOW (a)). + +## Grounds + +- pursued: a home linked into a checkout is refused for the abcd home with nothing written in the checkout and the keychain untouched; a Set that writes credentials.json under such a home, or a refusal of a home linked outside any checkout, would show it wrong diff --git a/ACKNOWLEDGEMENTS.md b/ACKNOWLEDGEMENTS.md index b0ef1d8fa..d06ebcb55 100644 --- a/ACKNOWLEDGEMENTS.md +++ b/ACKNOWLEDGEMENTS.md @@ -88,6 +88,11 @@ Ideas and methodologies that shaped the design — not code abcd depends on. *brevity bias* and *context collapse* — which itd-81 cites to strike itd-5's "shorter by >10%" prompt tiebreak. - **Amazon "Working Backwards"** — the press-release format of abcd's intents. +- **Apple's Keychain and its `security` command (Apple)** — the macOS home of + the credential store's keychain home: an item per credential under the + service name `abcd`, added through the command's interactive mode with the + value as hex on stdin so it never reaches an argument, and read back with + `find-generic-password -w` (itd-2609221017023290). - **Architecture Decision Records (MADR)** — the shape of the decision record. - **CARL (Context Augmentation & Reinforcement Layer, Christopher Kahler, MIT)** — the just-in-time rule-injection mechanism (a prompt hook, a JSON @@ -192,6 +197,11 @@ Ideas and methodologies that shaped the design — not code abcd depends on. once it has passed (itd-2609221656373558). "Leases: an efficient fault-tolerant mechanism for distributed file cache consistency", SOSP 1989. +- **libsecret's `secret-tool` (GNOME, LGPL)** — the Linux home of the + credential store's keychain home, reached through the freedesktop secret + service: `store` reads the value from stdin and `lookup` prints it, each + item keyed by service `abcd` and the credential's name + (itd-2609221017023290). - **The Linux kernel's coding-assistants policy** — the `Assisted-by:` attribution model abcd adopts for AI-assisted commits. - **mattpocock/skills (Matt Pocock, MIT)** — four adaptations: the diff --git a/AGENTS.md b/AGENTS.md index 36793cecf..8edee199d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -225,7 +225,10 @@ irreversible; guessing downward costs nothing.** session's own checkout lives at `~/.abcd/worktrees///`, keyed on the repository's root commit the way the history, transcript and voyage stores already are — a checkout moves, is renamed and is cloned twice on one - machine, while its root commit does none of that. Not beside the checkout, + machine, while its root commit does none of that. `` is the full + object name (forty hex digits under SHA-1, sixty-four under SHA-256), as the + verbs that create a store key it; an abbreviated key names a second directory + that no verb reads. Not beside the checkout, not in the directory the user keeps their projects in, and not inside the working tree, which every tree scan walks. A tool never creates a directory in space the user did not hand it, and beside a checkout there is no declared diff --git a/commands/abcd.md b/commands/abcd.md index baeeae7d5..347e17fa5 100644 --- a/commands/abcd.md +++ b/commands/abcd.md @@ -85,6 +85,13 @@ record in its store — any status folder or bucket — and renders it read-only "${CLAUDE_PLUGIN_ROOT}/abcd" --json ``` +The digits take one of two shapes, and both resolve: A short ordinal from +before ids were minted (`iss-188`), or the sixteen-digit timestamp the +minting verbs allocate (`iss-2609120452369809`). A ledger part-way through +adoption holds both, and neither is migrated away: Nothing renumbers an +existing record, because an id is a citation. An `N` in a placeholder such as +`` stands for either shape. + Summarise the `id`, `family`, `status`, `title`, `path`, the `links` edges (`spec_id`, `intent`, `intents`, `related_intents`, `related_issues`, `resolved_by.*`, `superseded_by` as present; `intents` is every member a bundle's shared spec lists), and each entry in `next_moves` — the concrete lifecycle move diff --git a/commands/ahoy.md b/commands/ahoy.md index 7e37aaa68..4a0205ffd 100644 --- a/commands/ahoy.md +++ b/commands/ahoy.md @@ -421,7 +421,7 @@ never for a frontier model, which a bundled vendor denylist (`anthropic/*` at minimum) keeps on the host. Everything works without one: with no provider configured, every delegated step runs on the host. Relay `explanation`, each of `providers` with its `key_state` (`set`, `not set`, `none`, or a refusal; never -the key), the `denylist`, the `routes`, every line of `diagnostics`, and the +the key) and `key_home` (the home it resolves from), the `denylist`, the `routes`, every line of `diagnostics`, and the `key_homes` prose verbatim: it recommends the platform keychain in prose, and the choice stays the person's, so never present one home as the marked option. Relay `dispatch` too: no delegating verb sends a step to a provider yet, so a @@ -433,24 +433,62 @@ adapter refuses as `oracle_api.config_refused`, naming the file and the key. Declining is not running `connect`, and it changes nothing. The setup is `abcd ahoy connect --base-url --model -[--model …] --home abcd [--key ]`, with the key piped in on stdin -from a file or a variable. **This writes, under `~/.abcd/` alone.** It verifies the provider with one call -to the first model listed, and only when that call succeeds writes the key into -the owner-only `~/.abcd/credentials.json` and the provider block (the base URL, -the key's name and the models, the allowlist) into `~/.abcd/config.json`. -Nothing goes into the repository or the harness's settings, and a failed -verification writes nothing. A `~/.abcd` that is a symlink (into a dotfiles -checkout, say) is refused with nothing written, naming the link: the key would -otherwise land wherever it points. `--home none` sets up a server that takes no key. -The `external` and `keychain` homes arrive with the credential store -(itd-2609221017023290) and are refused, naming it, before any call. +[--model …] --home [--key ]`. **This writes, under +`~/.abcd/` and, for the keychain home, into the platform keychain.** Set +`abcd mode facilitator` and ask the technical facilitator which home through +your question tool, after relaying `key_homes`, and offer the three without +marking one: `external` takes `--env ` or +`--file ~/.json --field ` (abcd keeps only where the key +is); `abcd` and `keychain` take the key piped in on stdin from a file or a +variable. It verifies the provider with one call to the first model listed, +and only when that call succeeds keeps the key in that home and writes the +provider block (the base URL, the key's name and the models, the allowlist) +into `~/.abcd/config.json`. Nothing goes into the repository or the harness's +settings, and a failed verification writes nothing. A `~/.abcd` that is a +symlink (into a dotfiles checkout, say) is refused with nothing written, in +any home, naming the link: the key would otherwise land wherever it points. +A `--file` pointer is refused, naming the link, when any directory between the +home and the tool's file is a symlink, wherever it leads (a `~/.config` linked +elsewhere, say); `--env` stays open. +`--home none` sets up a server that takes no key. The key is read from stdin and nowhere else, and never from a terminal, where it would be echoed. **Never ask the person for the key and never pass it yourself**: it would enter this conversation. Give them the command to run in their own shell, with the key piped in from a file or a variable they hold, and -relay the result — `verified` (the provider, the model asked for and the model -it reported), each `wrote` path, and `dispatch`. +relay the result — `verified` (the provider, the model asked for, the model +it reported and the credential's name), each `wrote` path, and `dispatch`. + +## `credential` — the credential store's walkthrough + +```bash +"${CLAUDE_PLUGIN_ROOT}/abcd" ahoy credential --json +"${CLAUDE_PLUGIN_ROOT}/abcd" ahoy credential --json +``` + +Every external credential abcd holds lives in one store, in the home the person +chooses once per credential. Bare, the sub-verb lists each credential an +adapter reads (`hosting.cloudflare` for the site setup, each configured +provider's key) with its `state` (`set`, `not set`, or a refusal) and `home`; +never a value. With a name it explains that credential and writes nothing: +relay `unlocks`, `without_it`, then `homes_prose` verbatim (it recommends the +platform keychain in the prose; never present one home as the marked option), +then the `homes` and the `setup` command for each. + +The walkthrough is `abcd ahoy credential --home `, with the same +three homes as `connect`: `external` with `--env`, or `--file` and `--field`; +`abcd` and `keychain` with the value piped in on stdin. **This writes the +chosen home only after the reading adapter's own verification call succeeds** +(the provider's one short exchange, the hosting provider's account read). Set +`abcd mode facilitator` and ask the technical facilitator for the home through +your question tool; never ask for the value, and never pass it yourself: give +the person the command to run in their own shell and relay `name`, `home`, +`verified` and each `wrote` entry. A name another home already holds, or a +different value for a name already kept, is refused: abcd never replaces a +stored secret. The abcd home is refused when `~/.abcd` lies inside a git +working tree (the keychain and an external home stay open +there), and a platform with no keychain tool refuses the keychain home and +names the other two. ## `--dry-run` — the canonical detection envelope diff --git a/commands/capture.md b/commands/capture.md index 7b684dde6..9390bf267 100644 --- a/commands/capture.md +++ b/commands/capture.md @@ -252,7 +252,12 @@ is a merge artefact rather than a hand edit: a record committed to the default branch after a branch was cut from it, and then resolved on that branch, arrives as an add on one side and a delete-plus-add on the other, which rename detection does not pair. Relay the refusal; the fix is to move or remove one of the two -files so the ledger says which status the record is in. Summarise each issue's `id`, `status`, +files so the ledger says which status the record is in. The convention that +keeps the state from arising holds in any repository abcd manages: A record is +resolved on the branch that carries it. A record captured on a branch is +resolved on that branch, or after it lands, and never re-added to the default +branch while the branch is still open; resolving it from a second branch +produces the same duplicate once both land. Summarise each issue's `id`, `status`, `severity`, and `slug`. The list is returned in **derived-priority order**: unblocked issues first, then by severity (`critical` → `nitpick`); rows still blocked by an open dependency are demoted and annotated `[blocked-by iss-N,…]`. diff --git a/commands/decide.md b/commands/decide.md index a3d2e2de1..22220cca9 100644 --- a/commands/decide.md +++ b/commands/decide.md @@ -34,6 +34,17 @@ and user-facing capability is an intent. The title is one quoted operand — a short noun phrase stating the decision in a line. The verb derives the record's slug from it, so keep it in plain words. +**The verb needs abcd 0.8.0 or later.** An older binary has no `decide` and +refuses the call as an unknown command, with nothing written. When that is the +refusal, run the release check through the same binary. A binary that old +spells it `"${CLAUDE_PLUGIN_ROOT}/abcd" version --check --json`, where 0.11.0 +and later spell it `update --check`. Relay the installed `version`. A 0.7.1 or +newer binary also answers `check.next_step`: relay it verbatim, since it names +the command that takes the update for this install's shape, the host's plugin +update for a plugin-root binary included. A 0.7.0 or older binary answers +without that field, so point the person at the install instructions for their +shape instead. Do not mint the record by hand in the meantime. + Report the `id` and the `path` from the JSON, then open the record and write the four sections with the user. Nothing else about the decision is the binary's to supply. diff --git a/commands/disembark.md b/commands/disembark.md index bf50c8c4f..ce2f77a82 100644 --- a/commands/disembark.md +++ b/commands/disembark.md @@ -1,7 +1,7 @@ --- name: disembark description: "Pack a repository into a lifeboat, probing and planning first: Writes nothing in the source, only inside the lifeboat; refuses an unknown sub-verb." -argument-hint: "pack | plan [] | probe []" +argument-hint: "pack | plan [] | probe [] | coverage ... | review [--review-json ] | principles [--principles-json ] | press-release [--press-release-json ] | graveyard --lessons-json " block: people --- diff --git a/commands/intent.md b/commands/intent.md index 45300036f..5a6bdb404 100644 --- a/commands/intent.md +++ b/commands/intent.md @@ -72,7 +72,8 @@ capture routes the pieces, it never files a monolith: the same session (or are captured so they are not lost). 4. **Grade.** Append the confirmed table — and whether the initial routing survived the human's confirmation — to the dated decomposition-calibration - note under the development record's `research/notes/`. That corpus (about + note under the development record's `research/notes/`, a folder no install + scaffolds, so create it where the record has none. abcd's own corpus (about 50 graded captures) is what gates the automated rung. **Partly automated.** The lexical candidate pass of step 2 runs at filing: @@ -206,14 +207,16 @@ borrowing the gate's exit 1. ```json { "grounds": { "intent_id": "…", "path": "…", "token": "pursued", - "text": "…", "entries": 1, "redacted": 0 }, + "text": "…", "entries": 1, "redacted": 2 }, "ready": { "…the usual ReadyResult…" } } ``` Read the verdict from `ready`, and report `grounds.path` and `grounds.entries` so the user knows a record was written. **Report `grounds.redacted` whenever it -is non-zero** — the text is scanned before it is committed, and the user needs to -know their wording was changed. There is no `degraded` member here: a scanner +is present** — the text is scanned before it is committed, and the user needs to +know their wording was changed. The key is omitted when the count is zero, as +every write verb's `redacted` count is, so an absent key means nothing was +rewritten. There is no `degraded` member here: a scanner that cannot be built, or whose pattern set a per-repo override weakened, refuses the write at exit 2 rather than writing under a weakened detector. Without the flag the payload is the readiness result unchanged. diff --git a/commands/reading.md b/commands/reading.md index a9947ad56..46dbaf8ad 100644 --- a/commands/reading.md +++ b/commands/reading.md @@ -213,15 +213,22 @@ many carry neither. No other position's report has the field. ```bash "${CLAUDE_PLUGIN_ROOT}/abcd" reading assemble \ - --position entailment --target HEAD \ - --out .abcd/.work.local/scratch/reading-runs/manual --json + --position entailment --target HEAD --json ``` -With `--out`, the assembled input (`bundle.json`) and the manifest -(`manifest.json`) are written into that directory as two separate files. -Without it, they land in the local-tier run directory -`.abcd/.work.local/scratch/reading-runs//`. With `--dry-run` and no -`--out`, nothing is written anywhere and the result is rendered only. +The assembled input (`bundle.json`) and the manifest (`manifest.json`) are +written as two separate files into the local-tier run directory +`.abcd/.work.local/scratch/reading-runs//`, which is where the run is +parked: `reading ingest` resolves a run's manifest there by its run id, and +nowhere else. With `--dry-run` and no `--out`, nothing is written anywhere and +the result is rendered only. + +`--out ` writes the two files into that directory instead, **as an +inspection copy that can never be ingested**: No ingest looks for a manifest +outside the run directory, so a reading commissioned on it has nowhere to +land. The result says so at assembly time, as `"ingestable": false` in the JSON +and an `ingest:` line in the text render; relay it before the reading is +commissioned. To commission a reading, assemble without `--out`. An output directory the include table can reach is refused, and the refusal names the item that would be admitted. Writing a run where the table reaches it diff --git a/commands/site.md b/commands/site.md index f391274c1..04d2336a9 100644 --- a/commands/site.md +++ b/commands/site.md @@ -134,8 +134,10 @@ the closed set (`landing`, `explorer`, `record_pages`, `graph`, `timeline`, `glossary`, `status`) off; it cannot add one. Report each stage's outcome, then the remaining steps in order. Never paste a -credential into the conversation: the store is the file `~/.abcd/credentials.json` -(mode `0600`), which the user writes themselves. +credential into the conversation: the user stores it themselves with `abcd ahoy +credential hosting.cloudflare` (see `/abcd:ahoy`), which explains where it can +live and verifies it before keeping it; `host.credential` names the credential, +never its value. ## The gate over what was rendered diff --git a/docs/reference/cli/README.md b/docs/reference/cli/README.md index a00bc0f8d..8665e4471 100644 --- a/docs/reference/cli/README.md +++ b/docs/reference/cli/README.md @@ -17,5 +17,9 @@ To refresh the page after changing a command, run: go generate ./internal/surface/cli ``` -For interactive help, the binary also documents itself: `abcd --help` and -`abcd --help` (e.g. `abcd disembark --help`). +For interactive help, the binary also documents itself: `abcd --help` +(e.g. `abcd disembark --help`) documents any verb, and `abcd --help` lists the +verbs a person types. The verbs agents and hosts call, such as `abcd peers`, +`abcd history` and `abcd reading`, are left off that list to keep it short; +`abcd --help --agent` lists them as well, each naming the command page to read +next. Every one of them is in [`commands.md`](commands.md) either way. diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 198b320c1..bddc5d85e 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -21,9 +21,11 @@ Agent-based configuration for development. Bare `abcd` renders the read-only status board — what can I do. A single positional matching a record id (`iss-N`, `itd-N`, `spc-N`, `adr-N`, `adm-N`, -`srp-N`, `rfm-N`) instead reports what that record is, where it lives, and -the next move for its lifecycle state — what is this. Both forms are strictly -read-only; any other positional is refused as an unknown command. +`srp-N` or `rfm-N`) instead reports what that record is, where it lives, and +the next move for its lifecycle state — what is this. N is either a short +ordinal from before ids were minted or the sixteen-digit stamp minted since; +both resolve. The bare and the id form are strictly read-only; any other +positional is refused as an unknown command. **Flags:** @@ -51,7 +53,7 @@ Detect abcd's install state and list its gaps, or report one mode a flag names: #### `abcd ahoy connect` -Verify a model provider with one call, then configure it: Writes its block and its key under ~/.abcd/; refuses a key typed at a terminal. +Verify a model provider with one call, then configure it: Writes its block under ~/.abcd/ and its key to the home chosen; refuses a key typed at a terminal. **Usage:** `abcd ahoy connect [flags]` @@ -59,7 +61,10 @@ Verify a model provider with one call, then configure it: Writes its block and i ``` --base-url string the provider's OpenAI-compatible base URL: https, or http to a server on this machine - --home string where the key lives: abcd (read from stdin into the owner-only ~/.abcd/credentials.json) | none (a server that takes no key); external and keychain arrive with the credential store + --env string for --home external: the environment variable that holds the value + --field string for --home external: the dotted field of --file that holds the value (auth.token) + --file string for --home external: a tool's JSON configuration file under the home directory, written from ~/ + --home string where the key lives: external (--env, or --file and --field) | abcd (read from stdin into the owner-only ~/.abcd/credentials.json) | keychain (read from stdin into the platform keychain) | none (a server that takes no key) --key string the credential's name (default: the provider's name) --model stringArray a model the provider may serve, repeated for each (the first allowlist; the verification call asks for the first) ``` @@ -70,6 +75,21 @@ Verify a model provider with one call, then configure it: Writes its block and i abcd ahoy connect local --base-url http://127.0.0.1:8080/v1 --model example-model --home none ``` +#### `abcd ahoy credential` + +List the credentials abcd reads, explain one, or verify and store it: Writes the chosen home only with --home; refuses a value the adapter's call fails. + +**Usage:** `abcd ahoy credential [] [flags]` + +**Flags:** + +``` + --env string for --home external: the environment variable that holds the value + --field string for --home external: the dotted field of --file that holds the value (auth.token) + --file string for --home external: a tool's JSON configuration file under the home directory, written from ~/ + --home string where the credential lives: external (--env, or --file and --field) | abcd (read from stdin into the owner-only ~/.abcd/credentials.json) | keychain (read from stdin into the platform keychain) +``` + #### `abcd ahoy doctor` Report every install gap, user-scope state included: Writes nothing; refuses any argument. @@ -802,7 +822,7 @@ Name the URLs directly, or pass --receipt with a receipt file. Both write the sa ``` --config string path to docs-lint.json (default: /.abcd/docs-lint.json) - --receipt string path to a receipt file listing the confirmed citations (the format the generated checklist page emits) + --receipt string path to a JSON receipt listing the confirmed citations: schema_version 1 and a confirmed list, each entry a url with an optional final_url and verified_on (YYYY-MM-DD) --root string repo root (default: current working directory) ``` @@ -2311,8 +2331,9 @@ and its hash, so a run is reproducible from the commit it names. ``` --dry-run write nothing; with --out the two artefacts still land in that directory - --out string an empty or absent directory the assembled input and the manifest are written to - (default: the local-tier run directory) + --out string an empty or absent directory the assembled input and the manifest are written to, + for inspection: reading ingest finds a run only in the local-tier run directory, + so a run written here cannot be ingested (default: the local-tier run directory) --position string the reading position: widening, entailment, comparative, detection (comparative derives its candidate set from the record: the one committed widening run at the target whose items carry no disposition and no diff --git a/docs/reference/terminology.md b/docs/reference/terminology.md index 5053d8d9b..d383469bd 100644 --- a/docs/reference/terminology.md +++ b/docs/reference/terminology.md @@ -55,7 +55,7 @@ deep-linked. | **Grounding** | Supplying a model use-case-specific information at inference time so responses connect to accurate, verifiable sources.[^ground] | **ADAPTS** — abcd's version is *cite-or-be-dropped* (adr-35): a claim in a synthesised artefact carries its source citation or does not ship, and whatever could not be grounded is reported loudly as coverage blanks rather than papered over. | | **Guardrails** | Programmable controls on an application's inputs and outputs — validation, filtering, dialogue constraints — enforced outside the model.[^guard] | **ADAPTS** — abcd's equivalents are deterministic gates: lint families, receipt gates, and pre-commit guards all fail closed. The pre-tool-use shell-hazard guard (`abcd guard`) deliberately does not — it is *fail-open-loud*, a mistake filter rather than a security boundary (adr-42): a guard that cannot answer warns unmissably and lets the command run, because a broken guard must never brick a session. Two principles sharpen the industry sense: *enforcement claims are facts* (a claimed gate must exist) and *guards prove themselves* (a guard ships with the test that shows it fires). | | **Human-in-the-loop** | Human-oversight capability for intervention in an AI system's decision cycle; EU law's term of art is "human oversight".[^hitl] | **ADAPTS** — *verifier selects, gates decide*: a model verdict is only ever a proposal; admission is decided by deterministic gates plus the human's adoption. Planning an intent is defined as the human's sign-off act, machine-checked before implementation may start. | -| **Memory** | An agent's capability to retain and retrieve information beyond one context window: short-term session context plus long-term knowledge persisted across sessions.[^mem] | **USES** — the per-project memory substrate (`.abcd/memory/`) with its own verb: raw sources are curated into a compounding knowledge artefact by a single writing curator, and retrieval is recall-matched and budget-bracketed rather than context-flooding. | +| **Memory** | An agent's capability to retain and retrieve information beyond one context window: short-term session context plus long-term knowledge persisted across sessions.[^mem] | **USES** — the per-project memory substrate (`.abcd/memory/`) with its own verb: raw sources are curated into a compounding knowledge artefact by a single writing curator, and retrieval is on demand rather than context-flooding: `abcd memory ask` ranks the pages by the words a question shares with each page's classes, domain and summary, and returns the top five with their citations. | | **Model Context Protocol (MCP)** | Open JSON-RPC protocol standardising how LLM applications connect to tools and data through hosts, clients, and servers (specification revision 2025-11-25).[^mcp] | **USES** — MCP is one of the four recorded oracle-adapter shapes, and an MCP front door over the transport-agnostic core is a recorded later step (adr-24, adr-25). No MCP server ships today; the architecture is built so that adding one changes no core code. | | **Multi-agent systems** | Multiple interacting autonomous agents coordinating to solve tasks beyond any individual agent's capability — three decades of distributed-AI literature that current usage narrows.[^mas] | **ADAPTS** — abcd's multi-agent shape exists for *independence*, not parallelism: the evaluator-outside-the-loop principle requires that whoever judges a change is not whoever proposed it. Cross-agent work coordination is a draft design (itd-33). | | **Observability** | A common telemetry schema for GenAI systems — spans, metrics, and events for inference, agents, and tools; the semantic conventions are pre-stable (pinned here at v1.41.1, the last tagged release carrying them).[^otel] | **WATCHING** — the record has a native, local, redacted transcript corpus (adr-29) and mid-run telemetry is out of the record (dropped with the superseded itd-29 until someone wants it), and a learned per-request model router is not adopted (itd-17, superseded): the model per role is a configured tier, and closed-option judgements are routed to a decision adapter measured in a lab first (itd-2609221009495079, planned). No tracing or metrics stack is adopted. | diff --git a/internal/README.md b/internal/README.md index 7d5190e5d..814c36cf7 100644 --- a/internal/README.md +++ b/internal/README.md @@ -18,6 +18,15 @@ plugin surface, and a future MCP server share one engine. entered the terminal folders since the anchor tag. It owns the enum so the lints that GATE the judgement (`core/lint`), the ledger reader that VALIDATES it (`core/capture`), and the derivation that CONSUMES it cannot drift apart. +- **`core/frontmatter/`** — the one record-frontmatter reader, a line scanner + rather than a YAML parser: the `---` delimiter rule, the top-level fields of + the leading block (first key wins, a trailing comment stripped), the duplicate + keys, and the scalar decoding every reader shares: `Unquote` is the one + scalar decoder and `QuoteScalar` the encoder it mirrors, and `EmptinessOf` is + the one blank-value judgement. A leaf importing only the standard library, because every + record family's reader and writer (`core/capture`, `core/intent`, `core/spec`, + `core/lint` and more) imports it, so a gate and the writer it judges read a + field the same way. - **`core/issueschema/`** — the issue record's required frontmatter properties, and nothing else. It is a leaf for the same reason `core/changelog` owns the impact enum: the ledger reader (`core/capture`) and the lint that gates the diff --git a/internal/adapter/hosting/cloudflare/cloudflare.go b/internal/adapter/hosting/cloudflare/cloudflare.go index 57f43e7e9..9812fc561 100644 --- a/internal/adapter/hosting/cloudflare/cloudflare.go +++ b/internal/adapter/hosting/cloudflare/cloudflare.go @@ -269,6 +269,13 @@ func (c *client) scrub(s string) string { return strings.ReplaceAll(s, c.token, "[credential]") } +// Verify proves the token reaches exactly one account, by the same read the +// host stage begins with. +func (c *client) Verify(ctx context.Context) error { + _, err := c.accountID(ctx) + return err +} + // accountID resolves the one account the token reaches, once. func (c *client) accountID(ctx context.Context) (string, error) { if c.account != "" { diff --git a/internal/adapter/hosting/hosting.go b/internal/adapter/hosting/hosting.go index 2cb4ddb61..920ff3320 100644 --- a/internal/adapter/hosting/hosting.go +++ b/internal/adapter/hosting/hosting.go @@ -43,6 +43,10 @@ type State struct { // Provider is the live half of the seam: one connected account. type Provider interface { + // Verify is the provider's own call proving the credential reaches an + // account it can act in. It writes nothing; the credential walkthrough + // makes it before the credential is stored. + Verify(ctx context.Context) error // Inspect reads what the host holds for s. It writes nothing. Inspect(ctx context.Context, s Site) (State, error) // Create creates the host for s. diff --git a/internal/core/ahoy/home_link_writers_test.go b/internal/core/ahoy/home_link_writers_test.go index 32f64b3d7..68d2e6305 100644 --- a/internal/core/ahoy/home_link_writers_test.go +++ b/internal/core/ahoy/home_link_writers_test.go @@ -7,6 +7,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/intentdriven/abcd/internal/fsutil" ) // TestMachineWritesRefuseASymlinkedAbcdHome: the model-tier routing table and @@ -42,3 +44,92 @@ func TestMachineWritesRefuseASymlinkedAbcdHome(t *testing.T) { }) } } + +// TestMachineWritesRefuseAnAbcdHomeSwappedForALink is iss-2609281310017733: +// ~/.abcd is a real directory when each writer judges it and a symlink into a +// dotfiles checkout by the time it writes (a same-uid race, staged through +// the vetting hook). A check by path followed by a create by path writes +// through the link; each writer here reaches the file through the descriptor +// of the directory that was judged, so the checkout is left as it was. +func TestMachineWritesRefuseAnAbcdHomeSwappedForALink(t *testing.T) { + writers := map[string]func(a *applyCtx){ + "path entry": func(a *applyCtx) { _ = writePathEntry("/example/bin/abcd", strings.Repeat("0", 64), "") }, + "oracle routing": func(a *applyCtx) { a.writeMachineRouting([]byte("{}\n")) }, + "history registry": func(a *applyCtx) { _, _ = bootstrapHistory() }, + "history lock": func(a *applyCtx) { _ = withHistoryLock(func() error { return nil }) }, + } + for name, write := range writers { + t.Run(name, func(t *testing.T) { + home, _ := setupHermetic(t) + abcd := filepath.Join(home, ".abcd") + dotfiles := filepath.Join(home, "dotfiles", "abcd") + for _, dir := range []string{abcd, dotfiles} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + swapped := false + t.Cleanup(fsutil.SwapHomeScopeVettedForTest(func(dir string) { + if swapped || dir != abcd { + return + } + swapped = true + if err := os.Rename(abcd, filepath.Join(home, "moved-aside")); err != nil { + t.Fatalf("swap: %v", err) + } + if err := os.Symlink(dotfiles, abcd); err != nil { + t.Fatalf("swap: %v", err) + } + })) + write(&applyCtx{}) + if !swapped { + t.Fatalf("the %s writer never judged ~/.abcd, so the race was not staged", name) + } + if entries, _ := os.ReadDir(dotfiles); len(entries) != 0 { + t.Fatalf("the %s write went through the swapped link: the checkout holds %q", name, entries[0].Name()) + } + }) + } +} + +// TestPathEntryRemovalRemovesNothingBehindAnAbcdHomeSwappedForALink is the +// remove half of iss-2609281310017733: ~/.abcd is a real directory when the +// provenance record's removal judges it and a symlink into a dotfiles checkout +// by the time it removes (staged through the vetting hook). A remove by path +// after a check by path unlinks the checkout's copy of path-entry; the remove +// through the descriptor of the directory that was judged leaves it alone. +func TestPathEntryRemovalRemovesNothingBehindAnAbcdHomeSwappedForALink(t *testing.T) { + home, _ := setupHermetic(t) + abcd := filepath.Join(home, ".abcd") + dotfiles := filepath.Join(home, "dotfiles", "abcd") + for _, dir := range []string{abcd, dotfiles} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + for _, dir := range []string{abcd, dotfiles} { + if err := os.WriteFile(filepath.Join(dir, "path-entry"), []byte("path=/example/bin/abcd\n"), 0o644); err != nil { + t.Fatal(err) + } + } + swapped := false + t.Cleanup(fsutil.SwapHomeScopeVettedForTest(func(dir string) { + if swapped || dir != abcd { + return + } + swapped = true + if err := os.Rename(abcd, filepath.Join(home, "moved-aside")); err != nil { + t.Fatalf("swap: %v", err) + } + if err := os.Symlink(dotfiles, abcd); err != nil { + t.Fatalf("swap: %v", err) + } + })) + removePathEntry() + if !swapped { + t.Fatal("the removal never judged ~/.abcd, so the race was not staged") + } + if _, err := os.Lstat(filepath.Join(dotfiles, "path-entry")); err != nil { + t.Fatalf("the removal went through the swapped link: the checkout's path-entry is gone (%v)", err) + } +} diff --git a/internal/core/ahoy/oracle_routing.go b/internal/core/ahoy/oracle_routing.go index e509b2347..db241eb94 100644 --- a/internal/core/ahoy/oracle_routing.go +++ b/internal/core/ahoy/oracle_routing.go @@ -134,12 +134,25 @@ func (a *applyCtx) writeMachineRouting(body []byte) { a.refuse("the model-tier routing was not written: ~/.abcd/oracle-routing.json appeared while the question was open, and it is left as it is.") return } - if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { + // ~/.abcd is created, judged and opened relative to home's descriptor and + // the table is written through it, so a link swapped in after the check + // above is refused rather than written through (iss-2609281310017733). + dir, err := fsutil.EnsureHomeScope(userHome(), ".abcd", 0o700) + if errors.Is(err, fsutil.ErrHomeScopeSymlinked) { + a.refuse("the model-tier routing was not written: " + err.Error() + ".") + return + } + if err != nil { a.refuse("could not create ~/.abcd for the model-tier routing (" + errText(err) + "); nothing was written.") return } + defer dir.Close() + if _, err := dir.Lstat(layered.OracleRouting.MachineRel); !errors.Is(err, os.ErrNotExist) { + a.refuse("the model-tier routing was not written: ~/.abcd/oracle-routing.json appeared while the question was open, and it is left as it is.") + return + } // 0600, never wider: the resolver refuses a machine file others can write. - if err := fsutil.WriteFileAtomic(p, body, 0o600); err != nil { + if err := fsutil.WriteFileAtomicInRoot(dir, layered.OracleRouting.MachineRel, body, 0o600); err != nil { a.refuse("could not write ~/.abcd/oracle-routing.json (" + errText(err) + "); the routing was not accepted.") return } diff --git a/internal/core/ahoy/owned_copy.go b/internal/core/ahoy/owned_copy.go index f7414db34..47b3014ef 100644 --- a/internal/core/ahoy/owned_copy.go +++ b/internal/core/ahoy/owned_copy.go @@ -5,6 +5,7 @@ import ( "encoding/hex" "errors" "os" + "path" "path/filepath" "runtime" "strings" @@ -209,22 +210,38 @@ func writePathEntry(target, shaHex, pluginRoot string) error { if err != nil { return err } - path := filepath.Join(home, filepath.FromSlash(pathEntryRel)) body := "path=" + target + "\nbinary_sha256=" + shaHex + "\n" if pluginRoot != "" { body += "plugin_root=" + pluginRoot + "\n" } - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + // ~/.abcd is created, judged and opened relative to home's descriptor and + // the record is written through it, so a link swapped in after + // homeScopeErr's check is refused rather than written through + // (iss-2609281310017733). + dir, err := fsutil.EnsureHomeScope(home, path.Dir(pathEntryRel), 0o755) + if err != nil { return err } - return fsutil.WriteFileAtomic(path, []byte(body), 0o644) + defer dir.Close() + return fsutil.WriteFileAtomicInRoot(dir, path.Base(pathEntryRel), []byte(body), 0o644) } -// removePathEntry drops the provenance record; absent is fine. +// removePathEntry drops the provenance record; absent is fine. ~/.abcd is +// judged and opened relative to home's descriptor and the record is removed +// through it, so a link swapped in after homeScopeErr's check removes nothing +// behind the link (iss-2609281310017733). A ~/.abcd that is a symlink, or is +// not there, leaves nothing to remove. func removePathEntry() { - if path := userPathEntryPath(); path != "" { - _ = os.Remove(path) + home, err := homeScopeErr() + if err != nil { + return + } + dir, err := fsutil.OpenHomeScope(home, path.Dir(pathEntryRel)) + if err != nil { + return } + defer dir.Close() + _ = dir.Remove(path.Base(pathEntryRel)) } // pathEntryNames reports whether the provenance record names target. It is the diff --git a/internal/core/ahoy/statusline_apply.go b/internal/core/ahoy/statusline_apply.go index beb8efa29..5bcfb5d48 100644 --- a/internal/core/ahoy/statusline_apply.go +++ b/internal/core/ahoy/statusline_apply.go @@ -21,6 +21,7 @@ import ( "encoding/json" "errors" "os" + "path" "path/filepath" "strconv" "strings" @@ -177,14 +178,28 @@ func (a *applyCtx) wireStatusLine(hs harnessSettings, entry string, switches map a.refuse("refused to wire the status line: " + displayPath(hs.path) + " could not be re-encoded (" + errText(err) + "); nothing was written.") return } + // ~/.abcd is created, judged and opened relative to home's descriptor and + // the setting is written (and, on a failed harness write, removed) through + // it, so a link swapped in after the check above is refused rather than + // written through (iss-2609281310017733). + settingLeaf := path.Base(statusline.SettingsRelPath) + var settingDir *os.Root if settingBytes != nil { var werr error - if created { - // 0600, and never wider: the setting's reader refuses a file others - // can write, so a default 0644 would be a setting that is never read. - werr = fsutil.WriteFileAtomic(settingPath, settingBytes, 0o600) - } else { - werr = fsutil.WriteFileAtomicPreserveMode(settingPath, settingBytes) + settingDir, werr = fsutil.EnsureHomeScope(userHome(), path.Dir(statusline.SettingsRelPath), 0o755) + if errors.Is(werr, fsutil.ErrHomeScopeSymlinked) { + a.refuse("refused to wire the status line: " + werr.Error() + "; nothing was written.") + return + } + if werr == nil { + defer settingDir.Close() + if created { + // 0600, and never wider: the setting's reader refuses a file others + // can write, so a default 0644 would be a setting that is never read. + werr = fsutil.WriteFileAtomicInRoot(settingDir, settingLeaf, settingBytes, 0o600) + } else { + werr = fsutil.WriteFileAtomicPreserveModeInRoot(settingDir, settingLeaf, settingBytes) + } } if werr != nil { a.refuse("could not write " + statusline.SettingsDisplay + " (" + errText(werr) + "); the status line was not wired.") @@ -192,8 +207,8 @@ func (a *applyCtx) wireStatusLine(hs harnessSettings, entry string, switches map } } if err := fsutil.WriteFileAtomicPreserveMode(hs.path, harnessBytes); err != nil { - if created { - _ = os.Remove(settingPath) + if created && settingDir != nil { + _ = settingDir.Remove(settingLeaf) } a.refuse("could not write " + displayPath(hs.path) + " (" + errText(err) + "); the status line was not wired and " + statusline.SettingsDisplay + " was not left behind.") diff --git a/internal/core/ahoy/store.go b/internal/core/ahoy/store.go index 3631d05a9..84aaa2e28 100644 --- a/internal/core/ahoy/store.go +++ b/internal/core/ahoy/store.go @@ -2,6 +2,8 @@ package ahoy import ( "bytes" + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "os" @@ -676,29 +678,27 @@ func historyRoot() (string, error) { return filepath.Join(home, filepath.FromSlash(historyRelPath)), nil } -// ensureHistoryRoot is historyRoot for a writer: it creates ~/.abcd/history one -// real directory at a time and proves every level, so a link planted after -// historyRoot's check is refused rather than followed (os.MkdirAll would follow -// it). The walk starts at home with its symlinks resolved, because home itself -// reached through a link (/home -> /usr/home) is the machine's layout and is -// never judged; ~/.abcd and ~/.abcd/history are. -func ensureHistoryRoot() (string, error) { - root, err := historyRoot() - if err != nil { - return "", err +// historyDir opens ~/.abcd/history as an *os.Root through +// fsutil.OpenHomeScope, after historyRoot's check, so the registry's files are +// read and written relative to the descriptor of the directory that was judged +// and never through a link swapped in after the judgement +// (iss-2609281310017733). create makes each missing level first +// (fsutil.EnsureHomeScope), one real directory at a time, where os.MkdirAll +// would follow a link. home itself reached through a link (/home -> /usr/home) +// is the machine's layout and is opened, never judged; ~/.abcd and +// ~/.abcd/history are. An absent registry without create is os.ErrNotExist. +func historyDir(create bool) (*os.Root, error) { + if _, err := historyRoot(); err != nil { + return nil, err } home, err := os.UserHomeDir() if err != nil { - return "", err - } - base, err := filepath.EvalSymlinks(home) - if err != nil { - return "", err + return nil, err } - if err := fsutil.EnsureRealDirAll(base, historyRelPath, 0o755); err != nil { - return "", err + if create { + return fsutil.EnsureHomeScope(home, historyRelPath, 0o755) } - return root, nil + return fsutil.OpenHomeScope(home, historyRelPath) } // historyIndex is the ~/.abcd/history/index.json registry. @@ -748,11 +748,15 @@ func loadHistoryIndex() (*historyIndex, error) { // included. Only the detector wants this: everything else goes through // loadHistoryIndex, which scrubs. func readHistoryIndexFile() (*historyIndex, error) { - root, err := historyRoot() + dir, err := historyDir(false) if err != nil { + if os.IsNotExist(err) { + return nil, nil + } return nil, err } - data, err := fsutil.ReadGuarded(filepath.Join(root, "index.json"), maxAhoyFileBytes) + defer dir.Close() + data, err := fsutil.ReadGuardedInRoot(dir, "index.json", maxAhoyFileBytes) if err != nil { if os.IsNotExist(err) { return nil, nil @@ -882,12 +886,12 @@ var beforeHistoryIndexCreateHook func() // any prompting before acquiring it and re-check the answer-relevant state inside // fn after re-loading. func withHistoryLock(fn func() error) error { - root, err := ensureHistoryRoot() + dir, err := historyDir(true) if err != nil { return err } - lockPath := filepath.Join(root, historyLockFilename) - return fsutil.WithFileLock(lockPath, historyLockTimeout, func() error { + defer dir.Close() + return fsutil.WithFileLockIn(dir, historyLockFilename, historyLockTimeout, func() error { if afterHistoryReloadHook != nil { afterHistoryReloadHook() } @@ -905,13 +909,13 @@ func withHistoryLock(fn func() error) error { // sees either no file yet or the finished index — never a 0-byte one that would // make a concurrent loadHistoryIndex parse-fail and drop its own registration. func bootstrapHistory() (bool, error) { - root, err := ensureHistoryRoot() + dir, err := historyDir(true) if err != nil { return false, err } - path := filepath.Join(root, "index.json") + defer dir.Close() // Fast path: already seeded (the common idempotent re-run). - if _, err := os.Stat(path); err == nil { + if _, err := dir.Stat("index.json"); err == nil { return false, nil } idx := historyIndex{Schema: 1, Description: historyIndexDescription, Repos: []historyRepo{}} @@ -923,12 +927,11 @@ func bootstrapHistory() (bool, error) { // Write a complete temp file first, then link it into place as the atomic, // single-winner publish. The temp is always cleaned up. - tmp, err := os.CreateTemp(root, ".index-*.tmp") + tmp, tmpName, err := createHistoryTemp(dir) if err != nil { return false, err } - tmpName := tmp.Name() - defer os.Remove(tmpName) + defer dir.Remove(tmpName) if _, err := tmp.Write(data); err != nil { tmp.Close() return false, err @@ -948,7 +951,7 @@ func bootstrapHistory() (bool, error) { if beforeHistoryIndexCreateHook != nil { beforeHistoryIndexCreateHook() } - if err := os.Link(tmpName, path); err != nil { + if err := dir.Link(tmpName, "index.json"); err != nil { if os.IsExist(err) { return false, nil // already seeded (or won by a concurrent run) } @@ -957,14 +960,40 @@ func bootstrapHistory() (bool, error) { return true, nil } +// createHistoryTemp creates bootstrapHistory's temp file exclusively inside +// dir under an unpredictable name (the ".index-*.tmp" shape os.CreateTemp +// gave it), returning the file and its name. +func createHistoryTemp(dir *os.Root) (*os.File, string, error) { + var buf [8]byte + for range 100 { + if _, err := rand.Read(buf[:]); err != nil { + return nil, "", err + } + name := ".index-" + hex.EncodeToString(buf[:]) + ".tmp" + f, err := dir.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600) + if err == nil { + return f, name, nil + } + if !errors.Is(err, os.ErrExist) { + return nil, "", err + } + } + return nil, "", errors.New("could not create a temp file in the history registry") +} + // writeHistoryIndex persists idx. It must be called from inside withHistoryLock // (registerRepo) so a re-loaded index is not clobbered by a concurrent writer. func writeHistoryIndex(idx *historyIndex) error { - root, err := historyRoot() + dir, err := historyDir(false) + if err != nil { + return err + } + defer dir.Close() + data, err := marshalJSON(*idx) if err != nil { return err } - return writeJSON(filepath.Join(root, "index.json"), *idx) + return fsutil.WriteFileAtomicPreserveModeInRoot(dir, "index.json", data) } // --------------------------------------------------------------------------- diff --git a/internal/core/credential/credential.go b/internal/core/credential/credential.go index e38d407f3..350605ac6 100644 --- a/internal/core/credential/credential.go +++ b/internal/core/credential/credential.go @@ -1,12 +1,10 @@ -// Package credential is the one reader every adapter resolves an external -// credential through, by NAME. -// -// This is the interim source itd-2609061543533170 ruled for `abcd site setup` -// (its `## Decisions`, 2026-09-25): the credential store proper, with its three -// homes and its walkthrough, is itd-2609221017023290, which is planned and not -// built. Until it lands, a credential is read from one machine-scoped file, -// ~/.abcd/credentials.json, a JSON object mapping a credential name to its -// value. The successor replaces the source behind Source; no reader changes. +// Package credential is the credential store (itd-2609221017023290, +// adr-2609221017021499): the one reader every adapter resolves an external +// credential through, by NAME (Store, store.go), the one write (Set), and the +// walkthrough that chooses a credential's home (Walk, walk.go). This file is +// the abcd home: one machine-scoped file, ~/.abcd/credentials.json, a JSON +// object mapping a credential name to its value. The keychain and external +// homes are keychain.go and external.go. // // The file is refused, loudly and never treated as absent, unless it is a // regular file (not a symlink), owned by the caller, and readable and writable @@ -17,9 +15,8 @@ // // The value never leaves Resolve except as its return: no error formats it, // nothing logs it, and nothing here writes to the repository. The one write is -// SetMachine, into this same file, for the setup of the OpenAI-compatible API -// adapter (itd-2609081951381895). A malformed file is refused without echoing -// a byte of it. +// SetMachine, into this same file, which Set calls for the abcd home. A +// malformed file is refused without echoing a byte of it. package credential import ( @@ -27,7 +24,6 @@ import ( "errors" "fmt" "os" - "path/filepath" "regexp" "strings" "time" @@ -38,7 +34,7 @@ import ( "github.com/intentdriven/abcd/internal/termsafe" ) -// StoreFileName is the interim store's file under ~/.abcd/. +// StoreFileName is the abcd home's file under ~/.abcd/. const StoreFileName = "credentials.json" // maxStoreBytes bounds the store read. @@ -62,22 +58,15 @@ var nameRe = regexp.MustCompile(`^[a-z0-9][a-z0-9._-]{0,63}$`) // it is read rather than at the first call. func ValidName(name string) bool { return nameRe.MatchString(name) } -// Machine is the interim machine-scoped source rooted at home (the caller's -// home directory). An empty home resolves every name to ErrNotSet. +// Machine is the abcd home alone, rooted at home (the caller's home +// directory). An empty home resolves every name to ErrNotSet. Every reader +// outside this package resolves through Store, which reads this home among +// the three; a test refuses any other. func Machine(home string) Source { return machine{home: home} } -// UserMachine is Machine at the caller's home directory. -func UserMachine() Source { - home, err := os.UserHomeDir() - if err != nil { - home = "" - } - return Machine(home) -} - type machine struct{ home string } -// StorePath is where the interim store lives, displayed with ~ so no +// StorePath is where the abcd home lives, displayed with ~ so no // developer-identity path reaches output. const StorePath = "~/.abcd/" + StoreFileName @@ -105,39 +94,36 @@ func (m machine) Resolve(name string) (string, error) { // readStore reads the store at home under every refusal the package doc // names. An absent store is an empty map and no error. +// +// Every guard is judged by fsutil.ReadHomeDeclarationDenying on the store it +// reads, never on a path first: absence on the Lstat that decides it (a +// symlinked ~/.abcd holding no store is no store), a store behind a symlinked +// ~/.abcd — which sits wherever the link points, a dotfiles checkout +// typically, and is refused as the rules loader refuses a rules.json there — +// on the descriptor walk of ~/.abcd, and the leaf's type, owner and mode on +// the opened file's own fstat. A mode judged by path would vouch for a file +// other than the one read: a store swapped for a group-readable file after +// that check would be read once (iss-2609281310017733). func readStore(home string) (map[string]string, error) { - p := filepath.Join(home, ".abcd", StoreFileName) - fi, err := os.Lstat(p) - if err != nil { - if os.IsNotExist(err) { - return map[string]string{}, nil - } - return nil, fmt.Errorf("credential: %s could not be examined, so it is not read", StorePath) - } - // A store that is there behind a symlinked ~/.abcd sits wherever the link - // points — a dotfiles checkout, typically — and is refused as the rules - // loader refuses a rules.json there; a symlinked ~/.abcd holding no store - // is no store (the Lstat above). - if err := fsutil.HomeScopeLink(home, storeRel); err != nil { - return nil, fmt.Errorf("credential: %s is not read: %v", StorePath, err) - } - if !fi.Mode().IsRegular() { - return nil, fmt.Errorf("credential: %s is not a regular file (a symlink is never followed), so it is not read", StorePath) - } - if fi.Mode().Perm()&0o077 != 0 { - return nil, fmt.Errorf("credential: %s can be read or written by group or other (mode %04o), so it is not read; `chmod 0600 %s`", StorePath, fi.Mode().Perm(), StorePath) - } - // ReadDeclaration re-checks the leaf on its own descriptor and refuses a - // file this uid does not own. - raw, refusal, err := fsutil.ReadHomeDeclaration(home, storeRel, maxStoreBytes) + raw, refusal, err := fsutil.ReadHomeDeclarationDenying(home, storeRel, maxStoreBytes, 0o077) + var mode *fsutil.DeclarationModeError switch { + case refusal == fsutil.DeclarationOK: case refusal == fsutil.DeclarationAbsent && errors.Is(err, os.ErrNotExist): return map[string]string{}, nil + case refusal == fsutil.DeclarationAbsent: + return nil, fmt.Errorf("credential: %s could not be examined, so it is not read", StorePath) case refusal == fsutil.DeclarationBehindSymlink: return nil, fmt.Errorf("credential: %s is not read: %v", StorePath, err) + case refusal == fsutil.DeclarationNotRegular: + return nil, fmt.Errorf("credential: %s is not a regular file (a symlink is never followed), so it is not read", StorePath) + case refusal == fsutil.DeclarationExposed && errors.As(err, &mode): + return nil, fmt.Errorf("credential: %s can be read or written by group or other (mode %04o), so it is not read; `chmod 0600 %s`", StorePath, uint32(mode.Perm), StorePath) + case refusal == fsutil.DeclarationWritableByOthers: + return nil, fmt.Errorf("credential: %s can be written by group or other, so it is not read; `chmod 0600 %s`", StorePath, StorePath) case refusal == fsutil.DeclarationForeignOwner: return nil, fmt.Errorf("credential: %s is not owned by you, so it is not read", StorePath) - case err != nil: + default: return nil, fmt.Errorf("credential: %s could not be read safely (mode 0600, owned by you, a regular file), so it is not read", StorePath) } // A repeated key, or a case twin encoding/json binds to the same entry, is @@ -159,10 +145,9 @@ func readStore(home string) (map[string]string, error) { // MaxValueBytes bounds one credential's value. const MaxValueBytes = 4096 -// SetMachine writes value under name in the interim store at home -// (~/.abcd/credentials.json): the one write this package makes, for the one -// home it reads (itd-2609081951381895's setup; the credential store, -// itd-2609221017023290, brings the other homes and replaces this backing). +// SetMachine writes value under name in the abcd home at home +// (~/.abcd/credentials.json): the abcd home's write, which Set makes for it +// and no reader outside this package calls. // // It refuses, before writing anything and without echoing either value: a // name that is not plain; a value that is empty, longer than MaxValueBytes, @@ -170,12 +155,12 @@ const MaxValueBytes = 4096 // character; a store Resolve would refuse (a symlink, group- or other- // readable, not owned by the caller, malformed), so a write never launders an // unsafe file; a ~/.abcd that is a symlink, because the secret would land -// wherever the link points (fsutil.HomeScopeLink); and a name already holding a +// wherever the link points (fsutil.EnsureHomeScope); and a name already holding a // different value, because a stored secret is never replaced by a second one // unasked. The same value already // stored is no change (changed is false). The file is written atomically at // mode 0600, and ~/.abcd is created owner-only when it is absent. The read, -// the change and the write hold the store's lock (fsutil.WithFileLock, beside +// the change and the write hold the store's lock (fsutil.WithFileLockIn, beside // the store), so concurrent writers never lose each other's entries. func SetMachine(home, name, value string) (changed bool, err error) { if !nameRe.MatchString(name) { @@ -191,17 +176,22 @@ func SetMachine(home, name, value string) (changed bool, err error) { // that repository, and the store's own read refuses a file behind the link // (iss-2609260958587561). Refused before anything is created, the lock // included. - if err := fsutil.HomeScopeLink(home, storeRel); err != nil { + // ~/.abcd is created, judged and opened in one walk relative to the + // descriptor of home (fsutil.EnsureHomeScope), and the lock and the store + // are reached through that descriptor, so a link swapped in after the + // judgement is refused rather than written through (iss-2609281310017733). + dir, err := fsutil.EnsureHomeScope(home, ".abcd", 0o700) + if errors.Is(err, fsutil.ErrHomeScopeSymlinked) { return false, fmt.Errorf("credential: nothing was written to %s: %v", StorePath, err) } - dir := filepath.Join(home, ".abcd") - if err := os.MkdirAll(dir, 0o700); err != nil { + if err != nil { return false, fmt.Errorf("credential: ~/.abcd could not be created, so nothing was written") } + defer dir.Close() // The store is read, changed and renamed into place, so a second writer // between the read and the rename would lose this entry or its own; the // write holds the store's lock across all three. - err = fsutil.WithFileLock(filepath.Join(dir, storeLockFileName), storeLockTimeout, func() error { + err = fsutil.WithFileLockIn(dir, storeLockFileName, storeLockTimeout, func() error { var werr error changed, werr = setLocked(home, dir, name, value) return werr @@ -225,7 +215,7 @@ var storeLockTimeout = 5 * time.Second // setLocked is SetMachine's read, change and write, run under the store's // lock. -func setLocked(home, dir, name, value string) (bool, error) { +func setLocked(home string, dir *os.Root, name, value string) (bool, error) { store, err := readStore(home) if err != nil { return false, err @@ -243,7 +233,7 @@ func setLocked(home, dir, name, value string) (bool, error) { if err != nil { return false, errors.New("credential: the store could not be encoded") } - if err := fsutil.WriteFileAtomic(filepath.Join(dir, StoreFileName), append(body, '\n'), 0o600); err != nil { + if err := fsutil.WriteFileAtomicInRoot(dir, StoreFileName, append(body, '\n'), 0o600); err != nil { return false, fmt.Errorf("credential: %s could not be written, so the credential was not stored", StorePath) } return true, nil diff --git a/internal/core/credential/external.go b/internal/core/credential/external.go new file mode 100644 index 000000000..7d5b0aa64 --- /dev/null +++ b/internal/core/credential/external.go @@ -0,0 +1,147 @@ +package credential + +// external.go is the external home: a pointer at a setup outside abcd, which +// abcd follows on every Resolve and never copies. A pointer names either an +// environment variable, or a field of a tool's JSON configuration file under +// the home directory, written in the tilde form (~/.config/tool/auth.json) +// with a dotted field path (auth.token). +// +// The file is read under the declaration guards (a regular file, never a +// symlink, owned by the caller, writable by nobody else, bounded), and a file +// inside a git working tree is refused: a secret there is one a commit can +// carry, and the store never points at one. + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "regexp" + "strings" + + "github.com/intentdriven/abcd/internal/core/jsonstrict" + "github.com/intentdriven/abcd/internal/fsutil" +) + +// fileRe is a pointer's file in the tilde form: plain segments, no dot-dot. +var fileRe = regexp.MustCompile(`^~/[A-Za-z0-9._-]+(/[A-Za-z0-9._-]+)*$`) + +// fieldRe is a pointer's dotted field path. +var fieldRe = regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}(\.[A-Za-z0-9_-]{1,64}){0,7}$`) + +// maxToolFileBytes bounds a tool's configuration file read. +const maxToolFileBytes = 256 << 10 + +// checkPointer refuses a malformed pointer: exactly one of an environment +// variable or a file with its field. +func checkPointer(p Pointer) error { + switch { + case p.Env != "" && (p.File != "" || p.Field != ""): + return errors.New("credential: an external pointer names an environment variable or a file, not both") + case p.Env != "": + if !envNameRe.MatchString(p.Env) { + return errors.New("credential: the environment variable's name is not letters, digits and '_'") + } + case p.File != "" || p.Field != "": + if !fileRe.MatchString(p.File) || strings.Contains(p.File, "/../") || strings.HasSuffix(p.File, "/..") || strings.Contains(p.File, "/./") { + return errors.New("credential: the file is not a plain path under the home directory, written from ~/") + } + if !fieldRe.MatchString(p.Field) { + return errors.New("credential: the file's field is not a dotted path of plain keys (auth.token)") + } + default: + return errors.New("credential: the external home needs a pointer: an environment variable, or a file and its field") + } + return nil +} + +// pointerLinkRefusal words the refusal of a pointer whose directory passes +// through a symlink. The file is the tool's, not abcd's, so the remedy +// fsutil.HomeScopeLinkError gives for abcd's own files (replace the link to +// keep abcd's files there) does not fit: a pointer is refused when any +// directory between the home and the tool's file is a symlink, wherever the +// link leads, and the person either names the file through real directories +// or uses the environment-variable pointer, which no link affects. Any other +// error keeps its own words. +func pointerLinkRefusal(name, file string, err error) error { + var le *fsutil.HomeScopeLinkError + if !errors.As(err, &le) { + return fmt.Errorf("credential: %s points at %s, which is not read: %v", name, file, err) + } + return fmt.Errorf("credential: %s points at %s, which is not read: %s is a symlink, and a pointer is refused when any directory between the home and the tool's file is one, wherever it leads; name the file through real directories under the home, or use an environment-variable pointer", name, file, le.Link) +} + +// resolvePointer follows p for name. What it points at being empty or absent +// is ErrNotSet, naming what was followed; anything unsafe is refused. +func resolvePointer(home, name string, p Pointer) (string, error) { + if err := checkPointer(p); err != nil { + return "", err + } + if p.Env != "" { + v := os.Getenv(p.Env) + if v == "" { + return "", notSetError{name: name, why: "the environment variable " + p.Env + " it points at is not set"} + } + if err := CheckValue(v); err != nil { + return "", fmt.Errorf("credential: %s, through the environment variable %s: %w", name, p.Env, err) + } + return v, nil + } + // A directory on the way to the file that is a symlink (~/.config linked + // into a dotfiles repository, say) is refused first, naming the link: the + // working-tree check below judges the lexical path and cannot see a + // repository the link leads into. The read then goes through the + // descriptor walk of the directories judged (fsutil.ReadHomeDeclaration), + // never the path again. + rel := strings.TrimPrefix(p.File, "~/") + if err := fsutil.HomeScopeLink(home, rel); err != nil { + return "", pointerLinkRefusal(name, p.File, err) + } + if tree := workingTreeAbove(home, filepath.Dir(filepath.FromSlash(rel))); tree != "" { + return "", fmt.Errorf("credential: %s points at %s, which lies inside a git working tree, where a commit could carry it, so it is not read", name, p.File) + } + raw, refusal, err := fsutil.ReadHomeDeclaration(home, rel, maxToolFileBytes) + switch { + case refusal == fsutil.DeclarationAbsent && errors.Is(err, os.ErrNotExist): + return "", notSetError{name: name, why: "the file " + p.File + " it points at does not exist"} + case refusal == fsutil.DeclarationBehindSymlink: + return "", pointerLinkRefusal(name, p.File, err) + case refusal == fsutil.DeclarationNotRegular: + return "", fmt.Errorf("credential: %s points at %s, which is not a regular file (a symlink is never followed), so it is not read", name, p.File) + case refusal == fsutil.DeclarationWritableByOthers: + return "", fmt.Errorf("credential: %s points at %s, which group or other can write, so it is not read", name, p.File) + case refusal == fsutil.DeclarationForeignOwner: + return "", fmt.Errorf("credential: %s points at %s, which is not owned by you, so it is not read", name, p.File) + case err != nil: + return "", fmt.Errorf("credential: %s points at %s, which could not be read safely, so it is not read", name, p.File) + } + if err := jsonstrict.NoDuplicateKeys(raw); err != nil { + return "", fmt.Errorf("credential: %s points at %s, which names one key twice, so it is not read", name, p.File) + } + var doc any + if err := json.Unmarshal(raw, &doc); err != nil { + // The decoder's message can quote the file's bytes; it is dropped. + return "", fmt.Errorf("credential: %s points at %s, which is not JSON, so it is not read", name, p.File) + } + for _, key := range strings.Split(p.Field, ".") { + obj, ok := doc.(map[string]any) + if !ok { + return "", notSetError{name: name, why: "the field " + p.Field + " of " + p.File + " is absent"} + } + if doc, ok = obj[key]; !ok { + return "", notSetError{name: name, why: "the field " + p.Field + " of " + p.File + " is absent"} + } + } + v, ok := doc.(string) + switch { + case !ok: + return "", fmt.Errorf("credential: %s points at the field %s of %s, which is not a string, so it is not read", name, p.Field, p.File) + case v == "": + return "", notSetError{name: name, why: "the field " + p.Field + " of " + p.File + " is empty"} + } + if err := CheckValue(v); err != nil { + return "", fmt.Errorf("credential: %s, through the field %s of %s: %w", name, p.Field, p.File, err) + } + return v, nil +} diff --git a/internal/core/credential/home_link_test.go b/internal/core/credential/home_link_test.go index a8266c0e3..14a1c27d2 100644 --- a/internal/core/credential/home_link_test.go +++ b/internal/core/credential/home_link_test.go @@ -3,10 +3,13 @@ package credential import ( + "errors" "os" "path/filepath" "strings" "testing" + + "github.com/intentdriven/abcd/internal/fsutil" ) // dotfilesHome returns a home whose ~/.abcd is a symlink to a directory in a @@ -97,3 +100,84 @@ func assertOwnerOnly(t *testing.T, p string) { t.Fatalf("%s is mode %04o; the lock must be the owner's alone", filepath.Base(p), fi.Mode().Perm()) } } + +// TestSetMachineWritesNothingThroughAnAbcdHomeSwappedForALink is +// iss-2609281310017733: ~/.abcd is a real directory when SetMachine judges it +// and a symlink into a dotfiles checkout by the time it writes. A check by path +// followed by a create by path lands the secret (and its lock) in the checkout; +// the write through the descriptor of the directory that was judged is refused, +// names the link, and leaves the checkout as it was. +func TestSetMachineWritesNothingThroughAnAbcdHomeSwappedForALink(t *testing.T) { + home := t.TempDir() + dotfiles := filepath.Join(home, "dotfiles", "abcd") + for _, dir := range []string{filepath.Join(home, ".abcd"), dotfiles} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + swapped := false + t.Cleanup(fsutil.SwapHomeScopeVettedForTest(func(dir string) { + if swapped || dir != filepath.Join(home, ".abcd") { + return + } + swapped = true + if err := os.Rename(dir, filepath.Join(home, "moved-aside")); err != nil { + t.Fatalf("swap: %v", err) + } + if err := os.Symlink(dotfiles, dir); err != nil { + t.Fatalf("swap: %v", err) + } + })) + changed, err := SetMachine(home, "openrouter", "sk-example-0123456789") + if !swapped { + t.Fatal("the vetting hook never ran, so the race was not staged") + } + if err == nil || changed || !strings.Contains(err.Error(), "~/.abcd is a symlink") { + t.Errorf("SetMachine must refuse a ~/.abcd swapped for a link, naming it: changed %v, err %v", changed, err) + } + entries, rerr := os.ReadDir(dotfiles) + if rerr != nil { + t.Fatal(rerr) + } + if len(entries) != 0 { + t.Fatalf("SetMachine wrote %d file(s) through the swapped link, first %q", len(entries), entries[0].Name()) + } +} + +// TestResolveRefusesAStoreSwappedForAGroupReadableOneAfterItsCheck is the +// mode half of iss-2609281310017733: the store is 0600 when a check by path +// judges it and a group- and other-readable file by the time it is opened (a +// same-uid race, staged through the vetting hook of the ~/.abcd walk). A mode +// judged by path vouches for a file other than the one read; the mode judged +// on the opened file's own fstat refuses it, and the value is never returned. +func TestResolveRefusesAStoreSwappedForAGroupReadableOneAfterItsCheck(t *testing.T) { + home := t.TempDir() + store := writeStore(t, home, `{"openrouter": "sk-example-owner-only"}`, 0o600) + exposed := filepath.Join(home, "exposed.json") + if err := os.WriteFile(exposed, []byte(`{"openrouter": "`+secretValue+`"}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Chmod(exposed, 0o644); err != nil { + t.Fatal(err) + } + swapped := false + t.Cleanup(fsutil.SwapHomeScopeVettedForTest(func(dir string) { + if swapped || dir != filepath.Join(home, ".abcd") { + return + } + swapped = true + if err := os.Rename(exposed, store); err != nil { + t.Fatalf("swap: %v", err) + } + })) + v, err := Machine(home).Resolve("openrouter") + if !swapped { + t.Fatal("the vetting hook never ran, so the race was not staged") + } + if err == nil || errors.Is(err, ErrNotSet) || v != "" { + t.Fatalf("a store swapped for a 0644 file after its check must be refused: value %q, err %v", v, err) + } + if strings.Contains(err.Error(), secretValue) || !strings.Contains(err.Error(), "chmod 0600") { + t.Fatalf("the refusal must name the remedy and never the value: %v", err) + } +} diff --git a/internal/core/credential/home_routing_test.go b/internal/core/credential/home_routing_test.go new file mode 100644 index 000000000..7ab139210 --- /dev/null +++ b/internal/core/credential/home_routing_test.go @@ -0,0 +1,278 @@ +//go:build unix + +package credential + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// linkedDotfilesHome returns a home whose ~/.abcd is a symlink into a git +// working tree (a dotfiles repository beside it, the layout AGENTS.md names), +// and the directory the link points at. The home itself is not a working +// tree, so a judgement of ~/.abcd by its lexical path sees no repository. +func linkedDotfilesHome(t *testing.T) (home, target string) { + t.Helper() + home = t.TempDir() + repo := filepath.Join(home, "dotfiles") + target = filepath.Join(repo, "abcd") + for _, dir := range []string{filepath.Join(repo, ".git"), target} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + if err := os.Symlink(target, filepath.Join(home, ".abcd")); err != nil { + t.Fatal(err) + } + return home, target +} + +// TestSetRefusesEveryHomeThroughAnAbcdHomeLinkedIntoARepository is the +// review's probe A (itd-2609221017023290, criterion 3): a ~/.abcd symlinked +// into a dotfiles repository is refused, naming the link, in every home, before +// anything is created: not the value, not the index, not either lock. A +// working-tree check on the lexical path does not see the repository, so the +// link itself is what is refused. +func TestSetRefusesEveryHomeThroughAnAbcdHomeLinkedIntoARepository(t *testing.T) { + const envName = "ABCD_TEST_ROUTING_TOKEN" + t.Setenv(envName, secretValue) + keychain := withFakeKeychain(t, "security") + for _, c := range []struct { + name string + choice Choice + }{ + {"abcd", Choice{Home: HomeABCD, Value: secretValue}}, + {"keychain", Choice{Home: HomeKeychain, Value: secretValue}}, + {"external", Choice{Home: HomeExternal, Pointer: Pointer{Env: envName}}}, + } { + t.Run(c.name, func(t *testing.T) { + home, target := linkedDotfilesHome(t) + changed, err := Set(home, "svc", c.choice) + if err == nil || changed { + t.Fatalf("Set wrote through a ~/.abcd linked into a repository: changed %v, err %v", changed, err) + } + if !strings.Contains(err.Error(), "~/.abcd is a symlink") { + t.Errorf("the refusal must name the link: %v", err) + } + if strings.Contains(err.Error(), secretValue) { + t.Fatal("the refusal echoes the value") + } + entries, rerr := os.ReadDir(target) + if rerr != nil { + t.Fatal(rerr) + } + if len(entries) != 0 { + t.Fatalf("Set left %d file(s) in the repository, first %q", len(entries), entries[0].Name()) + } + assertNowhere(t, filepath.Join(home, "dotfiles"), secretValue) + }) + } + if entries, _ := os.ReadDir(keychain); len(entries) != 0 { + t.Fatalf("the keychain was written: %d item(s)", len(entries)) + } +} + +// TestAPointerWhoseDirectoryLinksIntoARepositoryIsRefused is the review's +// probe C: an external pointer at ~/.config/tool.json, where ~/.config is a +// symlink into a dotfiles repository, is refused naming the link, rather than +// followed and read from inside a working tree; and Set records no such +// pointer. +func TestAPointerWhoseDirectoryLinksIntoARepositoryIsRefused(t *testing.T) { + home := t.TempDir() + repo := filepath.Join(home, "dotfiles") + cfg := filepath.Join(repo, "cfg") + for _, dir := range []string{filepath.Join(repo, ".git"), cfg} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(cfg, "tool.json"), []byte(`{"auth":{"token":"`+secretValue+`"}}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(cfg, filepath.Join(home, ".config")); err != nil { + t.Fatal(err) + } + p := Pointer{File: "~/.config/tool.json", Field: "auth.token"} + v, err := resolvePointer(home, "svc", p) + if err == nil || v != "" { + t.Fatalf("a pointer through ~/.config linked into a repository was read: value %q, err %v", v, err) + } + if !strings.Contains(err.Error(), "~/.config is a symlink") { + t.Errorf("the refusal must name the link: %v", err) + } + if strings.Contains(err.Error(), secretValue) { + t.Fatal("the refusal echoes the value") + } + if _, err := Set(home, "svc", Choice{Home: HomeExternal, Pointer: p}); err == nil { + t.Fatal("Set recorded a pointer through a link into a repository") + } + if _, err := os.Lstat(filepath.Join(home, ".abcd", IndexFileName)); !os.IsNotExist(err) { + t.Fatalf("the index was written: %v", err) + } +} + +// TestSetWritesNoIndexThroughAnAbcdHomeSwappedForALink is the index's half of +// iss-2609281310017733: ~/.abcd is a real directory when Set judges it and a +// symlink into a dotfiles checkout by a later use (a same-uid race, staged +// through the vetting hook of the ~/.abcd walk). The index, its lock and the +// read of the index are reached through the walk of the directory that was +// judged, so the swap is refused, the link is named, and the checkout is left +// as it was. A Set that judges ~/.abcd by path and writes by path never +// reaches the walk at all, which the test reports as a race it could not stage. +func TestSetWritesNoIndexThroughAnAbcdHomeSwappedForALink(t *testing.T) { + const envName = "ABCD_TEST_ROUTING_SWAP_TOKEN" + t.Setenv(envName, secretValue) + for _, c := range []struct { + name string + // swapAt is the vetting of ~/.abcd (1-based) that swaps it: the first + // is Set's own walk, before the lock; the second is the index read + // under the lock, once an index is there to read. + swapAt int + seedIndex bool + }{ + {"before the lock", 1, false}, + {"under the lock", 2, true}, + } { + t.Run(c.name, func(t *testing.T) { + home := t.TempDir() + abcd := filepath.Join(home, ".abcd") + dotfiles := filepath.Join(home, "dotfiles", "abcd") + for _, dir := range []string{abcd, dotfiles} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + if c.seedIndex { + if err := os.WriteFile(filepath.Join(abcd, IndexFileName), []byte(`{"other":{"home":"keychain"}}`+"\n"), 0o600); err != nil { + t.Fatal(err) + } + } + seen, swapped := 0, false + t.Cleanup(fsutil.SwapHomeScopeVettedForTest(func(dir string) { + if swapped || dir != abcd { + return + } + if seen++; seen < c.swapAt { + return + } + swapped = true + if err := os.Rename(dir, filepath.Join(home, "moved-aside")); err != nil { + t.Fatalf("swap: %v", err) + } + if err := os.Symlink(dotfiles, dir); err != nil { + t.Fatalf("swap: %v", err) + } + })) + changed, err := Set(home, "svc", Choice{Home: HomeExternal, Pointer: Pointer{Env: envName}}) + if !swapped { + t.Fatalf("Set never reached ~/.abcd through the walk that judges it (%d vetting(s) seen), so the race could not be staged: changed %v, err %v", seen, changed, err) + } + if err == nil || changed || !strings.Contains(err.Error(), "~/.abcd is a symlink") { + t.Errorf("Set must refuse a ~/.abcd swapped for a link, naming it: changed %v, err %v", changed, err) + } + entries, rerr := os.ReadDir(dotfiles) + if rerr != nil { + t.Fatal(rerr) + } + if len(entries) != 0 { + t.Fatalf("Set wrote %d file(s) through the swapped link, first %q", len(entries), entries[0].Name()) + } + }) + } +} + +// TestAHomeThatIsItselfALinkIntoACheckoutIsJudgedWhereItLeads is +// iss-2609290259108077 (review-integ14 LOW (a)): the home directory is itself +// a symlink into a git checkout (~ -> /home). The home is never refused +// for being a link, but the working-tree check judges where it leads as well +// as its lexical path, so the abcd home is refused with nothing written in the +// checkout and the keychain untouched, and a pointer at a file under that home +// is not read. +func TestAHomeThatIsItselfALinkIntoACheckoutIsJudgedWhereItLeads(t *testing.T) { + keychain := withFakeKeychain(t, "security") + base := t.TempDir() + repo := filepath.Join(base, "checkout") + real := filepath.Join(repo, "home") + for _, dir := range []string{filepath.Join(repo, ".git"), filepath.Join(real, ".config")} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + home := filepath.Join(base, "account") + if err := os.Symlink(real, home); err != nil { + t.Fatal(err) + } + + changed, err := Set(home, "svc", Choice{Home: HomeABCD, Value: secretValue}) + if err == nil || changed { + t.Fatalf("Set kept the value under a home that leads into a checkout: changed %v, err %v", changed, err) + } + if !strings.Contains(err.Error(), "git working tree") { + t.Errorf("the refusal must name the working tree: %v", err) + } + if strings.Contains(err.Error(), secretValue) { + t.Fatal("the refusal echoes the value") + } + if _, serr := os.Lstat(filepath.Join(real, ".abcd", StoreFileName)); !os.IsNotExist(serr) { + t.Fatalf("%s was written inside the checkout: %v", StoreFileName, serr) + } + assertNowhere(t, repo, secretValue) + if entries, _ := os.ReadDir(keychain); len(entries) != 0 { + t.Fatalf("the keychain was written: %d item(s)", len(entries)) + } + + if err := os.WriteFile(filepath.Join(real, ".config", "tool.json"), []byte(`{"auth":{"token":"`+secretValue+`"}}`), 0o600); err != nil { + t.Fatal(err) + } + v, err := resolvePointer(home, "svc", Pointer{File: "~/.config/tool.json", Field: "auth.token"}) + if err == nil || v != "" { + t.Fatalf("a pointer under a home that leads into a checkout was read: value %q, err %v", v, err) + } + if !strings.Contains(err.Error(), "git working tree") { + t.Errorf("the pointer's refusal must name the working tree: %v", err) + } +} + +// TestAPointerThroughALinkIsRefusedInAPointersWords is review-integ14 LOW (c): +// the file a pointer names is the tool's, not abcd's, so the refusal of a +// pointer whose directory passes through a symlink says what a pointer needs +// (directories that are real, or the environment-variable pointer) and never +// tells the person to keep abcd's files there. The link here leads outside any +// repository: the rule is the link, wherever it leads. +func TestAPointerThroughALinkIsRefusedInAPointersWords(t *testing.T) { + home := t.TempDir() + elsewhere := filepath.Join(t.TempDir(), "cfg") + if err := os.MkdirAll(elsewhere, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(elsewhere, "tool.json"), []byte(`{"auth":{"token":"`+secretValue+`"}}`), 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(elsewhere, filepath.Join(home, ".config")); err != nil { + t.Fatal(err) + } + p := Pointer{File: "~/.config/tool.json", Field: "auth.token"} + _, rerr := resolvePointer(home, "svc", p) + _, serr := Set(home, "svc", Choice{Home: HomeExternal, Pointer: p}) + for what, err := range map[string]error{"resolve": rerr, "Set": serr} { + if err == nil { + t.Fatalf("%s: a pointer through a symlinked ~/.config was followed", what) + } + msg := err.Error() + for _, want := range []string{"~/.config is a symlink", "a pointer", "real directories", "environment-variable pointer"} { + if !strings.Contains(msg, want) { + t.Errorf("%s: the refusal lacks %q: %s", what, want, msg) + } + } + if strings.Contains(msg, "abcd's files") { + t.Errorf("%s: the refusal calls the tool's file abcd's: %s", what, msg) + } + if strings.Contains(msg, secretValue) { + t.Fatalf("%s: the refusal echoes the value", what) + } + } +} diff --git a/internal/core/credential/keychain.go b/internal/core/credential/keychain.go new file mode 100644 index 000000000..78558cd7d --- /dev/null +++ b/internal/core/credential/keychain.go @@ -0,0 +1,159 @@ +package credential + +// keychain.go is the keychain home: the platform's own secret store, reached +// through its command-line tool as a subprocess (no Go dependency): the +// Keychain through /usr/bin/security on macOS, the secret service through +// secret-tool on Linux. The item is kept under abcd's service name with the +// credential's name as its account. +// +// The value never reaches an argv, which a process listing shows: security +// receives the add command on stdin in its interactive mode, the value as hex +// (-X), and secret-tool reads the value from stdin. The tool is run by an +// absolute path in a system directory, never resolved on PATH, so nothing a +// repository or a shell profile puts first is run. What the tool prints on +// failure is not repeated: a refusal names the exit status only. + +import ( + "bytes" + "context" + "encoding/hex" + "errors" + "fmt" + "os" + "os/exec" + "runtime" + "strings" + "time" +) + +// keychainService is the service name every abcd item is kept under. +const keychainService = "abcd" + +// The two platform tools. +const ( + keychainMacOS = "security" + keychainSecretService = "secret-tool" +) + +// keychainTool is the platform tool located: which one, and its path. +type keychainTool struct { + kind string + path string +} + +// errKeychainAbsent is a platform with no keychain tool abcd can run. +var errKeychainAbsent = errors.New("credential: the keychain home needs the platform keychain's tool " + + "(/usr/bin/security on macOS, secret-tool from libsecret on Linux), and none is present here; " + + "the external home and the abcd home remain") + +// keychainTimeout bounds one keychain command; the platform may ask the +// person to unlock the keychain first. +var keychainTimeout = 60 * time.Second + +// locateKeychain finds the platform's tool at its fixed system path. It is a +// variable so a test can point the home at a fake and never the real keychain. +var locateKeychain = func() (keychainTool, error) { + var t keychainTool + switch runtime.GOOS { + case "darwin": + t = keychainTool{kind: keychainMacOS, path: "/usr/bin/security"} + case "linux": + t = keychainTool{kind: keychainSecretService, path: "/usr/bin/secret-tool"} + default: + return t, errKeychainAbsent + } + if fi, err := os.Stat(t.path); err != nil || !fi.Mode().IsRegular() { + return keychainTool{}, errKeychainAbsent + } + return t, nil +} + +// runKeychain runs the tool with argv and stdin, returning stdout and the +// exit status (-1 when it did not run to an exit). +func runKeychain(t keychainTool, stdin []byte, args ...string) ([]byte, int, error) { + ctx, cancel := context.WithTimeout(context.Background(), keychainTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, t.path, args...) + cmd.Stdin = bytes.NewReader(stdin) + var out bytes.Buffer + cmd.Stdout = &out + err := cmd.Run() + var exit *exec.ExitError + switch { + case err == nil: + return out.Bytes(), 0, nil + case errors.As(err, &exit): + return out.Bytes(), exit.ExitCode(), nil + } + return nil, -1, fmt.Errorf("credential: the keychain tool %s could not be run", t.kind) +} + +// keychainLookup reads name's item. An absent item is ErrNotSet. +func keychainLookup(name string) (string, error) { + t, err := locateKeychain() + if err != nil { + return "", err + } + var out []byte + var code int + switch t.kind { + case keychainMacOS: + out, code, err = runKeychain(t, nil, "find-generic-password", "-s", keychainService, "-a", name, "-w") + if code == 44 { // errSecItemNotFound + return "", notSetError{name: name, why: "the keychain holds no item for it"} + } + default: + out, code, err = runKeychain(t, nil, "lookup", "service", keychainService, "account", name) + if code == 1 && len(out) == 0 { + return "", notSetError{name: name, why: "the keychain holds no item for it"} + } + } + if err != nil { + return "", err + } + if code != 0 { + return "", fmt.Errorf("credential: the keychain refused to read %s (%s exited %d); unlock it, or choose another home", name, t.kind, code) + } + v := strings.TrimSuffix(strings.TrimSuffix(string(out), "\n"), "\r") + if v == "" { + return "", notSetError{name: name, why: "the keychain's item for it is empty"} + } + if err := CheckValue(v); err != nil { + return "", fmt.Errorf("credential: the keychain's item for %s: %w", name, err) + } + return v, nil +} + +// keychainStore adds name's item holding value, then reads it back: the +// interactive mode security is driven through reports a failed command on +// stderr rather than in its exit, so the read is what proves the write. +func keychainStore(name, value string) error { + t, err := locateKeychain() + if err != nil { + return err + } + var code int + switch t.kind { + case keychainMacOS: + line := fmt.Sprintf("add-generic-password -s %s -a %s -l abcd:%s -X %s\n", keychainService, name, name, hex.EncodeToString([]byte(value))) + _, code, err = runKeychain(t, []byte(line), "-i") + default: + _, code, err = runKeychain(t, []byte(value), "store", "--label=abcd:"+name, "service", keychainService, "account", name) + } + if err != nil { + return err + } + if code != 0 { + return fmt.Errorf("credential: the keychain refused to store %s (%s exited %d), so nothing was stored", name, t.kind, code) + } + got, err := keychainLookup(name) + switch { + case errors.Is(err, ErrNotSet): + return fmt.Errorf("credential: the keychain did not keep %s (it may be locked, or have refused the item), so nothing was stored", name) + case err != nil: + return err + case got != value: + return fmt.Errorf("credential: the keychain's item for %s does not read back as written; remove it by hand", name) + } + return nil +} diff --git a/internal/core/credential/readers_test.go b/internal/core/credential/readers_test.go new file mode 100644 index 000000000..f90c19ec5 --- /dev/null +++ b/internal/core/credential/readers_test.go @@ -0,0 +1,104 @@ +package credential + +import ( + "io/fs" + "os" + "path/filepath" + "regexp" + "strings" + "testing" +) + +// bypassPatterns are the lines a reader that bypasses the store writes. +var bypassPatterns = []struct { + why string + re *regexp.Regexp +}{ + {"reads the abcd home directly instead of through Store", regexp.MustCompile(`credential\.(Machine|UserMachine)\(`)}, + {"writes the abcd home directly instead of through Set or Walk", regexp.MustCompile(`credential\.SetMachine\(`)}, + {"names a store file instead of resolving by name", regexp.MustCompile(`"[^"]*(credentials\.json|credential-homes\.json)"`)}, + {"runs a keychain command instead of resolving by name", regexp.MustCompile(`"(find-generic-password|add-generic-password|secret-tool)"`)}, + {"reads a secret-shaped environment variable instead of resolving by name", + regexp.MustCompile(`os\.(Getenv|LookupEnv)\("[A-Za-z0-9_]*(?i:token|secret|passw|api_?key|_key)[A-Za-z0-9_]*"\)`)}, +} + +// TestEveryReaderGoesThroughTheStore (criterion 4, adr-2609221017021499 +// ruling 3): outside this package, no production code reads a credential any +// way but Store(...).Resolve, writes one any way but Set or Walk, names the +// store's files, runs a keychain command, or reads a secret-shaped +// environment variable. It is a drift grep for an accidental bypass, not an +// evasion gate: a new reader written the obvious way fails here, naming the +// file and the line, while one written to slip past it (an aliased or dot +// import, a variable's name held in a variable, os.Environ, an argv built by +// concatenation) does not, and only cmd/ and internal/ are walked. +func TestEveryReaderGoesThroughTheStore(t *testing.T) { + root, err := filepath.Abs(filepath.Join("..", "..", "..")) + if err != nil { + t.Fatal(err) + } + if _, err := os.Stat(filepath.Join(root, "go.mod")); err != nil { + t.Fatalf("the module root is not where this test expects it: %v", err) + } + self := filepath.Join(root, "internal", "core", "credential") + var scanned int + for _, top := range []string{"cmd", "internal"} { + err := filepath.WalkDir(filepath.Join(root, top), func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + if p == self || d.Name() == "testdata" { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") { + return nil + } + raw, err := os.ReadFile(p) + if err != nil { + return err + } + scanned++ + for i, line := range strings.Split(string(raw), "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "//") { + continue + } + for _, b := range bypassPatterns { + if b.re.MatchString(line) { + rel, _ := filepath.Rel(root, p) + t.Errorf("%s:%d %s:\n\t%s", filepath.ToSlash(rel), i+1, b.why, strings.TrimSpace(line)) + } + } + } + return nil + }) + if err != nil { + t.Fatal(err) + } + } + if scanned < 100 { + t.Fatalf("scanned %d Go files; the walk did not reach the tree", scanned) + } +} + +// TestTheReaderGrepCatchesABypass proves the patterns bite: each line is one a +// bypassing reader would write. +func TestTheReaderGrepCatchesABypass(t *testing.T) { + for _, line := range []string{ + `v, err := credential.Machine(home).Resolve("x")`, + `credential.SetMachine(home, "x", v)`, + `p := filepath.Join(home, ".abcd", "credentials.json")`, + `exec.Command("security", "find-generic-password", "-w")`, + `tok := os.Getenv("CLOUDFLARE_API_TOKEN")`, + `k, _ := os.LookupEnv("OPENROUTER_API_KEY")`, + } { + hit := false + for _, b := range bypassPatterns { + hit = hit || b.re.MatchString(line) + } + if !hit { + t.Errorf("the grep misses a bypass: %s", line) + } + } +} diff --git a/internal/core/credential/store.go b/internal/core/credential/store.go new file mode 100644 index 000000000..9bab4481c --- /dev/null +++ b/internal/core/credential/store.go @@ -0,0 +1,490 @@ +package credential + +// store.go is the credential store proper (itd-2609221017023290, +// adr-2609221017021499): one reader, Store(home).Resolve, and one write, Set, +// over the three homes a person chooses among once per credential. +// +// - abcd: the owner-only ~/.abcd/credentials.json (credential.go), which +// holds the value itself. +// - keychain: the platform's keychain, under abcd's service name +// (keychain.go); the index holds only that the name lives there. +// - external: a pointer at a setup outside abcd, an environment variable or +// a named field of a tool's JSON configuration (external.go); the index +// holds the pointer, never the value. +// +// The index, ~/.abcd/credential-homes.json, maps a name to its keychain or +// external home. A name the index does not hold resolves from the abcd home, +// so a store written before the index existed reads unchanged. A name held in +// both is ambiguous and refused, never guessed. +// +// The index is the one file the store writes that must never hold a secret, +// so the secret scanner reads its bytes before they are written, and a +// finding refuses the write. The abcd home, the one that keeps a value under +// ~/.abcd, is never written inside a git working tree. + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "regexp" + "time" + + "github.com/intentdriven/abcd/internal/adapter/scanner" + "github.com/intentdriven/abcd/internal/core/jsonstrict" + "github.com/intentdriven/abcd/internal/fsutil" +) + +// The homes a credential lives in (adr-2609221017021499 ruling 2). +const ( + // HomeExternal is a setup outside abcd; the store holds the pointer. + HomeExternal = "external" + // HomeABCD is abcd-only: the owner-only ~/.abcd/credentials.json. + HomeABCD = "abcd" + // HomeKeychain is the platform keychain. + HomeKeychain = "keychain" +) + +// Homes returns the homes in the order every walkthrough offers them. None is +// marked: the keychain is recommended in HomesProse, never in the list. +func Homes() []string { return []string{HomeExternal, HomeABCD, HomeKeychain} } + +// IndexFileName is the index under ~/.abcd/, and IndexPath its tilde form. +const ( + IndexFileName = "credential-homes.json" + IndexPath = "~/.abcd/" + IndexFileName +) + +// Walkthrough is the command that explains a credential and stores it, named +// by every refusal of a name that is not set. +func Walkthrough(name string) string { return "abcd ahoy credential " + name } + +// Pointer is an external home's pointer: an environment variable, or a field +// of a tool's JSON configuration file under the home directory. +type Pointer struct { + Env string `json:"env,omitempty"` + File string `json:"file,omitempty"` + Field string `json:"field,omitempty"` +} + +// Choice is one home chosen for one credential: the value for the abcd and +// keychain homes, the pointer for the external one. +type Choice struct { + Home string + Value string + Pointer Pointer +} + +// indexEntry is one name's line in the index. +type indexEntry struct { + Home string `json:"home"` + Pointer +} + +// Store is the one reader every adapter resolves a credential through, by +// name, rooted at home (the caller's home directory). An empty home resolves +// every name to ErrNotSet. +func Store(home string) Source { return store{home: home} } + +// UserStore is Store at the caller's home directory. +func UserStore() Source { + home, err := os.UserHomeDir() + if err != nil { + home = "" + } + return Store(home) +} + +type store struct{ home string } + +// notSetError is ErrNotSet naming the walkthrough. +type notSetError struct{ name, why string } + +func (e notSetError) Error() string { + why := "" + if e.why != "" { + why = " (" + e.why + ")" + } + return fmt.Sprintf("credential %s is not set on this machine%s; `%s` explains what it unlocks and stores it", e.name, why, Walkthrough(e.name)) +} + +func (notSetError) Is(target error) bool { return target == ErrNotSet } + +func (s store) Resolve(name string) (string, error) { + if !nameRe.MatchString(name) { + return "", errors.New("credential: the name is not a plain credential name") + } + if s.home == "" { + return "", notSetError{name: name} + } + idx, err := readIndex(s.home) + if err != nil { + return "", err + } + v, err := Machine(s.home).Resolve(name) + e, indexed := idx[name] + switch { + case err != nil && !errors.Is(err, ErrNotSet): + return "", err + case !indexed && err != nil: + return "", notSetError{name: name} + case !indexed: + return v, nil + case err == nil: + return "", fmt.Errorf("credential: %s is named in two homes, %s and the %s home in %s, so it is not read; remove one by hand", + name, StorePath, e.Home, IndexPath) + } + switch e.Home { + case HomeKeychain: + return keychainLookup(name) + case HomeExternal: + return resolvePointer(s.home, name, e.Pointer) + } + return "", fmt.Errorf("credential: %s names an unknown home for %s, so it is not read", IndexPath, name) +} + +// Where names the home that holds name ("" when none does), reading the index +// and the abcd home and never a value: a surface shows presence and the home, +// and this is the home. Presence is Resolve. +func Where(home, name string) (string, error) { + if !nameRe.MatchString(name) { + return "", errors.New("credential: the name is not a plain credential name") + } + if home == "" { + return "", nil + } + idx, err := readIndex(home) + if err != nil { + return "", err + } + if e, ok := idx[name]; ok { + return e.Home, nil + } + if _, err := Machine(home).Resolve(name); err == nil { + return HomeABCD, nil + } else if !errors.Is(err, ErrNotSet) { + return "", err + } + return "", nil +} + +// Set stores a credential under name in the chosen home: the one write every +// setup goes through (the walkthrough, Walk, is its only caller outside this +// package's tests). It refuses, before writing a value or an index entry and +// never echoing a value: a ~/.abcd that is a symlink, in every home, before +// anything is created; the abcd home inside a git working tree; a name +// another home already holds, or a different value in the same home, because a +// stored secret is never replaced unasked; a keychain on a platform without +// one; and a pointer that does not resolve. The same value again is no change. +// The whole write, from reading where the name is held to the last write, +// holds the index's lock, so concurrent Sets of one name cannot land in two +// homes. +func Set(home, name string, c Choice) (changed bool, err error) { + if !nameRe.MatchString(name) { + return false, errors.New("credential: the name is not a plain credential name (lower case letters, digits, '.', '_' and '-')") + } + if home == "" { + return false, errors.New("credential: the home directory is unresolved, so there is nowhere to keep the credential") + } + switch c.Home { + case HomeABCD, HomeKeychain: + if err := CheckValue(c.Value); err != nil { + return false, err + } + if c.Pointer != (Pointer{}) { + return false, fmt.Errorf("credential: the %s home keeps a value, and a pointer was given", c.Home) + } + case HomeExternal: + if c.Value != "" { + return false, errors.New("credential: the external home keeps a pointer, never a value, and a value was given") + } + if err := checkPointer(c.Pointer); err != nil { + return false, err + } + default: + return false, fmt.Errorf("credential: home %q is not one of external, abcd, keychain", boundHome(c.Home)) + } + // A ~/.abcd that is a symlink (into a dotfiles repository, say) is + // refused first, in every home, before anything is created: the value, + // the index and both locks would land wherever the link points, and a + // working-tree check of the lexical path below cannot see a repository + // the link leads into. It is the rule every other reader and writer of + // ~/.abcd applies (fsutil.HomeScopeLink); the walk below holds it against + // a race. + if err := fsutil.HomeScopeLink(home, indexRel); err != nil { + return false, fmt.Errorf("credential: nothing was written: %v", err) + } + // The abcd home is the one home that writes a value under ~/.abcd, so it + // alone is refused inside a git working tree. The keychain keeps its value + // outside the home, and the index holds names and pointers only, scanned + // before every write, so a home directory that is itself a working tree (a + // dotfiles repository) keeps those homes. + if c.Home == HomeABCD && workingTreeAbove(home, ".abcd") != "" { + return false, errors.New("credential: ~/.abcd lies inside a git working tree, where a commit could carry the credential, so the abcd home is refused and nothing was written; choose the keychain or an external home") + } + // ~/.abcd is created, judged and opened in one walk relative to the + // descriptor of home (fsutil.EnsureHomeScope), and the index's lock and + // its write are reached through that descriptor, so a link swapped in + // after the judgement is refused rather than written through + // (iss-2609281310017733). + dir, err := fsutil.EnsureHomeScope(home, ".abcd", 0o700) + if errors.Is(err, fsutil.ErrHomeScopeSymlinked) { + return false, fmt.Errorf("credential: nothing was written: %v", err) + } + if err != nil { + return false, errors.New("credential: ~/.abcd could not be created, so nothing was written") + } + defer dir.Close() + // One lock, the index's, is held across the whole write: where the name + // is held, the value's write and the index's. Two Sets of one name to two + // homes therefore cannot both land. The abcd home's own lock (SetMachine) + // is taken inside this one, always in that order; no writer takes the two + // the other way round. + err = fsutil.WithFileLockIn(dir, indexLockFileName, indexLockTimeout, func() error { + var werr error + changed, werr = setUnderLock(home, dir, name, c) + return werr + }) + switch { + case errors.Is(err, fsutil.ErrLockContention): + return false, fmt.Errorf("credential: %s is being written by another abcd, so nothing was written; retry", IndexPath) + case errors.Is(err, fsutil.ErrLockPathUnsafe): + return false, fmt.Errorf("credential: the lock ~/.abcd/%s is not a regular file (a symlink, or something else), so it is refused and nothing was written; remove it, and the next write creates it afresh", indexLockFileName) + } + return changed, err +} + +// setUnderLock is Set's read of where name is held and its write, run under +// the index's lock. dir is ~/.abcd as Set's walk opened it; the index is +// written through it. +func setUnderLock(home string, dir *os.Root, name string, c Choice) (bool, error) { + held, err := Where(home, name) + if err != nil { + return false, err + } + if held != "" && held != c.Home { + return false, fmt.Errorf("credential: %s is already held in the %s home, and abcd never replaces a stored secret; remove it there by hand to choose another home", name, held) + } + switch c.Home { + case HomeABCD: + return SetMachine(home, name, c.Value) + case HomeKeychain: + if held == HomeKeychain { + return sameOrRefuse(home, name, c.Value) + } + if _, err := locateKeychain(); err != nil { + return false, err + } + // The index is judged before the keychain is touched, so a refusal + // leaves nothing in either. + if err := setIndex(home, dir, name, indexEntry{Home: HomeKeychain}, true); err != nil { + return false, err + } + // An item a failed earlier write left behind is adopted when it holds + // this value and refused when it holds another. + switch stored, err := keychainLookup(name); { + case err == nil && stored != c.Value: + return false, fmt.Errorf("credential: the keychain already holds a value for %s, and abcd never replaces a stored secret; remove that item by hand to store a new one", name) + case err == nil: + case !errors.Is(err, ErrNotSet): + return false, err + default: + if err := keychainStore(name, c.Value); err != nil { + return false, err + } + } + if err := setIndex(home, dir, name, indexEntry{Home: HomeKeychain}, false); err != nil { + return false, fmt.Errorf("%w; the keychain holds the item, and the next write of the same value records it", err) + } + return true, nil + } + if _, err := resolvePointer(home, name, c.Pointer); err != nil { + return false, err + } + if held == HomeExternal { + return false, samePointer(home, name, c.Pointer) + } + if err := setIndex(home, dir, name, indexEntry{Home: HomeExternal, Pointer: c.Pointer}, false); err != nil { + return false, err + } + return true, nil +} + +// samePointer refuses a pointer other than the one the index holds for name. +func samePointer(home, name string, p Pointer) error { + idx, err := readIndex(home) + if err != nil { + return err + } + if idx[name].Pointer == p { + return nil + } + return fmt.Errorf("credential: %s already points elsewhere in %s, and abcd never replaces a stored pointer; remove that entry by hand", name, IndexPath) +} + +// sameOrRefuse is a second Set of a name already in the keychain: the same +// value is no change, a different one is refused. +func sameOrRefuse(home, name, value string) (bool, error) { + stored, err := Store(home).Resolve(name) + if err != nil { + return false, err + } + if stored == value { + return false, nil + } + return false, fmt.Errorf("credential: the keychain already holds a value for %s, and abcd never replaces a stored secret; remove that item by hand to store a new one", name) +} + +// workingTreeAbove returns the first directory at or above home/rel that +// carries a .git entry, or "" when none does. The place is judged twice: by +// its lexical path, and with home replaced by where home resolves +// (filepath.EvalSymlinks), so a home directory that is itself a symlink into +// a checkout (~ -> /home) is seen as lying inside it +// (iss-2609290259108077). Home is never refused for being a link; only the +// working tree it leads into is judged. A home that does not resolve is +// judged by its lexical path alone: nothing can then be written under it. +func workingTreeAbove(home, rel string) string { + if tree := gitEntryAbove(filepath.Join(home, rel)); tree != "" { + return tree + } + real, err := filepath.EvalSymlinks(home) + if err != nil || real == filepath.Clean(home) { + return "" + } + return gitEntryAbove(filepath.Join(real, rel)) +} + +// gitEntryAbove returns the first directory at or above dir, climbed +// lexically, that carries a .git entry, or "" when none does. +func gitEntryAbove(dir string) string { + d := filepath.Clean(dir) + for { + if _, err := os.Lstat(filepath.Join(d, ".git")); err == nil { + return d + } + parent := filepath.Dir(d) + if parent == d { + return "" + } + d = parent + } +} + +func boundHome(h string) string { + if len(h) > 16 { + return h[:16] + "…" + } + return h +} + +// maxIndexBytes bounds the index read. +const maxIndexBytes = 64 << 10 + +// indexRel is the index's place in the home, in the slash form the +// home-scoped primitives take. +const indexRel = ".abcd/" + IndexFileName + +// readIndex reads the index under the same refusals as the abcd home: a +// regular file, owned by the caller, owner-only, naming each credential once, +// every entry a known home. An absent index is empty. +// +// Every guard is judged by fsutil.ReadHomeDeclarationDenying, as readStore's +// are: an index behind a symlinked ~/.abcd is refused on the descriptor walk +// of ~/.abcd, and the leaf's type, owner and mode on the opened file's own +// fstat, never on a path first. +func readIndex(home string) (map[string]indexEntry, error) { + raw, refusal, err := fsutil.ReadHomeDeclarationDenying(home, indexRel, maxIndexBytes, 0o077) + var mode *fsutil.DeclarationModeError + switch { + case refusal == fsutil.DeclarationOK: + case refusal == fsutil.DeclarationAbsent && errors.Is(err, os.ErrNotExist): + return map[string]indexEntry{}, nil + case refusal == fsutil.DeclarationAbsent: + return nil, fmt.Errorf("credential: %s could not be examined, so it is not read", IndexPath) + case refusal == fsutil.DeclarationBehindSymlink: + return nil, fmt.Errorf("credential: %s is not read: %v", IndexPath, err) + case refusal == fsutil.DeclarationNotRegular: + return nil, fmt.Errorf("credential: %s is not a regular file (a symlink is never followed), so it is not read", IndexPath) + case refusal == fsutil.DeclarationExposed && errors.As(err, &mode): + return nil, fmt.Errorf("credential: %s can be read or written by group or other (mode %04o), so it is not read; `chmod 0600 %s`", IndexPath, uint32(mode.Perm), IndexPath) + case refusal == fsutil.DeclarationWritableByOthers: + return nil, fmt.Errorf("credential: %s can be written by group or other, so it is not read; `chmod 0600 %s`", IndexPath, IndexPath) + case refusal == fsutil.DeclarationForeignOwner: + return nil, fmt.Errorf("credential: %s is not owned by you, so it is not read", IndexPath) + default: + return nil, fmt.Errorf("credential: %s could not be read safely (mode 0600, owned by you, a regular file), so it is not read", IndexPath) + } + var idx map[string]indexEntry + if err := jsonstrict.Decode(raw, &idx); err != nil || idx == nil { + // The decoder's message can quote the file's bytes; it is dropped. + return nil, fmt.Errorf("credential: %s is not a JSON object naming each credential once with its home, so it is not read", IndexPath) + } + for name, e := range idx { + ok := nameRe.MatchString(name) + switch e.Home { + case HomeKeychain: + ok = ok && e.Pointer == (Pointer{}) + case HomeExternal: + ok = ok && checkPointer(e.Pointer) == nil + default: + ok = false + } + if !ok { + return nil, fmt.Errorf("credential: %s holds an entry that is not a plain name with a keychain home or a well-formed external pointer, so it is not read", IndexPath) + } + } + return idx, nil +} + +// indexLockFileName is the lock every writer of the index takes, beside it. +const indexLockFileName = "." + IndexFileName + ".lock" + +// indexLockTimeout bounds the wait for another writer of the index. +var indexLockTimeout = 5 * time.Second + +// setIndex adds e under name to the index, or, with dryRun, judges the write +// (the scanner included) without making it. The caller, Set, holds the index's +// lock across the read, the scan and the write, and passes dir, ~/.abcd as its +// walk created, judged and opened it; the write goes through that descriptor. +func setIndex(home string, dir *os.Root, name string, e indexEntry, dryRun bool) error { + idx, err := readIndex(home) + if err != nil { + return err + } + if old, ok := idx[name]; ok && old != e { + return fmt.Errorf("credential: %s already names a home for %s, and abcd never replaces it; remove that entry by hand", IndexPath, name) + } + idx[name] = e + body, err := json.MarshalIndent(idx, "", " ") + if err != nil { + return errors.New("credential: the index could not be encoded") + } + body = append(body, '\n') + if err := scanIndex(body); err != nil { + return err + } + if dryRun { + return nil + } + if err := fsutil.WriteFileAtomicInRoot(dir, IndexFileName, body, 0o600); err != nil { + return fmt.Errorf("credential: %s could not be written, so the credential's home was not recorded", IndexPath) + } + return nil +} + +// scanIndex runs the secret scanner over the index's bytes before they are +// written (adr-2609221017021499 ruling 4). The index holds names, homes and +// pointers, never a value, so any finding refuses the write, and the refusal +// names the kind and never the match. +func scanIndex(body []byte) error { + findings := scanner.ScanText(string(body), scanner.Identity{}, scanner.DefaultPatterns(), nil, IndexFileName) + if len(findings) == 0 { + return nil + } + return fmt.Errorf("credential: the secret scanner found a %s in what %s would hold, which holds names and pointers only, so nothing was written", + findings[0].Kind, IndexPath) +} + +// envNameRe is an environment variable's name as a pointer names it. +var envNameRe = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_]{0,127}$`) diff --git a/internal/core/credential/store_test.go b/internal/core/credential/store_test.go new file mode 100644 index 000000000..4a54cbe32 --- /dev/null +++ b/internal/core/credential/store_test.go @@ -0,0 +1,534 @@ +package credential + +import ( + "context" + "encoding/hex" + "encoding/json" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/intentdriven/abcd/internal/fsutil" +) + +// The credential store proper (itd-2609221017023290): one Resolve and one Set +// over three homes, the walkthrough that chooses one, and the refusals that +// keep a value out of the tree and the harness's settings. +// +// No test here touches the real keychain: the keychain home runs a fake, +// which is this test binary re-executed (TestMain), keeping each item as a +// file under a temporary directory and logging every argv it was given. + +const ( + fakeKeychainDirEnv = "ABCD_TEST_FAKE_KEYCHAIN_DIR" + fakeKeychainKind = "ABCD_TEST_FAKE_KEYCHAIN_KIND" +) + +func TestMain(m *testing.M) { + if dir := os.Getenv(fakeKeychainDirEnv); dir != "" && len(os.Args) > 1 && os.Args[1] != "" && !strings.HasPrefix(os.Args[1], "-test.") { + os.Exit(fakeKeychain(dir, os.Getenv(fakeKeychainKind), os.Args[1:])) + } + os.Exit(m.Run()) +} + +// fakeKeychain emulates the two platform commands closely enough to exercise +// the argv and stdin the store hands them. +func fakeKeychain(dir, kind string, args []string) int { + logf, _ := os.OpenFile(filepath.Join(dir, "argv.log"), os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o600) + fmt.Fprintln(logf, strings.Join(args, " ")) + logf.Close() + item := func(account string) string { return filepath.Join(dir, "item-"+account) } + flag := func(args []string, name string) string { + for i := 0; i+1 < len(args); i++ { + if args[i] == name { + return args[i+1] + } + } + return "" + } + switch kind { + case keychainMacOS: + switch args[0] { + case "find-generic-password": + if flag(args, "-s") != keychainService { + return 2 + } + b, err := os.ReadFile(item(flag(args, "-a"))) + if err != nil { + fmt.Fprintln(os.Stderr, "security: SecKeychainSearchCopyNext: The specified item could not be found in the keychain.") + return 44 + } + fmt.Println(string(b)) + return 0 + case "-i": + in, _ := io.ReadAll(os.Stdin) + fields := strings.Fields(string(in)) + if len(fields) == 0 || fields[0] != "add-generic-password" || flag(fields, "-s") != keychainService { + return 0 // interactive mode reports a failed command on stderr, not in its exit + } + v, err := hex.DecodeString(flag(fields, "-X")) + if err != nil { + return 0 + } + _ = os.WriteFile(item(flag(fields, "-a")), v, 0o600) + return 0 + } + case keychainSecretService: + switch args[0] { + case "lookup": + b, err := os.ReadFile(item(args[len(args)-1])) + if err != nil { + return 1 + } + os.Stdout.Write(b) + return 0 + case "store": + v, _ := io.ReadAll(os.Stdin) + _ = os.WriteFile(item(args[len(args)-1]), v, 0o600) + return 0 + } + } + return 2 +} + +// withFakeKeychain points the keychain home at the fake for one test. +func withFakeKeychain(t *testing.T, kind string) string { + t.Helper() + dir := t.TempDir() + t.Setenv(fakeKeychainDirEnv, dir) + t.Setenv(fakeKeychainKind, kind) + old := locateKeychain + locateKeychain = func() (keychainTool, error) { return keychainTool{kind: kind, path: os.Args[0]}, nil } + t.Cleanup(func() { locateKeychain = old }) + return dir +} + +func withNoKeychain(t *testing.T) { + t.Helper() + old := locateKeychain + locateKeychain = func() (keychainTool, error) { return keychainTool{}, errKeychainAbsent } + t.Cleanup(func() { locateKeychain = old }) +} + +// assertNowhere fails when value appears in any file under root, except the +// paths allowed (absolute). +func assertNowhere(t *testing.T, root, value string, allowed ...string) { + t.Helper() + _ = filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() { + return nil + } + for _, a := range allowed { + if p == a { + return nil + } + } + b, _ := os.ReadFile(p) + if strings.Contains(string(b), value) { + t.Errorf("%s carries the value", p) + } + return nil + }) +} + +func TestStoreResolvesEveryHome(t *testing.T) { + for _, kind := range []string{keychainMacOS, keychainSecretService} { + t.Run(kind, func(t *testing.T) { + home := t.TempDir() + withFakeKeychain(t, kind) + t.Setenv("ABCD_TEST_TOKEN_FOR_STORE", "env-"+secretValue) + tool := filepath.Join(home, ".config", "tool") + if err := os.MkdirAll(tool, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(tool, "auth.json"), []byte(`{"auth":{"token":"file-`+secretValue+`"}}`), 0o600); err != nil { + t.Fatal(err) + } + for _, c := range []struct { + name string + ch Choice + want string + }{ + {"svc.abcd", Choice{Home: HomeABCD, Value: "abcd-" + secretValue}, "abcd-" + secretValue}, + {"svc.keychain", Choice{Home: HomeKeychain, Value: "kc-" + secretValue}, "kc-" + secretValue}, + {"svc.env", Choice{Home: HomeExternal, Pointer: Pointer{Env: "ABCD_TEST_TOKEN_FOR_STORE"}}, "env-" + secretValue}, + {"svc.file", Choice{Home: HomeExternal, Pointer: Pointer{File: "~/.config/tool/auth.json", Field: "auth.token"}}, "file-" + secretValue}, + } { + changed, err := Set(home, c.name, c.ch) + if err != nil || !changed { + t.Fatalf("%s: Set = %v, %v", c.name, changed, err) + } + got, err := Store(home).Resolve(c.name) + if err != nil || got != c.want { + t.Fatalf("%s: Resolve = %v; want the value set", c.name, err) + } + where, err := Where(home, c.name) + if err != nil || where != c.ch.Home { + t.Fatalf("%s: Where = %q, %v; want %s", c.name, where, err, c.ch.Home) + } + } + }) + } +} + +// TestAnUnsetNameRefusesNamingTheWalkthrough (criterion 1): a name no home +// holds is a refusal that is ErrNotSet and names the walkthrough. +func TestAnUnsetNameRefusesNamingTheWalkthrough(t *testing.T) { + home := t.TempDir() + _, err := Store(home).Resolve("hosting.cloudflare") + if !errors.Is(err, ErrNotSet) { + t.Fatalf("err = %v, want ErrNotSet", err) + } + if !strings.Contains(err.Error(), "abcd ahoy credential hosting.cloudflare") { + t.Fatalf("the refusal does not name the walkthrough: %v", err) + } + t.Setenv("ABCD_TEST_UNSET_VAR", "") + if _, err := Set(home, "svc.env", Choice{Home: HomeExternal, Pointer: Pointer{Env: "ABCD_TEST_UNSET_VAR"}}); err == nil { + t.Fatal("a pointer at an empty variable was stored") + } +} + +// TestAValueIntoAWorkingTreeIsRefused (criterion 3): a home whose ~/.abcd +// sits inside a git working tree would put credentials.json where a commit can +// reach it, so the abcd home's write is refused and nothing is written. The +// abcd home is the one home that writes a value there. +func TestAValueIntoAWorkingTreeIsRefused(t *testing.T) { + home := t.TempDir() + if err := os.Mkdir(filepath.Join(home, ".git"), 0o700); err != nil { + t.Fatal(err) + } + _, err := Set(home, "svc", Choice{Home: HomeABCD, Value: secretValue}) + if err == nil || !strings.Contains(err.Error(), "working tree") { + t.Fatalf("err = %v, want a working-tree refusal", err) + } + if strings.Contains(err.Error(), secretValue) { + t.Fatal("the refusal echoes the value") + } + if _, statErr := os.Stat(filepath.Join(home, ".abcd")); !errors.Is(statErr, os.ErrNotExist) { + t.Fatal("~/.abcd was created") + } + // A tool file inside a working tree is refused as a pointer too. + home = t.TempDir() + repo := filepath.Join(home, "repo") + if err := os.MkdirAll(filepath.Join(repo, ".git"), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(repo, "auth.json"), []byte(`{"token":"`+secretValue+`"}`), 0o600); err != nil { + t.Fatal(err) + } + if _, err := Set(home, "svc", Choice{Home: HomeExternal, Pointer: Pointer{File: "~/repo/auth.json", Field: "token"}}); err == nil || + !strings.Contains(err.Error(), "working tree") { + t.Fatalf("a pointer into a working tree: err = %v", err) + } +} + +// TestAHomeThatIsAWorkingTreeKeepsTheOtherHomes (criterion 3): a home +// directory that is itself a git working tree (a dotfiles repository) refuses +// the abcd home, whose file holds the value, and nothing else: the keychain +// keeps its value outside the home, and the index beside it holds names and +// pointers only, scanned before every write. An external pointer at a file +// inside the working tree is still refused by the pointer's own check. +func TestAHomeThatIsAWorkingTreeKeepsTheOtherHomes(t *testing.T) { + home := t.TempDir() + if err := os.Mkdir(filepath.Join(home, ".git"), 0o700); err != nil { + t.Fatal(err) + } + withFakeKeychain(t, keychainMacOS) + if changed, err := Set(home, "svc.keychain", Choice{Home: HomeKeychain, Value: "kc-" + secretValue}); err != nil || !changed { + t.Errorf("keychain: Set = %v, %v; want it stored", changed, err) + } + if v, err := Store(home).Resolve("svc.keychain"); err != nil || v != "kc-"+secretValue { + t.Errorf("keychain: Resolve = %v; want the value set", err) + } + t.Setenv("ABCD_TEST_TOKEN_FOR_STORE", "env-"+secretValue) + if changed, err := Set(home, "svc.env", Choice{Home: HomeExternal, Pointer: Pointer{Env: "ABCD_TEST_TOKEN_FOR_STORE"}}); err != nil || !changed { + t.Errorf("external: Set = %v, %v; want the pointer stored", changed, err) + } + if v, err := Store(home).Resolve("svc.env"); err != nil || v != "env-"+secretValue { + t.Errorf("external: Resolve = %v; want the value pointed at", err) + } + _, err := Set(home, "svc.abcd", Choice{Home: HomeABCD, Value: "abcd-" + secretValue}) + if err == nil || !strings.Contains(err.Error(), "working tree") { + t.Fatalf("abcd: err = %v, want a working-tree refusal", err) + } + if _, statErr := os.Lstat(filepath.Join(home, ".abcd", StoreFileName)); !errors.Is(statErr, os.ErrNotExist) { + t.Fatal("abcd: credentials.json was written inside the working tree") + } + tool := filepath.Join(home, ".config", "tool") + if err := os.MkdirAll(tool, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(tool, "auth.json"), []byte(`{"token":"file-`+secretValue+`"}`), 0o600); err != nil { + t.Fatal(err) + } + _, err = Set(home, "svc.file", Choice{Home: HomeExternal, Pointer: Pointer{File: "~/.config/tool/auth.json", Field: "token"}}) + if err == nil || !strings.Contains(err.Error(), "points at ~/.config/tool/auth.json, which lies inside a git working tree") { + t.Fatalf("external file: err = %v, want the pointer's working-tree refusal", err) + } + assertNowhere(t, home, secretValue, filepath.Join(tool, "auth.json")) +} + +// TestOneNameCannotLandInTwoHomes: Set reads where a name is held and writes +// it under one lock, so a second Set of the same name to another home that +// lands while the first holds the lock is refused, never written beside it. +// The test holds the lock itself, records the name in the keychain home as a +// concurrent Set would, and only then lets the abcd home's Set proceed. +func TestOneNameCannotLandInTwoHomes(t *testing.T) { + home := t.TempDir() + dir := filepath.Join(home, ".abcd") + if err := os.Mkdir(dir, 0o700); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + err := fsutil.WithFileLock(filepath.Join(dir, indexLockFileName), 5*time.Second, func() error { + go func() { + _, err := Set(home, "svc", Choice{Home: HomeABCD, Value: secretValue}) + done <- err + }() + time.Sleep(300 * time.Millisecond) + return os.WriteFile(filepath.Join(dir, IndexFileName), []byte(`{"svc": {"home": "keychain"}}`+"\n"), 0o600) + }) + if err != nil { + t.Fatal(err) + } + err = <-done + if err == nil || !strings.Contains(err.Error(), "already held in the keychain home") { + t.Fatalf("err = %v, want the second home refused", err) + } + if _, statErr := os.Lstat(filepath.Join(dir, StoreFileName)); !errors.Is(statErr, os.ErrNotExist) { + t.Fatal("the abcd home was written beside the keychain home") + } +} + +// TestTheIndexWriteRunsTheScanner (criterion 3): the one file the store writes +// that must never hold a secret is scanned before it is written, and a +// secret-shaped pointer is refused without echoing it. +func TestTheIndexWriteRunsTheScanner(t *testing.T) { + home := t.TempDir() + awsShaped := "AKIA" + strings.Repeat("Q", 16) + t.Setenv(awsShaped, "some-value-000") + _, err := Set(home, "svc", Choice{Home: HomeExternal, Pointer: Pointer{Env: awsShaped}}) + if err == nil || !strings.Contains(err.Error(), "scanner") { + t.Fatalf("err = %v, want the scanner's refusal", err) + } + if strings.Contains(err.Error(), awsShaped) { + t.Fatal("the refusal echoes the secret-shaped pointer") + } + if _, statErr := os.Stat(filepath.Join(home, ".abcd", IndexFileName)); !errors.Is(statErr, os.ErrNotExist) { + t.Fatal("the index was written") + } +} + +// TestNeitherTheTreeNorTheHarnessCarriesTheValue (criterion 3): after a set in +// each home, no file under the home (a repository and the harness's settings +// inside it) carries the value; the abcd home's own file is the one allowed +// holder, and the keychain's values live outside the home altogether. +func TestNeitherTheTreeNorTheHarnessCarriesTheValue(t *testing.T) { + for _, h := range Homes() { + t.Run(h, func(t *testing.T) { + home := t.TempDir() + withFakeKeychain(t, keychainMacOS) + for _, p := range []string{".claude/settings.json", "work/repo/.claude/settings.json", "work/repo/README.md"} { + if err := os.MkdirAll(filepath.Dir(filepath.Join(home, p)), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(home, p), []byte("{}\n"), 0o600); err != nil { + t.Fatal(err) + } + } + value := h + "-" + secretValue + ch := Choice{Home: h, Value: value} + if h == HomeExternal { + t.Setenv("ABCD_TEST_TOKEN_FOR_STORE", value) + ch = Choice{Home: h, Pointer: Pointer{Env: "ABCD_TEST_TOKEN_FOR_STORE"}} + } + if _, err := Set(home, "svc", ch); err != nil { + t.Fatal(err) + } + assertNowhere(t, home, value, filepath.Join(home, ".abcd", StoreFileName)) + }) + } +} + +// TestTheKeychainValueNeverReachesAnArgv: a process listing shows argv, so the +// value is handed to the keychain command on stdin, never as an argument. +func TestTheKeychainValueNeverReachesAnArgv(t *testing.T) { + for _, kind := range []string{keychainMacOS, keychainSecretService} { + home := t.TempDir() + dir := withFakeKeychain(t, kind) + if _, err := Set(home, "svc", Choice{Home: HomeKeychain, Value: secretValue}); err != nil { + t.Fatalf("%s: %v", kind, err) + } + log, _ := os.ReadFile(filepath.Join(dir, "argv.log")) + if len(log) == 0 { + t.Fatalf("%s: the keychain command was never run", kind) + } + if strings.Contains(string(log), secretValue) || strings.Contains(string(log), hex.EncodeToString([]byte(secretValue))) { + t.Fatalf("%s: an argv carries the value:\n%s", kind, log) + } + } +} + +// TestAPlatformWithoutAKeychainOffersTheOtherHomes (scope condition): the +// keychain home refuses naming the other two, and they still work. +func TestAPlatformWithoutAKeychainOffersTheOtherHomes(t *testing.T) { + home := t.TempDir() + withNoKeychain(t) + _, err := Set(home, "svc", Choice{Home: HomeKeychain, Value: secretValue}) + if err == nil || !strings.Contains(err.Error(), "external") || !strings.Contains(err.Error(), "abcd") { + t.Fatalf("err = %v, want a refusal naming the other homes", err) + } + if _, err := Set(home, "svc", Choice{Home: HomeABCD, Value: secretValue}); err != nil { + t.Fatalf("the abcd home without a keychain: %v", err) + } +} + +// TestAStoredSecretIsNeverReplaced: a name held in one home is refused in +// another, and a different value in the same home is refused; the same value +// again is no change. +func TestAStoredSecretIsNeverReplaced(t *testing.T) { + home := t.TempDir() + withFakeKeychain(t, keychainMacOS) + if _, err := Set(home, "svc", Choice{Home: HomeKeychain, Value: secretValue}); err != nil { + t.Fatal(err) + } + if changed, err := Set(home, "svc", Choice{Home: HomeKeychain, Value: secretValue}); err != nil || changed { + t.Fatalf("the same value again = %v, %v; want no change", changed, err) + } + for _, ch := range []Choice{{Home: HomeKeychain, Value: "other-" + secretValue}, {Home: HomeABCD, Value: secretValue}} { + _, err := Set(home, "svc", ch) + if err == nil || !strings.Contains(err.Error(), "never replaces") { + t.Fatalf("%s: err = %v, want a refusal", ch.Home, err) + } + if strings.Contains(err.Error(), secretValue) { + t.Fatal("the refusal echoes the value") + } + } + if v, _ := Store(home).Resolve("svc"); v != secretValue { + t.Fatal("the stored value changed") + } +} + +// TestANameInTwoHomesIsRefused: a hand edit that puts one name in the abcd +// file and the index is ambiguous, and it is refused rather than guessed. +func TestANameInTwoHomesIsRefused(t *testing.T) { + home := t.TempDir() + withFakeKeychain(t, keychainMacOS) + if _, err := Set(home, "svc", Choice{Home: HomeKeychain, Value: secretValue}); err != nil { + t.Fatal(err) + } + writeStore(t, home, `{"svc": "other-value-000"}`, 0o600) + if _, err := Store(home).Resolve("svc"); err == nil || errors.Is(err, ErrNotSet) || !strings.Contains(err.Error(), "two homes") { + t.Fatalf("err = %v, want the two-homes refusal", err) + } +} + +func testService(verify func(context.Context, string) error) Service { + return Service{ + Name: "svc", + Unlocks: "calls to the example service", + WithoutIt: "everything but those calls", + Verify: verify, + } +} + +// TestTheWalkthroughVerifiesBeforeItStores (criterion 2): the adapter's own +// call is made with the value, and only its success stores it; the result +// never carries the value. +func TestTheWalkthroughVerifiesBeforeItStores(t *testing.T) { + withFakeKeychain(t, keychainMacOS) + for _, h := range Homes() { + home := t.TempDir() + ch := Choice{Home: h, Value: secretValue} + if h == HomeExternal { + t.Setenv("ABCD_TEST_TOKEN_FOR_STORE", secretValue) + ch = Choice{Home: h, Pointer: Pointer{Env: "ABCD_TEST_TOKEN_FOR_STORE"}} + } + var seen string + _, err := Walk(context.Background(), home, testService(func(_ context.Context, v string) error { + seen = v + return errors.New("the service refused the credential") + }), ch) + if err == nil || seen != secretValue { + t.Fatalf("%s: a failed verification = %v (verified with the value: %v)", h, err, seen == secretValue) + } + if _, rerr := Store(home).Resolve("svc"); !errors.Is(rerr, ErrNotSet) { + t.Fatalf("%s: a failed verification stored the credential", h) + } + res, err := Walk(context.Background(), home, testService(func(context.Context, string) error { return nil }), ch) + if err != nil || !res.Verified || res.Home != h || res.Name != "svc" { + t.Fatalf("%s: Walk = %+v, %v", h, res, err) + } + if v, _ := Store(home).Resolve("svc"); v != secretValue { + t.Fatalf("%s: the verified credential does not resolve", h) + } + enc, _ := json.Marshal(res) + if strings.Contains(string(enc), secretValue) { + t.Fatalf("%s: the result carries the value", h) + } + } + if _, err := Walk(context.Background(), t.TempDir(), testService(nil), Choice{Home: HomeABCD, Value: secretValue}); err == nil { + t.Fatal("a walkthrough with no verification call stored the credential") + } +} + +// TestTheWalkthroughExplainsFirst (criterion 2): what it unlocks, what works +// without it, then the three homes, the keychain recommended in the prose and +// never as a marked option. +func TestTheWalkthroughExplainsFirst(t *testing.T) { + lines := testService(nil).Explain() + joined := strings.Join(lines, "\n") + for _, want := range []string{"calls to the example service", "everything but those calls", HomesProse} { + if !strings.Contains(joined, want) { + t.Fatalf("the explanation lacks %q:\n%s", want, joined) + } + } + if !strings.Contains(HomesProse, "keychain") || !strings.Contains(HomesProse, "recommend") { + t.Fatalf("the prose does not recommend the keychain: %s", HomesProse) + } + if strings.Index(joined, "unlocks") > strings.Index(joined, HomesProse) { + t.Fatal("the homes come before what the credential unlocks") + } + for _, l := range lines { + if strings.Contains(l, "(recommended)") || strings.Contains(l, "*") { + t.Fatalf("a home is marked: %q", l) + } + } + if got := Homes(); strings.Join(got, ",") != "external,abcd,keychain" { + t.Fatalf("homes = %v", got) + } +} + +// TestTheWalkthroughRefusesBeforeItsCall: a name held elsewhere, or a pointer +// other than the one kept, is refused before the verification call, so a +// setup that cannot store its credential is never verified for nothing. +func TestTheWalkthroughRefusesBeforeItsCall(t *testing.T) { + home := t.TempDir() + t.Setenv("ABCD_TEST_TOKEN_A", secretValue) + t.Setenv("ABCD_TEST_TOKEN_B", secretValue) + if _, err := Set(home, "svc", Choice{Home: HomeExternal, Pointer: Pointer{Env: "ABCD_TEST_TOKEN_A"}}); err != nil { + t.Fatal(err) + } + called := false + svc := testService(func(context.Context, string) error { called = true; return nil }) + for _, ch := range []Choice{ + {Home: HomeExternal, Pointer: Pointer{Env: "ABCD_TEST_TOKEN_B"}}, + {Home: HomeABCD, Value: secretValue}, + } { + if _, err := Walk(context.Background(), home, svc, ch); err == nil { + t.Fatalf("%s: a second home or pointer was accepted", ch.Home) + } + if called { + t.Fatalf("%s: the verification call was made before the refusal", ch.Home) + } + } + if res, err := Walk(context.Background(), home, svc, Choice{Home: HomeExternal, Pointer: Pointer{Env: "ABCD_TEST_TOKEN_A"}}); err != nil || res.Changed { + t.Fatalf("the same pointer again = %+v, %v; want no change", res, err) + } +} diff --git a/internal/core/credential/walk.go b/internal/core/credential/walk.go new file mode 100644 index 000000000..2d85b9f07 --- /dev/null +++ b/internal/core/credential/walk.go @@ -0,0 +1,133 @@ +package credential + +// walk.go is the walkthrough (itd-2609221017023290 criterion 2): the one +// sequence every setup of a credential runs, whichever adapter asks. It is a +// function the front doors call, in itd-63's explain-then-install shape: +// first what the credential unlocks and what works without it, then the three +// homes with the keychain recommended in the prose above them, then the +// adapter's own verification call with the value, and only when that call +// succeeds, the write through Set. The front door asks the person for the +// home (the CLI on its flags, the plugin page through the host's question +// tool); a value never passes through a question, because an answer is echoed +// into a transcript or an agent's context. + +import ( + "context" + "errors" + "fmt" +) + +// HomesProse is the prose above the choice of home: the keychain is +// recommended here, and never as a marked option. +const HomesProse = "Where the credential lives is your choice of three, made once. The platform keychain is " + + "the home abcd recommends, because the secret stays in the operating system's own store rather than in a " + + "file. A setup outside abcd keeps it with a tool you already use (an environment variable, or a field of " + + "that tool's configuration file), and abcd stores only where to find it. The abcd-only home keeps it in " + + "~/.abcd/credentials.json, readable by you alone. The value never enters the harness's settings or a repository." + +// Service is one credential's walkthrough, supplied by the adapter that reads +// it. +type Service struct { + // Name is the credential's name. + Name string + // Unlocks is what the credential unlocks. + Unlocks string + // WithoutIt is what works without it. + WithoutIt string + // Verify is the adapter's own call, made with the value before it is + // stored. It must not carry the value into its error. + Verify func(ctx context.Context, value string) error +} + +// Explain is the walkthrough's explanation, in the order it is given. +func (s Service) Explain() []string { + lines := []string{ + "credential " + s.Name + " unlocks " + s.Unlocks + ".", + "Without it: " + s.WithoutIt + ".", + HomesProse, + "The homes: " + HomeExternal + " (a setup outside abcd), " + HomeABCD + " (abcd-only, on this machine), " + + HomeKeychain + " (the platform keychain).", + } + return lines +} + +// WalkResult is what a walkthrough did. It never carries the value. +type WalkResult struct { + Name string `json:"name"` + Home string `json:"home"` + Verified bool `json:"verified"` + // Changed is false when the same credential was already stored there. + Changed bool `json:"changed"` + // Wrote names what the write touched, in the tilde form: the abcd home's + // file, or the index and the keychain. Empty when nothing changed. + Wrote []string `json:"wrote"` +} + +// KeychainItem names the keychain's item for name as a surface shows it. +func KeychainItem(name string) string { + return "the platform keychain (service " + keychainService + ", account " + name + ")" +} + +// Walk runs the walkthrough for s with the chosen home: the value (or, for the +// external home, what the pointer resolves to) is verified with the adapter's +// own call, and only then stored. A name another home holds, or a different +// value in the same home, is refused before the call, so a setup that cannot +// store its credential is never verified for nothing. +func Walk(ctx context.Context, home string, s Service, c Choice) (WalkResult, error) { + if s.Verify == nil { + return WalkResult{}, fmt.Errorf("credential: the walkthrough for %s has no verification call, so nothing was written", s.Name) + } + if !nameRe.MatchString(s.Name) { + return WalkResult{}, errors.New("credential: the name is not a plain credential name") + } + held, err := Where(home, s.Name) + if err != nil { + return WalkResult{}, err + } + if held != "" && held != c.Home { + return WalkResult{}, fmt.Errorf("credential: %s is already held in the %s home, and abcd never replaces a stored secret; remove it there by hand to choose another home", s.Name, held) + } + value := c.Value + if c.Home == HomeExternal { + if value != "" { + return WalkResult{}, errors.New("credential: the external home keeps a pointer, never a value, and a value was given") + } + if value, err = resolvePointer(home, s.Name, c.Pointer); err != nil { + return WalkResult{}, err + } + } else if err := CheckValue(value); err != nil { + return WalkResult{}, err + } + if held == HomeExternal && c.Home == HomeExternal { + if err := samePointer(home, s.Name, c.Pointer); err != nil { + return WalkResult{}, err + } + } else if held == c.Home { + stored, err := Store(home).Resolve(s.Name) + if err != nil { + return WalkResult{}, err + } + if stored != value { + return WalkResult{}, fmt.Errorf("credential: the %s home already holds a different value for %s, and abcd never replaces a stored secret; remove it there by hand to store a new one", held, s.Name) + } + } + if err := s.Verify(ctx, value); err != nil { + return WalkResult{}, fmt.Errorf("%w; the verification call failed, so nothing was written", err) + } + changed, err := Set(home, s.Name, c) + if err != nil { + return WalkResult{}, fmt.Errorf("%w; the credential verified, and nothing was written", err) + } + res := WalkResult{Name: s.Name, Home: c.Home, Verified: true, Changed: changed, Wrote: []string{}} + if changed { + switch c.Home { + case HomeABCD: + res.Wrote = []string{StorePath} + case HomeKeychain: + res.Wrote = []string{KeychainItem(s.Name), IndexPath} + default: + res.Wrote = []string{IndexPath} + } + } + return res, nil +} diff --git a/internal/core/oracle/call.go b/internal/core/oracle/call.go index 1db95e719..266025a73 100644 --- a/internal/core/oracle/call.go +++ b/internal/core/oracle/call.go @@ -23,12 +23,17 @@ import ( // CallRecord is the per-call record the run record carries (criterion 5, // adr-2609221009491186 Decision 5): the provider, the model asked for and the -// model the provider reported, side by side, so a substitution is visible. It -// never carries a key, a key's name or the brief. +// model the provider reported, side by side, so a substitution is visible, and +// the name of the credential the call used. It never carries a key or the +// brief. type CallRecord struct { Provider string `json:"provider"` ModelAsked string `json:"model_asked"` ModelReported string `json:"model_reported"` + // Credential is the name of the credential the call used, never its + // value; empty for a provider that takes no key (itd-2609221017023290 + // criterion 5). + Credential string `json:"credential,omitempty"` } // CallRequest is one call: where it goes, the brief, the settings as sent and @@ -59,7 +64,11 @@ func (c *APIConfig) Call(ctx context.Context, creds credential.Source, req CallR if err != nil { return nil, CallRecord{}, err } - return complete(ctx, p.Name, p.BaseURL, key, t.Model, req.Brief, req.Settings, req.Contract, c.denylist, opts...) + payload, rec, err := complete(ctx, p.Name, p.BaseURL, key, t.Model, req.Brief, req.Settings, req.Contract, c.denylist, opts...) + if err == nil { + rec.Credential = p.Key + } + return payload, rec, err } // resolveKey resolves a provider's key by name; a keyless block resolves to @@ -69,13 +78,13 @@ func resolveKey(creds credential.Source, p Provider) (string, error) { return "", nil } if creds == nil { - creds = credential.UserMachine() + creds = credential.UserStore() } key, err := creds.Resolve(p.Key) switch { case errors.Is(err, credential.ErrNotSet): return "", fmt.Errorf("oracle adapter: provider %s names credential %q, which is not set on this machine, so no call is made; "+ - "`abcd ahoy connect` stores one (`abcd ahoy --providers` explains where it can live)", p.Name, p.Key) + "`%s` explains where it can live and stores it", p.Name, p.Key, credential.Walkthrough(p.Key)) case err != nil: return "", fmt.Errorf("oracle adapter: provider %s: %w", p.Name, err) } diff --git a/internal/core/oracle/call_test.go b/internal/core/oracle/call_test.go index e4aafcf06..a6a51198c 100644 --- a/internal/core/oracle/call_test.go +++ b/internal/core/oracle/call_test.go @@ -99,7 +99,7 @@ func TestCallSendsTheBriefToThePointedModelWithTheKeyByName(t *testing.T) { if string(payload) != `{"verdict":"keep"}` { t.Fatalf("payload = %q", payload) } - want := CallRecord{Provider: "openrouter", ModelAsked: "typesafe/jev-1.13", ModelReported: "typesafe/jev-1.13-20260915"} + want := CallRecord{Provider: "openrouter", ModelAsked: "typesafe/jev-1.13", ModelReported: "typesafe/jev-1.13-20260915", Credential: "openrouter"} if rec != want { t.Fatalf("record = %+v, want %+v", rec, want) } @@ -123,9 +123,9 @@ func TestCallRefusesAnUnsetKeyWithoutACall(t *testing.T) { f := newFx(t) f.machineConfig(`{"oracle":{"api":{"openrouter":{"base_url":"` + p.base() + `","key":"openrouter","models":["typesafe/jev-1.13"]}}}}`) c := f.loadAPI() - _, _, err := c.Call(context.Background(), credential.Machine(f.roots.Home), CallRequest{ + _, _, err := c.Call(context.Background(), credential.Store(f.roots.Home), CallRequest{ Target: Target{Provider: "openrouter", Model: "typesafe/jev-1.13"}, Contract: verdictContract}) - if err == nil || !strings.Contains(err.Error(), `"openrouter"`) || !strings.Contains(err.Error(), "abcd ahoy connect") { + if err == nil || !strings.Contains(err.Error(), `"openrouter"`) || !strings.Contains(err.Error(), credential.Walkthrough("openrouter")) { t.Fatalf("err = %v, want a refusal naming the credential and the setup", err) } if n := p.calls.Load(); n != 0 { @@ -206,9 +206,11 @@ func TestTheReceiptCarriesTheProviderCall(t *testing.T) { if !strings.Contains(string(enc), `"provider_call":null`) { t.Fatalf("harness receipt = %s", enc) } - rec := CallRecord{Provider: "openrouter", ModelAsked: "typesafe/jev-1.13", ModelReported: "typesafe/jev-1.13-20260915"} + rec := CallRecord{Provider: "openrouter", ModelAsked: "typesafe/jev-1.13", ModelReported: "typesafe/jev-1.13-20260915", Credential: "openrouter"} enc, _ = json.Marshal(r.Receipt("").WithCall(rec)) - for _, want := range []string{`"provider_call":{"provider":"openrouter","model_asked":"typesafe/jev-1.13","model_reported":"typesafe/jev-1.13-20260915"}`} { + // The run's record names the credential the call used, and never a value + // (itd-2609221017023290 criterion 5). + for _, want := range []string{`"provider_call":{"provider":"openrouter","model_asked":"typesafe/jev-1.13","model_reported":"typesafe/jev-1.13-20260915","credential":"openrouter"}`} { if !strings.Contains(string(enc), want) { t.Fatalf("receipt = %s, want %s", enc, want) } diff --git a/internal/core/oracle/connect.go b/internal/core/oracle/connect.go index 7b3bd6f4c..173649d72 100644 --- a/internal/core/oracle/connect.go +++ b/internal/core/oracle/connect.go @@ -7,13 +7,10 @@ package oracle // written into the repository or into the harness's settings, and a // verification that fails writes nothing at all. // -// Of the three homes a key may live in, this lane builds the one the interim -// credential source already reads: abcd-only, ~/.abcd/credentials.json at mode -// 0600. The environment-variable-or-external-tool home and the platform -// keychain are the credential store's (itd-2609221017023290, planned), which -// replaces the source's backing and not its interface; asked for either, the -// setup refuses naming it, before any call and any write. A fourth answer, -// none, is a local server that takes no key. +// The key is kept through the credential store's walkthrough +// (itd-2609221017023290): in one of its three homes, the external setup, the +// abcd-only file or the platform keychain, verified first with this adapter's +// own call. A fourth answer, none, is a local server that takes no key. import ( "context" @@ -21,7 +18,7 @@ import ( "errors" "fmt" "os" - "path/filepath" + "path" "time" "github.com/intentdriven/abcd/internal/adapter/openaiapi" @@ -31,25 +28,22 @@ import ( "github.com/intentdriven/abcd/internal/fsutil" ) -// The homes a provider's key may live in (the intent's Decision 4). +// The homes a provider's key may live in (the intent's Decision 4): the +// credential store's three, and none. const ( // KeyHomeExternal is a setup outside abcd (an environment variable or an - // existing tool's configuration); abcd would store only its name. - KeyHomeExternal = "external" + // existing tool's configuration); abcd stores only where it is. + KeyHomeExternal = credential.HomeExternal // KeyHomeABCD is abcd-only: the owner-only ~/.abcd/credentials.json. - KeyHomeABCD = "abcd" + KeyHomeABCD = credential.HomeABCD // KeyHomeKeychain is the platform keychain. - KeyHomeKeychain = "keychain" + KeyHomeKeychain = credential.HomeKeychain // KeyHomeNone is a server that takes no key (a local one). KeyHomeNone = "none" ) // KeyHomes returns the homes in the order the setup offers them. -func KeyHomes() []string { return []string{KeyHomeExternal, KeyHomeABCD, KeyHomeKeychain, KeyHomeNone} } - -// CredentialStoreIntent is the intent that builds the external and keychain -// homes, named by every deferral. -const CredentialStoreIntent = "itd-2609221017023290" +func KeyHomes() []string { return append(credential.Homes(), KeyHomeNone) } // ConnectRequest is one provider's setup. type ConnectRequest struct { @@ -65,8 +59,10 @@ type ConnectRequest struct { Home string // KeyName is the credential's name; "" names it after the provider. KeyName string - // Key is the value, for the abcd home. It is never echoed. + // Key is the value, for the abcd and keychain homes. It is never echoed. Key string + // Pointer is where the key is, for the external home. + Pointer credential.Pointer // Timeout bounds the verification call; 0 keeps the adapter's default. Timeout time.Duration } @@ -111,48 +107,34 @@ func Connect(ctx context.Context, req ConnectRequest) (ConnectResult, error) { return ConnectResult{}, fmt.Errorf("oracle adapter: provider %s %s", req.Provider, deniedError(m, e)) } } - if req.Home == KeyHomeABCD { - // Refused before the call, so a setup that cannot store its key is - // never billed for. - stored, err := credential.Machine(req.Roots.Home).Resolve(req.KeyName) - switch { - case errors.Is(err, credential.ErrNotSet): - case err != nil: - return ConnectResult{}, err - case stored != req.Key: - return ConnectResult{}, fmt.Errorf("oracle adapter: %s already holds a different value for %s, and abcd never replaces a stored secret; "+ - "name another credential with --key, or remove that entry by hand", credential.StorePath, req.KeyName) - } - } - var opts []openaiapi.Option if req.Timeout > 0 { opts = append(opts, openaiapi.WithTimeout(req.Timeout)) } - _, rec, err := complete(ctx, req.Provider, req.BaseURL, req.Key, req.Models[0], verifyBrief, - Settings{"max_tokens": json.RawMessage(`16`)}, nil, cfg.denylist, opts...) - if err != nil { - return ConnectResult{}, fmt.Errorf("%w; the verification call failed, so nothing was written", err) - } - res := ConnectResult{Provider: req.Provider, BaseURL: req.BaseURL, Models: append([]string(nil), req.Models...), - KeyHome: req.Home, Verified: rec} + KeyHome: req.Home} + svc := providerService(Provider{Name: req.Provider, BaseURL: req.BaseURL, Key: req.KeyName, Models: req.Models}, + cfg.denylist, &res.Verified, opts...) block := map[string]any{"base_url": req.BaseURL, "models": req.Models} - if req.Home == KeyHomeABCD { - res.KeyName = req.KeyName - block["key"] = req.KeyName - changed, err := credential.SetMachine(req.Roots.Home, req.KeyName, req.Key) - if err != nil { - return ConnectResult{}, fmt.Errorf("%w; the connection verified, and nothing was written", err) + if req.Home == KeyHomeNone { + if err := svc.Verify(ctx, ""); err != nil { + return ConnectResult{}, fmt.Errorf("%w; the verification call failed, so nothing was written", err) } - if changed { - res.Wrote = append(res.Wrote, credential.StorePath) + } else { + // The store's walkthrough refuses a key it cannot keep before the + // call, so a setup that cannot store its key is never billed for. + walked, err := credential.Walk(ctx, req.Roots.Home, svc, credential.Choice{Home: req.Home, Value: req.Key, Pointer: req.Pointer}) + if err != nil { + return ConnectResult{}, fmt.Errorf("oracle adapter: %w", err) } + res.KeyName = req.KeyName + block["key"] = req.KeyName + res.Wrote = append(res.Wrote, walked.Wrote...) } if err := writeProviderBlock(req.Roots.Home, req.Provider, block); err != nil { if len(res.Wrote) > 0 { - return ConnectResult{}, fmt.Errorf("%w; the key was stored in %s under %s, and the provider block was not written", - err, credential.StorePath, req.KeyName) + return ConnectResult{}, fmt.Errorf("%w; the key was stored in the %s home under %s, and the provider block was not written", + err, req.Home, req.KeyName) } return ConnectResult{}, err } @@ -188,16 +170,23 @@ func checkConnect(req *ConnectRequest) error { seen[m] = true } switch req.Home { - case KeyHomeExternal, KeyHomeKeychain: - return fmt.Errorf("oracle adapter: the %s home is built by the credential store (%s), which is planned and not built; "+ - "until it lands a key lives in the abcd-only home (%s, owner-only), and nothing was written", req.Home, CredentialStoreIntent, credential.StorePath) case KeyHomeNone: - if req.Key != "" { - return errors.New("oracle adapter: a key was given for a provider set up with no key; choose the abcd home to store it") + if req.Key != "" || req.Pointer != (credential.Pointer{}) { + return errors.New("oracle adapter: a key was given for a provider set up with no key; choose a home to keep it") } req.KeyName = "" return nil - case KeyHomeABCD: + case KeyHomeExternal: + if req.Key != "" { + return errors.New("oracle adapter: the external home keeps where the key is, never the key, and a key was given") + } + if req.Pointer == (credential.Pointer{}) { + return errors.New("oracle adapter: the external home needs where the key is: an environment variable, or a file and its field") + } + case KeyHomeABCD, KeyHomeKeychain: + if req.Pointer != (credential.Pointer{}) { + return fmt.Errorf("oracle adapter: the %s home keeps the key itself, and a pointer was given", req.Home) + } default: return fmt.Errorf("oracle adapter: key home %q is not one of external, abcd, keychain, none", layered.BoundKey(req.Home)) } @@ -207,8 +196,11 @@ func checkConnect(req *ConnectRequest) error { if !credential.ValidName(req.KeyName) { return fmt.Errorf("oracle adapter: key name %q is not a plain credential name", layered.BoundKey(req.KeyName)) } + if req.Home == KeyHomeExternal { + return nil + } if req.Key == "" { - return errors.New("oracle adapter: the abcd home stores a key, and none was given") + return fmt.Errorf("oracle adapter: the %s home stores a key, and none was given", req.Home) } // The store's own value check, before the call rather than after it. return credential.CheckValue(req.Key) @@ -223,7 +215,7 @@ var configLockTimeout = 5 * time.Second // writeProviderBlock sets oracle.api. in ~/.abcd/config.json, keeping // every other key, written atomically at mode 0600. The file is read, changed -// and renamed into place under its lock (fsutil.WithFileLock), so concurrent +// and renamed into place under its lock (fsutil.WithFileLockIn), so concurrent // setups never lose each other's blocks, and a block another setup wrote // after this one's check is refused rather than replaced. func writeProviderBlock(home, name string, block map[string]any) error { @@ -232,15 +224,20 @@ func writeProviderBlock(home, name string, block map[string]any) error { // The machine layer refuses a file behind a symlinked ~/.abcd, so a block // written through the link would land wherever it points (a dotfiles // checkout) and never be read back. - if err := fsutil.HomeScopeLink(home, rel); err != nil { + // ~/.abcd is created, judged and opened in one walk relative to home's + // descriptor, and the lock and the file are reached through it, so a link + // swapped in after the judgement is refused rather than written through + // (iss-2609281310017733). + dir, err := fsutil.EnsureHomeScope(home, path.Dir(rel), 0o700) + if errors.Is(err, fsutil.ErrHomeScopeSymlinked) { return fmt.Errorf("oracle adapter: the provider block was not written to %s: %v", origin, err) } - p := filepath.Join(home, filepath.FromSlash(rel)) - if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { + if err != nil { return fmt.Errorf("oracle adapter: ~/.abcd could not be created, so the provider block was not written") } - err := fsutil.WithFileLock(filepath.Join(filepath.Dir(p), configLockFileName), configLockTimeout, func() error { - return writeProviderBlockLocked(home, name, block) + defer dir.Close() + err = fsutil.WithFileLockIn(dir, configLockFileName, configLockTimeout, func() error { + return writeProviderBlockLocked(home, dir, name, block) }) switch { case errors.Is(err, fsutil.ErrLockContention): @@ -255,10 +252,9 @@ func writeProviderBlock(home, name string, block map[string]any) error { // writeProviderBlockLocked is writeProviderBlock's read, change and write, // run under the file's lock. -func writeProviderBlockLocked(home, name string, block map[string]any) error { +func writeProviderBlockLocked(home string, dir *os.Root, name string, block map[string]any) error { origin := layered.Config.MachineOrigin() rel := ".abcd/" + layered.Config.MachineRel - p := filepath.Join(home, filepath.FromSlash(rel)) root := map[string]json.RawMessage{} raw, refusal, err := fsutil.ReadHomeDeclaration(home, rel, layered.MaxFileBytes) switch { @@ -308,7 +304,7 @@ func writeProviderBlockLocked(home, name string, block map[string]any) error { if err != nil { return err } - if err := fsutil.WriteFileAtomic(p, append(body, '\n'), 0o600); err != nil { + if err := fsutil.WriteFileAtomicInRoot(dir, path.Base(rel), append(body, '\n'), 0o600); err != nil { return fmt.Errorf("oracle adapter: %s could not be written, so the provider block was not written", origin) } return nil @@ -324,10 +320,44 @@ const AdapterExplanation = "An aggregator (OpenRouter, for one) serves many vend "every delegated step runs on the host." // KeyHomesProse is the prose above the choice of the key's home (criterion 8): -// the keychain is recommended here, in the prose, and never as a marked option. -const KeyHomesProse = "Where the key lives is your choice of three. The platform keychain is the safest home, " + - "because the secret stays in the operating system's own store rather than in a file. A setup outside abcd " + - "keeps it with a tool you already use, and abcd stores only its name. The abcd-only home keeps it in " + - "~/.abcd/credentials.json, readable by you alone. This version stores a key in the abcd-only home; the other " + - "two arrive with the credential store (" + CredentialStoreIntent + "). The key never enters the harness's " + - "settings or the repository." +// the credential store's, which recommends the keychain in the prose and never +// as a marked option. +const KeyHomesProse = credential.HomesProse + +// providerService is the credential walkthrough's service for a provider's +// key: what it unlocks, what works without it, and the adapter's own +// verification call, one short exchange with the first model listed. The +// call's record is written to rec when rec is non-nil. +func providerService(p Provider, denylist []DenyEntry, rec *CallRecord, opts ...openaiapi.Option) credential.Service { + return credential.Service{ + Name: p.Key, + Unlocks: "calls to the model provider " + p.Name + " at " + p.BaseURL + ", for the models its allowlist names", + WithoutIt: "everything: every delegated step runs on the host", + Verify: func(ctx context.Context, key string) error { + _, r, err := complete(ctx, p.Name, p.BaseURL, key, p.Models[0], verifyBrief, + Settings{"max_tokens": json.RawMessage(`16`)}, nil, denylist, opts...) + r.Credential = p.Key + if rec != nil { + *rec = r + } + return err + }, + } +} + +// CredentialService is the walkthrough's service for the credential name, when +// a configured provider names it as its key: the walkthrough then verifies a +// key with that provider's own call. A name no provider names is not the +// adapter's. +func CredentialService(roots layered.Roots, name string) (credential.Service, bool, error) { + cfg, err := LoadAPI(roots) + if err != nil { + return credential.Service{}, false, err + } + for _, p := range cfg.Providers() { + if p.Key == name && len(p.Models) > 0 { + return providerService(p, cfg.denylist, nil), true, nil + } + } + return credential.Service{}, false, nil +} diff --git a/internal/core/oracle/connect_test.go b/internal/core/oracle/connect_test.go index c2b7f868d..863198601 100644 --- a/internal/core/oracle/connect_test.go +++ b/internal/core/oracle/connect_test.go @@ -49,7 +49,7 @@ func TestConnectVerifiesThenWritesTheBlockAndTheKey(t *testing.T) { if p.auth.Load() != "Bearer "+callKey { t.Fatal("the verification call did not carry the key") } - want := CallRecord{Provider: "openrouter", ModelAsked: "typesafe/jev-1.13", ModelReported: "typesafe/jev-1.13-20260915"} + want := CallRecord{Provider: "openrouter", ModelAsked: "typesafe/jev-1.13", ModelReported: "typesafe/jev-1.13-20260915", Credential: "openrouter"} if res.Verified != want || res.KeyName != "openrouter" || res.KeyHome != KeyHomeABCD { t.Fatalf("result = %+v", res) } @@ -120,26 +120,62 @@ func TestConnectWritesNothingWhenVerificationFails(t *testing.T) { } } -// TestConnectDefersTheOtherHomes: the environment-variable and keychain homes -// are the credential store's (itd-2609221017023290); asked for, they are -// refused naming it, before any call and any write. -func TestConnectDefersTheOtherHomes(t *testing.T) { - for _, home := range []string{KeyHomeExternal, KeyHomeKeychain} { - p := newProvFake(t, 200, chat("m", "ok")) - f := newFx(t) - req := connectReq(f, p.base()) - req.Home = home - _, err := Connect(context.Background(), req) - if err == nil || !strings.Contains(err.Error(), "itd-2609221017023290") { - t.Fatalf("%s: err = %v, want the deferral named", home, err) - } - if p.calls.Load() != 0 { - t.Fatalf("%s: a call was made", home) - } - if _, statErr := os.Lstat(filepath.Join(f.roots.Home, ".abcd")); !errors.Is(statErr, os.ErrNotExist) { - t.Fatalf("%s: something was written", home) +// TestConnectKeepsAKeyInTheExternalHome: the external home stores where the +// key is, never the key, verifies with the key it points at, and the provider +// then resolves it through the store by name (itd-2609221017023290). +func TestConnectKeepsAKeyInTheExternalHome(t *testing.T) { + p := newProvFake(t, 200, chat("typesafe/jev-1.13-20260915", "ok")) + f := newFx(t) + t.Setenv("ABCD_TEST_PROVIDER_KEY", callKey) + req := connectReq(f, p.base()) + req.Home, req.Key = KeyHomeExternal, "" + req.Pointer = credential.Pointer{Env: "ABCD_TEST_PROVIDER_KEY"} + res, err := Connect(context.Background(), req) + if err != nil { + t.Fatalf("Connect: %v", err) + } + if p.auth.Load() != "Bearer "+callKey { + t.Fatal("the verification call did not carry the key the pointer names") + } + if res.KeyHome != KeyHomeExternal || res.KeyName != "openrouter" || res.Verified.Credential != "openrouter" { + t.Fatalf("result = %+v", res) + } + if !reflect.DeepEqual(res.Wrote, []string{credential.IndexPath, "~/.abcd/config.json"}) { + t.Fatalf("wrote = %v", res.Wrote) + } + if _, err := os.Lstat(machineFile(f, credential.StoreFileName)); !errors.Is(err, os.ErrNotExist) { + t.Fatal("the external home wrote the abcd-only store") + } + for _, name := range []string{"config.json", credential.IndexFileName} { + raw, _ := os.ReadFile(machineFile(f, name)) + if strings.Contains(string(raw), callKey) { + t.Fatalf("%s carries the key", name) } } + if v, err := credential.Store(f.roots.Home).Resolve("openrouter"); err != nil || v != callKey { + t.Fatal("the key does not resolve through the store after the setup") + } +} + +// TestConnectRefusesAPointerAtNothing: an external pointer that resolves to +// nothing is refused before any call and names the walkthrough. +func TestConnectRefusesAPointerAtNothing(t *testing.T) { + p := newProvFake(t, 200, chat("m", "ok")) + f := newFx(t) + t.Setenv("ABCD_TEST_PROVIDER_KEY", "") + req := connectReq(f, p.base()) + req.Home, req.Key = KeyHomeExternal, "" + req.Pointer = credential.Pointer{Env: "ABCD_TEST_PROVIDER_KEY"} + _, err := Connect(context.Background(), req) + if err == nil || !errors.Is(err, credential.ErrNotSet) { + t.Fatalf("err = %v, want not set", err) + } + if p.calls.Load() != 0 { + t.Fatal("a call was made") + } + if _, statErr := os.Lstat(filepath.Join(f.roots.Home, ".abcd")); !errors.Is(statErr, os.ErrNotExist) { + t.Fatal("something was written") + } } // TestConnectRefusesBeforeAnyCall: every fault the read would refuse is @@ -154,6 +190,9 @@ func TestConnectRefusesBeforeAnyCall(t *testing.T) { "bad provider name": func(r *ConnectRequest) { r.Provider = "Open Router" }, "plain http": func(r *ConnectRequest) { r.BaseURL = "http://api.example.com/v1" }, "abcd home no key": func(r *ConnectRequest) { r.Key = "" }, + "keychain no key": func(r *ConnectRequest) { r.Home, r.Key = KeyHomeKeychain, "" }, + "external with a key": func(r *ConnectRequest) { r.Home = KeyHomeExternal }, + "external no pointer": func(r *ConnectRequest) { r.Home, r.Key = KeyHomeExternal, "" }, "none home with key": func(r *ConnectRequest) { r.Home = KeyHomeNone }, "unknown home": func(r *ConnectRequest) { r.Home = "vault" }, "bad key name": func(r *ConnectRequest) { r.KeyName = "../x" }, diff --git a/internal/core/oracle/home_link_test.go b/internal/core/oracle/home_link_test.go index bc1ed5ca9..a9f8d5d9f 100644 --- a/internal/core/oracle/home_link_test.go +++ b/internal/core/oracle/home_link_test.go @@ -8,6 +8,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/intentdriven/abcd/internal/fsutil" ) // TestConnectRefusesASymlinkedAbcdHome: the provider block (and, in the abcd @@ -40,3 +42,43 @@ func TestConnectRefusesASymlinkedAbcdHome(t *testing.T) { }) } } + +// TestProviderBlockIsNotWrittenThroughAnAbcdHomeSwappedForALink is +// iss-2609281310017733: ~/.abcd is a real directory when the provider block's +// writer judges it and a symlink into a dotfiles checkout by the time it +// writes. The lock and the file are reached through the descriptor of the +// directory that was judged, so the write is refused, names the link, and +// leaves the checkout as it was. +func TestProviderBlockIsNotWrittenThroughAnAbcdHomeSwappedForALink(t *testing.T) { + home := t.TempDir() + abcd := filepath.Join(home, ".abcd") + dotfiles := filepath.Join(home, "dotfiles", "abcd") + for _, dir := range []string{abcd, dotfiles} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + swapped := false + t.Cleanup(fsutil.SwapHomeScopeVettedForTest(func(dir string) { + if swapped || dir != abcd { + return + } + swapped = true + if err := os.Rename(abcd, filepath.Join(home, "moved-aside")); err != nil { + t.Fatalf("swap: %v", err) + } + if err := os.Symlink(dotfiles, abcd); err != nil { + t.Fatalf("swap: %v", err) + } + })) + err := writeProviderBlock(home, "desk", map[string]any{"base_url": "http://127.0.0.1:1"}) + if !swapped { + t.Fatal("the writer never judged ~/.abcd, so the race was not staged") + } + if err == nil || !strings.Contains(err.Error(), "~/.abcd is a symlink") { + t.Errorf("err = %v, want a refusal naming the symlinked ~/.abcd", err) + } + if entries, _ := os.ReadDir(dotfiles); len(entries) != 0 { + t.Fatalf("the provider block went through the swapped link: the checkout holds %q", entries[0].Name()) + } +} diff --git a/internal/core/reading/assemble.go b/internal/core/reading/assemble.go index b245e34b2..617d5f754 100644 --- a/internal/core/reading/assemble.go +++ b/internal/core/reading/assemble.go @@ -85,6 +85,13 @@ type AssembleResult struct { OutDir string `json:"out_dir,omitempty"` Artefacts []string `json:"artefacts"` Written bool `json:"written"` + // Ingestable says whether `reading ingest` can find this run. The ingest + // resolves a run's manifest only under DefaultRunDir by its run id, so an + // assembly written anywhere else (an operator-named --out) is an inspection + // copy that no ingest will ever prove, and a dry run wrote nothing to find. + // It is reported at assembly time because the cost otherwise lands after + // the reading has been commissioned and returned (iss-2609091648476051). + Ingestable bool `json:"ingestable"` Bundle Bundle `json:"-"` Manifest Manifest `json:"-"` @@ -649,9 +656,21 @@ func Assemble(req AssembleRequest) (AssembleResult, error) { } res.Written = true res.Artefacts = []string{BundleFileName, ManifestFileName} + res.Ingestable = isParkedRunDir(req.RepoRoot, outDir, runID) return res, notExercisedError(notExercised, candidateRun) } +// isParkedRunDir reports whether outDir is the directory `reading ingest` +// resolves runID's manifest from: DefaultRunDir/ under the repository +// root. A relative outDir is taken against the root, as writeArtefacts takes it. +func isParkedRunDir(repoRoot, outDir, runID string) bool { + dir := filepath.FromSlash(outDir) + if !filepath.IsAbs(dir) { + dir = filepath.Join(repoRoot, dir) + } + return filepath.Clean(dir) == filepath.Join(repoRoot, filepath.FromSlash(DefaultRunDir), runID) +} + // bundleStamp is the reading kind's per-run context stamp: the run and the // sha256 over the bundle's item set as the canonical encoder serialises it. func bundleStamp(runID string, items []BundleItem) (string, error) { diff --git a/internal/core/reading/assemble_test.go b/internal/core/reading/assemble_test.go index 51769c689..7caaf05b5 100644 --- a/internal/core/reading/assemble_test.go +++ b/internal/core/reading/assemble_test.go @@ -456,6 +456,43 @@ func TestAssembleDefaultsToTheLocalTier(t *testing.T) { } } +// TestAssembleSaysWhetherTheRunCanBeIngested holds the assembly-time report +// of the one dead end the verb can walk into: the ingest resolves a manifest +// only under the default run directory by its run id, so a run written to an +// operator-named directory, or not written at all, can never be ingested, and +// the result says so before the reading is commissioned (iss-2609091648476051). +func TestAssembleSaysWhetherTheRunCanBeIngested(t *testing.T) { + root := fixtureRepo(t) + + def, err := Assemble(AssembleRequest{RepoRoot: root, Position: PositionWidening, Target: "HEAD"}) + if err != nil { + t.Fatalf("assemble into the default run directory: %v", err) + } + if !def.Ingestable { + t.Errorf("a run parked in the default run directory reads as not ingestable") + } + + named, err := Assemble(AssembleRequest{ + RepoRoot: root, Position: PositionWidening, Target: "HEAD", + OutDir: filepath.Join(t.TempDir(), "run"), + }) + if err != nil { + t.Fatalf("assemble into a named directory: %v", err) + } + if !named.Written || named.Ingestable { + t.Errorf("a run written to a named directory: written=%v ingestable=%v, want written and not ingestable", + named.Written, named.Ingestable) + } + + dry, err := Assemble(AssembleRequest{RepoRoot: root, Position: PositionWidening, Target: "HEAD", DryRun: true}) + if err != nil { + t.Fatalf("dry-run assemble: %v", err) + } + if dry.Ingestable { + t.Errorf("a dry run that wrote nothing reads as ingestable") + } +} + // treeSnapshot lists every tracked and untracked path with its size, so a test // can assert an assembly wrote nothing. func treeSnapshot(t *testing.T, root string) string { diff --git a/internal/core/site/credential.go b/internal/core/site/credential.go new file mode 100644 index 000000000..e853f8a2c --- /dev/null +++ b/internal/core/site/credential.go @@ -0,0 +1,63 @@ +package site + +// credential.go is the site setup's half of the credential walkthrough +// (itd-2609221017023290): what the hosting credential unlocks, what works +// without it, and the hosting adapter's own verification call. The store and +// the walkthrough itself are internal/core/credential's; the setup reads the +// credential through credential.Store by name and never any other way. + +import ( + "context" + "strings" + + "github.com/intentdriven/abcd/internal/adapter/hosting" + "github.com/intentdriven/abcd/internal/core/credential" +) + +// CredentialService is the walkthrough's service for name when adapter reads +// it: false for a name the site setup does not read. +func CredentialService(name string, adapter hosting.Adapter) (credential.Service, bool) { + if adapter == nil || name != adapter.CredentialName() { + return credential.Service{}, false + } + return credential.Service{ + Name: name, + Unlocks: "`abcd site setup`'s host stage on " + adapter.Name() + ": creating the site's host, routing its " + + "domain to it and reporting the live address", + WithoutIt: "the rest of `abcd site setup` (the files it writes and the forge environments) runs, and the host " + + "stage stops at no_credential, contacting nothing and naming what remains", + Verify: func(ctx context.Context, token string) error { + err := adapter.Connect(token).Verify(ctx) + if err != nil && token != "" && strings.Contains(err.Error(), token) { + return scrubbed{err.Error(), token} + } + return err + }, + }, true +} + +// CredentialServiceFor is CredentialService over every hosting provider the +// site setup knows. +func CredentialServiceFor(name string) (credential.Service, bool) { + for _, a := range adapters { + if svc, ok := CredentialService(name, a); ok { + return svc, true + } + } + return credential.Service{}, false +} + +// CredentialNames are the credentials the site setup reads, one per hosting +// provider. +func CredentialNames() []string { + out := make([]string, 0, len(adapters)) + for _, a := range adapters { + out = append(out, a.CredentialName()) + } + return out +} + +// scrubbed is an error whose message had the credential replaced. +type scrubbed struct{ msg, token string } + +func (s scrubbed) Error() string { return strings.ReplaceAll(s.msg, s.token, "[credential]") } diff --git a/internal/core/site/credential_service_test.go b/internal/core/site/credential_service_test.go new file mode 100644 index 000000000..189246b7b --- /dev/null +++ b/internal/core/site/credential_service_test.go @@ -0,0 +1,71 @@ +package site + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/intentdriven/abcd/internal/adapter/hosting/cloudflare" + "github.com/intentdriven/abcd/internal/adapter/hosting/cloudflare/cloudflaretest" + "github.com/intentdriven/abcd/internal/core/credential" +) + +// The site setup reads its hosting credential through the store by name +// (itd-2609221017023290 criterion 4), and its walkthrough verifies with the +// hosting adapter's own call (criterion 2). + +// TestSetupReadsTheHostingCredentialThroughTheStore: a credential kept in the +// external home (an environment variable) reaches the host through +// credential.Store, and the run's record names the credential and never the +// value (criterion 5). +func TestSetupReadsTheHostingCredentialThroughTheStore(t *testing.T) { + home := t.TempDir() + t.Setenv("ABCD_TEST_CLOUDFLARE_VALUE", cloudflaretest.Token) + if _, err := credential.Set(home, cloudflare.CredentialName, credential.Choice{ + Home: credential.HomeExternal, Pointer: credential.Pointer{Env: "ABCD_TEST_CLOUDFLARE_VALUE"}, + }); err != nil { + t.Fatal(err) + } + h := newHarness(t) + h.cred = credential.Store(home) + res := h.run(t) + if res.Host.Status != HostWritten { + t.Fatalf("host = %+v, want written", res.Host) + } + if res.Host.Credential != cloudflare.CredentialName { + t.Fatalf("the host outcome names credential %q, want %q", res.Host.Credential, cloudflare.CredentialName) + } + enc, _ := json.Marshal(res) + if strings.Contains(string(enc), cloudflaretest.Token) { + t.Fatal("the result carries the credential's value") + } +} + +// TestTheHostingWalkthroughVerifiesWithTheAdaptersOwnCall: the service the +// walkthrough runs for the hosting credential explains itself and verifies a +// token by the provider's own read, refusing one the provider refuses. +func TestTheHostingWalkthroughVerifiesWithTheAdaptersOwnCall(t *testing.T) { + srv := cloudflaretest.New(t) + svc, ok := CredentialService(cloudflare.CredentialName, cloudflare.Adapter{BaseURL: srv.URL}) + if !ok { + t.Fatal("the hosting credential has no walkthrough") + } + if svc.Name != cloudflare.CredentialName || svc.Unlocks == "" || svc.WithoutIt == "" { + t.Fatalf("service = %+v", svc) + } + if err := svc.Verify(context.Background(), cloudflaretest.Token); err != nil { + t.Fatalf("a good token: %v", err) + } + if n := len(srv.CallLog()); n != 1 { + t.Fatalf("verification made %d calls, want one: %v", n, srv.CallLog()) + } + const bad = "cf-wrong-token-1111111111111111111111111111" + err := svc.Verify(context.Background(), bad) + if err == nil || strings.Contains(err.Error(), bad) { + t.Fatalf("a refused token: err = %v (never carrying the token)", err) + } + if _, ok := CredentialService("openrouter", cloudflare.Adapter{BaseURL: srv.URL}); ok { + t.Fatal("a name the site does not read has a site walkthrough") + } +} diff --git a/internal/core/site/setup.go b/internal/core/site/setup.go index b4c657657..a080b6df6 100644 --- a/internal/core/site/setup.go +++ b/internal/core/site/setup.go @@ -152,13 +152,16 @@ type EnvironmentOutcome struct { // HostOutcome is the host stage's result. type HostOutcome struct { - Provider string `json:"provider"` - Name string `json:"name"` - Domain string `json:"domain,omitempty"` - Status string `json:"status"` - Changes []string `json:"changes,omitempty"` - Address string `json:"address,omitempty"` - Detail string `json:"detail,omitempty"` + Provider string `json:"provider"` + Name string `json:"name"` + Domain string `json:"domain,omitempty"` + // Credential is the name of the credential the stage resolved, never + // its value (itd-2609221017023290 criterion 5). + Credential string `json:"credential"` + Status string `json:"status"` + Changes []string `json:"changes,omitempty"` + Address string `json:"address,omitempty"` + Detail string `json:"detail,omitempty"` } // SetupResult is what the verb did, stage by stage, and what remains. @@ -297,8 +300,9 @@ func Setup(req SetupRequest) (SetupResult, error) { changed = true } if hc.Status == HostNoCredential { - step := fmt.Sprintf("store a %s API token under the name %s in %s (mode 0600) and re-run `abcd site setup`, "+ - "or create the host %s", adapter.Name(), adapter.CredentialName(), credential.StorePath, s.Name) + step := fmt.Sprintf("store a %s API token under the name %s with `%s` (it explains where the token can live and "+ + "verifies it) and re-run `abcd site setup`, or create the host %s", adapter.Name(), adapter.CredentialName(), + credential.Walkthrough(adapter.CredentialName()), s.Name) if s.Domain != "" { step += " and route " + s.Domain + " to it" } @@ -694,10 +698,10 @@ func containsPolicy(have []BranchPolicy, want BranchPolicy) bool { // setupHost is the host stage. func setupHost(ctx context.Context, adapter hosting.Adapter, s hosting.Site, req SetupRequest) (out HostOutcome, declined, refused bool) { - out = HostOutcome{Provider: adapter.Name(), Name: s.Name, Domain: s.Domain} + out = HostOutcome{Provider: adapter.Name(), Name: s.Name, Domain: s.Domain, Credential: adapter.CredentialName()} src := req.Credentials if src == nil { - src = credential.UserMachine() + src = credential.UserStore() } token, err := src.Resolve(adapter.CredentialName()) if errors.Is(err, credential.ErrNotSet) { diff --git a/internal/core/site/setup_test.go b/internal/core/site/setup_test.go index 460d26721..95e553982 100644 --- a/internal/core/site/setup_test.go +++ b/internal/core/site/setup_test.go @@ -260,7 +260,7 @@ func TestSetupWithoutACredentialWritesTheRepositoryHalfAndSaysWhatRemains(t *tes } remaining := strings.Join(res.Remaining, "\n") for _, want := range []string{ - cloudflare.CredentialName, credential.StorePath, + cloudflare.CredentialName, credential.Walkthrough(cloudflare.CredentialName), "gh secret set CLOUDFLARE_API_TOKEN --env site --repo example-owner/example-site", "gh secret set CLOUDFLARE_ACCOUNT_ID --env site --repo example-owner/example-site", "git add", diff --git a/internal/core/statusline/settings.go b/internal/core/statusline/settings.go index 69f8d4e7a..27e4f97cd 100644 --- a/internal/core/statusline/settings.go +++ b/internal/core/statusline/settings.go @@ -40,6 +40,7 @@ import ( "errors" "fmt" "os" + pathpkg "path" "path/filepath" "sort" @@ -285,6 +286,13 @@ func refusedPresence(why string, fallback Pair) string { "; the default " + fallback.Foreground + " on " + fallback.Background + " renders instead" } +// settingsDirRel and settingsLeaf are SettingsRelPath's directory and file, +// in the slash form fsutil.OpenHomeScope and an *os.Root take. +var ( + settingsDirRel = pathpkg.Dir(SettingsRelPath) + settingsLeaf = pathpkg.Base(SettingsRelPath) +) + // ReadSettingsFile performs the trust-boundary read of the user-level setting // at path. It is the ONE reader of that file: Load reads through it to render // the row, and ahoy's install and uninstall steps read through it to record @@ -308,8 +316,10 @@ func refusedPresence(why string, fallback Pair) string { // // The guard is the one the two sibling home-scoped declarations use // (rules.trustedRootDeclared, history.localDeclared): lstat first, the three -// refusals above, then fsutil.ReadGuarded under the byte cap — one open, -// O_NOFOLLOW, size-checked against both the fstat and the bytes read. A file +// refusals above, then fsutil.ReadGuardedInRoot under the byte cap, relative +// to the descriptor of the ~/.abcd fsutil.OpenHomeScope judged — a symlinked +// leaf refused, the descriptor confirmed to be the file lstat'd, and the size +// checked against both the fstat and the bytes read. A file // reached through a symlinked ~/.abcd is not the caller's word either // (fsutil.HomeScopeLink, the rule the rules loader applies to rules.json), so // the file is named by the home it lives in rather than by a path. @@ -333,7 +343,21 @@ func ReadSettingsFile(home string) (raw []byte, why string, err error) { case err != nil: return nil, "it is not owned by this session's uid", nil } - raw, err = fsutil.ReadGuarded(path, maxSettingsBytes) + // The bytes are read through the descriptor of the ~/.abcd that was + // judged (fsutil.OpenHomeScope), never by the path again, so a link + // swapped in after the check above is refused rather than read through + // (iss-2609281310017733). + dir, err := fsutil.OpenHomeScope(home, settingsDirRel) + switch { + case errors.Is(err, fsutil.ErrHomeScopeSymlinked): + return nil, err.Error(), nil + case os.IsNotExist(err): + return nil, "", nil + case err != nil: + return nil, "", fmt.Errorf("statusline: reading %s: %s", SettingsDisplay, termsafe.Sanitize(err.Error())) + } + defer dir.Close() + raw, err = fsutil.ReadGuardedInRoot(dir, settingsLeaf, maxSettingsBytes) switch { case err == nil: return raw, "", nil diff --git a/internal/core/surface/sentences.go b/internal/core/surface/sentences.go index 0e97d5465..ee60d8a19 100644 --- a/internal/core/surface/sentences.go +++ b/internal/core/surface/sentences.go @@ -27,7 +27,9 @@ var sentences = map[string]string{ "abcd ahoy": "Detect abcd's install state and list its gaps, or report one mode a flag names: " + "Writes nothing; refuses any argument or two modes at once.", "abcd ahoy connect": "Verify a model provider with one call, then configure it: " + - "Writes its block and its key under ~/.abcd/; refuses a key typed at a terminal.", + "Writes its block under ~/.abcd/ and its key to the home chosen; refuses a key typed at a terminal.", + "abcd ahoy credential": "List the credentials abcd reads, explain one, or verify and store it: " + + "Writes the chosen home only with --home; refuses a value the adapter's call fails.", "abcd ahoy doctor": "Report every install gap, user-scope state included: " + "Writes nothing; refuses any argument.", "abcd ahoy install": "Apply the install gaps the detection finds: " + diff --git a/internal/fsutil/fsutil.go b/internal/fsutil/fsutil.go index 8ac720326..ca18dd1fc 100644 --- a/internal/fsutil/fsutil.go +++ b/internal/fsutil/fsutil.go @@ -30,6 +30,7 @@ import ( "crypto/rand" "encoding/hex" "errors" + "fmt" "io" "os" "path" @@ -137,6 +138,10 @@ const ( // DeclarationBehindSymlink: the file is there, but a directory between the // home and it (~/.abcd first) is a symlink (ReadHomeDeclaration only). DeclarationBehindSymlink + // DeclarationExposed: the opened file's mode carries a permission bit its + // reader denies — a secret group or other can read + // (ReadHomeDeclarationDenying only). The error is a *DeclarationModeError. + DeclarationExposed ) // ErrDeclarationWritable and ErrDeclarationForeignOwner are the two guards that @@ -151,6 +156,18 @@ var ( ErrDeclarationSwapped = errors.New("fsutil: declaration was replaced between its vetting and its read") ) +// DeclarationModeError is the error of a DeclarationExposed refusal: Perm is +// the permission bits of the file that was opened, judged on its own +// descriptor, so a caller can name the mode it refused and the chmod that +// repairs it. +type DeclarationModeError struct { + Perm os.FileMode +} + +func (e *DeclarationModeError) Error() string { + return fmt.Sprintf("fsutil: declaration's mode %04o carries a permission bit its reader refuses", uint32(e.Perm)) +} + // ownerUID is the package's own view of OwnerUID, held as a var for the same // reason caseFoldingFS is: the foreign-owner branch cannot be provoked on a host // where the test process can create only its own files, so substituting the diff --git a/internal/fsutil/home.go b/internal/fsutil/home.go index 15190450e..9caed88f5 100644 --- a/internal/fsutil/home.go +++ b/internal/fsutil/home.go @@ -2,10 +2,12 @@ package fsutil import ( "errors" + "io" "os" "path" "path/filepath" "strings" + "syscall" ) // ErrHomeScopeSymlinked is the refusal for a home-scoped path whose DIRECTORY @@ -39,6 +41,13 @@ func (e *HomeScopeLinkError) Unwrap() error { return ErrHomeScopeSymlinked } // uid cannot search, is not a link and ends the walk: nothing below it can be // reached through a link that is not there. // +// It judges by path, so it answers for the layout and not for a race: a +// caller that must hold the answer through its use — every reader and writer of +// a file there — reaches the file through OpenHomeScope or EnsureHomeScope, +// which judge each level the same way and keep the descriptor of the directory +// judged. A caller may still call HomeScopeLink first to refuse early, before +// any other work, in the same words. +// // It exists because every home-scoped primitive guards the LEAF (O_NOFOLLOW, // an Lstat of the file) and resolves the directories above it through the // kernel, which follows a link without comment. A ~/.abcd symlinked into a @@ -83,11 +92,156 @@ func HomeScopeLink(home, rel string) error { return nil } +// ErrHomeScopeSwapped is the refusal for a directory of a home-scoped path that +// was a real directory when judged and was something else by the time it was +// opened: the descriptor OpenHomeScope obtained is not the directory its Lstat +// vetted, and that directory is not a symlink now either (a symlink found there +// is refused as a *HomeScopeLinkError, which names it). +var ErrHomeScopeSwapped = errors.New("fsutil: a directory of this home-scoped path was replaced while it was opened") + +// homeScopeVetted runs between OpenHomeScope's Lstat of one directory level and +// its open of that level. It does nothing in production; it is a var so a +// detector can swap the level for a symlink inside that window, which a real +// race cannot be relied on to hit, and so prove the open refuses what it did +// not vet. dir is the level's full path. +var homeScopeVetted = func(dir string) {} + +// SwapHomeScopeVettedForTest substitutes the hook that runs between a home +// scope level's vetting Lstat and its open, and returns the restore. It is +// exported because the writers whose refusal needs proving live in other +// packages (credential, oracle, ahoy). Tests only; never called in production +// code, and never safe to call from a parallel test. +func SwapHomeScopeVettedForTest(fn func(dir string)) (restore func()) { + prev := homeScopeVetted + homeScopeVetted = fn + return func() { homeScopeVetted = prev } +} + +// OpenHomeScope opens the directory dir below home (a slash path, ".abcd" or +// ".abcd/history") as an *os.Root, walking it one level at a time relative to +// the descriptor of the level above, so HomeScopeLink's rule holds against a +// race and not only against a layout. It is the descriptor form of that rule: +// HomeScopeLink judges each level by path and the caller then reaches the file +// by path again, so a process running as the same uid that swaps ~/.abcd for a +// symlink between the judgement and the use reads or writes through the link +// (iss-2609281310017733). Here every use goes through the returned root, and +// the root is the very directory each level's judgement was made about. +// +// Each level is judged and then opened: Lstat relative to the level above +// (never following), a symlink refused with a *HomeScopeLinkError naming it, a +// non-directory refused with ErrNotRealDir, then the level opened relative to +// the same descriptor and confirmed with os.SameFile to be the directory the +// Lstat vetted. The confirmation is what os.Root alone cannot give: it follows +// a symlink that stays inside the root, and a dotfiles ~/.abcd usually points +// inside home. A level replaced between its Lstat and its open is refused — as +// a *HomeScopeLinkError when a symlink stands there now, and as +// ErrHomeScopeSwapped otherwise. The guarantee is the one an openat with +// O_NOFOLLOW gives, which the standard library does not expose on every +// platform abcd builds for: the directory held is the one that stood at that +// name, a real directory, when it was vetted; a rename of it afterwards moves +// it without changing what the root refers to. +// +// home itself is opened by path and never judged, for HomeScopeLink's reason +// (decision 2): a home reached through a link is the machine's layout. dir is +// held to ValidRelPath, or is "." for home itself. An absent level returns the +// Lstat's error, which os.IsNotExist recognises. The caller closes the root. +func OpenHomeScope(home, dir string) (*os.Root, error) { + return openHomeScope(home, dir, false, 0) +} + +// EnsureHomeScope is OpenHomeScope for a writer: each missing level is created +// at perm (a single mkdir relative to the level above, which never follows a +// link at the name it creates) before it is judged and opened, so the walk +// that creates ~/.abcd is the same walk that proves it. It is what a writer +// uses in place of os.MkdirAll, which follows a symlinked ~/.abcd and creates +// under its target. A level that already exists keeps its mode. +func EnsureHomeScope(home, dir string, perm os.FileMode) (*os.Root, error) { + return openHomeScope(home, dir, true, perm) +} + +func openHomeScope(home, dir string, create bool, perm os.FileMode) (*os.Root, error) { + if dir != "." && !ValidRelPath(dir) { + return nil, &os.PathError{Op: "openhomescope", Path: dir, Err: os.ErrInvalid} + } + cur, err := os.OpenRoot(home) + if err != nil { + return nil, err + } + if dir == "." { + return cur, nil + } + full := home + shown := "~" + for _, part := range strings.Split(dir, "/") { + full = filepath.Join(full, part) + shown += "/" + part + next, err := openHomeScopeLevel(cur, part, full, shown, create, perm) + cur.Close() + if err != nil { + return nil, err + } + cur = next + } + return cur, nil +} + +// openHomeScopeLevel is one level of openHomeScope: part, inside parent, judged +// and then opened as the directory that was judged. +func openHomeScopeLevel(parent *os.Root, part, full, shown string, create bool, perm os.FileMode) (*os.Root, error) { + if create { + if err := parent.Mkdir(part, perm); err != nil && !errors.Is(err, os.ErrExist) { + return nil, err + } + } + fi, err := parent.Lstat(part) + if err != nil { + return nil, err + } + if fi.Mode()&os.ModeSymlink != 0 { + return nil, &HomeScopeLinkError{Link: shown} + } + if !fi.IsDir() { + return nil, &os.PathError{Op: "openhomescope", Path: full, Err: ErrNotRealDir} + } + homeScopeVetted(full) + next, err := parent.OpenRoot(part) + if err != nil { + return nil, swappedLevel(parent, part, full, shown, err) + } + st, err := next.Stat(".") + if err != nil { + next.Close() + return nil, err + } + if !os.SameFile(fi, st) { + next.Close() + return nil, swappedLevel(parent, part, full, shown, ErrHomeScopeSwapped) + } + return next, nil +} + +// swappedLevel names a level whose open did not reach the directory that was +// vetted: a symlink standing there now is refused as the link it is, so the +// operator reads the same sentence the unraced refusal gives; anything else is +// reported with err. +func swappedLevel(parent *os.Root, part, full, shown string, err error) error { + if fi, lerr := parent.Lstat(part); lerr == nil && fi.Mode()&os.ModeSymlink != 0 { + return &HomeScopeLinkError{Link: shown} + } + if errors.Is(err, ErrHomeScopeSwapped) { + return &os.PathError{Op: "openhomescope", Path: full, Err: err} + } + return err +} + // ReadHomeDeclaration is ReadDeclaration for a declaration file named by its // place in the caller's home: home joined with rel (a slash path, // ".abcd/trusted-roots"). It is the read every home-scoped declaration goes // through, and it adds the one fact ReadDeclaration cannot see from a path — -// that no directory between home and the file is a symlink (HomeScopeLink). +// that no directory between home and the file is a symlink — and holds it +// through the read: the directory is opened by OpenHomeScope, and the file is +// judged and read relative to that descriptor, never by its path again +// (iss-2609281310017733). // // The order keeps AGENTS.md's rule exactly: a file that is not there is // DeclarationAbsent whatever the directories are, so a symlinked ~/.abcd @@ -95,11 +249,31 @@ func HomeScopeLink(home, rel string) error { // that IS there behind a symlinked directory is DeclarationBehindSymlink, // refused before a byte of it is read. The Lstat that decides absence follows // the directories, which is what lets a file behind a link be found in order to -// be refused. +// be refused; it decides nothing else. +// +// The file's own guards are ReadDeclaration's, applied to the descriptor: a +// leaf that is not a regular file is DeclarationNotRegular, one writable by +// group or other or owned by another uid is DeclarationWritableByOthers or +// DeclarationForeignOwner, and a leaf replaced between its judgement and its +// open is DeclarationUnreadable with ErrDeclarationSwapped. A directory level +// replaced while it was opened is DeclarationBehindSymlink when a symlink +// stands there now and DeclarationUnreadable otherwise. // // A rel that is not a clean relative path is DeclarationUnreadable before // anything is looked at: it names no place in the home to read. func ReadHomeDeclaration(home, rel string, limit int64) ([]byte, DeclarationRefusal, error) { + return ReadHomeDeclarationDenying(home, rel, limit, 0) +} + +// ReadHomeDeclarationDenying is ReadHomeDeclaration for a file whose reader +// refuses more of its mode than a declaration's group- and other-write: deny +// holds the permission bits that refuse it (0o077 for a secret no one else +// may read). The bits are judged on the fstat of the file that was opened, +// never on a path, so a file swapped or re-moded after any check by path is +// judged as the file that is read (iss-2609281310017733). A refusal is +// DeclarationExposed with a *DeclarationModeError naming the mode. Every other +// guard, and its order, is ReadHomeDeclaration's. +func ReadHomeDeclarationDenying(home, rel string, limit int64, deny os.FileMode) ([]byte, DeclarationRefusal, error) { if !ValidRelPath(rel) { return nil, DeclarationUnreadable, &os.PathError{Op: "readhomedeclaration", Path: rel, Err: os.ErrInvalid} } @@ -107,8 +281,69 @@ func ReadHomeDeclaration(home, rel string, limit int64) ([]byte, DeclarationRefu if _, err := os.Lstat(p); err != nil { return nil, DeclarationAbsent, err } - if err := HomeScopeLink(home, rel); err != nil { + root, err := OpenHomeScope(home, path.Dir(rel)) + switch { + case err == nil: + case errors.Is(err, ErrHomeScopeSymlinked): return nil, DeclarationBehindSymlink, err + case notPresent(err): + return nil, DeclarationAbsent, err + default: + return nil, DeclarationUnreadable, err + } + defer root.Close() + return readDeclarationIn(root, path.Base(rel), p, limit, deny.Perm()) +} + +// readDeclarationIn is ReadDeclaration for the file leaf directly inside root: +// the same guards in the same order, with the Lstat, the open and the confirming +// fstat all relative to root's descriptor. p is the file's full path, which the +// owner lookup (ownerUID, the package's test seam) and the vetting hook are +// given; the owner is confirmed again on the opened descriptor, so the lookup +// by path cannot vouch for a file other than the one read. deny is judged on +// that same descriptor (ReadHomeDeclarationDenying). +func readDeclarationIn(root *os.Root, leaf, p string, limit int64, deny os.FileMode) ([]byte, DeclarationRefusal, error) { + fi, err := root.Lstat(leaf) + if err != nil { + return nil, DeclarationAbsent, err + } + if !fi.Mode().IsRegular() { + return nil, DeclarationNotRegular, ErrNotRegular + } + if err := CallersAlone(p, fi); err != nil { + if errors.Is(err, ErrDeclarationWritable) { + return nil, DeclarationWritableByOthers, err + } + return nil, DeclarationForeignOwner, err + } + declarationVetted(p) + f, err := root.OpenFile(leaf, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, DeclarationUnreadable, err + } + defer f.Close() + st, err := f.Stat() + if err != nil { + return nil, DeclarationUnreadable, err + } + if !st.Mode().IsRegular() || !os.SameFile(fi, st) { + return nil, DeclarationUnreadable, ErrDeclarationSwapped + } + if sys, ok := st.Sys().(*syscall.Stat_t); !ok || sys.Uid != uint32(os.Getuid()) { + return nil, DeclarationForeignOwner, ErrDeclarationForeignOwner + } + if perm := st.Mode().Perm(); perm&deny != 0 { + return nil, DeclarationExposed, &DeclarationModeError{Perm: perm} + } + if st.Size() > limit { + return nil, DeclarationUnreadable, ErrTooBig + } + data, err := io.ReadAll(io.LimitReader(f, limit+1)) + if err != nil { + return nil, DeclarationUnreadable, err + } + if int64(len(data)) > limit { + return nil, DeclarationUnreadable, ErrTooBig } - return ReadDeclaration(p, limit) + return data, DeclarationOK, nil } diff --git a/internal/fsutil/home_race_test.go b/internal/fsutil/home_race_test.go new file mode 100644 index 000000000..dae3e26ea --- /dev/null +++ b/internal/fsutil/home_race_test.go @@ -0,0 +1,197 @@ +//go:build unix + +package fsutil + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +// raceableHome lays out a home with a real ~/.abcd and, inside the same home, a +// "dotfiles checkout" directory, each holding its own copy of name. It returns +// the home and the dotfiles directory. +func raceableHome(t *testing.T, name string) (home, dotfiles string) { + t.Helper() + home = t.TempDir() + dotfiles = filepath.Join(home, "dotfiles", "abcd") + for _, dir := range []string{filepath.Join(home, ".abcd"), dotfiles} { + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + } + writeDeclaration(t, filepath.Join(home, ".abcd"), name, "/real\n") + writeDeclaration(t, dotfiles, name, "/dotfiles\n") + return home, dotfiles +} + +// swapAbcdForLinkOnce installs the vetting hook so that, the first time +// ~/.abcd has been judged, the real directory is moved aside and ~/.abcd +// becomes a symlink into the dotfiles checkout — the same-uid race that falls +// between a check by path and a use by path. +func swapAbcdForLinkOnce(t *testing.T, home, dotfiles string) { + t.Helper() + swapped := false + restore := SwapHomeScopeVettedForTest(func(dir string) { + if swapped || dir != filepath.Join(home, ".abcd") { + return + } + swapped = true + if err := os.Rename(dir, filepath.Join(home, "moved-aside")); err != nil { + t.Fatalf("swap: %v", err) + } + if err := os.Symlink(dotfiles, dir); err != nil { + t.Fatalf("swap: %v", err) + } + }) + t.Cleanup(restore) +} + +// TestReadHomeDeclarationRefusesAnAbcdHomeSwappedForALinkAfterItsCheck is +// iss-2609281310017733: ~/.abcd was a real directory when it was judged and a +// symlink into a dotfiles checkout by the time the declaration was opened. A +// check by path followed by a read by path reads the dotfiles copy; the read +// through the descriptor of the directory that was judged refuses it, naming +// the link, and returns no bytes. +func TestReadHomeDeclarationRefusesAnAbcdHomeSwappedForALinkAfterItsCheck(t *testing.T) { + home, dotfiles := raceableHome(t, "trusted-roots") + swapAbcdForLinkOnce(t, home, dotfiles) + raw, refusal, err := ReadHomeDeclaration(home, ".abcd/trusted-roots", 1024) + if raw != nil || refusal != DeclarationBehindSymlink || !errors.Is(err, ErrHomeScopeSymlinked) { + t.Fatalf("a ~/.abcd swapped for a link after its check must be refused as the link: refusal %d, err %v, raw %q", refusal, err, raw) + } +} + +// The file's own guards still hold on the descriptor route: a leaf that is a +// symlink is refused as not regular, and a leaf swapped after its judgement is +// refused as swapped, even inside a real ~/.abcd. +func TestReadHomeDeclarationStillRefusesAHostileLeaf(t *testing.T) { + home, dotfiles := raceableHome(t, "trusted-roots") + if err := os.Symlink(filepath.Join(dotfiles, "trusted-roots"), filepath.Join(home, ".abcd", "linked")); err != nil { + t.Fatal(err) + } + if raw, refusal, err := ReadHomeDeclaration(home, ".abcd/linked", 1024); refusal != DeclarationNotRegular || err == nil || raw != nil { + t.Fatalf("a symlinked leaf must be refused as not regular: refusal %d, err %v, raw %q", refusal, err, raw) + } + + other := writeDeclaration(t, filepath.Join(home, ".abcd"), "other", "/swapped\n") + prev := declarationVetted + t.Cleanup(func() { declarationVetted = prev }) + declarationVetted = func(p string) { + if err := os.Rename(other, p); err != nil { + t.Fatalf("swap: %v", err) + } + } + raw, refusal, err := ReadHomeDeclaration(home, ".abcd/trusted-roots", 1024) + if raw != nil || refusal != DeclarationUnreadable || !errors.Is(err, ErrDeclarationSwapped) { + t.Fatalf("a leaf swapped after its judgement must be refused as swapped: refusal %d, err %v, raw %q", refusal, err, raw) + } +} + +// EnsureHomeScope creates each missing level, proves it and returns a root at +// the last; a home that is itself a link is the machine's layout and is +// walked; a symlinked level is refused by name with nothing created behind it; +// a non-directory level is not a real directory. OpenHomeScope creates +// nothing, so an absent level is os.IsNotExist. +func TestEnsureHomeScopeCreatesAndProvesEveryLevelBelowHome(t *testing.T) { + realHome := t.TempDir() + home := filepath.Join(t.TempDir(), "home") + if err := os.Symlink(realHome, home); err != nil { + t.Fatal(err) + } + if _, err := OpenHomeScope(home, ".abcd/history"); !os.IsNotExist(err) { + t.Fatalf("OpenHomeScope of an absent level must be not-exist and create nothing: %v", err) + } + root, err := EnsureHomeScope(home, ".abcd/history", 0o700) + if err != nil { + t.Fatalf("EnsureHomeScope under a home reached through a link: %v", err) + } + if err := root.WriteFile("index.json", []byte("{}\n"), 0o600); err != nil { + t.Fatal(err) + } + root.Close() + if _, err := os.Stat(filepath.Join(realHome, ".abcd", "history", "index.json")); err != nil { + t.Fatalf("the root does not stand at ~/.abcd/history: %v", err) + } + if fi, err := os.Stat(filepath.Join(realHome, ".abcd")); err != nil || fi.Mode().Perm() != 0o700 { + t.Fatalf("a created level must carry perm: %v %v", fi, err) + } + + linked, target := symlinkedHome(t) + _, err = EnsureHomeScope(linked, ".abcd/history", 0o700) + var le *HomeScopeLinkError + if !errors.As(err, &le) || le.Link != "~/.abcd" { + t.Fatalf("a symlinked ~/.abcd must be refused by name: %v", err) + } + if entries, _ := os.ReadDir(target); len(entries) != 0 { + t.Fatalf("EnsureHomeScope created %q behind the link", entries[0].Name()) + } + + if err := os.Symlink(t.TempDir(), filepath.Join(realHome, ".abcd", "sub")); err != nil { + t.Fatal(err) + } + if _, err := OpenHomeScope(home, ".abcd/sub"); !errors.As(err, &le) || le.Link != "~/.abcd/sub" { + t.Fatalf("a symlinked level below ~/.abcd must be refused by name: %v", err) + } + if err := os.WriteFile(filepath.Join(realHome, ".abcd", "file"), nil, 0o600); err != nil { + t.Fatal(err) + } + if _, err := EnsureHomeScope(home, ".abcd/file", 0o700); !errors.Is(err, ErrNotRealDir) { + t.Fatalf("a non-directory level must be refused as not a real directory: %v", err) + } + for _, dir := range []string{"", "../x", "/etc", ".abcd/"} { + if _, err := OpenHomeScope(home, dir); !errors.Is(err, os.ErrInvalid) { + t.Errorf("OpenHomeScope(%q) = %v, want os.ErrInvalid", dir, err) + } + } +} + +// A level swapped for a link after its judgement is refused by EnsureHomeScope +// as it is by the reader, and nothing is created behind the link. +func TestEnsureHomeScopeRefusesALevelSwappedForALinkAfterItsCheck(t *testing.T) { + home, dotfiles := raceableHome(t, "path-entry") + swapAbcdForLinkOnce(t, home, dotfiles) + _, err := EnsureHomeScope(home, ".abcd/history", 0o700) + if !errors.Is(err, ErrHomeScopeSymlinked) { + t.Fatalf("a ~/.abcd swapped for a link after its check must be refused as the link: %v", err) + } + if _, err := os.Lstat(filepath.Join(dotfiles, "history")); !os.IsNotExist(err) { + t.Fatalf("EnsureHomeScope created history behind the swapped link: %v", err) + } +} + +// TestReadHomeDeclarationDenyingJudgesTheModeOfTheFileItOpened: deny is judged +// on the opened file's fstat, not on the Lstat that vetted it. A 0600 file is +// read; the same file opened wider than 0600 — the one the Lstat judged, re- +// moded inside the window between that Lstat and the open — is refused as +// DeclarationExposed naming its mode, and no byte is returned. deny 0 (the +// plain ReadHomeDeclaration) keeps admitting a declaration others can read. +func TestReadHomeDeclarationDenyingJudgesTheModeOfTheFileItOpened(t *testing.T) { + home, _ := raceableHome(t, "credentials.json") + p := filepath.Join(home, ".abcd", "credentials.json") + if err := os.Chmod(p, 0o600); err != nil { + t.Fatal(err) + } + if raw, refusal, err := ReadHomeDeclarationDenying(home, ".abcd/credentials.json", 1024, 0o077); refusal != DeclarationOK || err != nil || string(raw) != "/real\n" { + t.Fatalf("a 0600 file must be read: refusal %d, err %v, raw %q", refusal, err, raw) + } + + prev := declarationVetted + t.Cleanup(func() { declarationVetted = prev }) + declarationVetted = func(string) { + if err := os.Chmod(p, 0o644); err != nil { + t.Fatalf("chmod: %v", err) + } + } + raw, refusal, err := ReadHomeDeclarationDenying(home, ".abcd/credentials.json", 1024, 0o077) + var mode *DeclarationModeError + if raw != nil || refusal != DeclarationExposed || !errors.As(err, &mode) || mode.Perm != 0o644 { + t.Fatalf("a file opened at 0644 must be refused on its fstat: refusal %d, err %v, raw %q", refusal, err, raw) + } + + declarationVetted = prev + if raw, refusal, err := ReadHomeDeclaration(home, ".abcd/credentials.json", 1024); refusal != DeclarationOK || err != nil || string(raw) != "/real\n" { + t.Fatalf("with no mask a 0644 declaration is still read: refusal %d, err %v, raw %q", refusal, err, raw) + } +} diff --git a/internal/surface/cli/ahoy_connect.go b/internal/surface/cli/ahoy_connect.go index 6e1cb3192..13e801fd9 100644 --- a/internal/surface/cli/ahoy_connect.go +++ b/internal/surface/cli/ahoy_connect.go @@ -37,10 +37,11 @@ const dispatchPending = "no delegating verb sends a step to a provider until pro "(spc-2609251028149555); until then every delegated step runs on the host" // providerView is one configured provider as the board shows it: the block, -// and whether its key resolves (never the key). +// whether its key resolves and the home it resolves from (never the key). type providerView struct { oracle.Provider KeyState string `json:"key_state"` + KeyHome string `json:"key_home,omitempty"` } // providersBoard is `ahoy --providers`. @@ -70,7 +71,6 @@ func runAhoyProviders(cmd *cobra.Command, cwd string, asJSON bool) error { if err != nil { return &exitError{Code: 2, Msg: "abcd ahoy --providers: " + termsafe.Sanitize(fsutil.RedactHome(err.Error()))} } - creds := credential.Machine(roots.Home) b := providersBoard{ Explanation: oracle.AdapterExplanation, Providers: []providerView{}, @@ -86,7 +86,8 @@ func runAhoyProviders(cmd *cobra.Command, cwd string, asJSON bool) error { b.Routes = []oracle.PointedRoute{} } for _, p := range cfg.Providers() { - b.Providers = append(b.Providers, providerView{Provider: p, KeyState: keyState(creds, p.Key)}) + state, home := keyState(roots.Home, p.Key) + b.Providers = append(b.Providers, providerView{Provider: p, KeyState: state, KeyHome: home}) } return render(cmd.OutOrStdout(), asJSON, b, func(w io.Writer) { line := func(s string) { fmt.Fprintf(w, " %s\n", termsafe.Sanitize(s)) } @@ -99,6 +100,9 @@ func runAhoyProviders(cmd *cobra.Command, cwd string, asJSON bool) error { key := "no key" if p.Key != "" { key = "key " + p.Key + " (" + p.KeyState + ")" + if p.KeyHome != "" { + key = "key " + p.Key + " (" + p.KeyState + ", " + p.KeyHome + " home)" + } } line(fmt.Sprintf("provider %s: %s, %s, models %s", p.Name, p.BaseURL, key, strings.Join(p.Models, ", "))) } @@ -119,26 +123,29 @@ func runAhoyProviders(cmd *cobra.Command, cwd string, asJSON bool) error { }) } -// keyState says whether a named key resolves: set, not set, refused (the -// store is unsafe), or none for a keyless provider. Never the value. -func keyState(creds credential.Source, name string) string { +// keyState says whether a named credential resolves through the store, and +// from which home: set, not set, refused (the store is unsafe), or none for a +// keyless provider. Never the value. +func keyState(home, name string) (state, from string) { if name == "" { - return "none" + return "none", "" } - _, err := creds.Resolve(name) + _, err := credential.Store(home).Resolve(name) switch { case err == nil: - return "set" + from, _ = credential.Where(home, name) + return "set", from case errors.Is(err, credential.ErrNotSet): - return "not set" + return "not set", "" } - return "refused: " + err.Error() + return "refused: " + err.Error(), "" } // newAhoyConnectCommand builds `ahoy connect `. func newAhoyConnectCommand(asJSON *bool) *cobra.Command { var baseURL, home, keyName string var models []string + var ptr credential.Pointer cmd := &cobra.Command{ Use: "connect ", Args: cobra.MaximumNArgs(1), @@ -154,8 +161,8 @@ func newAhoyConnectCommand(asJSON *bool) *cobra.Command { for _, n := range notes { fmt.Fprintf(cmd.ErrOrStderr(), "abcd %s\n", termsafe.Sanitize(fsutil.RedactHome(n))) } - req := oracle.ConnectRequest{Roots: roots, Provider: args[0], BaseURL: baseURL, Models: models, Home: home, KeyName: keyName} - if home == oracle.KeyHomeABCD { + req := oracle.ConnectRequest{Roots: roots, Provider: args[0], BaseURL: baseURL, Models: models, Home: home, KeyName: keyName, Pointer: ptr} + if home == oracle.KeyHomeABCD || home == oracle.KeyHomeKeychain { key, err := readKey(cmd.InOrStdin()) if err != nil { return &exitError{Code: 2, Msg: "abcd ahoy connect: " + err.Error()} @@ -184,8 +191,9 @@ func newAhoyConnectCommand(asJSON *bool) *cobra.Command { } cmd.Flags().StringVar(&baseURL, "base-url", "", "the provider's OpenAI-compatible base URL: https, or http to a server on this machine") cmd.Flags().StringArrayVar(&models, "model", nil, "a model the provider may serve, repeated for each (the first allowlist; the verification call asks for the first)") - cmd.Flags().StringVar(&home, "home", "", "where the key lives: abcd (read from stdin into the owner-only ~/.abcd/credentials.json) | none (a server that takes no key); external and keychain arrive with the credential store") + cmd.Flags().StringVar(&home, "home", "", "where the key lives: external (--env, or --file and --field) | abcd (read from stdin into the owner-only ~/.abcd/credentials.json) | keychain (read from stdin into the platform keychain) | none (a server that takes no key)") cmd.Flags().StringVar(&keyName, "key", "", "the credential's name (default: the provider's name)") + pointerFlags(cmd, &ptr) return cmd } @@ -204,7 +212,7 @@ func readKey(in io.Reader) (string, error) { } key := strings.TrimSuffix(strings.TrimSuffix(string(raw), "\n"), "\r") if key == "" { - return "", errors.New("the abcd home stores a key, and none arrived on stdin; pipe it in (" + setupExample + ")") + return "", errors.New("the chosen home stores a key, and none arrived on stdin; pipe it in (" + setupExample + ")") } return key, nil } diff --git a/internal/surface/cli/ahoy_connect_test.go b/internal/surface/cli/ahoy_connect_test.go index e217b1944..0fb5c3e30 100644 --- a/internal/surface/cli/ahoy_connect_test.go +++ b/internal/surface/cli/ahoy_connect_test.go @@ -57,7 +57,7 @@ func TestAhoyProvidersExplainsWithNothingConfigured(t *testing.T) { for _, want := range []string{ "aggregator", "decision models", "every delegated step runs on the host", "none configured", "anthropic/* (bundled)", - "The platform keychain is the safest home", "abcd ahoy connect", + "The platform keychain is the home abcd recommends", "abcd ahoy connect", "--home abcd", } { if !strings.Contains(string(out), want) { @@ -121,15 +121,15 @@ func TestAhoyConnectVerifiesThenWrites(t *testing.T) { if err != nil { t.Fatalf("ahoy --providers: %v\n%s", err, board) } - if !strings.Contains(string(board), "openrouter") || !strings.Contains(string(board), "key openrouter (set)") || + if !strings.Contains(string(board), "openrouter") || !strings.Contains(string(board), "key openrouter (set, abcd home)") || strings.Contains(string(board), connectKey) { t.Fatalf("board after connect:\n%s", board) } } -// TestAhoyConnectRefusals: a deferred home names the credential store, an -// absent key and a refused verification each exit non-zero, write nothing -// and never print the key. +// TestAhoyConnectRefusals: a home without what it keeps, an absent key and a +// refused verification each exit non-zero, write nothing and never print the +// key. func TestAhoyConnectRefusals(t *testing.T) { cases := []struct { name, stdin, home string @@ -138,8 +138,8 @@ func TestAhoyConnectRefusals(t *testing.T) { wantCalls int32 want string }{ - {"keychain deferred", connectKey, "keychain", 200, completionReply("m"), 0, "itd-2609221017023290"}, - {"external deferred", connectKey, "external", 200, completionReply("m"), 0, "itd-2609221017023290"}, + {"keychain with no key on stdin", "", "keychain", 200, completionReply("m"), 0, "stdin"}, + {"external with no pointer", "", "external", 200, completionReply("m"), 0, "environment variable"}, {"no key on stdin", "", "abcd", 200, completionReply("m"), 0, "stdin"}, {"provider refuses the key", connectKey, "abcd", 401, `{"error":{"message":"bad key ` + connectKey + `"}}`, 1, "nothing was written"}, {"provider does not list the model", connectKey, "abcd", 404, `{"error":{"message":"No endpoints found"}}`, 1, "HTTP 404"}, diff --git a/internal/surface/cli/ahoy_credential.go b/internal/surface/cli/ahoy_credential.go new file mode 100644 index 000000000..4ea2d147c --- /dev/null +++ b/internal/surface/cli/ahoy_credential.go @@ -0,0 +1,213 @@ +package cli + +// ahoy_credential.go is the front door of the credential store's walkthrough +// (itd-2609221017023290): `abcd ahoy credential` lists the credentials abcd +// reads with whether each is set and in which home, `abcd ahoy credential +// ` explains one (what it unlocks, what works without it, the three +// homes with the keychain recommended in the prose), and `--home` runs the +// walkthrough: the reading adapter's own verification call, then the write. +// +// A value arrives on stdin and nowhere else, as for `ahoy connect`: never as a +// flag, never at a prompt, never from a terminal. The external home takes a +// pointer instead (--env, or --file and --field), and abcd stores only that. + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "sort" + + "github.com/intentdriven/abcd/internal/adapter/openaiapi" + "github.com/intentdriven/abcd/internal/core/credential" + "github.com/intentdriven/abcd/internal/core/layered" + "github.com/intentdriven/abcd/internal/core/oracle" + "github.com/intentdriven/abcd/internal/core/site" + "github.com/intentdriven/abcd/internal/fsutil" + "github.com/intentdriven/abcd/internal/termsafe" + "github.com/spf13/cobra" +) + +// pointerFlags adds the external home's pointer flags to cmd. +func pointerFlags(cmd *cobra.Command, p *credential.Pointer) { + cmd.Flags().StringVar(&p.Env, "env", "", "for --home external: the environment variable that holds the value") + cmd.Flags().StringVar(&p.File, "file", "", "for --home external: a tool's JSON configuration file under the home directory, written from ~/") + cmd.Flags().StringVar(&p.Field, "field", "", "for --home external: the dotted field of --file that holds the value (auth.token)") +} + +// credentialService finds the walkthrough's service for name among the +// adapters that read a credential: the site setup's hosting providers, then +// the configured model providers. +func credentialService(roots layered.Roots, name string) (credential.Service, bool, error) { + if svc, ok := site.CredentialServiceFor(name); ok { + return svc, true, nil + } + return oracle.CredentialService(roots, name) +} + +// credentialView is one credential as a surface shows it: presence and home, +// never the value. +type credentialView struct { + Name string `json:"name"` + State string `json:"state"` + Home string `json:"home,omitempty"` +} + +// credentialExplanation is `ahoy credential ` bare. +type credentialExplanation struct { + credentialView + Unlocks string `json:"unlocks"` + WithoutIt string `json:"without_it"` + HomesText string `json:"homes_prose"` + Homes []string `json:"homes"` + Setup []string `json:"setup"` +} + +// newAhoyCredentialCommand builds `ahoy credential []`. +func newAhoyCredentialCommand(asJSON *bool) *cobra.Command { + var home string + var ptr credential.Pointer + cmd := &cobra.Command{ + Use: "credential []", + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + cwd, err := os.Getwd() + if err != nil { + return err + } + roots, notes := layered.RootsFor(cwd) + for _, n := range notes { + fmt.Fprintf(cmd.ErrOrStderr(), "abcd %s\n", termsafe.Sanitize(fsutil.RedactHome(n))) + } + fail := func(err error, secret string) error { + msg := err.Error() + if secret != "" { + msg = openaiapi.Scrub(msg, secret) + } + return &exitError{Code: 2, Msg: "abcd ahoy credential: " + termsafe.Sanitize(fsutil.RedactHome(msg))} + } + if len(args) == 0 { + if home != "" { + return fail(errors.New("name the credential to store; `abcd ahoy credential` lists them"), "") + } + return runCredentialList(cmd, roots, *asJSON) + } + name := args[0] + if !credential.ValidName(name) { + return fail(errors.New("the name is not a plain credential name"), "") + } + svc, ok, err := credentialService(roots, name) + if err != nil { + return fail(err, "") + } + if !ok { + return fail(fmt.Errorf("no adapter abcd ships reads a credential named %s; `abcd ahoy credential` lists the ones it reads", name), "") + } + if home == "" { + return renderCredentialExplanation(cmd, roots, svc, *asJSON) + } + choice := credential.Choice{Home: home, Pointer: ptr} + if home == credential.HomeABCD || home == credential.HomeKeychain { + if choice.Value, err = readKey(cmd.InOrStdin()); err != nil { + return fail(err, "") + } + } + res, err := credential.Walk(context.Background(), roots.Home, svc, choice) + if err != nil { + return fail(err, choice.Value) + } + return render(cmd.OutOrStdout(), *asJSON, res, func(w io.Writer) { + line := func(s string) { fmt.Fprintf(w, " %s\n", termsafe.Sanitize(s)) } + fmt.Fprintf(w, "abcd ahoy credential — %s verified and kept in the %s home\n", res.Name, res.Home) + if len(res.Wrote) == 0 { + line("no change: the same credential was already kept there") + } + for _, p := range res.Wrote { + line("wrote: " + p) + } + }) + }, + } + cmd.Flags().StringVar(&home, "home", "", "where the credential lives: external (--env, or --file and --field) | abcd (read from stdin into the owner-only ~/.abcd/credentials.json) | keychain (read from stdin into the platform keychain)") + pointerFlags(cmd, &ptr) + return cmd +} + +// presence is a credential's state and home, never its value. +func presence(home, name string) credentialView { + state, from := keyState(home, name) + return credentialView{Name: name, State: state, Home: from} +} + +func (v credentialView) text() string { + if v.Home != "" { + return v.Name + ": " + v.State + ", " + v.Home + " home" + } + return v.Name + ": " + v.State +} + +// runCredentialList is `ahoy credential` bare: every credential an adapter +// reads, with its presence and home. It writes nothing and makes no call. +func runCredentialList(cmd *cobra.Command, roots layered.Roots, asJSON bool) error { + names := map[string]bool{} + for _, n := range site.CredentialNames() { + names[n] = true + } + cfg, err := oracle.LoadAPI(roots) + if err != nil { + return &exitError{Code: 2, Msg: "abcd ahoy credential: " + termsafe.Sanitize(fsutil.RedactHome(err.Error()))} + } + for _, p := range cfg.Providers() { + if p.Key != "" { + names[p.Key] = true + } + } + sorted := make([]string, 0, len(names)) + for n := range names { + sorted = append(sorted, n) + } + sort.Strings(sorted) + out := struct { + Credentials []credentialView `json:"credentials"` + }{Credentials: []credentialView{}} + for _, n := range sorted { + out.Credentials = append(out.Credentials, presence(roots.Home, n)) + } + return render(cmd.OutOrStdout(), asJSON, out, func(w io.Writer) { + fmt.Fprintln(w, "abcd ahoy credential") + for _, c := range out.Credentials { + fmt.Fprintf(w, " %s\n", termsafe.Sanitize(c.text())) + } + fmt.Fprintln(w, " `abcd ahoy credential ` explains one and where it can live.") + }) +} + +// renderCredentialExplanation is `ahoy credential ` without --home: the +// explanation first, the homes after, and the command for each. Writes +// nothing and makes no call. +func renderCredentialExplanation(cmd *cobra.Command, roots layered.Roots, svc credential.Service, asJSON bool) error { + e := credentialExplanation{ + credentialView: presence(roots.Home, svc.Name), + Unlocks: svc.Unlocks, + WithoutIt: svc.WithoutIt, + HomesText: credential.HomesProse, + Homes: credential.Homes(), + Setup: []string{ + "abcd ahoy credential " + svc.Name + " --home external --env ", + "abcd ahoy credential " + svc.Name + " --home external --file ~/.json --field ", + "abcd ahoy credential " + svc.Name + " --home abcd < ", + "abcd ahoy credential " + svc.Name + " --home keychain < ", + }, + } + return render(cmd.OutOrStdout(), asJSON, e, func(w io.Writer) { + fmt.Fprintf(w, "abcd ahoy credential %s — %s\n", svc.Name, termsafe.Sanitize(e.credentialView.text())) + for _, l := range svc.Explain() { + fmt.Fprintf(w, " %s\n", termsafe.Sanitize(l)) + } + fmt.Fprintln(w, " store it, verified first with the adapter's own call, the value piped in on stdin and never typed at a prompt:") + for _, s := range e.Setup { + fmt.Fprintf(w, " %s\n", s) + } + }) +} diff --git a/internal/surface/cli/ahoy_credential_test.go b/internal/surface/cli/ahoy_credential_test.go new file mode 100644 index 000000000..bc1cef751 --- /dev/null +++ b/internal/surface/cli/ahoy_credential_test.go @@ -0,0 +1,162 @@ +package cli + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" +) + +// `abcd ahoy credential` is the credential store's walkthrough at its front +// door (itd-2609221017023290): bare with a name it explains and writes +// nothing; with --home it verifies with the reading adapter's own call and +// only then stores. No test here chooses the keychain home, which would reach +// the real keychain; the store's own tests run it against a fake. + +// providerNamingKey writes a machine configuration whose provider names the +// credential openrouter, served by a fake on the loopback address. +func providerNamingKey(t *testing.T, base string) { + t.Helper() + dir := filepath.Join(os.Getenv("HOME"), ".abcd") + if err := os.MkdirAll(dir, 0o700); err != nil { + t.Fatal(err) + } + cfg := `{"oracle":{"api":{"openrouter":{"base_url":"` + base + `","key":"openrouter","models":["typesafe/jev-1.13"]}}}}` + if err := os.WriteFile(filepath.Join(dir, "config.json"), []byte(cfg), 0o600); err != nil { + t.Fatal(err) + } +} + +// abcdHomeEntries lists every path under ~/.abcd, so a test can prove a verb +// wrote nothing there. The hermetic environment seeds ~/.abcd itself (the +// cache attestation), so the absence of the directory proves nothing. +func abcdHomeEntries(t *testing.T) string { + t.Helper() + root := filepath.Join(os.Getenv("HOME"), ".abcd") + var names []string + err := filepath.WalkDir(root, func(p string, _ os.DirEntry, err error) error { + if err != nil { + return err + } + rel, _ := filepath.Rel(root, p) + names = append(names, rel) + return nil + }) + if err != nil && !os.IsNotExist(err) { + t.Fatal(err) + } + return strings.Join(names, "\n") +} + +func TestAhoyCredentialExplainsAndWritesNothing(t *testing.T) { + hermeticEnv(t) + t.Chdir(t.TempDir()) + before := abcdHomeEntries(t) + out, err := runCLIErr(t, "ahoy", "credential", "hosting.cloudflare") + if err != nil { + t.Fatalf("ahoy credential: %v\n%s", err, out) + } + text := string(out) + for _, want := range []string{"unlocks", "Without it", "The platform keychain is the home abcd recommends", "--home"} { + if !strings.Contains(text, want) { + t.Errorf("the explanation does not say %q:\n%s", want, text) + } + } + if strings.Index(text, "unlocks") > strings.Index(text, "The platform keychain") { + t.Fatal("the homes come before what the credential unlocks") + } + if after := abcdHomeEntries(t); after != before { + t.Fatalf("the explanation wrote under ~/.abcd:\nbefore: %q\nafter: %q", before, after) + } + if _, err := runCLIErr(t, "ahoy", "credential", "no.such.credential"); err == nil { + t.Fatal("a name no adapter reads was explained") + } +} + +// TestAhoyCredentialVerifiesThenStores: the provider's own call is made with +// the key from stdin, then the key is stored; the output and the result name +// the home and never the key; the board then reads it as set in that home. +func TestAhoyCredentialVerifiesThenStores(t *testing.T) { + hermeticEnv(t) + t.Chdir(t.TempDir()) + base, calls, auth := fakeProvider(t, 200, completionReply("typesafe/jev-1.13-20260915")) + providerNamingKey(t, base) + out, err := runCLIStdinErr(t, connectKey+"\n", "ahoy", "credential", "openrouter", "--home", "abcd", "--json") + if err != nil { + t.Fatalf("ahoy credential: %v\n%s", err, out) + } + if strings.Contains(string(out), connectKey) { + t.Fatal("the key reached the output") + } + if calls.Load() != 1 || auth.Load() != "Bearer "+connectKey { + t.Fatalf("verification: %d call(s)", calls.Load()) + } + var res struct { + Name string `json:"name"` + Home string `json:"home"` + Verified bool `json:"verified"` + Wrote []string `json:"wrote"` + } + if err := json.Unmarshal(out, &res); err != nil { + t.Fatalf("--json: %v\n%s", err, out) + } + if res.Name != "openrouter" || res.Home != "abcd" || !res.Verified || len(res.Wrote) != 1 || res.Wrote[0] != "~/.abcd/credentials.json" { + t.Fatalf("result = %+v", res) + } + board, err := runCLIErr(t, "ahoy", "--providers") + if err != nil || !strings.Contains(string(board), "key openrouter (set, abcd home)") { + t.Fatalf("board: %v\n%s", err, board) + } + list, err := runCLIErr(t, "ahoy", "credential") + if err != nil || !strings.Contains(string(list), "openrouter: set, abcd home") || !strings.Contains(string(list), "hosting.cloudflare: not set") { + t.Fatalf("list: %v\n%s", err, list) + } + if strings.Contains(string(list), connectKey) { + t.Fatal("the list carries the key") + } +} + +// TestAhoyCredentialKeepsAPointerInTheExternalHome: --env stores where the key +// is and never the key. +func TestAhoyCredentialKeepsAPointerInTheExternalHome(t *testing.T) { + hermeticEnv(t) + t.Chdir(t.TempDir()) + base, calls, _ := fakeProvider(t, 200, completionReply("typesafe/jev-1.13")) + providerNamingKey(t, base) + t.Setenv("ABCD_TEST_PROVIDER_KEY", connectKey) + out, err := runCLIErr(t, "ahoy", "credential", "openrouter", "--home", "external", "--env", "ABCD_TEST_PROVIDER_KEY") + if err != nil { + t.Fatalf("ahoy credential: %v\n%s", err, out) + } + if calls.Load() != 1 || strings.Contains(string(out), connectKey) { + t.Fatalf("%d call(s); output:\n%s", calls.Load(), out) + } + home := os.Getenv("HOME") + if _, err := os.Lstat(filepath.Join(home, ".abcd", "credentials.json")); err == nil { + t.Fatal("the external home wrote the abcd-only store") + } + raw, err := os.ReadFile(filepath.Join(home, ".abcd", "credential-homes.json")) + if err != nil || strings.Contains(string(raw), connectKey) || !strings.Contains(string(raw), "ABCD_TEST_PROVIDER_KEY") { + t.Fatalf("index: %v\n%s", err, raw) + } +} + +// TestAhoyCredentialRefusesAFailedVerification: a key the provider refuses is +// never stored, and the refusal never carries it. +func TestAhoyCredentialRefusesAFailedVerification(t *testing.T) { + hermeticEnv(t) + t.Chdir(t.TempDir()) + base, _, _ := fakeProvider(t, 401, `{"error":{"message":"bad key `+connectKey+`"}}`) + providerNamingKey(t, base) + out, err := runCLIStdinErr(t, connectKey, "ahoy", "credential", "openrouter", "--home", "abcd") + if err == nil { + t.Fatalf("a refused key was stored:\n%s", out) + } + if strings.Contains(string(out)+err.Error(), connectKey) { + t.Fatal("the refusal carries the key") + } + if _, statErr := os.Lstat(filepath.Join(os.Getenv("HOME"), ".abcd", "credentials.json")); statErr == nil { + t.Fatal("a refused key was stored") + } +} diff --git a/internal/surface/cli/cite.go b/internal/surface/cli/cite.go index d48b1e531..814b2a10d 100644 --- a/internal/surface/cli/cite.go +++ b/internal/surface/cli/cite.go @@ -159,7 +159,7 @@ func newCiteConfirmCommand(asJSON *bool) *cobra.Command { } addCiteFlags(cmd, &configPath, &rootDir) cmd.Flags().StringVar(&receiptPath, "receipt", "", - "path to a receipt file listing the confirmed citations (the format the generated checklist page emits)") + "path to a JSON receipt listing the confirmed citations: schema_version 1 and a confirmed list, each entry a url with an optional final_url and verified_on (YYYY-MM-DD)") return cmd } diff --git a/internal/surface/cli/cli.go b/internal/surface/cli/cli.go index 762bf7f29..33b9a9a7d 100644 --- a/internal/surface/cli/cli.go +++ b/internal/surface/cli/cli.go @@ -218,9 +218,11 @@ func NewRootCommand() *cobra.Command { Long: "Agent-based configuration for development.\n\n" + "Bare `abcd` renders the read-only status board — what can I do. A single\n" + "positional matching a record id (`iss-N`, `itd-N`, `spc-N`, `adr-N`, `adm-N`,\n" + - "`srp-N`, `rfm-N`) instead reports what that record is, where it lives, and\n" + - "the next move for its lifecycle state — what is this. Both forms are strictly\n" + - "read-only; any other positional is refused as an unknown command.", + "`srp-N` or `rfm-N`) instead reports what that record is, where it lives, and\n" + + "the next move for its lifecycle state — what is this. N is either a short\n" + + "ordinal from before ids were minted or the sixteen-digit stamp minted since;\n" + + "both resolve. The bare and the id form are strictly read-only; any other\n" + + "positional is refused as an unknown command.", SilenceUsage: true, SilenceErrors: true, // Bare answers "what can I do"; `abcd ` answers "what is this, and @@ -3470,6 +3472,7 @@ func newAhoyCommand(asJSON *bool) *cobra.Command { ahoyCmd.AddCommand(movedStub("identity-check", "abcd ahoy --identity")) ahoyCmd.AddCommand(newAhoyRemoteCommand(asJSON)) ahoyCmd.AddCommand(newAhoyConnectCommand(asJSON)) + ahoyCmd.AddCommand(newAhoyCredentialCommand(asJSON)) return ahoyCmd } diff --git a/internal/surface/cli/reading.go b/internal/surface/cli/reading.go index 449558b68..f95439d19 100644 --- a/internal/surface/cli/reading.go +++ b/internal/surface/cli/reading.go @@ -80,8 +80,7 @@ func newReadingCommand(asJSON *bool) *cobra.Command { "that file, reviewed and inside the dirty gate; the manifest records the entry applied\n" + "and its hash, so a run is reproducible from the commit it names.", Example: " abcd reading assemble --position widening --target HEAD --dry-run\n" + - " abcd reading assemble --position entailment --target HEAD \\\n" + - " --out .abcd/.work.local/scratch/reading-runs/manual --json", + " abcd reading assemble --position entailment --target HEAD --json", Args: func(_ *cobra.Command, args []string) error { if len(args) > 0 { return &exitError{Code: 2, Msg: "reading assemble: this verb takes no positional argument; " + @@ -180,8 +179,9 @@ func newReadingCommand(asJSON *bool) *cobra.Command { assembleCmd.Flags().StringVar(&target, "target", "", "the commit the assembly describes: HEAD, or a hexadecimal sha of 7 to 40 digits") assembleCmd.Flags().StringVar(&outDir, "out", "", - "an empty or absent directory the assembled input and the manifest are written to\n"+ - "(default: the local-tier run directory)") + "an empty or absent directory the assembled input and the manifest are written to,\n"+ + "for inspection: reading ingest finds a run only in the local-tier run directory,\n"+ + "so a run written here cannot be ingested (default: the local-tier run directory)") assembleCmd.Flags().BoolVar(&dryRun, "dry-run", false, "write nothing; with --out the two artefacts still land in that directory") @@ -428,6 +428,11 @@ func renderAssembleResult(w io.Writer, res reading.AssembleResult) { } fmt.Fprintf(w, " written: %s and %s in %s\n", reading.BundleFileName, reading.ManifestFileName, res.OutDir) + if !res.Ingestable { + fmt.Fprintf(w, " ingest: this run cannot be ingested; reading ingest finds a run only "+ + "under %s/, so this copy is for inspection. Assemble without --out to park "+ + "a run the ingest can prove\n", reading.DefaultRunDir) + } } // renderPreset writes the committed entry this run applied. diff --git a/internal/surface/cli/reading_surface_test.go b/internal/surface/cli/reading_surface_test.go index 38da8275b..60e8cc812 100644 --- a/internal/surface/cli/reading_surface_test.go +++ b/internal/surface/cli/reading_surface_test.go @@ -743,6 +743,33 @@ func TestSizeReportRendersBeforeTheWrittenLine(t *testing.T) { } } +// TestAssembleRenderSaysANamedRunCannotBeIngested holds the text half of the +// assembly-time report: a run written outside the default run directory is +// named as one `reading ingest` cannot find, and a parked run carries no such +// line (iss-2609091648476051). +func TestAssembleRenderSaysANamedRunCannotBeIngested(t *testing.T) { + base := reading.AssembleResult{ + RunID: "rdg-2608310000000002", Position: "detection", TargetCommit: "abcdef1234567890", + ItemCount: 1, ManifestHash: "cafe", Written: true, + } + named := base + named.OutDir = "out" + var buf bytes.Buffer + renderAssembleResult(&buf, named) + if out := buf.String(); !strings.Contains(out, "cannot be ingested") { + t.Errorf("a run written to a named directory renders no ingest warning:\n%s", out) + } + + parked := base + parked.OutDir = reading.DefaultRunDir + "/" + base.RunID + parked.Ingestable = true + buf.Reset() + renderAssembleResult(&buf, parked) + if out := buf.String(); strings.Contains(out, "cannot be ingested") { + t.Errorf("a parked run renders the ingest warning:\n%s", out) + } +} + // TestHumanBytesAndThousands holds the two formatters the report leans on. func TestHumanBytesAndThousands(t *testing.T) { for in, want := range map[int]string{