Repository navigation
Protect root upgrade throttle state - #644
Open
pithead-developer[bot] wants to merge 2 commits into
Open
pithead-developer[bot] wants to merge 2 commits into
pithead-developer[bot] wants to merge 2 commits into
Conversation
Part of #627. Implemented by an operator-run gpt-6-astra session at the owner's request. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Operator: hold lifted so the runner's reviewer takes this draft. It fixes a confirmed finding from the operator's RigForge audit; bugs go into the next patch release (v1.18.1, cut from develop). It merges only after the agent review passes and the owner approves at the exact head. |
VijitSingh97
reviewed
Nov 4, 2026
VijitSingh97
left a comment
Contributor
There was a problem hiding this comment.
Reviewed by Hermes agent (operator's delegate) against origin/develop b82758b: full diff read, make test-suite run per PR, body claims reproduced. Not a substitute for the owner's approval; merge stays with the operator.
Verdict: PASS
- The throttle move closes the #627 privilege crossing: lock+stamp live in root-only StateDirectory
rigforge-control-upgrade0700; type/owner/mode/nlink validated; stamp replaced via mktemp+mv; subshell function guarantees fd close + flock release on every path; every unsafe path fails closed rc 2. Reproduced before-fix FAIL and after-fix PASS ontests/test-upgrade-throttle-security.sh; full suite 2431 passed / 0 failed; shellcheck clean; file budget OK; net -3 lines as claimed. - Scope point: the PR functionally implements all three #636 acceptance criteria (flock failure rc 2 at
rigforge.sh:4705, write failure rc 2 at :4714-4717, malformed stamp rc 2 at :4710-4711, with fault-injection regressions) but declares onlyCloses #627, so #636 will not auto-close — link it or leave a deliberate note. - Nits (non-blocking): GNU
stat -cgates mean non-Linux invocations refuse (safe direction, but worth a doc line,rigforge.sh:4696-4702);RIGFORGE_CONTROL_UPGRADE_STATEoverride undocumented (rigforge.sh:4813); pre-existing:CONTROL_UPGRADE_MIN_INTERVAL=""silently disables throttling (rigforge.sh:4712).
VijitSingh97
marked this pull request as ready for review
November 4, 2026 07:30
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.
What's the problem
The root remote upgrader opened its throttle lock and timestamp in receiver-writable state. A compromised receiver could plant a symlink and cause a privileged write to its target.
How I fixed it
Give the root upgrade unit a separate persistent state directory with mode
0700. Reject unsafe ownership, modes, links and file types before opening either throttle object; fail closed on locking and timestamp errors, and replace the timestamp atomically. Never open or migrate the receiver's old throttle files, so the first attempt starts a fresh cooldown.Update the callers, isolated fixtures, historical-release hardware fixture and operating documentation. Add regressions for both existing-target symlinks, untouched legacy files, cooldown persistence, unsafe objects and failed I/O.
Here is the proof
The final security regression fails against the original product script, with exit 1 and
FAIL: .upgrade-throttle.lock did not fail the upgrade:After the fix:
bash tests/test-upgrade-throttle-security.sh rigforge.sh— PASS on macOS.shellcheck --severity=warning tests/test-upgrade-throttle-security.sh— PASS.shellcheck --severity=warning tests/test-upgrade-throttle-security.sh tests/test-control-upgrade.sh tests/test-e2e-gate-fail-closed.sh tests/e2e-real.sh— PASS.bash -n rigforge.sh tests/run.sh tests/test-control-upgrade.sh tests/test-upgrade-throttle-security.sh— PASS./tmp/rigforge-627-tools/bin/shfmt -i 4 -d $(git ls-files '*.sh') tests/test-upgrade-throttle-security.sh— PASS with the repository's pinned shfmt 3.14.1.FILE_BUDGET_REQUIRE_BASE=1 bash scripts/lint-file-budget.sh— PASS; the main script shrinks by three lines and the aggregate suite stays within its ceiling.git diff --check— PASS.The following command passes the complete security matrix as root on Linux, using real ownership, permissions and link counts:
That image is the arm64 manifest from the repository's pinned Ubuntu index. Both disposable deployment passes also pass:
Results: normal deployment 87 passed, 0 failed; appliance 28 passed, 0 failed. The compile and service manager are stubbed; the arm64 run skips the x86 MSR branch.
The historical-release fixture refuses to POST after failed cleanup, with a successful-cleanup control proving the POST observation works. This focused command passes:
make testpassed ShellCheck and failed formatting because the installed shfmt 3.13.1 differs from the repository's 3.14.1 pin on existing lines. The pinned formatter passes without changing those lines.timeout -k 30 3600 env PATH="/tmp/rigforge-627-tools/bin:$PATH" make lintthen passes the complete lint target.The first
make test-suiterun finished 2,430 passed, 1 failed: the unchanged API-server test considered the malformed-config process still alive after one second. All upgrade-throttle cases passed.git diff --exit-code HEAD -- util/api-server.pypasses, and this original-script control reproduced the timing failure: alive after one second, then exit 1 with the malformed config refused.timeout -k 30 3600 make test-suite— PASS on the unchanged retry: 2,431 passed, 0 failed, followed by passing credential, access-readiness, documentation-pin, staged-rebuild and tool-pin suites.Markdown verification:
make lint-md— could not use the existing npm cache.npm_config_cache=/tmp/rigforge-627-npm make lint-md— fails only on pre-existing consecutive blank lines in untrackedISSUE.md.npm_config_cache=/tmp/rigforge-627-npm make lint-md MD_FILES='"**/*.md" "#ISSUE.md"'— PASS, 0 issues, with the supplied issue artifact excluded and unchanged.The initial pinned-index container attempt with
--platform linux/amd64exited 255 (exec format error). Pulling its arm64 variant under the same index reference failed because Docker could not overwrite the cached digest; using the index's immutable arm64 child digest above succeeded.Independent code and security reviews found no code defects. No live bench, KVM or hardware jobs were run. Real systemd restart/reboot persistence and the worker-to-coordinator remote-upgrade path still need bench/hardware proof; no KVM-specific proof applies to this change. Coverage was not run; the amd64 container could not execute on this engine.
Closes #627
Draft from an operator-run gpt-6-astra session (owner request, 2026-11-01). The operator reviews and gates it before it leaves draft.
🤖 Generated with Claude Code