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>
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
1 participant
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