nimmo/nixos-config

Watch

Fork 0

fix(nixos-deploy): stop wrapping sudo in runtimeInputs (closes #138) #139

Merged

nimmo merged 3 commits from fix/nixos-deploy-sudo-path into main

2026-08-04 06:26:11 +01:00

nimmo commented

2026-08-04 05:55:33 +01:00

Owner

Goal

Restore the pre-PR-#125 behaviour of just deploy (uses nom, can run as a
non-root user via the system setuid sudo wrapper) while keeping the new
packages/nixos-deploy Nix package that replaced the inline Justfile bash.

Root cause

PR #125 moved the long inline bash recipe from justfiles/deploy.just into
scripts/nixos-deploy.sh and packaged it as
packages/nixos-deploy/default.nix (writeShellApplication with
runtimeInputs = [ coreutils hostname nix nvd nix-output-monitor openssh sudo ]).

The script still does the right things when invoked directly. However,
just deploy now shells out to nix run .#nixos-deploy -- ..., and the
writeShellApplication wrapper hard-codes a PATH that begins with the
store sudo (/nix/store/...-sudo-1.9.17p2/bin/sudo, mode 0555, not
setuid) before the user's PATH. The setuid sudo lives at
/run/wrappers/bin/sudo (mode 4551) and is only first on PATH in a
normal terminal session, where /etc/set-environment does
export PATH="/run/wrappers/bin:$PATH".

Symptom: just deploy aborts at sudo -v with
sudo: /run/current-system/sw/bin/sudo must be owned by uid 0 and have the setuid bit set — i.e. it "complains about not being root". It also
looks like just deploy no longer uses nom, because the Justfile
now only calls nix run .#nixos-deploy; the nom build call is buried
inside the packaged script and never runs.

Change

packages/nixos-deploy/default.nix: drop sudo from the function
inputs and from runtimeInputs. Add a comment explaining why.
tests/nixos-deploy-wrapper-regressions.sh (new): evaluates the
package's text attribute via nix eval and pins the wrapped PATH
contract — it must keep $PATH and must not contain a sudo-<digit>
segment. The test fails against the pre-fix wrapper and passes against
the fixed wrapper.
README.md: note that sudo is intentionally omitted from the
wrapper's runtime inputs and that NIXOS_DEPLOY_SUDO remains the
explicit override.

The script still honours NIXOS_DEPLOY_SUDO (and the other
NIXOS_DEPLOY_* env vars) for explicit overrides.

Validation

bash tests/nixos-deploy-regressions.sh ✓
bash tests/nixos-deploy-wrapper-regressions.sh ✓ (verified to fail
against the pre-fix wrapper text, pass against the fixed wrapper)
shellcheck scripts/nixos-deploy.sh tests/nixos-deploy-regressions.sh tests/nixos-deploy-wrapper-regressions.sh clean
nix flake check ✓

Live deployment evidence

None yet — the fix changes the wrapper's PATH contract, not the deployed
system. After merge, the natural canary is just deploy from a normal
terminal on electra (which uses /etc/set-environment and so already has
/run/wrappers/bin first on PATH). Per repo policy, no live canary was
attempted before opening the MR.

Closes #138.

## Goal Restore the pre-PR-#125 behaviour of `just deploy` (uses `nom`, can run as a non-root user via the system setuid sudo wrapper) while keeping the new `packages/nixos-deploy` Nix package that replaced the inline Justfile bash. ## Root cause PR #125 moved the long inline bash recipe from `justfiles/deploy.just` into `scripts/nixos-deploy.sh` and packaged it as `packages/nixos-deploy/default.nix` (`writeShellApplication` with `runtimeInputs = [ coreutils hostname nix nvd nix-output-monitor openssh sudo ]`). The script still does the right things when invoked directly. However, `just deploy` now shells out to `nix run .#nixos-deploy -- ...`, and the `writeShellApplication` wrapper hard-codes a PATH that begins with the *store* sudo (`/nix/store/...-sudo-1.9.17p2/bin/sudo`, mode 0555, not setuid) before the user's PATH. The setuid sudo lives at `/run/wrappers/bin/sudo` (mode 4551) and is only first on PATH in a normal terminal session, where `/etc/set-environment` does `export PATH="/run/wrappers/bin:$PATH"`. Symptom: `just deploy` aborts at `sudo -v` with `sudo: /run/current-system/sw/bin/sudo must be owned by uid 0 and have the setuid bit set` — i.e. it "complains about not being root". It also *looks* like `just deploy` no longer uses `nom`, because the Justfile now only calls `nix run .#nixos-deploy`; the `nom build` call is buried inside the packaged script and never runs. ## Change - `packages/nixos-deploy/default.nix`: drop `sudo` from the function inputs and from `runtimeInputs`. Add a comment explaining why. - `tests/nixos-deploy-wrapper-regressions.sh` (new): evaluates the package's `text` attribute via `nix eval` and pins the wrapped PATH contract — it must keep `$PATH` and must not contain a `sudo-<digit>` segment. The test fails against the pre-fix wrapper and passes against the fixed wrapper. - `README.md`: note that `sudo` is intentionally omitted from the wrapper's runtime inputs and that `NIXOS_DEPLOY_SUDO` remains the explicit override. The script still honours `NIXOS_DEPLOY_SUDO` (and the other `NIXOS_DEPLOY_*` env vars) for explicit overrides. ## Validation - `bash tests/nixos-deploy-regressions.sh` ✓ - `bash tests/nixos-deploy-wrapper-regressions.sh` ✓ (verified to fail against the pre-fix wrapper text, pass against the fixed wrapper) - `shellcheck scripts/nixos-deploy.sh tests/nixos-deploy-regressions.sh tests/nixos-deploy-wrapper-regressions.sh` clean - `nix flake check` ✓ ## Live deployment evidence None yet — the fix changes the wrapper's PATH contract, not the deployed system. After merge, the natural canary is `just deploy` from a normal terminal on electra (which uses `/etc/set-environment` and so already has `/run/wrappers/bin` first on PATH). Per repo policy, no live canary was attempted before opening the MR. Closes #138.

nimmo added 1 commit

2026-08-04 05:55:33 +01:00

fix(nixos-deploy): stop wrapping sudo in runtimeInputs

Build and Push Attic Cache / test (pull_request) Successful in 1m8s

Details

Build and Push Attic Cache / build (pull_request) Has been skipped

Details

Build and Push Attic Cache / remediate (pull_request) Has been skipped

Details

AI pull-request review / review (pull_request) Successful in 2m56s

Details

e2d7660500

The packaged nixos-deploy wrapper (introduced in PR #125 to replace the
inline Justfile bash) prepended the store sudo to PATH via
writeShellApplication's runtimeInputs. That store sudo is mode 0555, not
setuid; the setuid wrapper lives at /run/wrappers/bin/sudo and is only
first on PATH in a normal terminal session, where /etc/set-environment
prepends /run/wrappers/bin. Under 'nix run .#nixos-deploy' the wrapper's
PATH won, so 'sudo -v' inside scripts/nixos-deploy.sh hit the non-setuid
copy and bailed out with

  sudo: /nix/store/.../bin/sudo must be owned by uid 0 and have the
  setuid bit set

before nom build could run. The Justfile recipe looked like it no longer
used nom at all, because the nom call had moved into the packaged script.

Drop sudo from runtimeInputs (and from the callPackage input set) so the
script resolves sudo from the caller's PATH, which is the setuid wrapper
on every NixOS host. The existing NIXOS_DEPLOY_SUDO override still works
for non-default deployments.

Add tests/nixos-deploy-wrapper-regressions.sh to pin the wrapper's PATH
contract: it must keep '$PATH' and must not contain 'sudo-<version>'.
The test catches the regression when run against the pre-fix wrapper.

Refs #138.

nimmo commented

2026-08-04 05:58:30 +01:00

Author

Owner

Automated code review

Reviewed commit: 34815882727ab1d25547eb726b9043772d1fde45

Verdict: Ready to merge

No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory.

Overall assessment

The PR removes sudo from the nixos-deploy package's runtimeInputs to prevent the store-bundled sudo (mode 0555) from shadowing the system setuid wrapper at /run/wrappers/bin/sudo. This resolves issue #138 where non-root deployments failed. The change is documented in README.md and accompanied by a regression test that validates the wrapper's PATH construction against the store sudo path while confirming required tools remain present. flake.lock contains routine dependency updates.

Blocking findings

None.

Important findings

None.

Suggestions

Regression test relies on external nix CLI and --impure evaluation (tests/nixos-deploy-wrapper-regressions.sh:28; high confidence)
if ! command -v nix >/dev/null 2>&1; then\n echo "nix is required for this regression test" >&2\n exit 1\nfi...\nnix eval --raw --impure --expr '...'
Impact: Test execution will fail in restricted CI sandboxes, offline environments, or when the nix CLI is not provisioned.
Suggested fix: Verify that the CI workflow explicitly provisions nix, or consider integrating the evaluation into nix flake check if it aligns with repository testing standards.
Hardcoded tool assertions may require maintenance on Nixpkgs updates (tests/nixos-deploy-wrapper-regressions.sh:85; high confidence)
for tool in coreutils hostname nix nvd nix-output-monitor openssh; do\n assert_contains "$tool"\ndone
Impact: Future Nixpkgs renames, splits, or deprecations of these packages will cause the regression test to fail unnecessarily.
Suggested fix: Consider deriving the expected tools dynamically from runtimeInputs in the Nix expression, or document that this list requires review during Nixpkgs bumps.

Tests and validation

No configured deterministic checks.

Questions

Is the new regression test explicitly invoked in the CI workflow (e.g., via just test), or does it currently rely on manual execution?
Does the flake expose nixos-deploy for all architectures supported by this repository, given the test uses builtins.currentSystem to resolve the package?

Review limitations

Review is based solely on the provided diff and file contents. I cannot execute the Nix evaluation, run the regression script, or verify CI integration.
The flake.lock contains numerous dependency updates; their individual security and compatibility impacts were not assessed beyond format validation.

Diff coverage

README.md: reviewed — included in a context-limited batch
flake.lock: reviewed — included in a context-limited batch
packages/nixos-deploy/default.nix: reviewed — included in a context-limited batch
tests/nixos-deploy-wrapper-regressions.sh: reviewed — included in a context-limited batch

Review metadata

Reviewer: forgejo-ai-review v1
Model: qwen3.6:35b-a3b-q8_0
Guidance loaded: .forgejo/ai-review.md, .forgejo/ai-review.yml, AGENTS.md, README.md
Commits considered: 3
Previous automated review: updated
Findings: 2

## Automated code review Reviewed commit: `34815882727ab1d25547eb726b9043772d1fde45` ## Verdict: Ready to merge No blocking or important findings were retained, and deterministic checks passed. Suggestions remain advisory. ### Overall assessment The PR removes `sudo` from the `nixos-deploy` package's `runtimeInputs` to prevent the store-bundled sudo (mode 0555) from shadowing the system setuid wrapper at `/run/wrappers/bin/sudo`. This resolves issue #138 where non-root deployments failed. The change is documented in README.md and accompanied by a regression test that validates the wrapper's PATH construction against the store sudo path while confirming required tools remain present. flake.lock contains routine dependency updates. ### Blocking findings None. ### Important findings None. ### Suggestions - **Regression test relies on external `nix` CLI and `--impure` evaluation** (tests/nixos-deploy-wrapper-regressions.sh:28; high confidence) if ! command -v nix >/dev/null 2>&1; then\n echo "nix is required for this regression test" >&2\n exit 1\nfi...\nnix eval --raw --impure --expr '...' Impact: Test execution will fail in restricted CI sandboxes, offline environments, or when the `nix` CLI is not provisioned. Suggested fix: Verify that the CI workflow explicitly provisions `nix`, or consider integrating the evaluation into `nix flake check` if it aligns with repository testing standards. - **Hardcoded tool assertions may require maintenance on Nixpkgs updates** (tests/nixos-deploy-wrapper-regressions.sh:85; high confidence) for tool in coreutils hostname nix nvd nix-output-monitor openssh; do\n assert_contains "$tool"\ndone Impact: Future Nixpkgs renames, splits, or deprecations of these packages will cause the regression test to fail unnecessarily. Suggested fix: Consider deriving the expected tools dynamically from `runtimeInputs` in the Nix expression, or document that this list requires review during Nixpkgs bumps. ### Tests and validation - No configured deterministic checks. ### Questions - Is the new regression test explicitly invoked in the CI workflow (e.g., via `just test`), or does it currently rely on manual execution? - Does the flake expose `nixos-deploy` for all architectures supported by this repository, given the test uses `builtins.currentSystem` to resolve the package? ### Review limitations - Review is based solely on the provided diff and file contents. I cannot execute the Nix evaluation, run the regression script, or verify CI integration. - The `flake.lock` contains numerous dependency updates; their individual security and compatibility impacts were not assessed beyond format validation. ### Diff coverage - `README.md`: **reviewed** — included in a context-limited batch - `flake.lock`: **reviewed** — included in a context-limited batch - `packages/nixos-deploy/default.nix`: **reviewed** — included in a context-limited batch - `tests/nixos-deploy-wrapper-regressions.sh`: **reviewed** — included in a context-limited batch <details><summary>Review metadata</summary> - Reviewer: forgejo-ai-review v1 - Model: qwen3.6:35b-a3b-q8_0 - Guidance loaded: .forgejo/ai-review.md, .forgejo/ai-review.yml, AGENTS.md, README.md - Commits considered: 3 - Previous automated review: updated - Findings: 2 </details>

nimmo referenced this pull request from a commit

2026-08-04 06:06:24 +01:00

test(nixos-deploy-wrapper): match store sudo by path shape, not version

nimmo added 1 commit

2026-08-04 06:06:24 +01:00

test(nixos-deploy-wrapper): match store sudo by path shape, not version

AI pull-request review / review (pull_request) Successful in 2m34s

Details

Build and Push Attic Cache / test (pull_request) Successful in 2m39s

Details

Build and Push Attic Cache / build (pull_request) Has been skipped

Details

Build and Push Attic Cache / remediate (pull_request) Has been skipped

Details

c8c6d2075c

Address the AI pull-request review's medium-confidence suggestion on
#139: the previous assertion used 'sudo-1' and 'sudo-2' literals to
catch the store sudo in the wrapped PATH. That works today but couples
the test to the upstream sudo version number. Replace it with an
extended-regex match against '/nix/store/...-sudo-.../bin' so the test
stays valid across nixpkgs sudo version bumps.

Verified manually that the new pattern still matches the pre-fix
wrapper text (rc=0 / 'found') and does not match the fixed wrapper
text (rc=1 / 'not found').

flake.lock: pull in the latest auto-update (nixpkgs/nixpkgs-stable,
home-manager-stable, treefmt-nix) so the PR builds against the same
inputs as origin/main.

nimmo force-pushed fix/nixos-deploy-sudo-path from c8c6d2075c

AI pull-request review / review (pull_request) Successful in 2m34s

Details

Build and Push Attic Cache / test (pull_request) Successful in 2m39s

Details

Build and Push Attic Cache / build (pull_request) Has been skipped

Details

Build and Push Attic Cache / remediate (pull_request) Has been skipped

Details

to 12b8e0d6cf

Build and Push Attic Cache / test (pull_request) Successful in 1m6s

Details

Build and Push Attic Cache / build (pull_request) Has been skipped

Details

Build and Push Attic Cache / remediate (pull_request) Has been skipped

Details

AI pull-request review / review (pull_request) Successful in 2m52s

Details

2026-08-04 06:13:57 +01:00

Compare

nimmo referenced this pull request from a commit

2026-08-04 06:18:40 +01:00

test(nixos-deploy-wrapper): use builtins.currentSystem for portability

nimmo added 1 commit

2026-08-04 06:18:41 +01:00

test(nixos-deploy-wrapper): use builtins.currentSystem for portability

Build and Push Attic Cache / test (pull_request) Successful in 1m7s

Details

Build and Push Attic Cache / build (pull_request) Has been skipped

Details

Build and Push Attic Cache / remediate (pull_request) Has been skipped

Details

AI pull-request review / review (pull_request) Successful in 3m5s

Details

3481588272

Address the AI review's high-confidence suggestion on #139: the
'nix eval' expression in this test hard-coded
'flake.packages.x86_64-linux.nixos-deploy', which couples the
regression to the host architecture. Switch to
'flake.packages.${builtins.currentSystem}.nixos-deploy' so the test
stays portable if this flake ever adds aarch64 or another architecture.

The repo currently only exposes x86_64-linux packages, so on x86_64
hosts the resolved package is unchanged and the assertions still match
the same wrapper text.

nimmo merged commit 2ac7c0c97b into main

2026-08-04 06:26:11 +01:00

nimmo referenced this pull request from a commit

2026-08-04 06:26:12 +01:00

Merge pull request 'fix(nixos-deploy): stop wrapping sudo in runtimeInputs (closes #138)' (#139) from fix/nixos-deploy-sudo-path into main