Skip to content

Commit eaea427

Browse files
itcmsgrclaude
itcmsgr
and
claude
committed
fix(v1.100 PR-22A): uninstall dry-run boundary repair
Independent audit of merged PR #480 found three concrete violations of the PR-22 contract Stop Condition ("no writes under /var/lib/nftban/"): 1. os.WriteFile(<stateDir>/uninstall_plan.json, …) on every dry-run 2. sf.Transition(StateUninstallPlanning, …) persisted install_state 3. unconditional writeHistory() recorded a successful plan preview as "install_fail" in update-history.json — silently poisoning every operator dashboard that alerts on install_fail Plus one classifier false-negative: 4. partial nftban (table OR daemon, not both) WITHOUT external firewall fell through to AuthorityNone — PR-23 release logic would skip kernel cleanup of the orphan table Plus CI blind spot: 5. G3-UN-NO-MUTATION grep missed os.WriteFile/Create/MkdirAll/Rename; snapshot step did not cover /var/lib/nftban/ at all This PR is BOUNDARY REPAIR ONLY. It does not add uninstall mutation, does not change purge/remove semantics, does not expand prior-authority logic, does not begin PR-23. Scope locked to R1-R5 from the repair contract seed. R1 (uninstall_dryrun.go): remove os.WriteFile + sf.Transition. Plan renders to stdout only. Option B — no installer-state persistence during dry-run. R2 (main.go): guard writeHistory with cfg.mode != "uninstall" so a successful plan preview is not recorded as install_fail. R3 (authority.go): explicit case nftbanPartial && !extPresent → AuthorityAmbiguous with diagnostic note; tighten the AuthorityNone case with !nftbanPartial. Regression test added (TestClassify_PartialNFTBan_NoExternal_IsAmbiguous) plus symmetric daemon-up-no-table case. R4 (uninstall_dryrun_test.go): new falsifiable purity test calls runUninstallDryRun with MockExecutor + real tempdir and asserts: - zero executor writes (WriteFileAtomic) - zero directory creates - zero mutation commands (nft add/flush/delete, systemctl lifecycle, ufw/firewall-cmd, package-manager removal) - zero files on disk under temp stateDir Two tests: no-authority host and ambiguous host. R5 (ci-uninstall-canonization.yml): - extend grep patterns: os.WriteFile/Create/MkdirAll/Mkdir/Rename, sf.Transition, nft create, apt-get purge, dnf erase - widen E2E snapshot to include /var/lib/nftban/ (was only /etc/nftban + /usr/lib/nftban + /usr/sbin/nftban*) - new G3-UN-HISTORY-PURITY gate: seeds realistic update-history.json, runs dry-run twice (explicit + implicit), asserts byte-identical hash — catches the exact class that escaped PR #480 - run the new orchestrator purity test in CI Acceptance criteria (all pass by construction): - uninstall dry-run no longer writes misleading plan/history artifacts under /var/lib/nftban/ - successful uninstall dry-run does not create install_fail - partial nftban without external returns AuthorityAmbiguous - purity test exists and would fail on regression - CI would fail on the exact write-pattern class that escaped in PR #480 (os.WriteFile + history pollution + snapshot blind spot) - no uninstall mutation capability added - scope remains boundary repair only Depends on: 547aa08 (PR #480) Refs: V1100_LIFECYCLE_COMPLETION_CONTRACT.md §13 (frozen 2026-04-19) Refs: internal/installer/uninstall/contract.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 547aa08 commit eaea427

6 files changed

Lines changed: 428 additions & 29 deletions

File tree

‎.github/workflows/ci-uninstall-canonization.yml‎

Lines changed: 87 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -96,10 +96,15 @@ jobs:
9696
exit 1
9797
fi
9898
fail=0
99+
# PR-22A audit fix: the original grep missed os.WriteFile /
100+
# os.Create / os.MkdirAll / os.Rename, which is exactly how
101+
# the uninstall_plan.json write slipped past PR-22 review.
102+
# Direct os.* filesystem writers are now enumerated here.
99103
for pat in \
100104
'exec\.Run\("nft"[^)]*add' \
101105
'exec\.Run\("nft"[^)]*flush' \
102106
'exec\.Run\("nft"[^)]*delete' \
107+
'exec\.Run\("nft"[^)]*create' \
103108
'exec\.Run\("systemctl"[^)]*stop' \
104109
'exec\.Run\("systemctl"[^)]*start' \
105110
'exec\.Run\("systemctl"[^)]*restart' \
@@ -112,16 +117,29 @@ jobs:
112117
'exec\.Run\("iptables"[^-]' \
113118
'exec\.Run\("csf"' \
114119
'exec\.Run\("apt-get"[^)]*remove' \
120+
'exec\.Run\("apt-get"[^)]*purge' \
115121
'exec\.Run\("dnf"[^)]*remove' \
122+
'exec\.Run\("dnf"[^)]*erase' \
116123
'exec\.WriteFileAtomic\(' \
124+
'os\.WriteFile\(' \
125+
'os\.Create\(' \
126+
'os\.MkdirAll\(' \
127+
'os\.Mkdir\(' \
128+
'os\.Rename\(' \
117129
'os\.Remove\(' \
118-
'os\.RemoveAll\('; do
130+
'os\.RemoveAll\(' \
131+
'sf\.Transition\('; do
119132
# Note: ".conf.local" is NOT in this list — the contract
120133
# language in plan.go + contract.md LEGITIMATELY mentions
121134
# .conf.local when describing the §4.4 artifact policy.
122135
# The mutation danger is write/delete operations, which
123-
# the os.Remove + WriteFileAtomic patterns above already
124-
# cover regardless of which file they target.
136+
# the os.*/exec.WriteFileAtomic patterns above cover
137+
# regardless of which file they target.
138+
#
139+
# sf.Transition is forbidden in PR-22 scope because it
140+
# persists install_state under /var/lib/nftban/state. PR-22
141+
# is observational only — no state transitions during
142+
# dry-run.
125143
if grep -nE "$pat" $files 2>/dev/null; then
126144
echo "::error::G3-UN-NO-MUTATION FAIL: forbidden pattern '$pat' in PR-22 uninstall scope"
127145
fail=1
@@ -145,6 +163,17 @@ jobs:
145163
set -Eeuo pipefail
146164
go test -v ./internal/installer/uninstall/...
147165
166+
# PR-22A boundary repair: the orchestrator-level purity test lives
167+
# in cmd/nftban-installer/uninstall_dryrun_test.go. It falsifies
168+
# the "no mutation" claim directly by calling runUninstallDryRun
169+
# with a MockExecutor + real tempdir and asserting zero writes,
170+
# zero directory creates, and zero mutation-flavored commands.
171+
- name: Unit tests — runUninstallDryRun purity (R4)
172+
shell: bash
173+
run: |
174+
set -Eeuo pipefail
175+
go test -v -run 'TestRunUninstallDryRun_' ./cmd/nftban-installer/...
176+
148177
# ------------------------------------------------------------------
149178
# Build the installer binary so we can exercise end-to-end.
150179
# ------------------------------------------------------------------
@@ -172,6 +201,12 @@ jobs:
172201
shell: bash
173202
run: |
174203
set -Eeuo pipefail
204+
# PR-22A boundary repair: snapshot /var/lib/nftban/ truth
205+
# surfaces BEFORE the dry-run. After the dry-run, the snapshot
206+
# must be byte-identical. Any difference is a PR-22 contract
207+
# Stop Condition violation.
208+
before_varlib=$(find /var/lib/nftban -type f 2>/dev/null | sort | xargs -r sha256sum 2>/dev/null | sort || true)
209+
before_etcnftban=$(find /etc/nftban -type f 2>/dev/null | sort | xargs -r sha256sum 2>/dev/null | sort || true)
175210
set +e
176211
sudo ./bin/nftban-installer --mode=uninstall --dry-run \
177212
--state-dir=/var/lib/nftban/state 2>&1 | tee /tmp/uninstall-dryrun.out
@@ -182,6 +217,19 @@ jobs:
182217
echo "::error::uninstall dry-run must exit 0 even on a host with no firewall authority"
183218
exit 1
184219
fi
220+
after_varlib=$(find /var/lib/nftban -type f 2>/dev/null | sort | xargs -r sha256sum 2>/dev/null | sort || true)
221+
after_etcnftban=$(find /etc/nftban -type f 2>/dev/null | sort | xargs -r sha256sum 2>/dev/null | sort || true)
222+
if [[ "$before_varlib" != "$after_varlib" ]]; then
223+
echo "::error::PR-22 Stop Condition violated — uninstall dry-run modified /var/lib/nftban/"
224+
diff <(echo "$before_varlib") <(echo "$after_varlib") || true
225+
exit 1
226+
fi
227+
if [[ "$before_etcnftban" != "$after_etcnftban" ]]; then
228+
echo "::error::PR-22 Stop Condition violated — uninstall dry-run modified /etc/nftban/"
229+
diff <(echo "$before_etcnftban") <(echo "$after_etcnftban") || true
230+
exit 1
231+
fi
232+
echo "G3-UN-NO-WRITES PASS — /var/lib/nftban and /etc/nftban unchanged after dry-run"
185233
# Mandatory contract-language elements — each one corresponds
186234
# to a row PR-22 promised to render.
187235
for needle in \
@@ -212,24 +260,55 @@ jobs:
212260
shell: bash
213261
run: |
214262
set -Eeuo pipefail
215-
# Snapshot nothing should change outside /var/lib/nftban/state.
216-
before=$(find /etc/nftban /usr/lib/nftban /usr/sbin/nftban* -type f 2>/dev/null | sort | xargs -r sha256sum 2>/dev/null | sort || true)
263+
# PR-22A boundary repair: the earlier gate only snapshot
264+
# /etc/nftban + /usr/lib/nftban + /usr/sbin/nftban*. It did
265+
# NOT cover /var/lib/nftban/ — the exact directory that leaked
266+
# three writes in PR #480. The snapshot now includes all
267+
# three protected trees. No exceptions.
268+
before=$(find /etc/nftban /usr/lib/nftban /usr/sbin/nftban* /var/lib/nftban -type f 2>/dev/null | sort | xargs -r sha256sum 2>/dev/null | sort || true)
217269
set +e
218270
sudo ./bin/nftban-installer --mode=uninstall \
219271
--state-dir=/var/lib/nftban/state 2>&1 | tee /tmp/uninstall-implicit.out
220272
rc=${PIPESTATUS[0]}
221273
set -e
222-
after=$(find /etc/nftban /usr/lib/nftban /usr/sbin/nftban* -type f 2>/dev/null | sort | xargs -r sha256sum 2>/dev/null | sort || true)
274+
after=$(find /etc/nftban /usr/lib/nftban /usr/sbin/nftban* /var/lib/nftban -type f 2>/dev/null | sort | xargs -r sha256sum 2>/dev/null | sort || true)
223275
if [[ "$before" != "$after" ]]; then
224-
echo "::error::PR-22 contract violation — --mode=uninstall touched files outside /var/lib/nftban/state"
276+
echo "::error::PR-22 contract violation — --mode=uninstall touched protected filesystem"
225277
diff <(echo "$before") <(echo "$after") || true
226278
exit 1
227279
fi
228280
if [[ "$rc" -ne 0 ]]; then
229281
echo "::error::--mode=uninstall (no explicit dry-run) must exit 0 in PR-22 scope"
230282
exit 1
231283
fi
232-
echo "G3-UN-PLAN-RENDERS no-mutation PASS — no files changed under protected paths"
284+
echo "G3-UN-PLAN-RENDERS no-mutation PASS — no files changed under /etc/nftban, /usr/lib/nftban, /usr/sbin/nftban*, /var/lib/nftban"
285+
286+
# ------------------------------------------------------------------
287+
# G3-UN-HISTORY-PURITY — regression guard for audit finding A.3.
288+
# Seeds a realistic update-history.json, runs the dry-run twice
289+
# (explicit + implicit), then asserts the history file is byte-
290+
# identical. Before PR-22A, each dry-run appended an install_fail
291+
# entry here.
292+
# ------------------------------------------------------------------
293+
- name: G3-UN-HISTORY-PURITY — history file unchanged by dry-run
294+
shell: bash
295+
run: |
296+
set -Eeuo pipefail
297+
hist=/var/lib/nftban/update-history.json
298+
sudo mkdir -p /var/lib/nftban
299+
echo '{"schema_version":"1.0","entries":[{"from":"1.99.0","to":"1.100.0","status":"success","type":"rpm","duration":1,"hostname":"ci","timestamp":"2026-04-19T00:00:00Z"}]}' | sudo tee "$hist" >/dev/null
300+
before_hash=$(sha256sum "$hist" | awk '{print $1}')
301+
sudo ./bin/nftban-installer --mode=uninstall --dry-run --state-dir=/var/lib/nftban/state >/dev/null 2>&1 || true
302+
sudo ./bin/nftban-installer --mode=uninstall --state-dir=/var/lib/nftban/state >/dev/null 2>&1 || true
303+
after_hash=$(sha256sum "$hist" | awk '{print $1}')
304+
if [[ "$before_hash" != "$after_hash" ]]; then
305+
echo "::error::G3-UN-HISTORY-PURITY FAIL — uninstall dry-run modified update-history.json"
306+
echo "before: $before_hash"
307+
echo "after: $after_hash"
308+
cat "$hist"
309+
exit 1
310+
fi
311+
echo "G3-UN-HISTORY-PURITY PASS — update-history.json byte-identical after 2 dry-runs"
233312
234313
summary:
235314
name: Uninstall Canonization summary

‎cmd/nftban-installer/main.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -98,8 +98,17 @@ func main() {
9898

9999
exitCode := run(ctx, exec, sf, cfg, log)
100100

101-
// Write JSON update history (compatible with nftban update history --json)
102-
writeHistory(sf, cfg, previousVersion, hostname, log)
101+
// Write JSON update history (compatible with nftban update history --json).
102+
//
103+
// PR-22A boundary repair (audit finding A.3): uninstall-mode dry-run
104+
// is strictly observational and must not create install/update history
105+
// entries. Before this guard, a successful uninstall dry-run would
106+
// reach historyStatusForState(StateUninstallPlanning), fall through to
107+
// the default case, and be recorded as "install_fail" — poisoning the
108+
// audit trail and any dashboard that alerts on install_fail.
109+
if cfg.mode != "uninstall" {
110+
writeHistory(sf, cfg, previousVersion, hostname, log)
111+
}
103112

104113
// Write run footer with final state for post-mortem
105114
log.RunFooter(string(sf.State), exitCode)

‎cmd/nftban-installer/uninstall_dryrun.go‎

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,28 @@ import (
4545
"github.com/itcmsgr/nftban/internal/installer/uninstall"
4646
)
4747

48+
// PR-22A boundary repair (audit finding A):
49+
//
50+
// The original PR-22 orchestrator persisted two artifacts under
51+
// /var/lib/nftban/ during a dry-run that the CLI banner described as
52+
// "no mutation":
53+
//
54+
// 1. os.WriteFile(<stateDir>/uninstall_plan.json, …)
55+
// 2. sf.Transition(StateUninstallPlanning, …) — writes install_state
56+
//
57+
// Both tripped the PR-22 contract Stop Condition: "writing to anywhere
58+
// under /etc/nftban/ or /var/lib/nftban/ → STOP PR-22."
59+
//
60+
// PR-22A removes both. The plan is rendered to stdout (human) and,
61+
// optionally, JSON to stderr below the render (machine). No filesystem
62+
// persistence occurs during the dry-run. Operators who need a
63+
// machine-consumable plan can capture stdout; the dry-run remains
64+
// strictly observational, as PR-22 originally promised.
65+
//
66+
// Install-state persistence during dry-run is deliberately removed
67+
// (Option B in the repair contract). A read-only planning run has no
68+
// reason to mutate the installer state file.
69+
4870
// runUninstallDryRun orchestrates the PR-22 detect + plan-render flow.
4971
//
5072
// Exit codes:
@@ -86,26 +108,19 @@ func runUninstallDryRun(_ context.Context, exec executor.Executor, sf *state.Sta
86108
plan := uninstall.BuildPlan(mode, auth, prior, restoreRequested)
87109
log.PhaseEnd("Plan")
88110

89-
// 5. Render to stdout (operator-visible) and persist JSON to state
90-
// dir for audit trail. Persisting is NOT mutation of install state;
91-
// it is a plan artifact under the installer's own state dir.
111+
// 5. Render to stdout (operator-visible). No filesystem persistence —
112+
// the dry-run is strictly observational per PR-22 contract Stop
113+
// Condition. Machine consumers can capture stdout; see also Note A
114+
// above for the PR-22A audit rationale.
92115
plan.Render(os.Stdout)
93116

94-
if data, err := plan.JSON(); err == nil {
95-
dst := cfg.stateDir + "/uninstall_plan.json"
96-
if werr := os.WriteFile(dst, data, 0600); werr != nil { //lint:ignore G306 plan audit artifact under state dir
97-
log.Warn("uninstall dry-run: could not persist plan to %s: %v", dst, werr)
98-
} else {
99-
log.Info("uninstall dry-run: plan written to %s", dst)
100-
}
101-
}
102-
103-
// 6. Transition to the planning terminal state so the state file
104-
// reflects what happened, and exit 0. No further phases exist.
105-
_ = sf.Transition(state.StateUninstallPlanning, state.PhaseDetect,
106-
"uninstall plan computed; no mutation (PR-22 scope)")
107-
108-
// 7. Emit the literal scope-boundary log marker per PR-22 contract.
117+
// 6. Emit the literal scope-boundary log marker per PR-22 contract.
118+
// The state file is NOT transitioned here: a read-only plan computes
119+
// no state change, and persisting StateUninstallPlanning during a
120+
// dry-run would trip the same Stop Condition the orchestrator
121+
// exists to respect. sf is retained in the signature for symmetry
122+
// with install/update orchestrators and for PR-23 takeover.
123+
_ = sf
109124
log.Info("uninstall dry-run complete — PR-22 scope boundary: NO mutation phase exists in this release")
110125
fmt.Fprintln(os.Stderr, "uninstall dry-run: plan rendered; no mutation (PR-22 scope)")
111126
return state.ExitCommitted

0 commit comments

Comments
 (0)