Add enforcement verification, zombie-tunnel detection, single-instance guard, exit-IP observation - #39
Open
Behnam-RK wants to merge 6 commits into
Open
Add enforcement verification, zombie-tunnel detection, single-instance guard, exit-IP observation#39Behnam-RK wants to merge 6 commits into
Behnam-RK wants to merge 6 commits into
Conversation
…e guard, exit-IP observation Adopts four gaps found comparing dezhban against ClaudeVpnGuardian, a similar Windows kill-switch project: nothing ever re-checked that firewall rules were still installed after Apply, a hung-but-interface-up tunnel had no diagnosis or recovery path, two `dezhban run` processes could race to apply rules, and exit-IP failovers within an allowed country had no signal. - Enforcement verification (vpn.advanced.verifyInterval, default 1m): the run loop periodically confirms its rules are still installed and re-applies the standing posture if they're not. An unreadable backend is never treated as evidence the rules are gone. - Zombie-tunnel detection: diagnosed unconditionally (state.zombie, doctor, rendered posture) when exit lookups fail through an interface that reports up. Acting on it — opening an automatic redial window — is a separate, off-by-default key (vpn.advanced.livenessRedial, ADR-0010), since a censoring exit produces the identical symptom on a tunnel that was never down. - Single-instance guard: `run` takes an exclusive lock over the state directory for its lifetime (flock on Unix, a named mutex on Windows), released by the OS on process exit by any means. panic/unblock/service commands stay lock-free, as the escape hatch must. - Exit-IP change observation (state.exitIpChangedAt): purely informational, never touches posture or the hysteresis streak. - A startup self-test log line summarizing backend/state-dir/tunnel/endpoint reachability. Full docs pass: config.md, cli.md, glossary.md, troubleshooting.md, testing.md, ADR-0010 + README index, CHANGELOG. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…entry dg.verify is only ever set or cleared by the verifyC tick handler, which is skipped whenever standby is true. Without an explicit reset, a "rules missing" finding from before a tunnel drop would keep being republished forever after a control-driven unblock into standby, misreporting an enforcement problem while the daemon is correctly idle. Add resetVerify(), call it at the standby-entry transition and when a live reload disables verifyInterval, and add a regression test that fails without the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…check lock_windows_test.go mirrors lock_unix_test.go's two cases for the Windows side of the single-instance guard (CreateMutexW/ERROR_ALREADY_EXISTS instead of flock), which PR #39 shipped without. Also switch buildLivenessCheck's Missing/Err branch to an exhaustive switch, documenting that the two are mutually exclusive by construction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
PR #39 review flagged two gaps: - No test exercised enforcement verification's `force` path re-applying an OPEN, UNRESTRICTED switch window — the one case reapplyWindow's ordinary callers skip, since an unrestricted window already passes everything and no tunnel/endpoint change ever needs to touch it. Verification's reason is different (the pass itself vanished), so reapplyCurrent's force branch has to reach it anyway. TestVerifyTickRepairsAnOpenUnrestrictedWindow pins this; confirmed it fails without the force call and passes with it. - `run` called state.EnsureDir(stateDir()) twice — once for the single-instance lock in cmdRun, again inside assembleOptions. internal/svc.Builder documents assembleOptions runs exactly once per process, so the second call was pure repeated mkdir/chmod. assembleOptions now takes the already-computed error instead of re-establishing the directory itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
An audit (asked directly: "did you wire these into both CLI and GUI properly?") found the four PR #39 features were only partially adopted: - internal/render.Text() had zombieNote but no verifyNote — so "firewall rules were found missing and re-applied" (state.Verify) had ZERO human-facing text on either surface: not dezhban status, not the menubar app, since both derive from this one Go function (cmd/dezhban's stampAndRender/cmdStatus). Added verifyNote, covering both the Missing (already repaired) and Err (unreadable backend, correctly untouched) cases. - Neither Verify nor Zombie ever changed Display.Key, so the menubar showed a plain green "Guarding" icon throughout a "rules removed and repaired" event or a hung-tunnel streak. Text() now upgrades KeyOn to the existing KeyWarning (amber) tier when either condition is present — never downgrading an already-worse Key (KeyBlocked stays KeyBlocked), consistent with the project's established red-is-reserved-for-real-exposure rule. - gui/macos/Sources/DezhbanCore/Snapshot.swift had no verify/zombie/ exitIpChangedAt fields — silently unreachable from any Swift code (the strict Codable struct just ignored the unknown keys). Added VerifyState/ ZombieState mirroring their Go counterparts, plus the three Snapshot fields, following the same omitzero/optional-Date pattern as drop/hold/ redial. - gui/macos/Sources/DezhbanCore/SettingsFields.swift's hand-maintained `keys` array never got vpn.advanced.verifyInterval/livenessRedial — a GUI user had no way to view or edit either setting without dropping to Terminal. Added both keys, the bool accessor's boolKeys entry, named computed accessors matching the redialMinUptime/redialBudget pattern, and the two corresponding rows in SettingsView.swift's advancedGroup. DoctorReport.swift, HelpIndex.swift, and the GUI's daemon-lifecycle calls were audited and found already correctly wired (DoctorReport parses check sections generically; the bundled docs ship whole files so the new troubleshooting/config/glossary content needed no manifest change; the GUI never invokes `run` directly so the single-instance lock can't surface there). Verified with `go build/vet/test ./...` (incl. new render_test.go cases pinning the Key-upgrade-never-downgrades rule) and `swift build`/`swift test` (108 tests, incl. 10 new ones across Snapshot/SettingsFields decoding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The liveness-redial trigger (vpn.advanced.livenessRedial) shared maybeAutoWindow/grantAutoWindow with the ordinary tunnel-down trigger, but two of that machinery's assumptions only hold for a real drop: - Its refusal/suppression logs hardcoded "vpn tunnel down", which is false for a zombie tunnel (interface reports up throughout) — misleading anyone reading the daemon log during exactly the condition this feature exists to diagnose clearly. - retryAutoWindow's bound-lifted retry bailed whenever tunnelUp was true, which is always, for a zombie streak — so a refused liveness-redial attempt could only ever get a second chance from the zombie check calling maybeAutoWindow again on literally every geoTick, not from the intended once-the-budget-refills retry path. Fixes: reword the two shared log lines to not assert tunnel state; gate the zombie trigger to one maybeAutoWindow attempt per streak (zombieRedialTried), matching the ordinary trigger's edge-only call; teach retryAutoWindow's guard to recognise a standing zombie streak as "still open" alongside tunnelUp; and clear a stale refusal left over from a resolved zombie streak the same way a real tunnel-up edge already does. Also correct reapplyCurrent's doc comment, which claimed it never rebuilds policies — its guard-posture path does, via reapplyStanding, harmlessly since verification changes none of that rebuild's inputs. Added TestAZombieRefusedRedialRetriesWithoutTheTunnelGoingDown, which fails against the pre-fix retryAutoWindow guard (verified by hand) and passes with it restored. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds enforcement health monitoring, tunnel-liveness diagnostics, exit-IP observation, and single-instance protection to the kill-switch daemon.
Changes:
- Periodically verifies and repairs firewall enforcement.
- Detects zombie tunnels with optional automatic redial and tracks exit-IP changes.
- Prevents concurrent daemon instances and updates configuration, GUI, tests, and documentation.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
internal/state/state.go |
Adds verification, zombie, and exit-IP state. |
internal/runner/verify_test.go |
Tests enforcement verification. |
internal/runner/runner.go |
Implements verification and liveness behavior. |
internal/runner/runner_test.go |
Extends backend test doubles. |
internal/runner/reload.go |
Adds new live settings. |
internal/runner/reload_test.go |
Covers live-setting capture. |
internal/runner/recovery_test.go |
Updates snapshot publication tests. |
internal/runner/liveness_test.go |
Tests zombie detection and redial. |
internal/runner/exitip_test.go |
Tests exit-IP observation. |
internal/runner/control_test.go |
Tests verification reset on standby. |
internal/render/render.go |
Renders new diagnostic warnings. |
internal/render/render_test.go |
Tests diagnostic rendering. |
internal/config/schema.go |
Exposes new tunables. |
internal/config/schema_test.go |
Tests verification disablement. |
internal/config/reload.go |
Makes new keys live-reloadable. |
internal/config/reload_test.go |
Tests live configuration merging. |
internal/config/config.go |
Parses and serializes new settings. |
gui/macos/Tests/DezhbanCoreTests/SnapshotTests.swift |
Tests new snapshot fields. |
gui/macos/Tests/DezhbanCoreTests/SettingsFieldsTests.swift |
Tests new settings fields. |
gui/macos/Sources/DezhbanMenu/SettingsView.swift |
Adds settings controls. |
gui/macos/Sources/DezhbanCore/Snapshot.swift |
Decodes diagnostic state. |
gui/macos/Sources/DezhbanCore/SettingsFields.swift |
Maps new configuration keys. |
docs/usage/troubleshooting.md |
Documents diagnostics and locking. |
docs/usage/config.md |
Documents new tunables. |
docs/usage/cli.md |
Documents state fields and instance locking. |
docs/contribute/testing.md |
Adds privileged verification checks. |
docs/concepts/glossary.md |
Defines new concepts. |
docs/adr/README.md |
Registers ADR-0010. |
docs/adr/0010-tunnel-liveness.md |
Records liveness design decisions. |
cmd/dezhban/reload_test.go |
Tests live-setting mapping. |
cmd/dezhban/main.go |
Wires features into startup and doctor. |
cmd/dezhban/lock_windows.go |
Implements Windows instance locking. |
cmd/dezhban/lock_windows_test.go |
Tests Windows mutex contention. |
cmd/dezhban/lock_unix.go |
Implements Unix instance locking. |
cmd/dezhban/lock_unix_test.go |
Tests Unix lock lifecycle. |
cmd/dezhban/config_roundtrip_test.go |
Tests new key round trips. |
cmd/dezhban/config_cmd.go |
Adds CLI configuration accessors. |
CHANGELOG.md |
Records user-visible features. |
Suppressed comments (1)
docs/usage/troubleshooting.md:334
- This is the opposite of what a missing ruleset means: the host may have been open from the external removal until the next verification tick, and the log is emitted before the re-apply attempt, which can itself fail. The
repairscounter also resets at daemon startup, so it cannot keep climbing across restarts. Document the possible exposure interval and require confirming restoration; describe recurrence as a rising count within one run or a finding that reappears after restart.
**This is not a leak that happened** — verification found the gap and repaired
it before you saw this message, not after. What it tells you is that *something
on this host keeps removing dezhban's rules*, which is worth tracking down
regardless: a `repairs` count that keeps climbing across restarts means it is
recurring, not a one-off.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+110
to
+115
| // IsBlocked reports whether dezhban's rules are currently installed. The | ||
| // run loop uses it for enforcement verification, and it is part of this | ||
| // narrow interface rather than an optional capability discovered by type | ||
| // assertion on purpose: a backend that silently could not be verified would | ||
| // reintroduce the very silent-failure mode verification exists to close. | ||
| IsBlocked() (bool, error) |
Comment on lines
+2425
to
+2432
| verifyRepairs++ | ||
| o.Log.Error("dezhban's firewall rules are MISSING — something removed them; re-applying now", | ||
| "posture", postureName(blocked, windowActive, standby), "repairs", verifyRepairs) | ||
| dg.verify = &state.VerifyState{At: time.Now(), Missing: true, Repairs: verifyRepairs} | ||
| // force: the rules are absent, so even an unrestricted window — | ||
| // which no tunnel/endpoint change would ever need to re-apply — | ||
| // has lost its pass and must be reinstalled. | ||
| reapplyCurrent("enforcement verification: rules missing", true) |
Comment on lines
2495
to
+2497
| if windowActive { | ||
| continue // window suppresses the geo state machine | ||
| resetZombie() // a window is already the response to a suspected problem | ||
| continue // window suppresses the geo state machine |
| } | ||
| o.VerifyInterval = ls.VerifyInterval | ||
| } | ||
| o.LivenessRedial = ls.LivenessRedial |
Comment on lines
+34
to
+40
| h := fnv.New64a() | ||
| _, _ = h.Write([]byte(dir)) | ||
| name, err := syscall.UTF16PtrFromString(fmt.Sprintf(`Global\dezhban-run-%x`, h.Sum64())) | ||
| if err != nil { | ||
| return fmt.Errorf("lock name: %w", err) | ||
| } | ||
| ret, _, callErr := procCreateMutex.Call(0, 0, uintptr(unsafe.Pointer(name))) |
| "Tunnel interface reports up, but %d consecutive exit checks through it have failed — "+ | ||
| "it may need reconnecting. Guard holds either way; this is diagnosis, not a leak.", snap.Zombie.Checks)) | ||
| } | ||
| c.Summary = "enforcement is holding, but something needs attention." |
Comment on lines
+323
to
+324
| **Cause.** Every other rule change dezhban makes is triggered by something it | ||
| itself did — a tunnel change, an endpoint refresh, a posture flip. This message |
Comment on lines
+2526
to
+2532
| ip := lastRes.Reading.IP | ||
| if ip.IsValid() { | ||
| if lastGoodIP.IsValid() && ip != lastGoodIP { | ||
| o.Log.Info("exit IP changed", "from", lastGoodIP, "to", ip) | ||
| dg.exitIPChangedAt = time.Now() | ||
| } | ||
| lastGoodIP = ip |
Comment on lines
+640
to
+642
| "stateDirWritable", stateDirErr == nil, | ||
| "tunnelsConfiguredOrDetected", len(tunnels) > 0, | ||
| "endpointsKnown", len(cfg.VPN.Endpoints) > 0 || cfg.VPN.AutoDiscoverEndpoints, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adopts four ideas from comparing dezhban against ClaudeVpnGuardian, a similar Windows-only kill-switch project — closing gaps dezhban had, without adopting anything that would weaken its stronger default-deny/enforce-at-rest model.
vpn.advanced.verifyInterval, default1m) — the run loop periodically confirmsBackend.IsBlocked()still says the rules are installed, and re-applies the standing posture the instant they are not. Previously, nothing ever noticed a ruleset removed from outside the daemon (another firewall tool,pfctl -F all,nft flush ruleset, an OS reset) — the daemon kept reporting GUARD while the host was open. An unreadable backend is never treated as evidence the rules are gone, mirroring the "undeterminable country HOLDS" discipline.state.zombie,dezhban doctor, the rendered posture sentence), always on. Acting on it — opening an automatic redial window — is a separate, off-by-default key (vpn.advanced.livenessRedial), because an exit that censors the geo providers produces the identical symptom on a tunnel that was never actually down. Full trade-off and alternatives in ADR-0010.runnow takes an exclusive lock over the state directory for its whole lifetime (flock on Unix, a named mutex on Windows), so a seconddezhban run(with or without--no-daemon) can't race an already-running daemon to callBackend.Apply. Released by the OS on process exit by any means, so a killed daemon never wedges the next start.panic/unblock/service-lifecycle commands deliberately take no such lock — they stay the escape hatch.state.exitIpChangedAt) — purely informational, logged and published when the exit IP differs from the previous successful reading; never touchesblocked,countryCode, or the hysteresis streak.Infoline at daemon start: firewall backend reachable, state dir writable, tunnels configured/detected, endpoints known, whether this host has ever observed a tunnel up.Deliberately not adopted from the comparison: CVG's DNS-guard/IPv6-disable (dezhban already blocks both at layer 3 with no persistent OS mutation to restore), its backup-bundle system (only needed because it does mutate OS state), per-check blocking policy profiles (would recreate the à-la-carte security ADR-0001 removed), and killing the protected app (a second enforcement model).
Test plan
go build ./...,go vet ./...,go test ./...— clean on macOSGOOS=windows GOARCH=amd64andGOOS=linux GOARCH=amd64build + vet — cleango test -race ./...— cleaninternal/runner/verify_test.go,liveness_test.go,exitip_test.go,cmd/dezhban/lock_unix_test.gointernal/firewall/*.gotoucheddocs/contribute/testing.md(flush ruleset by hand and confirm repair; black-hole a tunnel's traffic and confirm zombie diagnosis with no window by default; secondrunrefuses) — not run in this environment, flagged for manual verification before merge🤖 Generated with Claude Code