🔒 fix(security): stop hive-snapshot.service executing code planted under /tmp (#5483)

[clubanderson]

Security Fix

Files claimed: systemd/hive-snapshot.service, src/deploy/test_hive_snapshot_unit_contract.sh (new), .github/workflows/v2-ci.yml, bin/hive-deploy.sh (comment only), CHANGELOG.md.

hive-snapshot.service ran ExecStart=/tmp/hive/dashboard/publish-snapshot.sh, resolving the executed script out of a world-writable parent that is cleared on every reboot. /tmp's sticky bit stops a user deleting or renaming entries owned by someone else — it does not stop them creating /tmp/hive/dashboard/publish-snapshot.sh in the window after a reboot wipes /tmp and before hive-deploy repopulates the checkout. This was the last remaining unit in systemd/ with that shape.

Severity, stated precisely. Lower than #5435 and I am not claiming otherwise: the unit is Type=oneshot driven by hive-snapshot.timer, not Restart=always, so planted code runs on the timer's 15-minute schedule rather than immediately at boot, and the unit carries no EnvironmentFile, so systemd hands it no credential. It is not zero, though, and the issue understates it slightly: publish-snapshot.sh itself reads a GitHub App token from /var/run/hive-metrics/gh-app-token.cache and uses it to push branches and merge PRs into kubestellar/docs. So the payoff is dev execution that reaches a write credential, just not one systemd placed in the environment.

The guard generalized unmodified

bin/hive-checkout-guard.sh is byte-identical to what #5481 landed — git diff against origin/v4 shows no change to it. It already resolves the service user from the running EUID rather than hardcoding dev, and already takes the directory and entrypoint list as arguments, so it needed nothing:

ExecStartPre=/usr/local/bin/hive-checkout-guard.sh /tmp/hive/dashboard publish-snapshot.sh

As the issue notes, publish-snapshot.sh ships mode 755 rather than 644, so the relevant assertion is the guard's group/other-writable check on the file; the exec bit is irrelevant to it. The test pins both directions of that — 755 must be accepted (or every real run breaks) while 775 and 757 must be refused, so the guard keys on the write bits rather than on mode equality.

bin/hive-deploy.sh needed only a comment update: #5481 already added the explicit bootstrap block, and it installs the one file both units now depend on. The comment named only the discord unit and said "refuse to start the bot"; it now names both units. No behaviour change there.

Demonstrated failing, then passing

Per the #5388 standard, the test was run against the unfixed unit before the fix was written.

Against the unfixed unit (git show origin/v4:systemd/hive-snapshot.service) — 18 passed, 3 failed, exit 1:

--- the unit file ---
  FAIL: hive-snapshot.service has an ExecStartPre guard
        without one, systemd executes whatever publish-snapshot.sh is present when the timer fires
  PASS: PrivateTmp is not set (it would hide the /tmp checkout and the /tmp docs clone)
  FAIL: NoNewPrivileges is set
        unit lost a hardening directive
  FAIL: ProtectSystem is set
        unit lost a hardening directive
  PASS: ProtectSystem is not 'strict' (which would make the /tmp checkout read-only)
  PASS: ProtectHome is not read-only (git and gh in publish-snapshot.sh write to $HOME)
...
=== Results: 18 passed, 3 failed ===

The behaviour assertions pass in both states because they exercise the guard script directly against real trees — that is the point of them, and it is why the unit-file assertions above are what actually distinguish fixed from unfixed.

Against the fixed unit — 24 passed, 0 failed, exit 0:

--- the unit file ---
  PASS: hive-snapshot.service has an ExecStartPre guard
  PASS: the guard is hive-checkout-guard.sh (exit status is the assertion)
  PASS: the guard checks the file ExecStart actually runs (publish-snapshot.sh)
  PASS: the guard checks the directory ExecStart runs from (/tmp/hive/dashboard)
  PASS: PrivateTmp is not set (it would hide the /tmp checkout and the /tmp docs clone)
  PASS: NoNewPrivileges is set
  PASS: ProtectSystem is set
  PASS: ProtectSystem is not 'strict' (which would make the /tmp checkout read-only)
  PASS: ProtectHome is not read-only (git and gh in publish-snapshot.sh write to $HOME)

--- the guard, run against real trees ---
  PASS: healthy checkout under a sticky /tmp, entrypoint 755 as shipped: guard allows startup (rc=0)
  PASS: post-reboot window, publish-snapshot.sh absent (the #5483 race): guard REFUSES startup (rc=1)
  PASS: checkout directory is world-writable: guard REFUSES startup (rc=1)
  PASS: checkout directory is world-writable even with the sticky bit: guard REFUSES startup (rc=1)
  PASS: an ancestor is world-writable without the sticky bit: guard REFUSES startup (rc=1)
  PASS: an ancestor is group-writable: guard REFUSES startup (rc=1)
  PASS: publish-snapshot.sh is world-writable (and executable, as it ships): guard REFUSES startup (rc=1)
  PASS: publish-snapshot.sh is group-writable (and executable, as it ships): guard REFUSES startup (rc=1)
  PASS: publish-snapshot.sh is a symlink: guard REFUSES startup (rc=1)
  PASS: the checkout directory is a symlink: guard REFUSES startup (rc=1)
  PASS: the checkout directory does not exist: guard REFUSES startup (rc=1)
...
=== Results: 24 passed, 0 failed ===

src/deploy/test_hive_discord_unit_contract.sh still reports 21 passed, 0 failed — this PR does not disturb it.

Hardening differs from hive-discord.service on purpose

NoNewPrivileges, ProtectSystem=full, PrivateDevices and RestrictSUIDSGID are set, matching #5481. Two deliberate divergences, both pinned by the test so a later "make the units consistent" pass cannot undo them silently:

• PrivateTmp absent and ProtectSystem not strict — same trap as fix(security): stop hive-discord.service executing code planted under /tmp (#5435) #5481, with an extra reason here: besides the checkout, DOCS_REPO_DIR=/tmp/kubestellar-docs-snapshot is also under /tmp, and the script clones into it, commits from it and pushes. A private or read-only /tmp breaks the job twice over.
• ProtectHome is NOT read-only, unlike the discord unit. I checked what publish-snapshot.sh actually does rather than mirroring the sibling: it drives git and gh, both of which write under $HOME (gh refreshes ~/.config/gh state, git writes lock files alongside ~/.gitconfig). Copying ProtectHome=read-only across would have broken the push/PR path rather than hardened it. The test asserts its absence with that reasoning attached.

Registered in v2-ci.yml next to the other deploy contract tests — src/deploy/test_*.sh are wired individually there, so an unregistered test would never run.

Not verified

• Not exercised on a live host. No reboot-race reproduction against real systemd, and the ExecStartPre was not observed refusing a real timer firing. The guard's logic is verified directly; its integration with systemd is verified only by reading the unit.
• ProtectHome's absence is reasoned, not measured. I established that git and gh write under $HOME from the script's contents and general tool behaviour, not by running the unit with ProtectHome=read-only and observing a failure. The conservative choice (leave $HOME writable) is the one that cannot break the job, so an over-cautious call here costs nothing.
• PrivateDevices / RestrictSUIDSGID untested at runtime — carried over from fix(security): stop hive-discord.service executing code planted under /tmp (#5435) #5481 on the assumption the snapshot job needs neither device nodes nor setuid execution. Nothing in publish-snapshot.sh or build-snapshot.mjs suggests it does, but this was not exercised.
• The other six units in systemd/ still carry no hardening directives. The issue calls a consistent pass across them the natural follow-up; I deliberately kept this PR to the one unit named in hive-snapshot.service executes publish-snapshot.sh from /tmp/hive with no ownership guard (same class as #5435) #5483, and the ProtectHome divergence found here is evidence that such a pass needs to be per-unit rather than uniform.
• Guard portability across BSD/GNU stat was inherited from fix(security): stop hive-discord.service executing code planted under /tmp (#5435) #5481 unchanged; the GNU branch is exercised by CI on Linux, which is the platform that matters.

Fixes #5504

[clubanderson]

Retargeting this at #5504 rather than closing it — #5499 merged first for #5483, but shipped two defects this PR gets right.

1. The contract test is registered nowhere. v2-ci.yml wires deploy tests individually, and on v4 line 553 runs only the discord test. test_hive_snapshot_unit_contract.sh merged as a file and was never added, so the 184-line guard test has never executed in CI and would not execute on any future PR. Present, looks like coverage, asserts nothing — the exact #5388 shape. This PR has the registration.

2. ProtectHome=read-only. The merged unit sets it, with a comment claiming it "still lets git/gh read ~/.gitconfig while blocking writes outside /tmp and /var". But publish-snapshot.sh runs gh pr create (line 94), gh pr checks (113), and gh pr merge (135), and gh maintains state under $HOME/.config/gh. Your reasoning to omit it — and to pin the absence in the test so a later consistency sweep cannot reintroduce it — was correct.

To be precise about what I established: I verified the script calls those commands; I did not reproduce a live failure with ProtectHome=read-only set. Since the unit is Type=oneshot on a 15-minute timer, a failure would show up as a silently non-updating snapshot, not a loud crash.

Please rebase onto current v4 and reduce this to the delta over what #5499 landed: the v2-ci.yml registration, the ProtectHome removal with its pinning test, and the hive-deploy.sh comment. The unit file and guard script are already in. Change Fixes #5483 to Fixes #5504.

Your report’s judgement was the right one throughout — verifying the guard generalized unmodified before writing the fix, and refusing to copy the sibling unit’s hardening wholesale. The ProtectHome divergence is exactly why the remaining six units in systemd/ need a per-unit sweep rather than a uniform one.

[clubanderson]

Rebased onto current v4 and reduced to the delta over merged #5499.

#5499 landed the guard wiring and the test file first; both conflicted here. I resolved by taking this PR's side of systemd/hive-snapshot.service and src/deploy/test_hive_snapshot_unit_contract.sh, because the difference between the two versions is precisely the two defects #5504 tracks:

1. ProtectHome=read-only removed. The merged unit sets it, with a comment claiming it "still lets git/gh read ~/.gitconfig while blocking writes outside /tmp and /var". But publish-snapshot.sh runs gh pr create (line 94), gh pr checks (113), and gh pr merge (135), and gh maintains state under $HOME/.config/gh. This PR omits the directive and pins the absence in the test with the reasoning attached, so a later "make the units consistent" sweep cannot quietly reintroduce it and break the push path.

2. v2-ci.yml registration added. Deploy tests are wired individually there. As merged, test_hive_snapshot_unit_contract.sh exists in the tree and is registered nowhere — so it has never executed in CI and would not execute on any future PR. Present, reads as coverage, asserts nothing.

Verified the test actually bites

Ran it three ways rather than trusting a green result — the vacuous-guard failure has appeared four times in this repo today:

unit state	result
rebased (fixed)	24 passed, 0 failed
ProtectHome=read-only reintroduced	23 passed, 1 failed
restored	24 passed, 0 failed

The assertion fails for the right reason on the mutation and passes on the fix. This is a local demonstration of the guard's behaviour, not a merge signal — CI remains the gate, and the point of this PR is that CI will now actually run it.

Delta is 5 files: the v2-ci.yml registration, the ProtectHome removal, the test that pins it, a hive-deploy.sh comment, and a CHANGELOG entry. The guard script and the ExecStartPre wiring are already on v4 from #5499 and are untouched here.

Retargeted Fixes #5483 → Fixes #5504, since #5483 is closed by the merged #5499.

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

Thank you for your contribution! Your PR has been merged.

Check out what's new:

• KubeStellar Console — Live multi-cluster dashboard
• Marketplace — Community extensions
• Knowledge Base — Troubleshooting and how-tos

Stay connected: Slack #kubestellar-dev | Multi-Cluster Survey