Specify and plan M3b: the runner and the tools
A draft spec for the owner's review and 13 offline tasks with their given tests: shared tool arguments and host rules in proto, the sealed fetch target (M3a finding 14), the toolkit tools and SOCKS5 egress proxy, and brokerd's [runner], podman argument lists, runtime and proxy lifecycle. Each task's tests were run against a reference at that task's end state (560 to 638 tests, clippy clean); the reference is not in the repository. Adds the runner-unavailable runbook entry and tip T23 (ETXTBSY in script tests). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,104 @@
|
||||
# M3b task 02: seal the fetch target
|
||||
|
||||
**Branch:** `m3b` (run `git switch m3b`; `git status --short` must be empty, otherwise stop)
|
||||
**Commit subject:** `Seal the fetch target: one value holds the URL and its host`
|
||||
|
||||
## Goal
|
||||
|
||||
Today `ToolArgs::HttpFetch { url, host }` has two public fields. Policy matches `host` against the
|
||||
grant; from M3b on, the runtime fetches `url`. Any code could build one where the two disagree
|
||||
(`url` of `evil.example`, `host` of `example.com`), and policy would allow a fetch of the wrong
|
||||
host. Make that impossible: one sealed value, built only by `args::parse` after the URL checks.
|
||||
This is M3a review finding 14.
|
||||
|
||||
## Files
|
||||
|
||||
- Copy: `crates/brokerd/tests/args.rs` (replaces the old one; one test now reads the parsed value
|
||||
through getters, because it can no longer build one)
|
||||
- Modify: `crates/brokerd/src/args.rs`, `crates/brokerd/src/policy.rs`,
|
||||
`crates/brokerd/src/runner.rs`, `docs/implementer-log.md`
|
||||
|
||||
## Interfaces
|
||||
|
||||
In `args.rs`, the variant becomes a tuple variant holding a new struct:
|
||||
|
||||
```rust
|
||||
pub enum ToolArgs {
|
||||
ReadFile { path: String },
|
||||
WriteFile { path: String, content: String },
|
||||
Shell { command: String, cwd: Option<String> },
|
||||
HttpFetch(FetchUrl),
|
||||
}
|
||||
|
||||
/// A URL that passed the checks, and its host. Only `parse` makes one, so the host policy matched
|
||||
/// is always the host of the URL the tool fetches.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub struct FetchUrl {
|
||||
url: String, // private
|
||||
host: String, // private
|
||||
}
|
||||
|
||||
impl FetchUrl {
|
||||
pub fn url(&self) -> &str;
|
||||
pub fn host(&self) -> &str;
|
||||
}
|
||||
```
|
||||
|
||||
No `pub fn new`, no `Default`, no `Deserialize`, no public field: `parse` (in the same module) is
|
||||
the only code that writes `FetchUrl { url, host }`.
|
||||
|
||||
Put these two doctests in `FetchUrl`'s doc comment, word for word. The first proves the struct
|
||||
cannot be built outside `args`; the second proves the first fails for that reason and not a typo:
|
||||
|
||||
````rust
|
||||
/// ```compile_fail
|
||||
/// let _ = brokerd::args::FetchUrl {
|
||||
/// url: "https://evil.example/".to_string(),
|
||||
/// host: "example.com".to_string(),
|
||||
/// };
|
||||
/// ```
|
||||
///
|
||||
/// ```
|
||||
/// let args = brokerd::args::parse(brokerd::args::ToolName::HttpFetch, r#"{"url":"https://example.com/a"}"#);
|
||||
/// let Ok(brokerd::args::ToolArgs::HttpFetch(target)) = args else { panic!("{args:?}") };
|
||||
/// assert_eq!((target.url(), target.host()), ("https://example.com/a", "example.com"));
|
||||
/// ```
|
||||
````
|
||||
|
||||
Every place that matched the old variant changes. There are four:
|
||||
|
||||
1. `ToolArgs::tool`: `ToolArgs::HttpFetch(_) => ToolName::HttpFetch`.
|
||||
2. `ToolArgs::canonical_json`: `ToolArgs::HttpFetch(target) =>` serialise
|
||||
`HttpFetchArgs { url: target.url.clone() }` (still only the URL: the host is not an argument).
|
||||
3. `parse`'s `ToolName::HttpFetch` arm: `Ok(ToolArgs::HttpFetch(FetchUrl { url: value.url, host }))`.
|
||||
4. `policy.rs`, in `covers`: `ToolArgs::HttpFetch(target) =>` and match the grant's patterns with
|
||||
`host_matches(pattern, target.host())`.
|
||||
|
||||
In `runner.rs`: `ToolArgs::HttpFetch { .. }` becomes `ToolArgs::HttpFetch(_)`.
|
||||
|
||||
Search for any other: `grep -rn "HttpFetch {" crates/` must show nothing when you are done.
|
||||
|
||||
## Steps
|
||||
|
||||
- [ ] **1. Copy.** `git switch m3b`, then
|
||||
`cp docs/plans/M3b/files/crates/brokerd/tests/args.rs crates/brokerd/tests/`
|
||||
- [ ] **2. See it fail.** `cargo test -p brokerd --test args`. Expected: it does not compile
|
||||
(`ToolArgs::HttpFetch(target)` is not a tuple variant yet).
|
||||
- [ ] **3. Make the change.** Run `cargo fmt --all`.
|
||||
- [ ] **4. See it pass.** `cargo test -p brokerd --test args --test policy --test runner`.
|
||||
Expected: `args` 13 passed; the others pass unchanged. `cargo test -p brokerd --doc`: every
|
||||
doctest passes, including your two new ones (`compile_fail` counts as passing when it fails to
|
||||
compile).
|
||||
- [ ] **5. Check the seal.** `grep -rn "FetchUrl {" crates/brokerd/src/` shows the struct
|
||||
definition, the `parse` line, and the doctest only.
|
||||
- [ ] **6. Run the gate.** `make gate`. Expected last line: `gate: ok`.
|
||||
- [ ] **7. Log and commit.** `git add crates/brokerd docs/implementer-log.md && git commit`
|
||||
|
||||
## Done when
|
||||
|
||||
- `cargo test -p brokerd` and `cargo test -p brokerd --doc` pass; `make gate` prints `gate: ok`.
|
||||
|
||||
## Stop and report if
|
||||
|
||||
- Some code outside `args.rs` needs to build a `FetchUrl`: that would break the seal.
|
||||
- A test other than `args.rs` needs a change.
|
||||
Reference in New Issue
Block a user