update: one command that updates a machine and cannot strand it - #81
Merged
Conversation
The verification core of `fabric update`, with no IO in it yet, so every decision it makes is testable without a network or a filesystem. WHAT IT REFUSES, which is the point of the module. `--url` without `--sha256` is rejected outright. There is nothing sensible to default to: the whole point of `--url` is that fabric does not know what is there, so installing it unchecked would be remote code execution with good manners. `--tag` with `--url` names two artifacts. `--sha256` without `--url` is a mistake worth saying out loud rather than ignoring, because a release carries its own checksum. The archive must hold EXACTLY ONE MEMBER NAMED `fabric`. Not two, not a directory, not `./fabric`. WHAT THE CHECKSUM DOES AND DOES NOT DO, said in the module doc rather than implied. With `--url` and an explicit hash it is a real check that the bytes are the ones the caller named. On the release paths the sidecar comes from the SAME server as the artifact, so it protects against corruption and truncation and NOT against a compromised release. Ordinary for a release install, and written down because "verify" reads as security to whoever arrives next. THE SIDECAR PARSE IS A REAL TRAP, inherited from the shell script. The published file reads `<hash> dist/fabric-<target>.tar.gz`, carrying the path it had on the builder. That path does not exist on the machine installing, so `shasum -c` fails on the directory name rather than on the bytes. Field one is taken directly. `--check` gets THREE exit states, not two: 0 up to date, 1 update available, 2 error. A sweep that cannot tell "the release server is unreachable" from "an update is available" will act on the wrong one. TWO THINGS THE TESTS TAUGHT ME, both worth more than the code they changed. The `./fabric` case cannot be fixtured with `tar::Builder`, because it normalises the name on the way in. The test was passing against an archive that said `fabric`. It now builds a 512-byte header by hand AND asserts the fixture really carries the dot-slash before testing anything, so it cannot quietly go back to proving nothing. And I wrote a comment claiming the parsed path would ACCEPT `./fabric`, so comparing raw bytes was a fix. Mutating the code back proved it would not: the crate normalises on write, not on read, and both forms reject it. The raw comparison stays, as belt and braces rather than a fix, and the comment now says so. A flattering explanation of a change is worse than none. Adds `tar` and `flate2`. `reqwest`, `rustls` and `sha2` were already compiled as transitive dependencies of iroh, so naming them costs no new crates; the reqwest feature pin keeps `aws-lc-rs` out from beside the ring provider iroh already uses. Agent: Silber.fabric
`fabric update` replaces this machine's fabric with a verified build, then
re-renders the service and restarts it.
WHAT IT REPLACES. A shell script that lived on one machine and was
base64-encoded across the wire to the others, plus two more copies of the
same recipe in `install.sh` and the README. Each copy knew a trap the
others did not. This is one copy, in the binary, with the traps as tests.
fabric update [--tag T] [--url U --sha256 H]
[--check] [--dry-run] [--no-restart] [--rollback]
WHAT IT REFUSES. `--url` without `--sha256`, because installing bytes
nobody checked is remote code execution with good manners and there is
nothing sensible to default to. Two sources at once. An archive that is
not exactly one member named `fabric`. A staged binary whose `--version`
disagrees with the tag that promised it. Every refusal happens before
anything is moved.
`--check` HAS THREE EXIT STATES, NOT TWO: 0 up to date, 1 update
available, 2 error. The error path is special-cased in `main` rather than
left to propagate, because propagating would exit 1, and a fleet sweep
would read an unreachable release server as work to do.
WHAT THE CHECKSUM DOES AND DOES NOT DO, in the module doc and in `--help`.
With `--url` and an explicit hash it proves the bytes are the ones the
caller named. On the release paths the sidecar comes from the SAME SERVER
as the artifact, so it catches corruption and truncation and NOT a
compromised release. A test fails if that wording disappears, because
"verify" reads as security to whoever arrives next.
IT INSTALLS AT THE PATH THE SERVICE MANAGER RUNS, read from the plist or
from `ExecStart`, not from `$PATH`. Installing at the wrong one leaves the
daemon on the old binary while `--version` reports the new.
ON LINUX THE RESTART SUPERVISES ITSELF, because nothing outside can reach
a machine whose daemon is down: `fabric exec` stops working and the tool
that would repair it is the tool that broke it. A transient unit runs the
ROLLBACK binary, the copy already proven to work there, waits for the
control socket, and puts the old binary back if the daemon does not
return. It cannot run in the updating process, which dies with the cgroup.
A BUG I ALMOST SHIPPED, found by reading rather than by a failing test.
`service::install` renders the unit from `env::current_exe()`, so
re-rendering during an update would have pointed the daemon at whatever
binary ran the update — a `target/debug` build while testing. That is the
wrong-path trap entered from the other side. `install_at` takes the exe
explicitly, and a test asserts the rendered unit names the binary it was
GIVEN and specifically not the one rendering it.
AND A TEST THAT WAS PROVING NOTHING. The `./fabric` case was passing
against an archive that said `fabric`, because `tar::Builder` normalises
the name on the way in. It now builds a 512-byte header by hand AND
asserts the fixture really carries the dot-slash before testing anything.
I also claimed in a comment that comparing raw bytes fixed a bug there;
mutating the code back proved it did not, and the comment now says so.
VERIFIED ON SILBER against the real release server: `--check` 0, an older
tag 1, a nonexistent tag 2; `--dry-run` left the binary byte-identical
with no temporary files; `--url file:///` installed a locally built
artifact through the same hash check; both refusals changed nothing.
`tar` and `flate2` are new. `reqwest`, `rustls` and `sha2` were already
compiled under iroh, so naming them costs no crates.
Agent: Silber.fabric
`schedule_supervisor` built its command inline inside a `cfg(target_os = "linux")` block, so the one shape whose regression leaves a Linux machine down with no way back in could only be checked on the platform where it does the damage. The argv is now built by `supervisor_argv`, which is not cfg-gated, and a test pins it: scheduled rather than inline, invoking the supervising subcommand, and told what to restore. IT ALSO PINS THAT THE SUPERVISOR IS THE ROLLBACK BINARY. Asking a possibly-broken new binary to supervise its own installation is not supervision, and that is the sort of detail a later refactor simplifies away without noticing. I mutated it to run inline and watched it fail with "the supervisor would die with the cgroup it is meant to outlive". Same reasoning as `systemd_restart_argv` in #80. A Linux-only string cannot be tested from a Mac, and these two are the strings that strand a remote machine. Agent: Silber.fabric
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fabric updatereplaces this machine's fabric with a verified build, then re-renders the service and restarts it.What it replaces
A shell script that lived on one machine and was base64-encoded across the wire to the others, plus two more copies of the same recipe in
install.shand the README. Each copy knew a trap the others did not. This is one copy, in the binary, with the traps as tests.What it refuses
--urlwithout--sha256— installing bytes nobody checked is remote code execution with good manners, and there's nothing sensible to default to. Two sources at once. An archive that isn't exactly one member namedfabric. A staged binary whose--versiondisagrees with the tag that promised it. Every refusal happens before anything is moved.--checkhas three exit states, not two0up to date,1update available,2error. The error path is special-cased inmainrather than left to propagate — propagating would exit1, and a fleet sweep would read an unreachable release server as work to do.What the checksum does and does not do
With
--urland an explicit hash it proves the bytes are the ones the caller named. On the release paths the sidecar comes from the same server as the artifact, so it catches corruption and truncation and not a compromised release. A test fails if that wording disappears from--help, because "verify" reads as security to whoever arrives next.Two traps carried over from the shell script
It installs at the path the service manager runs, read from the plist or
ExecStart, not from$PATH. Installing at the wrong one leaves the daemon on the old binary while--versionreports the new.On Linux the restart supervises itself. Nothing outside can reach a machine whose daemon is down —
fabric execstops working and the tool that would repair it is the tool that broke it. A transient unit runs the rollback binary, the copy already proven to work there, waits for the control socket, and puts the old binary back if the daemon doesn't return. It can't run in the updating process, which dies with the cgroup.A bug I almost shipped
Found by reading, not by a failing test.
service::installrenders the unit fromenv::current_exe(), so re-rendering during an update would have pointed the daemon at whatever binary ran the update — atarget/debugbuild while testing. That's the wrong-path trap entered from the other side.install_atnow takes the exe explicitly, and a test asserts the rendered unit names the binary it was given and specifically not the one rendering it.And a test that was proving nothing
The
./fabriccase was passing against an archive that saidfabric, becausetar::Buildernormalises the name on the way in. It now builds a 512-byte header by hand and asserts the fixture really carries the dot-slash before testing anything.I also claimed in a comment that comparing raw bytes fixed a bug there. Mutating the code back proved it did not — the crate normalises on write, not read, and both forms reject it. The comparison stays as belt and braces, and the comment says so. A flattering explanation of a change is worse than none.
Verified on Silber, against the real release server
--checkup to date, exit 0--check --tag <older>update available, exit 1--check --tag <nonexistent>--dry-run--url file:///…--urlwith no hash / wrong hashFull suite green: 267 lib, plus every integration target including the new
updateone, which is wired into CI as the #76 guard requires.Dependencies
tarandflate2are new.reqwest,rustlsandsha2were already compiled under iroh, so naming them costs no crates; the reqwest feature pin keepsaws-lc-rsout from beside the ring provider iroh already uses.Builds on #80.
Agent: Silber.fabric