allod secret declare: write a credential's non-secret half in canonical shape #206
No reviewers
Labels
No labels
blocked
bug
decision
duplicate
enhancement
help wanted
invalid
landed?
question
ready-to-merge
stale
wontfix
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
allod/tools!206
Loading…
Reference in a new issue
No description provided.
Delete branch "agent/secret-declare"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
allod secret declare <name> ...writes a new credential's non-secret half into a secrets checkout in the canonical shape: thecredentials.nixentry inrotation_state = "pending", thesecrets.nixrecipient line, and the rotation registry group. It exists on every machine, dev VMs included, because it writes no ciphertext and reads no identity;createandrekeystay behind thesecrettag and remain unknown words untagged. The command never edits an existing declaration: any match for the name refuses with what was found. Nothing changes for a running machine until anallod-toolspin moves. The proof is a fixture-checkout test suite pinning the exact inserted text and every refusal. If the output is wrong, the diff it produces is reviewed in a PR before any ciphertext exists, and reverting that PR undoes it.What changes
secretnamespace dispatcher moves to an untagged file, following thesitepattern:declareis registered untagged,createandrekeyregister themselves from the tagged file. Untagged builds list onlydeclare; the absent-tag test now pins that shape. The flake comment about the tag is corrected.allod secret declare <name> [<checkout>] --kind <kind> --owner <owner> --to <machine>[,<machine>...] --format <format> --deployed-path <path> --verify <probe-type> [--verify-repo-url <url>] [--verify-context <text>] [--verify-user <user>] [--service none|forgejo] [--account <a>] [--ui-token-name <n>] [--strategy overlap|in-place].--tois comma-separated and repeatable. Every enum is validated against the listsrotate-token's validator accepts;forgejorequires the account and UI token name andnonerefuses them.--kindaccepts onlyuser,agent, andservice:machine-hostandforge-gitentries are generated by the template and carry a public key and consumers this shape cannot express.git-ls-remoterequires--verify-repo-urlandforge-token-verifyrequires--verify-user, because those are the fieldsrotate-token's printed verification commands substitute.kindis derived, not typed:nix evalof the inventory flake'smachines.<name>.type, mappeddev→dev-vm,privacy→privacy-vm,hypervisor→nixos-host,service→service-vm. A machine the inventory does not know, or an unmapped type, is refused. The inventory checkout comes from the repository registry, orINVENTORY.agent-pr-tokenlayout, the recipient line[ hostKey ] ++ vmKeys "a" ++ vmKeys "b"after the last such line, and the JSON group before the final brace with a comma added. The built text is validated before writing (brace balance for Nix,json.Validfor JSON); each file is replaced by temp-file-and-rename; a file that changed since it was read is refused, and files already replaced are restored on a later failure.^[a-z][a-z0-9-]*$(an unquoted Nix attribute and a filename), identifiers^[A-Za-z0-9][A-Za-z0-9_.@-]*$, the deployed path absolute with no whitespace, control character, quote, backslash, or$; JSON strings go through the encoder.credentials.nixattribute and thesecrets.nixpath as text, matched across whitespace, line breaks, and line comments; a registry group keyed by the name, and any group whosecredentials[]names the credential or its secret path; and the evaluated credential inventory (nix evalof the checkout'slib.credentials), because the template's credentials.nix merges generated entries the text scan cannot see. A failed evaluation refuses rather than trusting the text checks alone. Nix keywords are refused as names.declaresection indocs/allod-secret.mdahead of the landing half, describing the agent's step before the human runscreate.Risk
R2. Writes three files in a checkout, never commits, and produces a diff the inventory check validates before any ciphertext exists. Worst credible failure after the tests pass is inserted text the template's flake rejects, which fails the PR's checks rather than landing. Rollback is a revert.
Validation
gofmt -l .empty;go vetandgo test ./...green untagged and withsite,secret, andsite,secret, run by the overseer in the branch worktree after the final commit.noneandforgejogroups; every collision variant, including declarations split across lines, refuses and leaves all three files byte-identical; each missing or invalid flag names the flag or accepted values; injection attempts through every string flag refuse; a leading-digit name refuses; an unknown machine and an unmapped type refuse; the sabotaged registry refuses and restores; a file edited underneath refuses and restores the file already replaced; atomic replacement is unit-tested.forge-gitandmachine-hostaccepted as kinds, verify fields missing for the printed probes, generated inventory entries invisible to the text scan, Nix keywords as names, comments defeating the collision regexes), fixed in the fourth commit and read by the overseer. Two race findings between the re-read and the rename were discarded as inherent without file locking, and one finding that the JSON guard test does not run through the command was discarded because a registry that parses cannot become invalid by insertion after its last token; the end-to-end sabotage test covers the command path.checks.x86_64-linux.go-checksderivation builds green on the final commit.Refs allod/tools#182
Closes allod/tools#196
Cross-family review findings, all fixed: 1. Every flag value that lands in generated nix or JSON text is validated as a shape before it reaches any template. --owner, --kind, --account, and --ui-token-name must match ^[A-Za-z0-9][A-Za-z0-9_.@-]*$; --deployed-path must be an absolute path with no whitespace, control character, quote, backslash, or $. JSON content already went through encoding/json rather than raw formatting; this closes the one field that did not, --owner, interpolated into credentials.nix. 2. The name pattern allowed a leading digit, but the credentials.nix attribute is unquoted nix, so 1token = { ... } would not evaluate. Now ^[a-z][a-z0-9-]*$. 3. The credentials.nix and secrets.nix collision checks only matched their name/line on one line; a declaration split across lines slipped past and would have been duplicated. Both now match across whitespace including newlines. 4 & 5. Writes are now atomic (temp file, then rename, so a failed write never leaves a file truncated) and guarded against a concurrent edit: immediately before each file is replaced, declare re-reads it and refuses, restoring every file already replaced, if the bytes on disk no longer match what was read at the start. 6. The reparse-guard unit test is kept (it is the only way to exercise declareInsertRegistryGroup's post-build json.Valid check at all — for any JSON that parses to begin with, appending at the point this function finds can never produce invalid JSON, so no CLI-reachable fixture trips that specific guard). The existing end-to-end test that drives secretDeclare against the sabotage registry and asserts the refusal and byte-identical files is what review finding 6 asked for. 7. Usage text and docs/allod-secret.md now describe what the code actually does: validate-then-build in memory, atomic per-file replacement, refusal on a file changed underneath, and restoring every file already replaced on a later failure. A test was added for each behavior change. Refs allod/tools#196credential inventory for collisions, refuse nix keywords, and span comments in the collision regexes Second review pass, all five verified findings fixed: 1. --kind now accepts only user, agent, service. forge-git and machine-host are real credential-inventory kinds, but the shape declare writes (public_key null, one agenix consumer) is exactly what the inventory check rejects for them; the refusal names the three accepted kinds and says the other two are generated by the template. 2. Added --verify-repo-url, --verify-context, and --verify-user. rotate-token's printed verification (print_group_verification, scripts/rotate-token) reads .verify.repo_url and .verify.credential_context for git-ls-remote (printing the literal command 'git ls-remote - HEAD' without them) and .user for forge-token-verify on a nixos-host target ('sudo -u <user> forge token verify'). --verify-repo-url and --verify-user are required with their respective probe and refused with any other; --verify-context is optional but still refused outside git-ls-remote. All three are validated before use and emitted through the JSON encoder. 3. Before writing, declare now also evaluates the secrets checkout's own credential inventory (nix eval --json <checkout>#lib.credentials, confirmed against README.md and flake.nix in the secrets repository) and refuses a name found there, naming the evaluated source. credentials.nix is 'hostEntries // activeEntries // stagedEntries // forgeGitEntries // { ...literal...; }', so a name one of the generated attrsets already produces is invisible to the textual scan and the appended literal entry would silently shadow it. The textual checks stay, since they also catch a name on a branch that does not evaluate yet; a failed evaluation refuses outright rather than falling through to the text checks alone. 4. The name pattern now refuses nix keywords (let, in, with, rec, assert, if, then, else, inherit, or), which match the pattern but are not usable as unquoted attribute names. 5. The collision regexes now allow a nix line comment interleaved with whitespace between the name and '=', not just whitespace alone. A test was added for each behavior change, including one whose fixture credentials.nix text names only one credential while the fake evaluated inventory also knows a second, isolating the evaluated-only collision path, and one where the evaluated-inventory check itself fails. Refs allod/tools#196Three review findings on the branch, each a value that reached a consumer in a shape the consumer could not use. --to appended every comma-separated machine name to the target list without validating it, while --owner, --kind, --account, --ui-token-name, and --verify-user all went through validateIdentifier. A machine name is interpolated into the nix eval attribute path 'machines.<name>.type', into the secrets.nix recipient line, and into the JSON group, so it reaches the same generated text those flags do. --to '"dev-a"' evaluated successfully and wrote a broken declaration. Every name now goes through validateIdentifier("--to", machine) at parse time, so the usage error names the flag and the accepted pattern like the others. validateVerifyRepoURL checked only the 'https://' prefix, so the bare string 'https://' was accepted and rotate-token's printed verification became 'git ls-remote https:// HEAD'. The first character after the scheme must now be a letter or digit: refusing only a leading '/' would still let 'https://?query' through, which leaves the same command with no repository in it. The unsafe-character check is unchanged. inventoryCheckout trimmed trailing slashes off $INVENTORY, which turns the root into the empty string; nix would then have run in the process working directory instead of /. The trim stays and an empty result becomes "/". filepath.Clean would have done both in one call but resolves '..' lexically, so INVENTORY=/base/link/../inventory, with link a symlink elsewhere, would name a different directory than the one the OS reaches. Tests: three --to cases (a quoted name, a nix interpolation, whitespace) and three --verify-repo-url cases (no host, a host starting with '/', a host starting with '?') join the existing injection table, each asserting exit 1, the flag named on stderr, and all three fixture files untouched; a unit test pins inventoryCheckout for INVENTORY=/, /tmp/x/, and /tmp/x. All seven fail against the unfixed code. The two docs sentences that stated the old --verify-repo-url shape and omitted --to from the identifier list are corrected. Refs allod/tools#1962eb07b4baato551163f0a3