Skip to content

Address the post-acceptance review of #15-#17 before merging #18

Closed

Description

@isaacslee

An independent, read-only review of main..01ca427 (#15–#17, 8 commits) was done after the ARM64 acceptance. Recommendation: merge with fixes. It found no critical or high issue, and nothing in these commits gives root a new way to remove, chown or overwrite something RunnerSlice did not create.

The reviewer ran the drills as an ordinary user: host 73, apply 142, check 45, fabric 84, render 40, cli 21, docs 7, status 15.

Medium

  1. RestrictNamespaces on older systemd. fabric-check.sh now expects exactly yes/no, which was observed on systemd 255 only. Preflight accepts systemd ≥ 249. On an older systemd that prints another form, a correct baseline host would fail its check. That fails closed, but the failure is false. Either raise the preflight minimum to the verified version, or verify 249 and accept both forms there.

Low

  1. The leftover-process check fails open on pgrep errors. Under pipefail, an exit of 2, 3 or 127 reads as "no processes". pgrep is also missing from the apply preflight, and -u matches only the effective uid. Use -U, treat exit codes above 1 as unknown, and add pgrep to the preflight.
  2. Retirement is not all-or-nothing when userdel fails at the end. By then the unit and the root are already gone, and the message "nothing else was changed for it" is false. The threat model overstates the same guarantee.
  3. Some malformed JSON escapes the Python handler. A deep nesting raises RecursionError, and a non-iterable libraries raises TypeError. Python exits 1, which is labelled "differs" instead of "could not tell". Catch Exception and exit 2.
  4. A runtime root without its marker cannot be recovered by the tool. That happens if apply is killed between the mkdir and the marker write, or the root predates the marker. The refusal messages for root, group and home are also missing from the troubleshooting table. Document the manual recovery.
  5. With USERGROUPS_ENAB no, userdel leaves the group behind, and a later apply refuses it forever.

Info

  1. Docs overstate two things:
    • O_NOFOLLOW protects only the last path component, which is still safe as implemented;
    • the exported token also lives in the operator's shell until unset. Suggest printing the commands as a ( … ) subshell.
  2. The unpack directory is not 0700 for the whole extraction. tar restores the archive's . mode before chown -R. Still safe, because everything is root-owned until then, but the comment overstates it.

Fixes with no test that fails when they are reverted

  • the fresh-directory unpack;
  • retirement refusing a root with no marker, a wrong marker, or a marker that is a symlink;
  • retirement refusing a foreign account, at the apply level;
  • unset TMPDIR, pwd -P, and the whole-tree ownership check;
  • a symlinked, FIFO or oversized .runner, and the escaping of what it contains;
  • the pgrep failure modes;
  • the --replace assertion, which is weak.

Verified on the real ARM64 host (docs/acceptance/2026-10-arm64.md), and not in question here: the never-executed runner, the busy guard, retirement, the archive modes, the printed commands, and namespaces on systemd 255.

Activity

  1. isaacslee commented on Oct 16, 2026

    @isaacslee
    MemberAuthor

    Addressed in #19 (stacked on #17, not merged). Each item is mapped to its commit in the PR description. A and B (the process check and retirement order) are RUNTIME VERIFIED on the ARM64 VM with a never-registered test profile. C: official support is systemd 255 only; 249–254 is accepted with a preflight warning, and documented as source-reviewed, not verified. Hosted CI is still BLOCKED by billing.

  2. added 5 commits that reference this issue on Oct 16, 2026
  3. isaacslee commented on Oct 16, 2026

    @isaacslee
    MemberAuthor

    Resolved. The fixes merged to main through #19 → #17 → #16 → #15, by rebase. The final tree is 84a0cec, now under the docs-only commits of #20; main is e9e2997 (tree 19c6221). Hosted gate on main: run 38926650605, success (20 scripts shellcheck-clean, 545 drill cases). The changed retirement paths were RUNTIME VERIFIED on the ARM64 VM at 7f84c01 and 90f7ec8 (docs/acceptance/2026-10-arm64.md). An independent review of #19 found one High (the mount regression), fixed in e2e58f8. Its re-review found no Critical or High.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions