fix(daemon): replace shell grants with bounded action helpers - #429
Conversation
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at 82bb53ff77ad95565aa4597c544d92077c0368c7.
This removes the two grants that made everything else in packaging/sysknife-sudoers advisory:
-sysknife ALL=(root) NOPASSWD: /usr/bin/sh
-sysknife ALL=(root) NOPASSWD: /usr/sbin/runuser
The file's own header admitted what they meant ("those make the daemon effectively root-capable, so the security boundary is the daemon's own authorization"). The TODO(post-launch hardening) above the sh grant has been sitting there asking for exactly this refactor. You did it, across nine call sites, without changing what the actions do.
The guard bites
I proved it rather than reading it. Reverting one production file to main's version, in a network-isolated container:
$ git checkout origin/main -- crates/sysknife-daemon/src/actions/ssh.rs
$ cargo test -p sysknife-daemon --test sudoers_grants_match_argv --offline
running 2 tests
test every_sudo_action_is_authorised_by_a_packaged_grant ... FAILED
panicked at crates/sysknife-daemon/tests/sudoers_grants_match_argv.rs:108:13:
AddAuthorizedKey must use bounded argv instead of a privileged shell/user launcher
test result: FAILED. 1 passed; 1 failed
Unmutated at your head, the same command reports running 2 tests ... 2 passed. So the assertion fails when a shell launcher comes back, and it names the action that brought it. That is the guard I want attached to this change, because the failure mode is someone quietly reintroducing a sh -c call site a year from now.
The helper
The nine sudoers entries end in *, and sudo matches command arguments as a single concatenated string, so each grant admits arbitrary trailing text. That makes action-steps its own boundary: anything that can run sudo action-steps ssh-add ... reaches it directly, skipping the preview, the approval interlock and the signed chain. I read it on that basis rather than as a convenience wrapper, and it holds up.
The credential drop is in the right order, which is the detail that most implementations get wrong:
os.initgroups(account.pw_name, account.pw_gid)
os.setgid(account.pw_gid)
os.setuid(account.pw_uid)
if os.getuid() != account.pw_uid or os.geteuid() != account.pw_uid:
raise PermissionError("failed to drop root credentials")Supplementary groups first (only possible while root), then GID, then UID, then a verification that it actually took. Reversing the last two would silently leave the process in root's group. Refusing pw_uid == 0 and requiring an absolute home closes the obvious way to ask it to operate on root's account.
edit_key opens with O_NOFOLLOW | O_NONBLOCK and then checks S_ISREG. The O_NONBLOCK is load-bearing and easy to mistake for noise: without it, a FIFO planted at ~/.ssh/authorized_keys would block the open forever, since O_NOFOLLOW only refuses symlinks. Whole-line comparison keeps the fix from #133 intact, so a key like ssh-ed25519 .* still cannot act as a pattern.
#!/usr/bin/python3 -I is the right shebang. Isolated mode ignores PYTHONPATH and user site-packages, so a writable directory in the daemon user's environment cannot inject a module into a root-run helper.
I checked one behavior I expected to be a regression and it is not. The new ssh-add path opens ~/.ssh/authorized_keys with O_CREAT but never creates ~/.ssh. main's ADD_KEY_SCRIPT is ... || printf '%s\n' "$key" >> "$path", which also fails when the directory is absent. Same behavior on a fresh account, so nothing is lost.
Installation is correct in both paths: install -Dm 755 root-owned in the Makefile and in ubuntu-provision.sh, and daemon-uninstall removes it.
Why I am not merging it today
This touches .github/workflows/**. My rule is that a workflow diff gets a human reading it before the merge button, regardless of how small the hunk is, so this one waits for that rather than for anything you need to do. It is in my run report as ready.
Two coordination notes, neither your fault:
#421 edits the same function at the same insertion point, so the two conflict textually. Both additions are compatible and the resolution is to keep both lines. I will sequence them.
Your lockfile commit clears RUSTSEC-2026-0285, which is red on main right now. I am landing that through #426 first so the rest of the queue goes green.
One note for the release
You have this right in the body and I want it recorded in the merge too: user-scoped operations now refuse UID 0, SSH edits refuse symlinks, user startup files no longer execute, and the daemon, helper and sudoers file must be installed together. That is a behavior change, and under 0.y it moves the middle digit. I will write the CHANGELOG entry to say so plainly rather than filing it under a fix.
Approving. This is the most consequential change anyone has contributed to this repo, and the fact that it comes with a guard that fails when the grants come back is why I am comfortable with it.
|
Two merges from me put your three open branches behind, and every conflict below is my merge order rather than anything you wrote. Recording it on one thread instead of three.
One more on #413: Say the word if you would rather I take the rebases. Otherwise push when it suits you and I will re-review at the new heads. #427 and #429 both touch |
Three conflicts, all from my merge order rather than from this branch. sudoers_grants_match_argv.rs: lacs-project#421's `checked += 1` and this branch's shell-launcher assertion were inserted at the same point. Both kept, the counter first so every examined spec is counted before the assertion can panic. The test compiles and passes: 2 passed, 0 failed. CHANGELOG.md: a `### Changed` section against main's `### Fixed`. Both kept, Changed first per Keep a Changelog ordering. docs/action-reference.md: not hand-merged. The file is generated, and the two sides were different generations: this branch rewrote the command column to the action-steps helper, lacs-project#426 stripped "; Ubuntu only" from descriptions. Hand- merging a generated file is how it drifts from its source, so it was regenerated from the merged catalogue: $ UPDATE_ACTION_REFERENCE=1 cargo test -p sysknife-daemon --test action_reference_doc test action_reference_doc_is_current ... ok $ cargo test -p sysknife-daemon --test action_reference_doc test action_reference_doc_is_current ... ok The result carries both changes: 12 action-steps flatpak commands, zero `runuser -u testuser`, and the stripped descriptions.
# Conflicts: # .github/workflows/ci.yml
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Re-approving at c38c9ba. I have merged main into this branch twice tonight, and I caused both rounds of conflicts by the order I chose to merge other PRs in. None of it is on you.
This round was one file. #427 landed a Check local gate discovery and required-check warnings step at the same anchor in ci.yml where you add Check bounded actions and removal of shell grants, so git could not pick an order. I kept both, #427's first. Against main, the resolved workflow adds your four lines and nothing else:
- name: Check local gate discovery and required-check warnings
run: bash tests/release/ci-local.test.sh
+ - name: Check bounded actions and removal of shell grants
+ # Root is used only in an isolated fixture subprocess to prove that the
+ # helper irreversibly drops credentials before touching user files.
+ run: sudo -n bash tests/release/action-steps.test.sh
+
- name: Check the audit export states its confidentiality classEverything else in the merge is git's own three-way result. I checked that by diffing my merge tree against git merge-tree --write-tree, and .github/workflows/ci.yml is the only path that differs.
The earlier round was three files: sudoers_grants_match_argv.rs, where I kept both the new counter and your shell-launcher assertion; CHANGELOG.md; and docs/action-reference.md, which is generated, so I regenerated it with UPDATE_ACTION_REFERENCE=1 cargo test -p sysknife-daemon --test action_reference_doc instead of hand-merging. The regenerated file carries both sides: 12 action-steps flatpak commands and zero runuser -u testuser.
One consequence of #427 you could not have seen coming. Its local runner discovers every tests/release/*.test.sh and runs it with plain bash, so your test now runs for contributors who have no sudo. I ran it as an unprivileged user:
test_real_drop_cannot_regain_root_or_write_through_a_symlink ... skipped 'real credential-drop test requires a root test subprocess'
Ran 11 tests in 0.018s
OK (skipped=1)
Your skipUnless(os.geteuid() == 0) does the right thing there, and the other ten still assert. scripts/ci-local.sh stays green without root.
Nothing left for you to do. I will merge once the board is green.
|
Merged. #417 is closed with it. Nine bounded grants in place of the Two things worth saying about the end of this one, neither of them yours. Both rounds of conflicts came from the order I merged other pull requests in, and I resolved them rather than handing them back. And the gate could not prove this one at all when I started: your diff touches Your You have eleven merged here. If you want the next one, #301 came back into the pool tonight: |
2 participants
Fixes #417.
Remove the unrestricted
/usr/bin/shand/usr/sbin/runusergrants. The complete call-site inventory is in the issue comment: firewall chaining, Fedora group materialization, snap install-then-hold, SSH key edits, Podman/Toolbox login-shell operations, and the Flatpak consumers of runuser.These paths now invoke the root-owned
action-stepshelper with one of nine explicit operations. Each sudoers grant names the operation; the helper independently validates the full argument grammar and invokes fixed absolute binaries without a shell. Failed install/firewall/group preparation stops subsequent work. Snap retains its exclusive-resource classification.For user operations, NSS resolves the target account, then the helper replaces supplementary groups and drops GID/UID before accessing the home or running user-configurable tools. It sets an explicit HOME/XDG/session-bus environment. It never accepts an arbitrary command, environment assignment, or file path. SSH edits compare literal whole lines, retain the inode/mode, and refuse symlinks. Both source-install and Ubuntu provisioning paths install the helper; uninstall removes it.
Compatibility: user-scoped operations now refuse UID 0, SSH edits refuse symlinks, and user startup files are not executed. The new daemon/helper/sudoers must be installed together. These restrictions are a behavior change for the next minor release under the project's 0.y versioning policy. Existing unrelated unprivileged Bash readers (APT history/upgradable packages/reboot sentinel) do not depend on either grant and remain outside this privilege-boundary change. Other powerful administrative grants remain; this PR does not claim sudoers alone is a complete security boundary.
Validation:
CI also surfaced the newly published RUSTSEC-2026-0285 in the inherited lockfile. A separate commit updates rustls to 0.23.45 and its required TLS dependencies; the final security-audit check passes.
Codex assisted with implementation and validation.