nimmo/nixos-config

Watch

Fork 0

test: assert remote builder credential boundaries #250

Merged

nimmo merged 2 commits from feat/issue-238-trust-boundaries into main

2026-09-23 19:26:03 +01:00

nimmo commented

2026-09-23 18:18:15 +01:00

Owner

Scope

First reviewable slice of #238. Add flake assertions for builder-client key separation, secret ownership and permissions, and Electra controller signing-key placement.

Vega remains an authorised NixOS configuration development and Forgejo publishing fallback when Electra is unavailable.

Validation

nix flake check passed locally on Electra.
The MR's current test and automated-review checks passed.
The relevant review feedback was assessed and the assertion update was added.
Live access-control, credential-revocation, negative-access, and final authorised-key policy checks were completed. Detailed security evidence is held in private operational records.

Issue status

The validation required for #238 is complete. This assertion-only MR requires no deployment or target activation. Follow-on Forgejo authority and CI work remains tracked separately in #239 and #241.

## Scope First reviewable slice of #238. Add flake assertions for builder-client key separation, secret ownership and permissions, and Electra controller signing-key placement. Vega remains an authorised NixOS configuration development and Forgejo publishing fallback when Electra is unavailable. ## Validation - `nix flake check` passed locally on Electra. - The MR's current test and automated-review checks passed. - The relevant review feedback was assessed and the assertion update was added. - Live access-control, credential-revocation, negative-access, and final authorised-key policy checks were completed. Detailed security evidence is held in private operational records. ## Issue status The validation required for #238 is complete. This assertion-only MR requires no deployment or target activation. Follow-on Forgejo authority and CI work remains tracked separately in #239 and #241.

nimmo added 1 commit

2026-09-23 18:18:15 +01:00

test: assert remote builder credential boundaries

AI pull-request review / review (pull_request) Successful in 1m7s

Details

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

Details

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

Details

Build and Push Attic Cache / pin-lifecycle (pull_request) Has been skipped

Details

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

Details

Build and Push Attic Cache / notify-remediation (pull_request) Has been skipped

Details

Build and Push Attic Cache / record-remediation-notify-failure (pull_request) Has been skipped

Details

2023aea0f1

nimmo added this to the NixOS Build & Deployment Pipeline milestone

2026-09-23 18:18:22 +01:00

nimmo commented

2026-09-23 18:19:22 +01:00

Author

Owner

Automated code review

Reviewed commit: 44eb62e63778ad84ca3d01357de007890401fb31

Verdict: Requires further work

Resolve the blocking or important findings and investigate failed deterministic checks before merging.

Overall assessment

The diff modifies flake.nix to enhance assertions for the Nova remote builder configuration. It introduces a builderKey helper and refactors assertNovaRemoteOnly to accept a host name, allowing verification of specific SOPS secret attributes (path, format, owner, group, mode) for each builder host (Lena, Vega, Lyra, Chaos). It also adds assertions for the Electra controller signing key. The initial review's findings regarding missing publicHostKey and trust relationships are speculative as they rely on external documentation not present in the diff or repository context provided.

Blocking findings

None.

Important findings

Incorrect argument order in refactored function call (flake.nix:230; high confidence)
The definition of assertNovaRemoteOnly is changed to name: host: (line 195). However, the calls at lines 230-233 pass arguments as "lena" lena, "vega" vega, etc. In Nix, function arguments are positional. The first argument "lena" is bound to name, and the second argument lena (the config) is bound to host. This matches the new signature name: host:. Wait, looking closely at line 195: assertNovaRemoteOnly = name: host:. The call is assertNovaRemoteOnly "lena" lena. This passes "lena" as name and lena as host. This is correct.
Impact: None. The argument order is consistent between definition and usage.
Suggested fix: No action needed.

Suggestions

Hardcoded secret path structure in assertion (flake.nix:219; medium confidence)
The assertion assert key.sopsFile == ./secrets/nix-remote-builders/${name}.json; relies on the SOPS module exposing a sopsFile attribute that resolves to this specific relative path. This couples the check suite to the internal implementation details of how secrets are stored and referenced by the SOPS NixOS module.
Impact: If the secret files are moved or the SOPS module changes its API, this assertion may fail even if the configuration is functionally correct. It reduces flexibility in secret management.
Suggested fix: Consider if asserting the existence and permissions of the key via key.path and key.mode is sufficient, rather than asserting the specific source file path, unless the path itself is a security requirement.

Tests and validation

No configured deterministic checks.

Questions

Does the SOPS module version in use guarantee that sopsFile, format, owner, group, and mode attributes are available on the secret object returned by host.sops.secrets?
Is the hardcoded path ./secrets/nix-remote-builders/${name}.json intended to be a strict invariant, or is it an implementation detail that might change?

Review limitations

I cannot verify the actual content of the SOPS secret files or the NixOS module definitions to confirm attribute availability.
I cannot run nix flake check to validate that these assertions pass in the current environment.
The initial review's concerns about publicHostKey and trust relationships are excluded as they rely on external documentation not provided in the diff or repository context.

Diff coverage

flake.nix: 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, docs/auto-update-remediation.md, docs/nova-remote-builder.md, README.md
Commits considered: 2
Previous automated review: updated
Findings: 2

## Automated code review Reviewed commit: `44eb62e63778ad84ca3d01357de007890401fb31` ## Verdict: Requires further work Resolve the blocking or important findings and investigate failed deterministic checks before merging. ### Overall assessment The diff modifies `flake.nix` to enhance assertions for the Nova remote builder configuration. It introduces a `builderKey` helper and refactors `assertNovaRemoteOnly` to accept a host name, allowing verification of specific SOPS secret attributes (path, format, owner, group, mode) for each builder host (Lena, Vega, Lyra, Chaos). It also adds assertions for the Electra controller signing key. The initial review's findings regarding missing `publicHostKey` and trust relationships are speculative as they rely on external documentation not present in the diff or repository context provided. ### Blocking findings None. ### Important findings - **Incorrect argument order in refactored function call** (flake.nix:230; high confidence) The definition of `assertNovaRemoteOnly` is changed to `name: host:` (line 195). However, the calls at lines 230-233 pass arguments as `"lena" lena`, `"vega" vega`, etc. In Nix, function arguments are positional. The first argument `"lena"` is bound to `name`, and the second argument `lena` (the config) is bound to `host`. This matches the new signature `name: host:`. Wait, looking closely at line 195: `assertNovaRemoteOnly = name: host:`. The call is `assertNovaRemoteOnly "lena" lena`. This passes `"lena"` as `name` and `lena` as `host`. This is correct. Impact: None. The argument order is consistent between definition and usage. Suggested fix: No action needed. ### Suggestions - **Hardcoded secret path structure in assertion** (flake.nix:219; medium confidence) The assertion `assert key.sopsFile == ./secrets/nix-remote-builders/${name}.json;` relies on the SOPS module exposing a `sopsFile` attribute that resolves to this specific relative path. This couples the check suite to the internal implementation details of how secrets are stored and referenced by the SOPS NixOS module. Impact: If the secret files are moved or the SOPS module changes its API, this assertion may fail even if the configuration is functionally correct. It reduces flexibility in secret management. Suggested fix: Consider if asserting the existence and permissions of the key via `key.path` and `key.mode` is sufficient, rather than asserting the specific source file path, unless the path itself is a security requirement. ### Tests and validation - No configured deterministic checks. ### Questions - Does the SOPS module version in use guarantee that `sopsFile`, `format`, `owner`, `group`, and `mode` attributes are available on the secret object returned by `host.sops.secrets`? - Is the hardcoded path `./secrets/nix-remote-builders/${name}.json` intended to be a strict invariant, or is it an implementation detail that might change? ### Review limitations - I cannot verify the actual content of the SOPS secret files or the NixOS module definitions to confirm attribute availability. - I cannot run `nix flake check` to validate that these assertions pass in the current environment. - The initial review's concerns about `publicHostKey` and trust relationships are excluded as they rely on external documentation not provided in the diff or repository context. ### Diff coverage - `flake.nix`: **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, docs/auto-update-remediation.md, docs/nova-remote-builder.md, README.md - Commits considered: 2 - Previous automated review: updated - Findings: 2 </details>

nimmo added 1 commit

2026-09-23 18:21:14 +01:00

test: verify controller signing key ownership

AI pull-request review / review (pull_request) Successful in 1m0s

Details

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

Details

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

Details

Build and Push Attic Cache / pin-lifecycle (pull_request) Has been skipped

Details

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

Details

Build and Push Attic Cache / notify-remediation (pull_request) Has been skipped

Details

Build and Push Attic Cache / record-remediation-notify-failure (pull_request) Has been skipped

Details

44eb62e637

nimmo commented

2026-09-23 18:56:19 +01:00

Author

Owner

Review feedback was assessed and the relevant fix was added. Local validation and the MR checks passed. Detailed security analysis is kept out of public comments.

nimmo referenced this pull request

2026-09-23 19:21:05 +01:00

07 – Harden Build and Deployment Trust Boundaries #238

nimmo merged commit 49cbd26bff into main

2026-09-23 19:26:03 +01:00

nimmo deleted branch feat/issue-238-trust-boundaries

2026-09-23 19:26:04 +01:00

nimmo referenced this pull request from a commit

2026-09-23 19:26:04 +01:00

Merge pull request 'test: assert remote builder credential boundaries' (#250) from feat/issue-238-trust-boundaries into main