Repository navigation
feat(minimal): read client-owned store rules and refuse rules in the deny set - #2514
norrietaylor merged 14 commits into
Conversation
Add the [secret-store-rules] rule type and its checks: a rule naming a GitHub host, Minimal's own infrastructure, a denied injection header or the GitHub sign-in item is refused by name (BEP-034). An ask rule denies without a terminal (BEP-035). A store reference whose registered upstream the egress does not admit refuses with exit 3 (BEP-036). A project table is ignored with a warning (BEP-037).
Only the user's own config.toml can set secret-store rules (BEP-037). The table, at the top level or under [session], now gets its own warning in place of the generic unknown-field one.
config.toml now accepts the table. Reading the config refuses each rule the deny set covers, prints it by name on stderr, and leaves it out of the rules it returns (BEP-034).
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Reviewed: no findings; approval withheld, 3 threads unresolved at APPROVE withheld: 3 unresolved threads — minimal-foundry[bot] crates/boxes/src/core/bep.rs:1104, minimal-arch[bot] crates/minimal/src/config.rs:219, minimal-arch[bot] crates/boxes/src/core/bep.rs:1276. Resolving the last thread releases the approval on this head. Resolve the open threads. Resolving the last one releases the approval on this head; no new review is required. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none seen. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 3 conditions · the full ruling
The deny set for store rules is incomplete. MINIMAL_INFRASTRUCTURE is a second list, and it leaves out min.internal, so a rule aimed at host.min.internal is accepted. Gatehouse §6.10 line 849 says no upstream can name the BEP endpoints. C1 makes the check read bep::ca::DENY_SET. Line 849 also says a rule registers exact authorities with no patterns. C2 refuses *.example.com and the like. BEP-035 and BEP-036 are pure functions that no path calls. C3 asks for a filed issue for each piece of the deferred wiring. The TOML shape of a rule is an escalation, not a condition.
Ruled at 0a302e1 against gominimal/arch@109541a48c31 by claude-opus-5-5.
An upstream with userinfo, a path, brackets, a wildcard or whitespace named a host the deny set did not see. A header that is not an HTTP token, or a prefix with a control character, could inject a header line. Refuse each as a malformed field, by name.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
When a min command reads the client config.toml, a [secret-store-rules] rule aimed at a GitHub host or the Host header prints a refusal on stderr that names the rule and the file. The rule is then left out. A rule with an upstream such as x@api.github.com or *.github.com, or a header or prefix that contains CR or LF, is now refused by name too. Notes from the agent: This round adds StoreRuleRefusal::Malformed. It refuses an upstream that is not a plain host or host:port, a header that is not an HTTP token, and a prefix with a control character. The new message is mine because the task fixes no wording for it: "The secret-store rule in has the "", which is not . Change the rule." No part of this task calls check_store_references, store_consent or store_refusals in a real command. To connect them, box creation in crates/minimal/src/cmd/boxes/create.rs (near line 909) must call read_client_config_and_store_rules and pass the rules to expansion. That file is outside this task's layers, so a later task must make that change. Until then, the only enforcement that runs is the deny-set refusal at config read. I did not run just clippy-strict. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 7 conditions · the full ruling
This head adds only the malformed-field refusals, and those conform. The rest of the earlier findings stay open. The rules table breaks the attach path, because Config refuses unknown fields (C1). The rule shape is not the design's {store, id_glob, upstream, inject, action} array (C2). A rule with no action allows, but the design default is ask (C3). The deny set admits host.min.internal, the BEP address (C4). C5 to C7 cover the unmatched reference, the org file and the unfiled wiring residual.
Ruled at 8752a66 against gominimal/arch@109541a48c31 by claude-opus-5-5.
The store-rule check now takes bep::ca::DENY_SET from the client, so host.min.internal is refused. An upstream that is an IP address, has an empty label or has a port outside 1-65535 is refused as malformed. The client prints each refused rule once per process.
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
When min reads config.toml, it refuses a [secret-store-rules] rule aimed at a GitHub host such as api.github.com, at the Host header, or at host.min.internal or minimal.dev. It names the rule and the file on stderr once per command, and leaves the rule out. An upstream written as an IP address is refused too. Notes from the agent: Revise round 2. The infrastructure names now come from bep::ca::DENY_SET, through the new StoreDenySet::infrastructure field. MINIMAL_INFRASTRUCTURE is removed. Residuals for the PR body: no expansion path calls check_store_references (BEP-036, and the reference half of BEP-034), and no path calls store_consent (BEP-035). These calls belong in the box create path, which is outside this task's layers. I cannot file issues from this box, so a person must file them. clippy for -p boxes -p minimal printed warnings only in minimald and minimald-rpc. I did not change those crates. |
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The deny set now covers host.min.internal and refuses IP-address upstreams. This closes the earlier points. Two items stay open. store_refusals lets a store reference with no rule through, but Gatehouse §6.10 line 849 gates every store reference by a rule. Refuse that reference (C1). No code calls check_store_references or store_consent yet, and the PR body names no issue for that wiring. Link one (C2).
Ruled at 56abd89 against gominimal/arch@109541a48c31 by claude-opus-5-5.
…rsing Config now carries [secret-store-rules], so the attach reader parses a config.toml that holds rules. The client reads the rules of the organization config too and names that file in its refusals. Refusals print once per file in a process.
A rule upstream such as gist.github.com is refused as a GitHub host, as the infrastructure check already does for its names.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
A store rule in the user or the organization config.toml that is aimed at a GitHub host (or a name under one, such as gist.github.com) or at the Host header is refused by name on stderr when the configuration is read. The attach path still parses a config.toml that holds rules. Notes from the agent: Out of layers: crates/boxes/src/client/config.rs. Config now has a secret_store_rules field (serde rename secret-store-rules). Without it, the attach reader (read_config_or_default, deny_unknown_fields) fails on any config.toml that holds a rule (thread C1). No layer file can fix that. Organization rules are read from ORGANIZATION_CONFIG_PATH. A user rule replaces an organization rule for the same item. This precedence is my choice, and the spec does not set it. Still not done, and a later task owns it: no create or expansion path calls check_store_references or store_consent. That wiring needs crates/minimal/src/cmd/boxes/create.rs, which is outside this task's layers. I have no credential to file an issue for it, so the PR body or a person must record it as a residual. The rule shape (flat header/prefix/basic_auth, keyed by item id, optional store) and the default of no consent action are unchanged. They follow BEP-079 and BEP-052 in this task's spec excerpt, not the Gatehouse shape that threads C2 and C3 cite. cargo fmt ran after the test runs and changed only formatting. The task proof says when the configuration is read: the refusal prints when a command reads config.toml through read_client_config. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The store-rule read and refusal conform to Gatehouse §6.10 line 849. This head fixes the open items from the last round: it reads the org config, keeps the attach parser working and refuses names under a module host. One gap is left. The deny set holds only min.internal and minimal.dev, but line 849 also names the tenant issuer, the relay endpoints and the node-plane baseline set. Issue #2441 tracks that gap, but the PR body does not name it. Add it to the Residuals section, as C1 says.
Ruled at 0a54305 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 4 of 10 on a failed check suite: the coder changed nothing. Revise round 4. No code change in this round. CI: the cargo-deny failure is a new RUSTSEC advisory (timing variability) in ecdsa/primeorder/rfc6979, pulled in by russh 0.64.1, plus an unused RUSTSEC-2026-0009 ignore in deny.toml. This branch does not change Cargo.lock or deny.toml. The fix is a dependency bump or a deny.toml change, which is outside this task's layers. ci-success fails only because cargo-deny fails. For the PR body: under 'Wiring still owed by later tasks', no expansion path calls check_store_references or store_refusals, and no consent path calls store_consent. This session cannot file an issue, so a person must file one for that wiring and link it there. Under 'Residuals', name #2441: config.rs passes bep::ca::DENY_SET (min.internal, minimal.dev) as the infrastructure deny set. Gatehouse also names the tenant issuer, the BEP/EGW/relay endpoints and the node-plane baseline set, and #2441 owes those. Review threads the coder judged:
|
There was a problem hiding this comment.
Babysitter review at 0a54305.
Four files, all in the task's config layer as #2280 now reads (crates/boxes/src/client/config.rs was added for arch C1). bep.rs holds the rule type, the deny set, the refusals and their messages, plus the pure check_store_references and store_consent; config.rs reads the user and organization files, takes the rules out of Config and prints each refusal once per file per process; mfile ignores a project's table with the BEP-037 warning; the client Config type now parses a file that holds rules.
Checked against the diff:
- The deny set is one list: GitHub's module hosts and
bep::ca::DENY_SET, both matched by exact name or any name under it. Sign-in item identifiers come frombep::signin. - A rule reaches the name compare only as a plain
hostorhost:port: userinfo, paths, brackets, wildcards, whitespace, an empty host, IP literals (decimal and hex last label) and ports outside 1 to 65535 are refused as malformed and named back as written. Headers must be RFC 9110 tokens and prefixes visible ASCII, so no CRLF injection passes. - Every refusal states the rule, its file and one action, in the task's fixed words;
store_rules_read_through_client_configpins the stderr text for three refusals. - The organization file is read, a user rule wins for the same item, and a second read in one process prints nothing.
store_consentdenies anaskrule without a terminal (BEP-035);prop_store_upstream_outside_egress_is_exit_3pins BEP-036's exit 3 exactly when a registered host is outside the egress.
Not in this PR, by design and filed: no creation or expansion path calls check_store_references or store_consent yet (#2516), and the names Gatehouse lists beyond bep::ca::DENY_SET are #2441's. Both sit in the Residuals section.
No findings. Arch PASS_WITH_CONDITIONS stands at 0a54305 with its one condition met in the body.
Generated by Claude Code
A user rule replaces the fields of an organization rule for the same item but never loosens its deny or ask. Commands that use no store rules no longer fail on an organization file that does not parse.
|
Round 5 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A store rule in the client configuration that names a GitHub host or the Host header is refused by name on stderr when the configuration is read. A user rule can no longer loosen an organization deny or ask. A broken /etc/minimal/config.toml no longer fails min ls, create, task or loadout resolution. Notes from the agent: Round 5 changed only crates/minimal/src/config.rs. read_client_config, which list, create, task and loadouts use, now treats an organization file that is not read or does not parse as non-fatal. It prints one stderr line, logs one debug line and leaves out the organization rules. read_client_config_and_store_rules, for callers that use rules, still fails on that file. New printed text: "Minimal did not use /etc/minimal/config.toml. . The secret-store rules in that file do not apply to this command." |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
This push changes one doc comment and one stderr message in crates/minimal/src/config.rs. Neither change affects the store-rule checks that the ruling on ae74c8d3 covered, so that ruling stands. The doc comment is correct: read_client_config_and_store_rules fails closed on an unusable organization file. The wiring in #2516 must use that reader. Two conditions stay open as filed residuals. C1 (#2516) calls the consent and egress checks from box creation and expansion. C2 (#2441) adds the remaining infrastructure names that Gatehouse §6.10 line 849 lists.
Ruled at eb10c0e against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Revise round 4 ran against |
|
Round 7 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A secret-store rule in the user or organization client configuration that names a GitHub host or the Host header is refused by name on stderr when the configuration is read, and it is left out of the rules. A refused organization rule still keeps its deny or ask action over a user rule for the same item. Notes from the agent: This round changed one file: crates/minimal/src/config.rs. The organization rules as read, before refusal, now set the action floor after the merge, through keep_org_actions. The new test refused_org_rule_keeps_its_action_over_user_rule pins this (cargo nextest run -p minimal refused_org_rule: 1 passed). The other store_rule tests in the minimal crate also pass. Threads C1 and C2 from minimal-arch and the MEDIUM wiring finding repeat findings that earlier rounds answered. Issues #2441 and #2516 own that work, and it is outside this task's layers. I did not run just clippy-strict. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
The compare keeps the action of an org store rule as a floor, also when the deny set refuses that rule. A user rule can no longer loosen an org deny or ask that way. Gatehouse §6.10 line 849 says nothing on the org and user merge, and this change is fail closed. The two baseline conditions stay open as filed residuals: C1 (#2516) and C2 (#2441).
Ruled at e9c8c13 against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 8 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A store rule in the client configuration that names a GitHub host or the Host header is refused by name on stderr when the configuration is read. Its item no longer loses consent: the reader returns a deny rule for an item that has only refused rules. Notes from the agent: Revise round 8. I ran the five boxes test lines as one nextest invocation with five filters, not as five separate runs. The wiring of store_refusals, check_store_references and read_client_config_and_store_rules into box creation is still outside this task's layers. crates/minimal/src/cmd/boxes/create.rs and the box spec expansion code are not in layers. A later task must call them. |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
The compare conforms to Gatehouse §6.10 line 849. An item with only refused store rules now gets a deny rule, so its consent fails closed. A refused rule from either file also keeps its stricter action. The two filed residuals stay open as conditions: C1 (#2441) for the full infrastructure deny set and C2 (#2516) for the creation and expansion wiring.
Ruled at 959d878 against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
This head merges main into the branch. The merge does not change the four store-rule files. The ruling on 959d878 stands. The configuration-read refusal and the project-section warning conform to Gatehouse §6.10 line 849. C1 (#2441, the full infrastructure deny set) and C2 (#2516, the creation and expansion wiring) stay open as filed residuals.
Ruled at 1b3b848 against gominimal/arch@109541a48c31 by claude-opus-5-5.
A refused organization deny rule was dropped before the merge, so a user rule for the same item loosened it. The organization actions as read now set the floor after the merge.
A refused rule no longer drops its item's consent. The item gets a deny rule with no upstream, and a refused user rule keeps its stricter action.
1b3b848 to
4e536dc
Compare
There was a problem hiding this comment.
Babysitter verification at 4e536dc. This head is a message-only rewrite of 1b3b848: the two commit messages whose body lines ran to 176 and 145 characters are rewrapped at 100 under the repository's commitlint rule, and the base merge of main fa31776 is recreated on top. The tree is byte-identical to 1b3b848 (git diff 1b3b848d 4e536dc6 is empty), so the review at that head carries: arch PASS, zero open threads, every lane green there. The lanes and the arch review rerun here. Squash title feat(minimal): read client-owned store rules and refuse rules in the deny set and the squashed message now pass commitlint. Residuals #2516 and #2441 are scheduled as T54 and T55 in slice S9.
Generated by Claude Code
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
This head has the same tree as 1b3b848d. Only the history changed, so the earlier ruling stands. The config-read refusal and the project warning conform to Gatehouse §6.10 line 849. The infrastructure deny set is still short of that line (C1, #2441). No creation or expansion path calls the BEP-035 and BEP-036 checks yet (C2, #2516). Both are filed residuals, and the change can merge with them open.
Ruled at 4e536dc against gominimal/arch@109541a48c31 by claude-opus-5-5.
|
Round 10 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A secret-store rule in the client configuration that names a GitHub host or the Host header is refused by name when the configuration is read. A rule whose basic_auth is not user or password is now refused too, with a message that names the field and its value. Notes from the agent: Revise round 10. Only the basic_auth finding needed a code change. store_rule_refusal now refuses a basic_auth other than user or password as Malformed, and the message says the field must be user or password. The malformed test pins this. cargo clippy -p boxes --all-targets is clean. The two arch threads ask for work in files outside this task's layers: crates/bep/src/ca.rs for the full deny set (#2441), and the box creation and expansion paths for the callers (#2516). |
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 2 conditions · the full ruling
This push makes store_rule_refusal refuse a basic_auth that is not user or password. Gatehouse §6.10 line 849 says basic_auth fills one field of the credential, so the new check conforms and fails closed. The change adds no deviation. The two filed conditions from the baseline stay open: C1 (#2441) completes the infrastructure deny set, and C2 (#2516) wires the consent and egress checks into creation and expansion.
Ruled at 97cf1be against gominimal/arch@109541a48c31 by claude-opus-5-5.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 3 unresolved threads — minimal-foundry[bot] crates/boxes/src/core/bep.rs:1104, minimal-arch[bot] crates/minimal/src/config.rs:219, minimal-arch[bot] crates/boxes/src/core/bep.rs:1276. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
There was a problem hiding this comment.
Babysitter verification at 97cf1be. Round 10 answers the review bot's LOW on store_rule_refusal: a basic_auth value other than user or password is now refused as Malformed with its own Display case ("has the basic_auth "anything", which is not user or password. Change the rule."), pinned by the new cases in the malformed-rule test, and the two valid values still pass. The diff from 4e536dc is two commits touching only crates/boxes/src/core/bep.rs, inside T25's layers, and the messages pass commitlint. The arch review and the lanes rerun at this head. Squash title feat(minimal): read client-owned store rules and refuse rules in the deny set passes commitlint. Residuals #2516 and #2441 stay scheduled as T54 and T55 in slice S9.
Generated by Claude Code
There was a problem hiding this comment.
Babysitter approval at 97cf1be: every merge gate passes at this head (arch PASS with its two conditions held on the filed residuals #2441 and #2516, zero open threads, every lane aggregator green including ci-macos-success, bot resolutions re-verified). Squash title feat(minimal): read client-owned store rules and refuse rules in the deny set and the squashed message pass commitlint. Queued for squash merge.
Generated by Claude Code
T25 — Read client-owned store rules and refuse rules in the deny set
slice: S3
kind: task
covers: BEP-034, BEP-035, BEP-036, BEP-037
size: L
est: 350 code, 300 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: a refused rule prints "The secret-store rule in names , which a secret can never be sent to. Remove or change the rule." on stderr when the configuration is read, pinned by store_rule_in_deny_set_is_refused
observability: one debug log line per configuration read naming each rule refused and why
layers:
- config:
- crates/minimal/src/config.rs: reads
[secret-store-rules]from the client configuration- crates/boxes/src/core/bep.rs: the BEP-034, BEP-035 and BEP-036 refusals
- crates/mfile/src/lib.rs: ignores a project's
[secret-store-rules]with a warningtest: cargo nextest run -p boxes store_rule_in_deny_set_is_refused
test: cargo nextest run -p boxes store_rule_naming_sign_in_item_is_refused
test: cargo nextest run -p boxes ask_rule_without_tty_denies
test: cargo nextest run -p boxes prop_store_upstream_outside_egress_is_exit_3
test: cargo nextest run -p boxes project_store_rules_are_ignored_with_warning
produces: none
consumes: none
proof: A store rule aimed at a GitHub host or the Host header is refused by name when the configuration is read.
depends on:
- blocked by #2276
Proof
A person can put a [secret-store-rules.] rule in their config.toml. If the rule's upstream is a GitHub host (for example api.github.com) or its header is Host, any min command that reads the client configuration prints "The secret-store rule in names api.github.com, which a secret can never be sent to. Remove or change the rule." on stderr (for a header, "names the header Host"), and leaves that rule out.
Notes from the agent: Rule shape (the spec does not fix it): [secret-store-rules.] with the table key as the item identifier and the rule name, and the fields store (optional, None = the host's native store), upstream, header + optional prefix or basic_auth, and action = allow|ask|deny (optional). The spec names only "ask". allow and deny are my additions. A rule with no upstream, or with not exactly one injection form, is refused too, with its own message. Store references: the tree has no source = "store". Existing code and tests treat a non-github: scope on a broker grant (
<store>:<id>, for example keychain:npm) as a store reference, so StoreReference follows that convention and no GrantSource variant was added. Adding one would also need create.rs, which is out of scope. Minimal's own infrastructure is the domain minimal.dev and its subdomains (MINIMAL_INFRASTRUCTURE). Extend it if Gatehouse or other hosts belong there. The GitHub sign-in item identifiers come from bep::signin::SERVICE and META_SERVICE, through StoreDenySet, because boxes does not depend on bep. Wiring still owed by later tasks: check_store_references (BEP-034 reference half, BEP-036 exit 3) and store_consent (BEP-035) are pure functions with tests, but no create or expansion path calls them yet, because create.rs and the expansion code are not in this task's layers. read_client_config_and_store_rules returns the accepted rules, and nothing consumes them yet (the bep crate still reads no rules). read_client_config now prints refusals on each read. A command th…[truncated]Tests
cargo nextest run -p boxes store_rule_in_deny_set_is_refusedcargo nextest run -p boxes store_rule_naming_sign_in_item_is_refusedcargo nextest run -p boxes ask_rule_without_tty_deniescargo nextest run -p boxes prop_store_upstream_outside_egress_is_exit_3cargo nextest run -p boxes project_store_rules_are_ignored_with_warningVerify
min task run agent-verifyexited 101 under rustc 1.98.1, which the repository does not pin (1.97.0 in rust-toolchain.toml); the PR is not a draft on that alone — CI, which honours the pin, is the gate.Residuals
check_store_references(BEP-034 reference half, BEP-036 exit 3) andstore_consent(BEP-035) from box creation and expansion, and hand the accepted rules to the proxy's registration (arch C3 at 0a302e1, C7 at 8752a66). This PR adds the checks as pure functions; only the config-read deny-set refusal runs today.bep::ca::DENY_SET, which holdsmin.internalandminimal.devonly; Gatehouse §6.10 line 849 also names the tenant issuer, the BEP, EGW and relay endpoints and the node-plane baseline set, and Decide and share the fixed deny set for the box CA name constraints (BEP-013, BEP-034) #2441 owes those names (arch C1 at 0a54305).Closes #2280
Generated by Claude Code