From 464336b4f8cd852d74dba0416bc8f5094ced622c Mon Sep 17 00:00:00 2001 From: itcmsgr Date: Tue, 28 Apr 2026 12:35:24 +0300 Subject: [PATCH] feat(v1.100 PR-26-code-B): typed executor.ServiceUnmask + executor.Rename + raw-Run policy tightening MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR-26-code-B — restore verification / evidence hardening, slice B. Executor hardening per §43 lock + §51.5-A2 invariant. CSF restore A.1 + A.3 migrate from raw Run("systemctl","unmask",…) + Run("mv",…) indirections to typed executor methods. Authority: - PR #512 / contract.md Part IV §§37-50 - PR #513 / §51 lock record (§51.5-A2: read-only typed introspection is outside the mutation cap; this commit's ServiceUnmask + Rename ARE mutation surfaces, but already enumerated by §44 row 2) - PR #514 / code-A merge 4e98ff56 - §43 executor hardening - §46 CI gate requirements - §51 code-B authorization Behavior delta: - Before: A.1 used exec.Run("systemctl","unmask",csfServiceUnit) wrapped by helper unmaskCSFService; A.3 used exec.Run("mv",old,new) wrapped by helper renameAtomicViaExec. - After: A.1 uses m.exec.ServiceUnmask(csfServiceUnit); A.3 uses m.exec.Rename(csfBinaryDisabled, csfBinary). Both helpers REMOVED. Mutation surface is unchanged in operational meaning; the typed call shape lets the CI gate enforce per-call discipline at the symbol level instead of via fragile Run-arg parsing. Files changed (6): internal/installer/executor/executor.go - Executor interface gains: ServiceUnmask(unit string) error // inverse of ServiceMask Rename(oldpath, newpath string) error // atomic same-FS rename - Header doc updated to list both new methods. internal/installer/executor/real.go - RealExecutor.ServiceUnmask runs systemctl unmask via Run with the same typed error wrapping pattern as ServiceMask. - RealExecutor.Rename calls os.Rename directly (consistent with WriteFileAtomic's existing os.Rename usage). No process spawn. internal/installer/executor/mock.go - MockExecutor.ServiceUnmask records ("systemctl","unmask",unit) and returns m.ServiceUnmaskErr (nil by default). - MockExecutor.Rename records ("rename",oldpath,newpath), returns m.RenameErr if non-nil; otherwise simulates the rename in the in-memory Files map. - New error-injection fields: ServiceUnmaskErr, RenameErr — mirror the RunResults exit-code injection pattern. cmd/nftban-installer/restore_deps_csf.go - Helpers unmaskCSFService and renameAtomicViaExec REMOVED. Replaced by a comment block documenting the §43.2 lock. - A.1 call site: m.exec.ServiceUnmask(csfServiceUnit). - A.3 call site: m.exec.Rename(csfBinaryDisabled, csfBinary). - No raw Run("systemctl","unmask",…) and no raw Run("mv",…) remain. - Log messages preserved; error wrapping (ErrCSFRestoreUnmaskFailed + ErrCSFRestoreBinaryRestoreFailed) preserved. cmd/nftban-installer/restore_deps_csf_test.go - buildCSFFixture: unmaskFailsExit injects mock.ServiceUnmaskErr; mvBinaryFailsExit injects mock.RenameErr (the previous RunResults-based simulation is removed; the OnCommand callback that simulated the rename in the Files map is also removed — Mock.Rename does that natively now). - TestCSFMutate_4B3csf_A3_* tests updated: assertions move from CommandCalled("mv", …) to CommandCalled("rename", …) because Mock.Rename records "rename", not "mv". - HappyPath_NoOutOfTargetMutation allow-list and OrderingPin expected sequences updated: A.3's recorded shape becomes ("rename", oldpath, newpath) instead of ("mv", oldpath, newpath). - Seven new TestCSFMutate_PR26B_* tests added: 1. A1_ServiceUnmaskOnlyCSFService — pins ServiceUnmask called only on csf.service 2. A3_RenameOnlyCSFBinaryRestore — pins Rename called only with (csf.disabled → csf) 3. NoRawSystemctlUnmaskRun_FileScan — pins no raw Run("systemctl","unmask",…) remains in production source 4. NoRawMvRun_FileScan — pins no raw Run("mv",…) remains 5. A1_UnmaskFailure_TypedErrorPreserved — error contract preserved through the migration 6. A3_RenameFailure_TypedErrorPreserved — same for A.3 7. RemovedHelpersGone_FileScan — pins removal of the unmaskCSFService and renameAtomicViaExec function definitions .github/workflows/ci-restore-canonization.yml - G4-RESTORE-EXEC-NO-OUT-OF-TARGET strengthened per §43.4 + §46.1: * \bexec\.ServiceUnmask\( REMOVED from forbidden list (now the authorized typed method for A.1). * Added forbidden patterns for raw Run("systemctl",…) with any mutating verb (start/stop/enable/disable/mask/unmask/restart/ reload/daemon-reload). Read-only Run("systemctl","is-enabled",…) remains authorized. * Added forbidden pattern for raw Run("mv",…). Typed Rename is the only authorized atomic-rename path. * §46.1 line-skipping discipline applied: gate strips line-leading "//" comments before pattern matching, preventing the false-positive class that broke Policy Gates on PR #511. * Header rewritten to reflect the post-PR-26-code-B authorized mutation set (typed ServiceUnmask / typed Rename; no raw Run for mutating systemctl verbs or mv). Constraints honored (per §51.6 + operator scope): IN scope: - typed executor.ServiceUnmask ✓ - typed executor.Rename ✓ - migration of CSF restore A.1 + A.3 to typed methods ✓ - raw Run policy tightening (CI gate) ✓ - G4-RESTORE-EXEC-NO-OUT-OF-TARGET strengthened ✓ OUT of scope (and untouched): - cron backup / A.4 (PR-26-code-C) - destructive soak (PR-26-code-E) - IptablesRuleExists / iptables introspection (Option B lock) - target-specific predicate changes (already done in code-A) - inline verification behavior changes - restore decision lattice - TargetAuthority / planner - main.go history gate - state machine / exit codes - contract.md - repo hygiene / UX / GOTH / metrics / module cleanup Verified on lab2 (Ubuntu 24.04, go1.22.2): - go build ./... clean - go test ./cmd/nftban-installer/... ./internal/installer/restore/... ./internal/installer/state/... ./internal/installer/executor/... PASS - go test ./... PASS (full suite) - go test -race -count=1 ./cmd/nftban-installer ./internal/installer/restore/... ./internal/installer/state/... PASS - go vet (cmd + restore + state + executor) clean - go mod tidy no-op - 7 new TestCSFMutate_PR26B_* tests all PASS - Local replay of strengthened G4-RESTORE-EXEC-NO-OUT-OF-TARGET gate: FAIL=0 against the migrated restore_deps_csf.go (only authorized NftDeleteTable("ip","nftban") + NftDeleteTable("ip6","nftban") calls; no raw mutating Run, no os.* bypass, no custombuild/iptables/rebuild/purge symbols). Awaiting auditor pass before push. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/workflows/ci-restore-canonization.yml | 96 ++++--- cmd/nftban-installer/restore_deps_csf.go | 37 +-- cmd/nftban-installer/restore_deps_csf_test.go | 243 +++++++++++++++--- internal/installer/executor/executor.go | 24 +- internal/installer/executor/mock.go | 34 +++ internal/installer/executor/real.go | 20 ++ 6 files changed, 360 insertions(+), 94 deletions(-) diff --git a/.github/workflows/ci-restore-canonization.yml b/.github/workflows/ci-restore-canonization.yml index 02738db8d..80159726c 100644 --- a/.github/workflows/ci-restore-canonization.yml +++ b/.github/workflows/ci-restore-canonization.yml @@ -263,31 +263,35 @@ jobs: # ------------------------------------------------------------------ # G4-RESTORE-EXEC-NO-OUT-OF-TARGET — closed mutation surface in # the cmd/-side restore-execution code. Per Amendment 1 §31 / §32, - # the only authorized mutations on the CSF restore path are: + # the authorized mutations on the CSF restore path are: # - # - Run("systemctl", "unmask", "csf.service") A.1 - # - ServiceEnable("csf.service") A.2 - # - Run("mv", "/usr/sbin/csf.disabled", - # "/usr/sbin/csf") A.3 - # - ServiceStart("csf.service") A.5 - # - ServiceStop("nftband.service") A.6 - # - NftDeleteTable("ip", "nftban") A.7 - # - NftDeleteTable("ip6", "nftban") A.7 + # - exec.ServiceUnmask("csf.service") A.1 (typed; PR-26-code-B) + # - exec.ServiceEnable("csf.service") A.2 + # - exec.Rename("/usr/sbin/csf.disabled", + # "/usr/sbin/csf") A.3 (typed; PR-26-code-B) + # - exec.ServiceStart("csf.service") A.5 + # - exec.ServiceStop("nftband.service") A.6 + # - exec.NftDeleteTable("ip", "nftban") A.7 + # - exec.NftDeleteTable("ip6", "nftban") A.7 # - # The gate's job is forbidden-symbol coverage at the call-expression - # level. Per-call argument enforcement (which unit is started, which - # path is renamed) is delegated to the in-Go runtime tests in - # restore_deps_csf_test.go (TestCSFMutate_4B3csf_A1_NoUnmaskOfOtherServices, + # PR-26-code-B (§43) promoted A.1 + A.3 from raw `Run("systemctl", + # "unmask", …)` and `Run("mv", …)` to the typed methods listed + # above. The gate's old `\bexec\.ServiceUnmask\(` forbid is + # therefore REMOVED, and explicit raw-Run forbids replace it. + # + # The gate's job is forbidden-symbol coverage. Per-call argument + # enforcement (which unit is started, which path is renamed) is + # delegated to the in-Go runtime tests in restore_deps_csf_test.go + # (TestCSFMutate_4B3csf_A1_NoUnmaskOfOtherServices, # …_A2_EnableOnlyCSFService, …_A5_StartsOnlyCSFService, - # …_A6_StopsOnlyNftband_AfterCSFStarts, - # …_HappyPath_NoOutOfTargetMutation) which assert against - # MockExecutor with full Go-level type information. Trying to - # reproduce that in shell regex on identifier arguments - # (csfServiceUnit, oldpath/newpath) gives false confidence. + # …_A6_StopsOnlyNftband_AfterCSFStarts, …_HappyPath_NoOutOfTargetMutation, + # TestCSFMutate_PR26B_A1_ServiceUnmaskOnlyCSFService, + # TestCSFMutate_PR26B_A3_RenameOnlyCSFBinaryRestore). # - # Forbidden patterns are call-expression-anchored (\bexec\.) so - # they catch real call sites and skip prose / error messages / - # comments / const definitions. + # §46.1 line-skipping discipline: the gate scans only the + # production file (no _test.go), and skips line-leading "//" + # comments to avoid the false-positive class that hit Policy + # Gates on PR #511. # ------------------------------------------------------------------ - name: G4-RESTORE-EXEC-NO-OUT-OF-TARGET — restore_deps_csf.go closed mutation surface shell: bash @@ -300,23 +304,24 @@ jobs: exit 1 fi + # §46.1 production-only scan: build a comment-stripped view + # so doc-comment lines never trip the forbidden-pattern grep. + stripped=$(grep -vE '^[[:space:]]*//' "$target" || true) + # Forbidden: out-of-Amendment-1 mutation surface. Each pattern # is anchored to a call expression (\bexec\. or similar) so - # legitimate doc strings, error messages, and const - # definitions don't false-match. + # legitimate string literals don't false-match. Doc comments + # are already excluded by the line-leading // strip above. forbidden_patterns=( - # Service-policy mutations (mask/disable/unmask-typed/daemon-reload). - # The csf restore path uses Run("systemctl","unmask",csf.service) - # for A.1; the typed exec.ServiceUnmask method is forbidden so - # we don't accidentally land on a future executor surface that - # bypasses the audit trail. + # Service-policy mutations (mask/disable/daemon-reload) are + # forbidden by Amendment 1 §34. ServiceUnmask is no longer + # forbidden — PR-26-code-B promoted it to authorized typed + # method for A.1. '\bexec\.ServiceMask\(' '\bexec\.ServiceDisable\(' - '\bexec\.ServiceUnmask\(' '\bexec\.DaemonReload\(' - # Filesystem-mutation primitives — A.4 cron-restore is a - # soft-skip in 4B-3-csf; any WriteFileAtomic in this file is - # therefore an out-of-scope mutation. + # Filesystem-mutation primitives. A.4 cron-restore is a + # soft-skip; any WriteFileAtomic here is out of scope. '\bexec\.WriteFileAtomic\(' # Direct OS bypass (must route through the executor). '"os/exec"' @@ -326,6 +331,22 @@ jobs: '\bos\.WriteFile\(' '\bos\.Create\(' '\bsyscall\.' + # PR-26-code-B raw-Run policy tightening (§43.3): mutating + # systemctl verbs MUST go through their typed methods. + # Read-only `Run("systemctl", "is-enabled", …)` and similar + # probes remain authorized. + 'Run\("systemctl",[[:space:]]*"start"' + 'Run\("systemctl",[[:space:]]*"stop"' + 'Run\("systemctl",[[:space:]]*"enable"' + 'Run\("systemctl",[[:space:]]*"disable"' + 'Run\("systemctl",[[:space:]]*"mask"' + 'Run\("systemctl",[[:space:]]*"unmask"' + 'Run\("systemctl",[[:space:]]*"restart"' + 'Run\("systemctl",[[:space:]]*"reload"' + 'Run\("systemctl",[[:space:]]*"daemon-reload"' + # Raw mv via Run is forbidden — typed exec.Rename is the + # only authorized atomic-rename surface. + 'Run\("mv"\b' # DirectAdmin custombuild rewrite forbidden (§34). '\bcustombuild\b' '"build set csf"' @@ -343,8 +364,11 @@ jobs: fail=0 for pat in "${forbidden_patterns[@]}"; do - if grep -nE "$pat" "$target" 2>/dev/null; then + if echo "$stripped" | grep -nE "$pat" >/dev/null 2>&1; then + # Re-grep against the original file (with line numbers) + # so the error message points at the right line. echo "::error::G4-RESTORE-EXEC-NO-OUT-OF-TARGET: forbidden call expression matching '$pat' found in $target" + grep -nE "$pat" "$target" || true fail=1 fi done @@ -354,8 +378,8 @@ jobs: # NftDeleteTable's args in the production code ARE literal # quoted strings (no constants), so this allow-list pin works # cleanly without identifier resolution. Per-unit / per-path - # enforcement for systemctl + mv is delegated to the Go runtime - # tests against MockExecutor. + # enforcement for systemctl + Rename is delegated to the Go + # runtime tests against MockExecutor. while IFS= read -r line; do args=$(echo "$line" | grep -oE 'NftDeleteTable\([^)]*\)' || true) if [[ -n "$args" ]]; then @@ -368,7 +392,7 @@ jobs: ;; esac fi - done < <(grep -n 'NftDeleteTable(' "$target" || true) + done < <(echo "$stripped" | grep -n 'NftDeleteTable(' || true) if [[ "$fail" -ne 0 ]]; then echo "::error::Amendment 1 §30 / §34 violation — restore_deps_csf.go must contain only the closed §31/§32 mutation surface." diff --git a/cmd/nftban-installer/restore_deps_csf.go b/cmd/nftban-installer/restore_deps_csf.go index 6c31f1620..15085bf1b 100644 --- a/cmd/nftban-installer/restore_deps_csf.go +++ b/cmd/nftban-installer/restore_deps_csf.go @@ -182,32 +182,13 @@ func isCSFServiceMasked(exec executor.Executor) bool { return strings.TrimSpace(res.Stdout) == "masked" } -// unmaskCSFService runs systemctl unmask via the executor abstraction. -// The Executor interface does not expose a typed ServiceUnmask method -// (only ServiceMask). Routing the inverse through Run is the chosen -// in-scope option for 4B-3-csf — it stays inside cmd/ and uses the -// existing executor seam without expanding the interface mid-PR. -func unmaskCSFService(exec executor.Executor) error { - res := exec.Run("systemctl", "unmask", csfServiceUnit) - if res.ExitCode != 0 { - return fmt.Errorf("systemctl unmask %s: %s", csfServiceUnit, strings.TrimSpace(res.Stderr)) - } - return nil -} - -// renameAtomicViaExec performs an atomic rename via the executor's -// Run("mv", ...) — same-filesystem mv is atomic at the syscall level. -// The Executor interface does not expose a Rename method; routing -// through Run keeps the surface inside the executor abstraction. The -// direct stdlib rename API (in the os package) is forbidden by the -// file-scan and intentionally not used here. -func renameAtomicViaExec(exec executor.Executor, oldpath, newpath string) error { - res := exec.Run("mv", oldpath, newpath) - if res.ExitCode != 0 { - return fmt.Errorf("mv %s -> %s: %s", oldpath, newpath, strings.TrimSpace(res.Stderr)) - } - return nil -} +// (PR-26-code-B / §43.2 lock — the prior unmaskCSFService and +// renameAtomicViaExec helper functions are removed. Both routed +// through the raw `Run("systemctl","unmask",…)` and `Run("mv",…)` +// indirections that PR-26-code-B closes by promoting the operations +// to typed `executor.ServiceUnmask` and `executor.Rename` methods. +// The §31 A.1 + A.3 call sites in mutateToCSFTarget call those typed +// methods directly — no thin wrapper is needed.) // ============================================================================= // mutateToCSFTarget — the §31/§32 entry point invoked by @@ -292,7 +273,7 @@ func mutateToCSFTarget(ctx context.Context, m *productionMutationDep) error { if m.log != nil { m.log.Info("restore csf: A.1 unmasking %s", csfServiceUnit) } - if err := unmaskCSFService(m.exec); err != nil { + if err := m.exec.ServiceUnmask(csfServiceUnit); err != nil { return fmt.Errorf("%w: %v", ErrCSFRestoreUnmaskFailed, err) } } else if m.log != nil { @@ -317,7 +298,7 @@ func mutateToCSFTarget(ctx context.Context, m *productionMutationDep) error { if m.log != nil { m.log.Info("restore csf: A.3 renaming %s -> %s", csfBinaryDisabled, csfBinary) } - if err := renameAtomicViaExec(m.exec, csfBinaryDisabled, csfBinary); err != nil { + if err := m.exec.Rename(csfBinaryDisabled, csfBinary); err != nil { return fmt.Errorf("%w: %v", ErrCSFRestoreBinaryRestoreFailed, err) } } else if m.log != nil { diff --git a/cmd/nftban-installer/restore_deps_csf_test.go b/cmd/nftban-installer/restore_deps_csf_test.go index 75941fb9e..0fb5c342c 100644 --- a/cmd/nftban-installer/restore_deps_csf_test.go +++ b/cmd/nftban-installer/restore_deps_csf_test.go @@ -91,25 +91,19 @@ func buildCSFFixture(t *testing.T, f csfTestFixture) (*productionMutationDep, *e } } - // systemctl unmask csf.service. + // PR-26-code-B: systemctl unmask now goes through the typed + // executor.ServiceUnmask method. Simulate failure via the + // MockExecutor.ServiceUnmaskErr injection field. if f.unmaskFailsExit != 0 { - mock.RunResults["systemctl:unmask:"+csfServiceUnit] = executor.Result{ - ExitCode: f.unmaskFailsExit, Stderr: "simulated unmask failure", - } + mock.ServiceUnmaskErr = errors.New("simulated unmask failure") } - // mv csf.disabled csf. + // PR-26-code-B: mv now goes through the typed executor.Rename + // method. Failure is injected via MockExecutor.RenameErr; success + // updates mock.Files automatically (the typed-method semantics + // match the prior OnCommand callback that simulated the rename). if f.mvBinaryFailsExit != 0 { - mock.RunResults["mv:"+csfBinaryDisabled+":"+csfBinary] = executor.Result{ - ExitCode: f.mvBinaryFailsExit, Stderr: "simulated mv failure", - } - } else { - // Successful mv: simulate the rename in the mock's Files map - // via OnCommand so subsequent FileExists reflects reality. - mock.OnCommand(func() { - delete(mock.Files, csfBinaryDisabled) - mock.Files[csfBinary] = []byte{} - }, "mv", csfBinaryDisabled, csfBinary) + mock.RenameErr = errors.New("simulated rename failure") } // nftban tables. @@ -352,9 +346,11 @@ func TestCSFMutate_4B3csf_A3_Renames_DisabledToCSF(t *testing.T) { }) _ = mutateToCSFTarget(context.Background(), dep) - mvCalled := mock.CommandCalled("mv", csfBinaryDisabled, csfBinary) - if !mvCalled { - t.Errorf("A.3 did not call mv %s -> %s", csfBinaryDisabled, csfBinary) + // PR-26-code-B: A.3 now goes through typed executor.Rename, which + // MockExecutor records as "rename" (not "mv"). + renameCalled := mock.CommandCalled("rename", csfBinaryDisabled, csfBinary) + if !renameCalled { + t.Errorf("A.3 did not call Rename(%s, %s)", csfBinaryDisabled, csfBinary) } } @@ -367,8 +363,8 @@ func TestCSFMutate_4B3csf_A3_SkipsWhenDisabledAbsent(t *testing.T) { }) _ = mutateToCSFTarget(context.Background(), dep) - if mock.CommandCalled("mv", csfBinaryDisabled, csfBinary) { - t.Errorf("A.3 called mv when .disabled was absent") + if mock.CommandCalled("rename", csfBinaryDisabled, csfBinary) { + t.Errorf("A.3 called Rename when .disabled was absent") } } @@ -385,8 +381,8 @@ func TestCSFMutate_4B3csf_A3_AmbiguousAlreadyCovered(t *testing.T) { if !errors.Is(err, ErrCSFRestoreAmbiguousBinary) { t.Fatalf("err = %v; want ErrCSFRestoreAmbiguousBinary", err) } - if mock.CommandCalled("mv", csfBinaryDisabled, csfBinary) { - t.Errorf("ambiguous-binary refusal still ran A.3's mv") + if mock.CommandCalled("rename", csfBinaryDisabled, csfBinary) { + t.Errorf("ambiguous-binary refusal still ran A.3's Rename") } } @@ -789,7 +785,8 @@ func TestCSFMutate_4B3csf_HappyPath_NoOutOfTargetMutation(t *testing.T) { {"systemctl", []string{"enable", csfServiceUnit}}, {"systemctl", []string{"start", csfServiceUnit}}, {"systemctl", []string{"stop", nftbandUnit}}, - {"mv", []string{csfBinaryDisabled, csfBinary}}, + // PR-26-code-B: typed Rename records as "rename" (not "mv"). + {"rename", []string{csfBinaryDisabled, csfBinary}}, } matchAllowed := func(cmd executor.RecordedCommand) bool { for _, a := range allowed { @@ -909,13 +906,14 @@ func TestCSFMutate_4B3csf_OrderingPin(t *testing.T) { } // Build the list of expected signatures (relative order). + // PR-26-code-B: A.3 records "rename" (typed Rename), not "mv". want := []sig{ - {"systemctl", "is-enabled"}, // §32 step 1 / A.1 gate - {"systemctl", "unmask"}, // A.1 - {"systemctl", "enable"}, // A.2 - {"mv", csfBinaryDisabled}, // A.3 - {"systemctl", "start"}, // A.5 - {"systemctl", "stop"}, // A.6 + {"systemctl", "is-enabled"}, // §32 step 1 / A.1 gate + {"systemctl", "unmask"}, // A.1 + {"systemctl", "enable"}, // A.2 + {"rename", csfBinaryDisabled}, // A.3 + {"systemctl", "start"}, // A.5 + {"systemctl", "stop"}, // A.6 } // Verify `seen` contains `want` as a subsequence. @@ -929,3 +927,190 @@ func TestCSFMutate_4B3csf_OrderingPin(t *testing.T) { t.Errorf("ordering subsequence mismatch — at %d of %d. seen=%+v want=%+v", wi, len(want), seen, want) } } + +// ============================================================================= +// ============================================================================= +// PR-26-code-B — typed executor migration tests +// ============================================================================= +// ============================================================================= + +// ============================================================================= +// PR-26-code-B test #1: A.1 calls typed exec.ServiceUnmask only on +// csf.service — never on any other unit. +// ============================================================================= + +func TestCSFMutate_PR26B_A1_ServiceUnmaskOnlyCSFService(t *testing.T) { + dep, mock := buildCSFFixture(t, csfTestFixture{ + priorRecCSF: true, + priorRecActive: true, + csfDisabledPresent: true, + csfMasked: true, + }) + _ = mutateToCSFTarget(context.Background(), dep) + + unmaskCount := 0 + for _, cmd := range mock.Commands { + if cmd.Name == "systemctl" && len(cmd.Args) >= 2 && cmd.Args[0] == "unmask" { + unmaskCount++ + if cmd.Args[1] != csfServiceUnit { + t.Errorf("ServiceUnmask called on unauthorized unit: %+v", cmd) + } + } + } + if unmaskCount != 1 { + t.Errorf("ServiceUnmask call count = %d; want exactly 1 (csf.service)", unmaskCount) + } +} + +// ============================================================================= +// PR-26-code-B test #2: A.3 calls typed exec.Rename only with the +// csf.disabled → csf path pair. +// ============================================================================= + +func TestCSFMutate_PR26B_A3_RenameOnlyCSFBinaryRestore(t *testing.T) { + dep, mock := buildCSFFixture(t, csfTestFixture{ + priorRecCSF: true, + priorRecActive: true, + csfDisabledPresent: true, + }) + _ = mutateToCSFTarget(context.Background(), dep) + + renameCount := 0 + for _, cmd := range mock.Commands { + if cmd.Name == "rename" { + renameCount++ + if len(cmd.Args) != 2 || cmd.Args[0] != csfBinaryDisabled || cmd.Args[1] != csfBinary { + t.Errorf("Rename called outside A.3 scope: %+v", cmd) + } + } + } + if renameCount != 1 { + t.Errorf("Rename call count = %d; want exactly 1 (csf.disabled -> csf)", renameCount) + } +} + +// ============================================================================= +// PR-26-code-B test #3: no raw Run("systemctl", "unmask", …) remains +// in the CSF restore path. File-scan against restore_deps_csf.go. +// ============================================================================= + +func TestCSFMutate_PR26B_NoRawSystemctlUnmaskRun_FileScan(t *testing.T) { + body, err := os.ReadFile("restore_deps_csf.go") + if err != nil { + t.Fatalf("read restore_deps_csf.go: %v", err) + } + src := string(body) + + // Exclude doc-comment lines (production-code-only scan per §46.1). + var prodLines []string + for _, line := range strings.Split(src, "\n") { + trimmed := strings.TrimLeft(line, " \t") + if strings.HasPrefix(trimmed, "//") { + continue + } + prodLines = append(prodLines, line) + } + prodSrc := strings.Join(prodLines, "\n") + + if strings.Contains(prodSrc, `Run("systemctl", "unmask"`) { + t.Errorf("restore_deps_csf.go still contains a raw Run(\"systemctl\", \"unmask\", …) call — PR-26-code-B requires the typed exec.ServiceUnmask") + } +} + +// ============================================================================= +// PR-26-code-B test #4: no raw Run("mv", …) remains in the CSF restore +// path. File-scan against restore_deps_csf.go. +// ============================================================================= + +func TestCSFMutate_PR26B_NoRawMvRun_FileScan(t *testing.T) { + body, err := os.ReadFile("restore_deps_csf.go") + if err != nil { + t.Fatalf("read restore_deps_csf.go: %v", err) + } + src := string(body) + + var prodLines []string + for _, line := range strings.Split(src, "\n") { + trimmed := strings.TrimLeft(line, " \t") + if strings.HasPrefix(trimmed, "//") { + continue + } + prodLines = append(prodLines, line) + } + prodSrc := strings.Join(prodLines, "\n") + + if strings.Contains(prodSrc, `Run("mv"`) { + t.Errorf("restore_deps_csf.go still contains a raw Run(\"mv\", …) call — PR-26-code-B requires the typed exec.Rename") + } +} + +// ============================================================================= +// PR-26-code-B test #5: A.1 unmask failure still surfaces the same +// typed CSF restore error (ErrCSFRestoreUnmaskFailed). Migration must +// not change the error contract. +// ============================================================================= + +func TestCSFMutate_PR26B_A1_UnmaskFailure_TypedErrorPreserved(t *testing.T) { + dep, _ := buildCSFFixture(t, csfTestFixture{ + priorRecCSF: true, + priorRecActive: true, + csfDisabledPresent: true, + csfMasked: true, + unmaskFailsExit: 1, // injects ServiceUnmaskErr via fixture + }) + err := mutateToCSFTarget(context.Background(), dep) + if !errors.Is(err, ErrCSFRestoreUnmaskFailed) { + t.Errorf("err = %v; want ErrCSFRestoreUnmaskFailed (PR-26-code-B migration must preserve the error contract)", err) + } +} + +// ============================================================================= +// PR-26-code-B test #6: A.3 rename failure still surfaces the same +// typed CSF restore error (ErrCSFRestoreBinaryRestoreFailed). +// ============================================================================= + +func TestCSFMutate_PR26B_A3_RenameFailure_TypedErrorPreserved(t *testing.T) { + dep, _ := buildCSFFixture(t, csfTestFixture{ + priorRecCSF: true, + priorRecActive: true, + csfDisabledPresent: true, + mvBinaryFailsExit: 1, // injects RenameErr via fixture + }) + err := mutateToCSFTarget(context.Background(), dep) + if !errors.Is(err, ErrCSFRestoreBinaryRestoreFailed) { + t.Errorf("err = %v; want ErrCSFRestoreBinaryRestoreFailed (PR-26-code-B migration must preserve the error contract)", err) + } +} + +// ============================================================================= +// PR-26-code-B test #7: removed helper symbols are gone. Compile-time +// check (the symbols would not parse if referenced). The file-scan +// pin documents §43.2 lock visibly. +// ============================================================================= + +func TestCSFMutate_PR26B_RemovedHelpersGone_FileScan(t *testing.T) { + body, err := os.ReadFile("restore_deps_csf.go") + if err != nil { + t.Fatalf("read restore_deps_csf.go: %v", err) + } + src := string(body) + + var prodLines []string + for _, line := range strings.Split(src, "\n") { + trimmed := strings.TrimLeft(line, " \t") + if strings.HasPrefix(trimmed, "//") { + continue + } + prodLines = append(prodLines, line) + } + prodSrc := strings.Join(prodLines, "\n") + + // Function definitions, not just any identifier mention. Use the + // "func name(" pattern to avoid false-matching the removal-marker + // doc comment. + for _, sym := range []string{"func unmaskCSFService(", "func renameAtomicViaExec("} { + if strings.Contains(prodSrc, sym) { + t.Errorf("restore_deps_csf.go still defines %q — §43.2 lock requires removal in favor of typed executor methods", sym) + } + } +} diff --git a/internal/installer/executor/executor.go b/internal/installer/executor/executor.go index 5df461bf6..7c520806a 100644 --- a/internal/installer/executor/executor.go +++ b/internal/installer/executor/executor.go @@ -45,7 +45,10 @@ type Result struct { // // systemd: // - ServiceActive / ServiceEnable / ServiceStart / ServiceStop -// - ServiceDisable / ServiceMask / DaemonReload +// - ServiceDisable / ServiceMask / ServiceUnmask / DaemonReload +// +// File operations (atomic): +// - Rename — atomic same-filesystem rename via os.Rename // // System: // - CommandExists / UserExists / GroupExists / Getenv @@ -83,6 +86,18 @@ type Executor interface { // Symlink creates a symbolic link (newname -> oldname). Symlink(oldname, newname string) error + // Rename atomically renames oldpath to newpath. Same-filesystem + // semantics equivalent to syscall.Rename. Implementations route + // through whatever primitive their host abstraction provides + // (RealExecutor uses os.Rename; MockExecutor records the call and + // updates its in-memory file map). + // + // Added in PR-26-code-B per §43.2 lock to replace the previous + // Run("mv", ...) indirection used by restore_deps_csf.go's A.3 + // binary-restore step. Mutation surface is bounded by + // INV-PR26-NEW-MUTATION-SURFACES-BOUNDED (§44 row 2). + Rename(oldpath, newpath string) error + // NftTableExists returns true if the given nft table exists in the kernel. // family: "ip", "ip6", or "inet". table: e.g. "nftban". NftTableExists(family, table string) bool @@ -119,6 +134,13 @@ type Executor interface { // ServiceMask masks a systemd unit (prevents start by any means). ServiceMask(unit string) error + // ServiceUnmask unmasks a systemd unit (inverse of ServiceMask). + // Used by the CSF restore A.1 step. Added in PR-26-code-B per + // §43.2 lock to replace the previous + // Run("systemctl", "unmask", ...) indirection. Mutation surface + // is bounded by INV-PR26-NEW-MUTATION-SURFACES-BOUNDED (§44 row 2). + ServiceUnmask(unit string) error + // DaemonReload runs systemctl daemon-reload. DaemonReload() error diff --git a/internal/installer/executor/mock.go b/internal/installer/executor/mock.go index ec9aa85f4..28171a551 100644 --- a/internal/installer/executor/mock.go +++ b/internal/installer/executor/mock.go @@ -67,6 +67,13 @@ type MockExecutor struct { // ExistingCommands maps "name" -> exists. ExistingCommands map[string]bool + // PR-26-code-B: typed-method error injection. When set non-nil, + // the corresponding typed method returns the assigned error + // instead of nil. Mirrors the RunResults pattern that controls + // Run() exit codes. + ServiceUnmaskErr error + RenameErr error + // callbacks maps "name:args" -> function to call when command is executed. callbacks map[string]func() } @@ -183,6 +190,25 @@ func (m *MockExecutor) Remove(path string) error { func (m *MockExecutor) Symlink(_, _ string) error { return nil } +// Rename simulates atomic rename in the mock's in-memory file map and +// records a "rename" command for trace assertions. Returns +// m.RenameErr (nil by default); when non-nil, the file map is left +// unchanged (matching real-world atomic-rename failure semantics). +// PR-26-code-B addition. +func (m *MockExecutor) Rename(oldpath, newpath string) error { + m.recordCommand("rename", oldpath, newpath) + if m.RenameErr != nil { + return m.RenameErr + } + m.mu.Lock() + defer m.mu.Unlock() + if data, ok := m.Files[oldpath]; ok { + m.Files[newpath] = data + delete(m.Files, oldpath) + } + return nil +} + // --- nftables --- func (m *MockExecutor) NftTableExists(family, table string) bool { @@ -263,6 +289,14 @@ func (m *MockExecutor) ServiceMask(unit string) error { return nil } +// ServiceUnmask records a systemctl unmask call and returns +// m.ServiceUnmaskErr (nil by default). Mirrors ServiceMask semantics +// for parity. PR-26-code-B addition. +func (m *MockExecutor) ServiceUnmask(unit string) error { + m.recordCommand("systemctl", "unmask", unit) + return m.ServiceUnmaskErr +} + func (m *MockExecutor) DaemonReload() error { m.recordCommand("systemctl", "daemon-reload") return nil diff --git a/internal/installer/executor/real.go b/internal/installer/executor/real.go index 5fa938c2e..8ca917081 100644 --- a/internal/installer/executor/real.go +++ b/internal/installer/executor/real.go @@ -124,6 +124,18 @@ func (r *RealExecutor) Symlink(oldname, newname string) error { return os.Symlink(oldname, newname) } +// Rename atomically renames oldpath to newpath via os.Rename. Same- +// filesystem semantics; cross-filesystem renames will return an error +// per the os.Rename contract (the caller must ensure same-FS). +// +// Added in PR-26-code-B per §43.2: replaces the prior Run("mv", ...) +// indirection used by restore_deps_csf.go's A.3 step. Routing through +// os.Rename (rather than shelling to mv) avoids a process spawn and +// gives a typed error path consistent with WriteFileAtomic. +func (r *RealExecutor) Rename(oldpath, newpath string) error { + return os.Rename(oldpath, newpath) +} + // --- nftables --- func (r *RealExecutor) NftTableExists(family, table string) bool { @@ -227,6 +239,14 @@ func (r *RealExecutor) ServiceMask(unit string) error { return nil } +func (r *RealExecutor) ServiceUnmask(unit string) error { + res := r.Run("systemctl", "unmask", unit) + if res.ExitCode != 0 { + return fmt.Errorf("systemctl unmask %s: %s", unit, strings.TrimSpace(res.Stderr)) + } + return nil +} + func (r *RealExecutor) DaemonReload() error { res := r.Run("systemctl", "daemon-reload") if res.ExitCode != 0 {