fix(apply): stop the status from reporting a state the daemon is not in
Running was the constant true. The panel builds its header from it, so the "offline" branch was unreachable code: with the engine dead and the LAN behind a fail-closed hold, the operator saw a pulsing green lamp and, on the page people open to fix things, "engine: running". The honest field sat beside it, documented as the honest answer to are-we-proxying, and was read nowhere. running now means shater is running: the daemon answered and its engine has a started instance. active stays what it always was and is documented as such — the "meant to be running" latch that gates hotplug and cron, not a health signal. It is deliberately not cleared on hold, because the cron loop gates on it and clearing it would switch off the reconcile that brings the engine back. Two paths published nothing and so left the previous config's verdict standing for as long as the fault lasted. A rollback with no snapshot re-applied the engine and the plane and never touched the traffic verdict, so a router rolled back to a direct default kept reporting the tunnel. And an apply that failed in the netplane stage had already swapped the engine, then returned before every publisher, so status described the config that was no longer running — and the next reconcile, seeing an unchanged hash, failed the same way and published nothing again. Both now publish, with an unknown verdict: after a no-snapshot rollback the engine runs options this process does not hold, and guessing from UCI would describe the config we rolled away from. The severity classifier had drifted from the texts production emits. Markers were compared case-sensitively against wording that had since changed, and the entity pattern could not match a message beginning with an upper-case tag — so a blocklist that failed to load graded as a warning while a typo in its URL graded critical, and the panel's banner, which only lights for criticals, stayed dark for the outage. RULESET-NOT-APPLIED and DNS-FILTER-NOT-APPLIED are now read as the structural markers their producer documents them to be, so severity no longer depends on wording at all. Five markers that matched no living text are deleted; three protection-section texts drop to warning, because a blocklist that is stale but still blocking lights the alarm on most reconciles behind a flaky link, and an alarm that is always on is how the real one goes unread. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
+307
-109
@@ -418,7 +418,7 @@ func (a *Applier) applyLocked(m *model.Model) (bool, error) {
|
||||
|
||||
// (2) engine swap. On error the old engine keeps running and we do NOT touch
|
||||
// netplane — abort and surface the error.
|
||||
changed, err := a.eng.Apply(opts)
|
||||
changed, err := engineApply(a, opts)
|
||||
if err != nil {
|
||||
// ...unless the engine is not running AT ALL. A failed apply that left the
|
||||
// PREVIOUS engine running still has a valid, loaded data plane — replacing
|
||||
@@ -434,99 +434,19 @@ func (a *Applier) applyLocked(m *model.Model) (bool, error) {
|
||||
|
||||
// (3) netplane, fail-closed: on any failure return the error WITHOUT tearing
|
||||
// the engine/table down (kill-switch/table stay up; the watchdog decides).
|
||||
//
|
||||
// Render FIRST, then decide whether anything needs re-asserting. Gating the
|
||||
// whole data plane on the engine's `changed` flag alone was wrong: the engine
|
||||
// hash covers option.Options only, so a purely netplane-visible change (the
|
||||
// kill-switch flipping open->closed, DNS force-intercept, a rule gaining an
|
||||
// iface:/zone: source, an inbound device rename) hashed identical, took the
|
||||
// fast-path, and left the OLD ruleset loaded while the panel reported success.
|
||||
// A stale-but-loaded fail-open table is exactly the leak this audit is about.
|
||||
// Resolve WHERE the untunnelable-protocol drop applies before rendering. The
|
||||
// engine is already running by this point (step 2), so its loaded rule-sets can
|
||||
// supply the addresses a geoip list contributes; a stopped engine or a list that
|
||||
// has not downloaded yet yields no plan and the conservative blanket drop.
|
||||
//
|
||||
// The resolved addresses are rendered INTO the ruleset text, so the idempotence
|
||||
// check below sees them: a refreshed geoip list changes the text and triggers a
|
||||
// real reload, rather than leaving a stale plan loaded (the D3 trap).
|
||||
untunPlan := a.untunnelablePlanFor(m, opts)
|
||||
ruleset, nftWarnings, err := netplane.RenderNftPlanAt(m, untunPlan, now)
|
||||
if err != nil {
|
||||
// Includes the refusal on an unusable interface name with a closed
|
||||
// kill-switch: the previous ruleset stays loaded and keeps protecting the
|
||||
// LAN while the operator fixes the name.
|
||||
return changed, err
|
||||
}
|
||||
nftCurrent := ruleset == a.lastNft && netplane.TableExists()
|
||||
if !nftCurrent {
|
||||
if err := netplane.ApplyNft(ruleset); err != nil {
|
||||
// The engine may be up, but with no table loaded nothing is diverted into
|
||||
// it — LAN traffic goes straight out the WAN. That is the same silent
|
||||
// fail-open as a dead engine, so it gets the same answer: if there is no
|
||||
// table at all, hold the line rather than leave the LAN exposed.
|
||||
if !netplane.TableExists() {
|
||||
a.holdLocked(m, err)
|
||||
}
|
||||
return changed, err
|
||||
}
|
||||
a.lastNft = ruleset
|
||||
}
|
||||
// ApplyRouting is idempotent by del-then-add, which means it opens a brief
|
||||
// window with NO fwmark rule installed — during it, diverted packets miss the
|
||||
// `local default dev lo` table. Harmless on a real change (the plane is being
|
||||
// rebuilt anyway), but pointless churn on a no-op reconcile, so skip it when
|
||||
// the ruleset is unchanged AND the rule is verifiably still installed.
|
||||
// routeWarnings carries an egress that was BUILT but cannot route (no nexthop on a
|
||||
// non-point-to-point device). It is deliberately part of the status warning set:
|
||||
// such an egress looks applied everywhere in the UI while being unable to reach
|
||||
// anything off its own subnet. On the fast path nothing was rebuilt, so there is
|
||||
// nothing new to report and the previous set stands.
|
||||
var routeWarnings []string
|
||||
if !nftCurrent || !netplane.RoutingPresent(m.Globals) {
|
||||
var rerr error
|
||||
routeWarnings, rerr = netplane.ApplyRoutingWithWarnings(m)
|
||||
if rerr != nil {
|
||||
return changed, rerr
|
||||
}
|
||||
}
|
||||
if err := netplane.ApplySysctl(); err != nil {
|
||||
return changed, err
|
||||
}
|
||||
// Per-diverted-ingress-iface knobs (accept_local/rp_filter): the static
|
||||
// ApplySysctl above cannot know the LAN device names, and without
|
||||
// accept_local=1 on the ingress iface the tproxied packet never reaches the
|
||||
// engine socket — it escapes to the fail-closed forward drop and the LAN goes
|
||||
// dark. Re-asserted on EVERY apply, never fast-pathed: netifd recreating a
|
||||
// bridge (`ifup lan`, a VLAN change) hands back a device with the kernel
|
||||
// defaults, silently un-setting accept_local behind our back. Fail-closed like
|
||||
// the other netplane steps: return without teardown.
|
||||
if err := netplane.ApplyIfaceSysctlsAt(m, now); err != nil {
|
||||
return changed, err
|
||||
}
|
||||
|
||||
// The plane is COMPLETE only here: table + policy routing + sysctls. ApplyNft
|
||||
// already flushed the DNS conntrack when it loaded the ruleset, but that is
|
||||
// one step too early — ApplyRouting is idempotent BY del-then-add, so it opens
|
||||
// a window in which the fwmark rule is momentarily absent, and any DNS flow
|
||||
// that crosses that window is tracked against a plane that is still being
|
||||
// assembled. Flushing once more now that every piece is in place is what makes
|
||||
// "no entry survives the transition" actually true. Only on a real change (the
|
||||
// fast path assembled nothing), best-effort, and cheap: the :53 entry count is
|
||||
// bounded by the number of clients.
|
||||
if !nftCurrent {
|
||||
if n, ferr := netplane.FlushDNSConntrack(); ferr != nil {
|
||||
a.log.Debug("flush DNS conntrack after plane change: ", ferr)
|
||||
} else if n > 0 {
|
||||
a.log.Debug("plane changed: dropped ", n, " stale DNS conntrack entries")
|
||||
}
|
||||
plane, perr := applyDataPlane(a, m, opts, now)
|
||||
if perr != nil {
|
||||
// The engine is ALREADY running the new config at this point, and the data
|
||||
// plane is not — so nothing the previous apply published is true any more.
|
||||
// Publishing is what this branch used to skip entirely; see abortAfterSwap.
|
||||
return changed, a.abortAfterSwap(m, plane, warnings, configWarnings, perr)
|
||||
}
|
||||
|
||||
// (4) success. Bump the effective-state generation ONLY when something really
|
||||
// moved: a no-op reconcile must not invalidate an armed commit-confirm window
|
||||
// (cron reconciles every minute — counting those would cancel every rollback
|
||||
// that commit-confirm exists to guarantee).
|
||||
if changed || !nftCurrent {
|
||||
if changed || plane.changed {
|
||||
a.stateGen.Add(1)
|
||||
}
|
||||
a.setHolding(false)
|
||||
@@ -544,7 +464,8 @@ func (a *Applier) applyLocked(m *model.Model) (bool, error) {
|
||||
// correctly so here: an egress that cannot reach off its own subnet is a configured
|
||||
// path that silently carries nothing, exactly the class of fault that channel exists
|
||||
// for.
|
||||
ws := collectWarnings(m.Globals, warnings, append(nftWarnings, routeWarnings...), configWarnings, untunPlan.Notes()...)
|
||||
ws := collectWarnings(m.Globals, warnings,
|
||||
append(plane.nftWarnings, plane.routeWarnings...), configWarnings, plane.planNotes...)
|
||||
a.setWarnings(ws)
|
||||
// The log only hears about a CHANGE. Status above always carries the full set;
|
||||
// reprinting it on every no-op reconcile (cron, once a minute, plus every
|
||||
@@ -559,6 +480,187 @@ func (a *Applier) applyLocked(m *model.Model) (bool, error) {
|
||||
return changed, nil
|
||||
}
|
||||
|
||||
// planeOutcome is what the netplane half of an apply did, carried back to
|
||||
// applyLocked so the SAME facts can be published whether it succeeded or failed.
|
||||
// Before it existed, everything the netplane stage learned — the warnings it
|
||||
// rendered, the notes the untunnelable plan produced — was thrown away on any
|
||||
// error, which is why a failed apply left the previous config's verdict standing.
|
||||
type planeOutcome struct {
|
||||
// changed is true when the nft ruleset was actually (re)loaded, i.e. the data
|
||||
// plane moved. Distinct from the engine's own `changed`: the engine hash covers
|
||||
// option.Options only, so a purely netplane-visible change hashes identical.
|
||||
changed bool
|
||||
// stage names the netplane step that failed, in operator words, or "" on
|
||||
// success. It is what the failure warning is addressed to.
|
||||
stage string
|
||||
planNotes []string
|
||||
nftWarnings []string
|
||||
routeWarnings []string
|
||||
}
|
||||
|
||||
// engineApply and applyDataPlane are the two heavy halves of applyLocked, behind
|
||||
// package-level seams for exactly one reason: the PUBLISHING behaviour around
|
||||
// them (what Status says after a stage fails) is the thing this file gets wrong
|
||||
// most easily and can otherwise only be tested on a router, with root, a real
|
||||
// sing-box instance and a real nft binary — i.e. never, in the gate. Production
|
||||
// always runs the real methods; a test substitutes a stage that fails and asserts
|
||||
// what the operator is then told. Same seam pattern as applyHoldNft.
|
||||
var (
|
||||
engineApply = func(a *Applier, opts option.Options) (bool, error) { return a.eng.Apply(opts) }
|
||||
applyDataPlane = (*Applier).applyDataPlaneLocked
|
||||
)
|
||||
|
||||
// applyDataPlaneLocked is step (3) of the pipeline: nft ruleset, policy routing
|
||||
// and sysctls, in that order, fail-closed. Caller holds a.mu and has ALREADY
|
||||
// swapped the engine, so every failure here leaves the router in a mixed state —
|
||||
// which is why the outcome is returned even on error.
|
||||
func (a *Applier) applyDataPlaneLocked(m *model.Model, opts option.Options, now time.Time) (planeOutcome, error) {
|
||||
// Render FIRST, then decide whether anything needs re-asserting. Gating the
|
||||
// whole data plane on the engine's `changed` flag alone was wrong: the engine
|
||||
// hash covers option.Options only, so a purely netplane-visible change (the
|
||||
// kill-switch flipping open->closed, DNS force-intercept, a rule gaining an
|
||||
// iface:/zone: source, an inbound device rename) hashed identical, took the
|
||||
// fast-path, and left the OLD ruleset loaded while the panel reported success.
|
||||
// A stale-but-loaded fail-open table is exactly the leak this audit is about.
|
||||
// Resolve WHERE the untunnelable-protocol drop applies before rendering. The
|
||||
// engine is already running by this point (step 2), so its loaded rule-sets can
|
||||
// supply the addresses a geoip list contributes; a stopped engine or a list that
|
||||
// has not downloaded yet yields no plan and the conservative blanket drop.
|
||||
//
|
||||
// The resolved addresses are rendered INTO the ruleset text, so the idempotence
|
||||
// check below sees them: a refreshed geoip list changes the text and triggers a
|
||||
// real reload, rather than leaving a stale plan loaded (the D3 trap).
|
||||
untunPlan := a.untunnelablePlanFor(m, opts)
|
||||
out := planeOutcome{planNotes: untunPlan.Notes()}
|
||||
|
||||
ruleset, nftWarnings, err := netplane.RenderNftPlanAt(m, untunPlan, now)
|
||||
out.nftWarnings = nftWarnings
|
||||
if err != nil {
|
||||
// Includes the refusal on an unusable interface name with a closed
|
||||
// kill-switch: the previous ruleset stays loaded and keeps protecting the
|
||||
// LAN while the operator fixes the name.
|
||||
out.stage = "rendering the nft ruleset"
|
||||
return out, err
|
||||
}
|
||||
nftCurrent := ruleset == a.lastNft && netplane.TableExists()
|
||||
if !nftCurrent {
|
||||
if err := netplane.ApplyNft(ruleset); err != nil {
|
||||
// The engine may be up, but with no table loaded nothing is diverted into
|
||||
// it — LAN traffic goes straight out the WAN. That is the same silent
|
||||
// fail-open as a dead engine, so it gets the same answer: if there is no
|
||||
// table at all, hold the line rather than leave the LAN exposed.
|
||||
if !netplane.TableExists() {
|
||||
a.holdLocked(m, err)
|
||||
}
|
||||
out.stage = "loading the nft ruleset"
|
||||
return out, err
|
||||
}
|
||||
a.lastNft = ruleset
|
||||
out.changed = true
|
||||
}
|
||||
// ApplyRouting is idempotent by del-then-add, which means it opens a brief
|
||||
// window with NO fwmark rule installed — during it, diverted packets miss the
|
||||
// `local default dev lo` table. Harmless on a real change (the plane is being
|
||||
// rebuilt anyway), but pointless churn on a no-op reconcile, so skip it when
|
||||
// the ruleset is unchanged AND the rule is verifiably still installed.
|
||||
// routeWarnings carries an egress that was BUILT but cannot route (no nexthop on a
|
||||
// non-point-to-point device). It is deliberately part of the status warning set:
|
||||
// such an egress looks applied everywhere in the UI while being unable to reach
|
||||
// anything off its own subnet. On the fast path nothing was rebuilt, so there is
|
||||
// nothing new to report and the previous set stands.
|
||||
if !nftCurrent || !netplane.RoutingPresent(m.Globals) {
|
||||
routeWarnings, rerr := netplane.ApplyRoutingWithWarnings(m)
|
||||
out.routeWarnings = routeWarnings
|
||||
if rerr != nil {
|
||||
out.stage = "installing the policy routing"
|
||||
return out, rerr
|
||||
}
|
||||
}
|
||||
if err := netplane.ApplySysctl(); err != nil {
|
||||
out.stage = "setting the kernel sysctls"
|
||||
return out, err
|
||||
}
|
||||
// Per-diverted-ingress-iface knobs (accept_local/rp_filter): the static
|
||||
// ApplySysctl above cannot know the LAN device names, and without
|
||||
// accept_local=1 on the ingress iface the tproxied packet never reaches the
|
||||
// engine socket — it escapes to the fail-closed forward drop and the LAN goes
|
||||
// dark. Re-asserted on EVERY apply, never fast-pathed: netifd recreating a
|
||||
// bridge (`ifup lan`, a VLAN change) hands back a device with the kernel
|
||||
// defaults, silently un-setting accept_local behind our back. Fail-closed like
|
||||
// the other netplane steps: return without teardown.
|
||||
if err := netplane.ApplyIfaceSysctlsAt(m, now); err != nil {
|
||||
out.stage = "setting the per-interface sysctls"
|
||||
return out, err
|
||||
}
|
||||
|
||||
// The plane is COMPLETE only here: table + policy routing + sysctls. ApplyNft
|
||||
// already flushed the DNS conntrack when it loaded the ruleset, but that is
|
||||
// one step too early — ApplyRouting is idempotent BY del-then-add, so it opens
|
||||
// a window in which the fwmark rule is momentarily absent, and any DNS flow
|
||||
// that crosses that window is tracked against a plane that is still being
|
||||
// assembled. Flushing once more now that every piece is in place is what makes
|
||||
// "no entry survives the transition" actually true. Only on a real change (the
|
||||
// fast path assembled nothing), best-effort, and cheap: the :53 entry count is
|
||||
// bounded by the number of clients.
|
||||
if out.changed {
|
||||
if n, ferr := netplane.FlushDNSConntrack(); ferr != nil {
|
||||
a.log.Debug("flush DNS conntrack after plane change: ", ferr)
|
||||
} else if n > 0 {
|
||||
a.log.Debug("plane changed: dropped ", n, " stale DNS conntrack entries")
|
||||
}
|
||||
}
|
||||
return out, nil
|
||||
}
|
||||
|
||||
// abortAfterSwap publishes an honest status for an apply that got PAST the engine
|
||||
// swap and then failed in the data plane, and returns the cause unchanged.
|
||||
//
|
||||
// The defect it exists for: every netplane failure used to `return changed, err`
|
||||
// before setTraffic/setWarnings, so Status kept serving the verdict and the
|
||||
// findings of the PREVIOUS config while the engine was already running the new
|
||||
// one. Worse, it did not self-heal — the next cron reconcile hashes identical,
|
||||
// fails at the same stage, and returns at the same place, so the stale verdict
|
||||
// stood for as long as the fault did. A green "Protected" over a half-installed
|
||||
// plane is the exact inversion this audit is about: not an error shown when
|
||||
// things are fine, but calm shown when they are not.
|
||||
//
|
||||
// What it publishes:
|
||||
//
|
||||
// - Traffic goes back to UNKNOWN (the zero value). It is tempting to publish
|
||||
// TrafficOf(opts) here, since the ENGINE really is running those options — but
|
||||
// the verdict describes where the LAN's traffic ends up, and that is decided by
|
||||
// the engine and the data plane together. With one of them from this config and
|
||||
// the other from the last one, the honest answer is that we do not know; the
|
||||
// panel renders unknown, and is required never to render it as protected.
|
||||
// - The warning set is replaced by THIS config's warnings, with a critical entry
|
||||
// naming the stage that failed at the front. The operator gets the news about
|
||||
// the config that is actually loaded, plus the fact that it is only half loaded.
|
||||
//
|
||||
// Caller holds a.mu.
|
||||
func (a *Applier) abortAfterSwap(m *model.Model, plane planeOutcome, generateWarnings []string, configWarnings []model.Warning, cause error) error {
|
||||
a.setTraffic(generate.Traffic{})
|
||||
|
||||
stage := plane.stage
|
||||
if stage == "" {
|
||||
stage = "installing the data plane"
|
||||
}
|
||||
ws := gatherWarnings(m.Globals, generateWarnings,
|
||||
append(plane.nftWarnings, plane.routeWarnings...), configWarnings, plane.planNotes...)
|
||||
ws = finalizeWarnings(append([]Warning{{
|
||||
Severity: SeverityCritical,
|
||||
Section: "netplane",
|
||||
Name: stage,
|
||||
Message: fmt.Sprintf("the engine was switched to this configuration but the data plane could NOT be "+
|
||||
"completed — %s failed: %v. What the kernel holds is part of this configuration and part of the "+
|
||||
"previous one, so where your traffic goes is UNKNOWN: treat this router as unprotected until a "+
|
||||
"reconcile succeeds. It is retried every minute; if it keeps failing, fix the cause or roll back.",
|
||||
stage, cause),
|
||||
}}, ws...))
|
||||
a.setWarnings(ws)
|
||||
a.logWarningsIfChanged(ws)
|
||||
return cause
|
||||
}
|
||||
|
||||
// holdLocked installs the fail-closed HOLDING PLANE when the engine is not
|
||||
// running and the kill-switch is closed. Caller holds a.mu.
|
||||
//
|
||||
@@ -940,12 +1042,28 @@ func (a *Applier) Rollback() error {
|
||||
defer release()
|
||||
a.mu.Lock()
|
||||
defer a.mu.Unlock()
|
||||
if err := a.eng.Rollback(); err != nil {
|
||||
m, err := rollbackEngineAndPlane(a)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
a.publishEngineRollback(m)
|
||||
return nil
|
||||
}
|
||||
|
||||
// rollbackEngineAndPlane is the ACTION half of the no-snapshot rollback: drive the
|
||||
// engine back to its predecessor config and re-assert the data plane from current
|
||||
// UCI. It returns the model the plane was rebuilt from. Caller holds a.mu.
|
||||
//
|
||||
// It is a variable for the same reason as engineApply/applyDataPlane: what this
|
||||
// rollback PUBLISHES afterwards is the part that was wrong, and it cannot be
|
||||
// exercised at all without two real engine generations and a real nft binary.
|
||||
var rollbackEngineAndPlane = func(a *Applier) (*model.Model, error) {
|
||||
if err := a.eng.Rollback(); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
m, err := model.ReadUCI()
|
||||
if err != nil {
|
||||
return err
|
||||
return nil, err
|
||||
}
|
||||
// One clock for the whole re-assert, same as applyLocked: the ruleset's
|
||||
// divert set and the iface sysctls below must agree on the profile-effective
|
||||
@@ -957,14 +1075,14 @@ func (a *Applier) Rollback() error {
|
||||
a.log.Warn("netplane: ", w)
|
||||
}
|
||||
if err != nil {
|
||||
return err
|
||||
return nil, err
|
||||
}
|
||||
if err := netplane.ApplyNft(ruleset); err != nil {
|
||||
return err
|
||||
return nil, err
|
||||
}
|
||||
a.lastNft = ruleset
|
||||
if err := netplane.ApplyRouting(m); err != nil {
|
||||
return err
|
||||
return nil, err
|
||||
}
|
||||
// The sysctl half must be re-asserted here too. It used to be missing: a
|
||||
// rollback that changes the set of diverted ingress devices (a different
|
||||
@@ -973,9 +1091,60 @@ func (a *Applier) Rollback() error {
|
||||
// then never reaches the engine socket and that network goes dark after a
|
||||
// rollback, which is precisely when the operator can least afford it.
|
||||
if err := netplane.ApplySysctl(); err != nil {
|
||||
return err
|
||||
return nil, err
|
||||
}
|
||||
return netplane.ApplyIfaceSysctlsAt(m, now)
|
||||
if err := netplane.ApplyIfaceSysctlsAt(m, now); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return m, nil
|
||||
}
|
||||
|
||||
// publishEngineRollback makes Status describe the router the no-snapshot rollback
|
||||
// just produced, instead of the one it rolled away FROM. Caller holds a.mu.
|
||||
//
|
||||
// The defect: this path touched none of the publishers. Apply a tunnel config,
|
||||
// Confirm it (which consumes the snapshot), then roll back later — the engine goes
|
||||
// to its predecessor, which may well be the `default -> direct` config, and the
|
||||
// panel keeps showing the tunnel verdict and the tunnel config's warnings, in
|
||||
// green, indefinitely. The whole LAN is on the plain WAN with its real address and
|
||||
// the UI says Protected. Nothing else corrects it: the verdict is only ever
|
||||
// rewritten by a successful apply, and a rollback is not one.
|
||||
//
|
||||
// The verdict published is UNKNOWN, not a computed one, and that is the honest
|
||||
// answer rather than a lazy one: engine.Rollback re-applies option.Options that
|
||||
// this process no longer holds (the engine keeps them, apply does not), so there
|
||||
// is nothing here to run generate.TrafficOf over. Guessing from current UCI would
|
||||
// be worse than saying nothing — UCI is the config we rolled AWAY from. Unknown is
|
||||
// rendered as unknown by the panel and never as protected, and the next reconcile
|
||||
// (cron, within a minute) replaces it with the truth.
|
||||
func (a *Applier) publishEngineRollback(m *model.Model) {
|
||||
a.setTraffic(generate.Traffic{})
|
||||
a.setHolding(false) // a full plane was just loaded; whatever hold there was is over
|
||||
// lastGood is the teardown/rollback target, and the data plane was just built
|
||||
// from m — so m is what a later Teardown must know about to remove the right
|
||||
// marks and routing tables.
|
||||
a.lastGood = m
|
||||
// The observatory's reachability plan was built from the config we rolled away
|
||||
// from: left running it probes outbounds that may no longer exist and files the
|
||||
// results against tags the running box does not have. There is no plan to
|
||||
// replace it with (see above), so stop probing until the next apply installs one.
|
||||
if a.eng != nil {
|
||||
a.eng.StopObservatory()
|
||||
}
|
||||
ws := finalizeWarnings([]Warning{{
|
||||
Severity: SeverityCritical,
|
||||
Section: "engine",
|
||||
Name: "rollback",
|
||||
Message: "the engine was rolled back to the configuration that ran before the current one. " +
|
||||
"That configuration is not the one on disk, so where your traffic goes and what was left " +
|
||||
"un-applied are both UNKNOWN until the next reconcile (within a minute) re-applies the " +
|
||||
"saved config and reports on it. Do not read this router as protected in the meantime.",
|
||||
}})
|
||||
a.setWarnings(ws)
|
||||
a.logWarningsIfChanged(ws)
|
||||
// The plane moved, so an armed commit-confirm watcher must see a changed
|
||||
// generation and stand down rather than clobber what we just restored.
|
||||
a.stateGen.Add(1)
|
||||
}
|
||||
|
||||
// canRollback reports whether Rollback would actually revert something: an armed
|
||||
@@ -1019,11 +1188,30 @@ func ActiveFlagPresent() bool {
|
||||
|
||||
// Status is the read-side snapshot printed by `shaterd status` as JSON.
|
||||
//
|
||||
// running the daemon process is up (a live socket reply => true; the offline
|
||||
// stub reports false). Whether the ENGINE is intercepting is carried by
|
||||
// active/table/hash, not by running — a daemon can be up but inert.
|
||||
// running SHATER IS RUNNING: the daemon answered AND its engine has a started
|
||||
// sing-box instance carrying a config. false therefore covers every way
|
||||
// of not proxying — daemon down, daemon up with a dead engine, disabled,
|
||||
// torn down — and the fields below say which.
|
||||
//
|
||||
// It used to be the literal `true`, on the reasoning that Status() is
|
||||
// only ever called from inside the live daemon. That was true and it was
|
||||
// useless: a constant cannot report anything, and the panel built its
|
||||
// headline on `running && active`, so the "not running" branch was
|
||||
// physically unreachable and an engine that never started showed green.
|
||||
// A field whose only possible value is the reassuring one is worse than
|
||||
// no field: it is a promise the code cannot break.
|
||||
//
|
||||
// "Is the daemon process alive?" is a different question and is answered
|
||||
// by whether the status call returned at all (plus uptime_seconds, which
|
||||
// only a live daemon can produce).
|
||||
// enabled globals.enabled in UCI.
|
||||
// active ACTIVE_FLAG present (a successful enabled apply raised it).
|
||||
// active ACTIVE_FLAG present. This is the "the service is meant to be running"
|
||||
// latch that gates hotplug and cron, NOT a health signal: it is raised by
|
||||
// a successful enabled apply and cleared only by teardown, so it stays up
|
||||
// while the engine is down and the fail-closed holding plane is blocking
|
||||
// the LAN — deliberately, because clearing it would switch off the very
|
||||
// cron reconcile that brings the engine back. Never render it as "we are
|
||||
// proxying"; that is what running/plane/traffic are for.
|
||||
// table the `inet shater` nft table is loaded.
|
||||
// hash the running engine's config hash ("" when the engine is not started).
|
||||
// kill_switch globals.kill_switch in UCI (closed = fail-closed, open = leaky).
|
||||
@@ -1044,9 +1232,14 @@ type Status struct {
|
||||
PanelPort int `json:"panel_port"`
|
||||
CanRollback bool `json:"can_rollback"`
|
||||
|
||||
// EngineRunning is whether a sing-box instance is actually started. It is the
|
||||
// honest answer to "are we proxying?", which running/active/table each only
|
||||
// approximate.
|
||||
// EngineRunning is whether a sing-box instance is actually started.
|
||||
//
|
||||
// It was added as the honest field to stand beside a `running` that was hard-wired
|
||||
// true, and no consumer ever read it. Now that running carries the same fact it is
|
||||
// kept as its explicit, unambiguous name — the two are equal by construction from
|
||||
// the daemon — because it is already in the published API and reading
|
||||
// `engine_running` in a client is self-documenting where `running` needs this
|
||||
// comment.
|
||||
EngineRunning bool `json:"engine_running"`
|
||||
|
||||
// Plane describes what is loaded in the kernel RIGHT NOW:
|
||||
@@ -1141,18 +1334,23 @@ func processUptime(now time.Time) (startedUnix, uptimeSeconds int64) {
|
||||
return now.Unix() - uptimeSeconds, uptimeSeconds
|
||||
}
|
||||
|
||||
// Status returns the live status from this daemon's engine + kernel state. It is
|
||||
// only ever called from within the running daemon, so running=true; engine state
|
||||
// is reflected by Active/Table/Hash (an inert daemon reports running=true but
|
||||
// active=false/table=false/hash="").
|
||||
// Status returns the live status from this daemon's engine + kernel state.
|
||||
//
|
||||
// It is only ever called from within the running daemon, which is exactly why
|
||||
// `running` is read off the engine rather than set to true: from in here the
|
||||
// daemon's own liveness is a tautology, and the only thing left worth reporting
|
||||
// under that name is whether shater is carrying any traffic. A daemon that is up
|
||||
// with a dead engine reports running=false, plane="hold"/"none" and an unknown
|
||||
// traffic verdict — which is the state this field exists to make expressible.
|
||||
func (a *Applier) Status() Status {
|
||||
engineUp := a.eng != nil && a.eng.Running()
|
||||
s := Status{
|
||||
Running: true,
|
||||
Running: engineUp,
|
||||
Active: ActiveFlagPresent(),
|
||||
Table: netplane.TableExists(),
|
||||
Hash: a.eng.Hash(),
|
||||
CanRollback: a.canRollback(),
|
||||
EngineRunning: a.eng.Running(),
|
||||
EngineRunning: engineUp,
|
||||
Traffic: a.Traffic(),
|
||||
Warnings: a.Warnings(),
|
||||
}
|
||||
|
||||
@@ -0,0 +1,178 @@
|
||||
package apply
|
||||
|
||||
// Regression tests for the three ways this package used to report calm over a
|
||||
// router that was not doing what its config said. Each of them is the INVERTED
|
||||
// failure — not an error shown when things are fine, but green shown when they
|
||||
// are not — which is the only kind that gets someone hurt.
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/sagernet/sing-box/option"
|
||||
"github.com/sagernet/sing-box/shater/engine"
|
||||
"github.com/sagernet/sing-box/shater/generate"
|
||||
"github.com/sagernet/sing-box/shater/model"
|
||||
)
|
||||
|
||||
// TestStatusRunningReportsTheEngine pins the contract the panel headline is built
|
||||
// on.
|
||||
//
|
||||
// `running` used to be the literal `true` in the only code path that produces it,
|
||||
// so `running && active` — what the panel reads — could not go false however dead
|
||||
// the engine was, and the "not running" branch was unreachable code. A status
|
||||
// field that can only ever hold the reassuring value is not a weak signal, it is
|
||||
// an unfalsifiable claim.
|
||||
func TestStatusRunningReportsTheEngine(t *testing.T) {
|
||||
a := New(engine.New(), nil)
|
||||
|
||||
// A fresh applier's engine has never started: nothing is being proxied, and
|
||||
// the status must be able to say so.
|
||||
s := a.Status()
|
||||
if s.Running {
|
||||
t.Errorf("Status().Running = true with a stopped engine — the field is a constant again")
|
||||
}
|
||||
if s.Running != s.EngineRunning {
|
||||
t.Errorf("running (%v) and engine_running (%v) must agree: they are the same fact",
|
||||
s.Running, s.EngineRunning)
|
||||
}
|
||||
|
||||
// And the hold state — engine down, LAN blocked — must not read as running
|
||||
// either. This is the three-green-lamps case: holding does not clear the
|
||||
// ACTIVE flag (cron needs it to keep retrying), so `active` alone cannot say it.
|
||||
g := model.DefaultGlobals()
|
||||
g.KillSwitch = "open" // the early-return branch of holdLocked
|
||||
a.mu.Lock()
|
||||
a.holdLocked(&model.Model{Globals: g}, errors.New("engine start failed"))
|
||||
a.mu.Unlock()
|
||||
if a.Status().Running {
|
||||
t.Errorf("Status().Running = true while the engine is down and the plane is held")
|
||||
}
|
||||
}
|
||||
|
||||
// TestApplyFailingAfterEngineSwapDropsTheOldVerdict is the defect-3 regression.
|
||||
//
|
||||
// Everything after the engine swap used to `return changed, err` before
|
||||
// setTraffic/setWarnings, so a netplane failure left Status serving the VERDICT
|
||||
// and the FINDINGS of the configuration that no longer runs. It did not self-heal
|
||||
// either: the next cron reconcile hashes identical, fails at the same stage and
|
||||
// returns at the same place, so the stale green stood for as long as the fault.
|
||||
func TestApplyFailingAfterEngineSwapDropsTheOldVerdict(t *testing.T) {
|
||||
a := New(engine.New(), nil)
|
||||
|
||||
// What the previous, fully successful apply published.
|
||||
a.setTraffic(generate.Traffic{Verdict: generate.VerdictTunnel, Default: "auto", TunnelRules: 3})
|
||||
a.setWarnings([]Warning{{
|
||||
Severity: SeverityCritical, Section: "ruleset", Name: "stale",
|
||||
Message: "a finding of the configuration that is no longer running",
|
||||
}})
|
||||
|
||||
// The engine swap succeeds; the data plane does not.
|
||||
restore := stubApplyStages(t,
|
||||
func(a *Applier, opts option.Options) (bool, error) { return true, nil },
|
||||
func(a *Applier, m *model.Model, opts option.Options, now time.Time) (planeOutcome, error) {
|
||||
return planeOutcome{stage: "loading the nft ruleset"}, errors.New("nft: permission denied")
|
||||
})
|
||||
defer restore()
|
||||
|
||||
a.mu.Lock()
|
||||
_, err := a.applyLocked(holdModel("closed"))
|
||||
a.mu.Unlock()
|
||||
if err == nil {
|
||||
t.Fatalf("applyLocked must surface the netplane failure")
|
||||
}
|
||||
|
||||
s := a.Status()
|
||||
if s.Traffic.Verdict != "" {
|
||||
t.Errorf("Traffic.Verdict = %q after a half-installed plane, want \"\" (unknown): "+
|
||||
"the engine runs the new config and the kernel does not, so nobody knows where traffic goes",
|
||||
s.Traffic.Verdict)
|
||||
}
|
||||
var sawAbort bool
|
||||
for _, w := range s.Warnings {
|
||||
if strings.Contains(w.Message, "a finding of the configuration that is no longer running") {
|
||||
t.Errorf("the previous config's findings are still published: %+v", w)
|
||||
}
|
||||
if w.Section == "netplane" && strings.Contains(w.Message, "could NOT be completed") {
|
||||
sawAbort = true
|
||||
if w.Severity != SeverityCritical {
|
||||
t.Errorf("an incomplete data plane is critical, got %q", w.Severity)
|
||||
}
|
||||
if !strings.Contains(w.Message, "loading the nft ruleset") {
|
||||
t.Errorf("the warning must name the stage that failed: %q", w.Message)
|
||||
}
|
||||
}
|
||||
}
|
||||
if !sawAbort {
|
||||
t.Errorf("no warning says the data plane is incomplete; warnings = %+v", s.Warnings)
|
||||
}
|
||||
}
|
||||
|
||||
// TestRollbackWithoutSnapshotRepublishes is the defect-2 regression.
|
||||
//
|
||||
// Apply a tunnel config, Confirm it (which consumes the commit-confirm snapshot),
|
||||
// then roll back later. The no-snapshot branch drives engine.Rollback and rebuilds
|
||||
// the plane — and used to touch none of the publishers, so the panel kept showing
|
||||
// the tunnel verdict and the tunnel config's warnings in green while the engine
|
||||
// had gone back to a predecessor that may route `default -> direct`. The entire
|
||||
// LAN on the plain WAN, under a green "Protected", indefinitely.
|
||||
func TestRollbackWithoutSnapshotRepublishes(t *testing.T) {
|
||||
a := New(engine.New(), nil)
|
||||
a.setTraffic(generate.Traffic{Verdict: generate.VerdictTunnel, Default: "auto", TunnelRules: 2})
|
||||
a.setWarnings([]Warning{{
|
||||
Severity: SeverityWarning, Section: "chain", Name: "hop",
|
||||
Message: "a finding of the configuration we are rolling away from",
|
||||
}})
|
||||
|
||||
m := holdModel("closed")
|
||||
orig := rollbackEngineAndPlane
|
||||
rollbackEngineAndPlane = func(*Applier) (*model.Model, error) { return m, nil }
|
||||
defer func() { rollbackEngineAndPlane = orig }()
|
||||
|
||||
before := a.stateGen.Load()
|
||||
if err := a.Rollback(); err != nil {
|
||||
t.Fatalf("Rollback: %v", err)
|
||||
}
|
||||
|
||||
s := a.Status()
|
||||
if s.Traffic.Verdict != "" {
|
||||
t.Errorf("Traffic.Verdict = %q after an engine rollback, want \"\" (unknown): the running "+
|
||||
"config is one this process cannot describe", s.Traffic.Verdict)
|
||||
}
|
||||
var sawRollback bool
|
||||
for _, w := range s.Warnings {
|
||||
if strings.Contains(w.Message, "rolling away from") {
|
||||
t.Errorf("the pre-rollback findings are still published: %+v", w)
|
||||
}
|
||||
if w.Section == "engine" && w.Name == "rollback" {
|
||||
sawRollback = true
|
||||
if w.Severity != SeverityCritical {
|
||||
t.Errorf("an undescribable running config is critical, got %q", w.Severity)
|
||||
}
|
||||
}
|
||||
}
|
||||
if !sawRollback {
|
||||
t.Errorf("nothing says the router is running a rolled-back config; warnings = %+v", s.Warnings)
|
||||
}
|
||||
if a.LastGood() != m {
|
||||
t.Errorf("last-good must become the model the data plane was rebuilt from")
|
||||
}
|
||||
if a.stateGen.Load() == before {
|
||||
t.Errorf("the plane moved but stateGen did not: an armed commit-confirm watcher " +
|
||||
"would clobber the config we just restored")
|
||||
}
|
||||
}
|
||||
|
||||
// stubApplyStages replaces the two heavy halves of applyLocked for the duration of
|
||||
// a test and returns the restore func.
|
||||
func stubApplyStages(t *testing.T,
|
||||
eng func(*Applier, option.Options) (bool, error),
|
||||
plane func(*Applier, *model.Model, option.Options, time.Time) (planeOutcome, error),
|
||||
) func() {
|
||||
t.Helper()
|
||||
origEngine, origPlane := engineApply, applyDataPlane
|
||||
engineApply, applyDataPlane = eng, plane
|
||||
return func() { engineApply, applyDataPlane = origEngine, origPlane }
|
||||
}
|
||||
+175
-25
@@ -74,25 +74,76 @@ type Warning struct {
|
||||
Message string `json:"message"`
|
||||
}
|
||||
|
||||
// notAppliedTags are the SCREAMING-KEBAB prefixes generate stamps on the one
|
||||
// class of warning that means "you configured this protection and it is NOT in
|
||||
// force right now". They exist precisely so the condition is greppable and
|
||||
// machine-recognisable (generate/ruleset.go, generate/dnsfilter.go say so where
|
||||
// they emit them), which makes them a STRUCTURAL signal rather than a guess at
|
||||
// wording — so they decide severity outright, before anything else is consulted.
|
||||
//
|
||||
// This is the fix for the defect that made this whole classifier untrustworthy:
|
||||
// the tagged texts say "NOT ACTIVE"/"unreachable right now" in words that matched
|
||||
// none of the old markers ("UNREACHABLE" upper-case against "unreachable"
|
||||
// lower-case, "is NOT applied" against "is configured but NOT ACTIVE"), and the
|
||||
// tag also breaks entityRe below, so a blocklist that failed to download — the
|
||||
// single most common real-world fault on this router, and the one the panel has
|
||||
// no other way to show — was published as a plain `warning` under section
|
||||
// "generate" with no name. Meanwhile `ruleset "x": url source with empty url`
|
||||
// parsed cleanly and was graded critical. Severity was, in effect, inverted:
|
||||
// a typo shouted, a network outage whispered.
|
||||
var notAppliedTags = []string{
|
||||
"RULESET-NOT-APPLIED", // generate/ruleset.go:821,997,1242
|
||||
"DNS-FILTER-NOT-APPLIED", // generate/dnsfilter.go:132
|
||||
}
|
||||
|
||||
// criticalMarkers are substrings that identify a warning as "protection you
|
||||
// configured is not in effect".
|
||||
// configured is not in effect", for the texts that carry neither a tag above nor
|
||||
// a protection section below.
|
||||
//
|
||||
// This is a heuristic over free text, and it is one on purpose: generate emits
|
||||
// plain strings today, and inventing a parallel structured warning API across a
|
||||
// package boundary owned by another agent would be a far larger change than the
|
||||
// problem warrants. The markers below are taken verbatim from the actual warning
|
||||
// texts, so they are exact rather than speculative. If generate ever emits its
|
||||
// own severity, this list becomes dead code and the conversion simplifies.
|
||||
// problem warrants. Every entry below is quoted from a warning that a producer
|
||||
// ACTUALLY emits, with the file it comes from — because the previous list had
|
||||
// drifted into fiction: five of its nine entries matched no living text at all.
|
||||
// Three of those five ("not covered", "fail-closed", "REJECTED") described ONE
|
||||
// netplane message (netplane/nft.go:606), which reaches us on the netplane
|
||||
// channel and is graded critical wholesale before classify() ever runs; one
|
||||
// ("left un-blocked") named a message generate/doh.go:178 records as deleted;
|
||||
// one ("UNREACHABLE") was upper-case against a lower-case text. A marker with no
|
||||
// producer is not harmless: it reads as coverage, and it is what let the real
|
||||
// texts go ungraded for as long as they did.
|
||||
//
|
||||
// If generate ever emits its own severity, this list becomes dead code and the
|
||||
// conversion simplifies.
|
||||
var criticalMarkers = []string{
|
||||
"UNREACHABLE", // remote rule-set/blocklist not applied
|
||||
"is NOT applied", // ''
|
||||
"NOT emitted", // block_doh NXDOMAIN rules missing
|
||||
"left un-blocked", // block_doh: upstream resolver excluded
|
||||
"inert", // dns_filter / per-device DNS configured but not working
|
||||
"has NO effect", // dns_mode=fakeip with no fakeip resolver
|
||||
"not covered", // an interface outside the fail-closed guard
|
||||
"fail-closed", // ''
|
||||
"REJECTED", // an unusable interface name
|
||||
// The DoH NXDOMAIN rules were not built (generate/dns.go:41,67), and a routing
|
||||
// rule whose sources the engine cannot see is not built either
|
||||
// (generate/route.go:113) — in both cases the operator's block simply is not there.
|
||||
"NOT emitted",
|
||||
// dns_filter / per-device DNS / dns_intercept configured but not working
|
||||
// (generate/dns.go:32,35,38,58,61,64) — the filter is on in the UI and filtering nothing.
|
||||
"inert",
|
||||
// A dns_rule that survived parsing but matches nothing (generate/dns.go:987).
|
||||
"has NO effect",
|
||||
// A routing rule that was emitted but whose target is never reached
|
||||
// (generate/route.go:113,115): the traffic the operator sent through a tunnel
|
||||
// follows the rules below it and the default instead. Present tense on purpose —
|
||||
// "never applied" (past) is warnUnreachableRules' wording, which is graded by
|
||||
// consequence a few lines below, not swept in here.
|
||||
"never applies",
|
||||
// A rule scoped to one source that now matches the WHOLE network
|
||||
// (generate/route.go:407, generate/dns.go:992). Whatever the rule does — send a
|
||||
// device direct, point it at another resolver — it now does it to every client,
|
||||
// and nothing else in the UI shows that the scope collapsed.
|
||||
"applies to EVERY client on the router",
|
||||
"apply to ALL clients",
|
||||
// DNS that leaves the router in plaintext to the provider while the UI shows a
|
||||
// configured resolver (generate/dns.go:38,64,410) and node hostnames resolved
|
||||
// direct from the real address (generate/dns.go:469). These are leaks of exactly
|
||||
// the kind the tunnel exists to prevent.
|
||||
"in the clear",
|
||||
"your provider sees",
|
||||
// A condition-less rule retired by a later condition-less rule whose target is
|
||||
// `direct` (generate/route.go warnUnreachableRules): the operator's default
|
||||
// policy — a tunnel, or a block — is not the one the router uses, so everything
|
||||
@@ -117,6 +168,22 @@ var protectionSections = map[string]bool{
|
||||
"allowlist": true,
|
||||
}
|
||||
|
||||
// degradedProtectionMarkers are the exceptions to the section rule above: texts
|
||||
// about a protection list that is STILL IN EFFECT.
|
||||
//
|
||||
// Grading these critical is the same defect pointed the other way. The panel's
|
||||
// alarm banner lights on critical and on nothing else, so every critical that
|
||||
// turns out to be cosmetic teaches the operator that the banner means nothing —
|
||||
// and the next one, the one about the blocklist that really did not load, is the
|
||||
// one they will not read. In particular the refresh failure is the NORMAL state
|
||||
// of a Russian router for minutes at a time: the list is served from the copy
|
||||
// compiled earlier and keeps blocking, which is a degradation, not a gap.
|
||||
var degradedProtectionMarkers = []string{
|
||||
"continuing with the copy compiled earlier", // generate/ruleset.go:819 — stale but blocking
|
||||
"is IGNORED", // generate/ruleset.go:445,472 — a redundant field, the list loads
|
||||
"bad update_interval", // generate/ruleset.go:862,1010 — falls back to the default interval
|
||||
}
|
||||
|
||||
// infoMarkers identify operational notes that are not protection gaps.
|
||||
var infoMarkers = []string{"cache:"}
|
||||
|
||||
@@ -124,26 +191,67 @@ var infoMarkers = []string{"cache:"}
|
||||
// consistently, so Section/Name can be recovered from a plain string.
|
||||
var entityRe = regexp.MustCompile(`^([a-z_]+) "([^"]*)": (.*)$`)
|
||||
|
||||
// tagRe matches the SCREAMING-KEBAB prefix of a tagged warning (see notAppliedTags).
|
||||
var tagRe = regexp.MustCompile(`^([A-Z][A-Z0-9-]*): `)
|
||||
|
||||
// taggedEntityRe recovers the entity from a TAGGED warning, whose shape is
|
||||
//
|
||||
// RULESET-NOT-APPLIED: ruleset "ads" is configured but NOT ACTIVE: ...
|
||||
//
|
||||
// i.e. the tag sits where entityRe expects the kind, and the entity is followed by
|
||||
// prose rather than by ": ". Without this the single most important warning on the
|
||||
// router arrived with Section "generate" and no Name, so the panel could neither
|
||||
// group it nor link to the list it is about.
|
||||
var taggedEntityRe = regexp.MustCompile(`^[A-Z][A-Z0-9-]*: ([a-z_]+) "([^"]*)"`)
|
||||
|
||||
// warningFromText normalises one free-text warning. defaultSection is used when
|
||||
// the text carries no `kind "name":` prefix.
|
||||
func warningFromText(text, defaultSection, severity string) Warning {
|
||||
w := Warning{Severity: severity, Section: defaultSection, Message: strings.TrimSpace(text)}
|
||||
if m := entityRe.FindStringSubmatch(w.Message); m != nil {
|
||||
w.Section, w.Name, w.Message = m[1], m[2], m[3]
|
||||
return w
|
||||
}
|
||||
if m := taggedEntityRe.FindStringSubmatch(w.Message); m != nil {
|
||||
// Attribution only — the message is deliberately left WHOLE. The tag is the
|
||||
// operator's grep handle into `logread` (it is documented as such where it is
|
||||
// emitted), so stripping it to save one repetition of the list's name would
|
||||
// cost the one thing the tag exists for.
|
||||
w.Section, w.Name = m[1], m[2]
|
||||
}
|
||||
return w
|
||||
}
|
||||
|
||||
// classify picks a severity from the parsed section plus the message text.
|
||||
// Section wins where it is decisive (see protectionSections); the markers then
|
||||
// catch the global warnings that carry no entity prefix at all.
|
||||
//
|
||||
// Order is the whole design:
|
||||
//
|
||||
// 1. a not-applied TAG is structural and decides outright — it is the producer
|
||||
// saying "this protection is off", not us guessing from prose;
|
||||
// 2. info markers, so a cache relocation never reads as a fault;
|
||||
// 3. the section, for the entity kinds whose entire purpose is to block
|
||||
// something — minus the handful of texts that say the list still works;
|
||||
// 4. the free-text markers, which catch the global warnings that carry no entity
|
||||
// prefix at all.
|
||||
func classify(section, text string) string {
|
||||
if m := tagRe.FindStringSubmatch(text); m != nil {
|
||||
for _, tag := range notAppliedTags {
|
||||
if m[1] == tag {
|
||||
return SeverityCritical
|
||||
}
|
||||
}
|
||||
}
|
||||
for _, m := range infoMarkers {
|
||||
if strings.Contains(text, m) {
|
||||
return SeverityInfo
|
||||
}
|
||||
}
|
||||
if protectionSections[section] {
|
||||
for _, m := range degradedProtectionMarkers {
|
||||
if strings.Contains(text, m) {
|
||||
return SeverityWarning
|
||||
}
|
||||
}
|
||||
return SeverityCritical
|
||||
}
|
||||
for _, m := range criticalMarkers {
|
||||
@@ -163,6 +271,15 @@ func classify(section, text string) string {
|
||||
// fail-closed guard does not cover that interface — always critical.
|
||||
// - configWarnings come from model.Validate (already structured).
|
||||
func collectWarnings(g model.Globals, generateWarnings, netplaneWarnings []string, configWarnings []model.Warning, planWarnings ...string) []Warning {
|
||||
return finalizeWarnings(gatherWarnings(g, generateWarnings, netplaneWarnings, configWarnings, planWarnings...))
|
||||
}
|
||||
|
||||
// gatherWarnings is collectWarnings without the sort and the cap, so a caller
|
||||
// that must FOLD IN a warning of its own (applyLocked's post-swap failure, which
|
||||
// has to say that the data plane is incomplete) can do so and then finalize once.
|
||||
// Sorting and capping a list twice is not equivalent: the second pass would drop
|
||||
// the "N further warning(s) suppressed" disclosure the first pass appended.
|
||||
func gatherWarnings(g model.Globals, generateWarnings, netplaneWarnings []string, configWarnings []model.Warning, planWarnings ...string) []Warning {
|
||||
out := make([]Warning, 0, len(generateWarnings)+len(netplaneWarnings)+len(configWarnings)+1)
|
||||
// The untunnelable-protocol policy always reports what it costs the user; it is
|
||||
// the only one of these that describes correct behaviour rather than a fault.
|
||||
@@ -186,7 +303,12 @@ func collectWarnings(g model.Globals, generateWarnings, netplaneWarnings []strin
|
||||
Message: cw.Message,
|
||||
})
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// finalizeWarnings sorts critical-first and applies the cap. Call it exactly once
|
||||
// per published set.
|
||||
func finalizeWarnings(out []Warning) []Warning {
|
||||
// Stable sort by descending severity so the cap can only drop the least
|
||||
// important entries, and the panel gets the worst news first.
|
||||
sort.SliceStable(out, func(i, j int) bool {
|
||||
@@ -241,33 +363,61 @@ func untunnelablePolicyWarnings(g model.Globals, planNotes []string) []Warning {
|
||||
// With the kill switch open the forward chain has no drops at all, so nothing
|
||||
// is restricted whatever the policy says. Saying that is more useful than
|
||||
// repeating a promise which is not being kept.
|
||||
//
|
||||
// Neither this note nor the `direct` one below may claim IPTV, for the same
|
||||
// reason the `block` note disclaims it: multicast does not cross this router
|
||||
// under ANY of the three settings. The stream itself is WAN-side inbound and
|
||||
// these rules never match it, and a client's outbound multicast UDP is dropped
|
||||
// by the fail-closed guard regardless of the policy. Promising it here would be
|
||||
// the identical lie to the one just removed from `block`, only in the branch
|
||||
// where the operator is least likely to go looking for the cause.
|
||||
if !killSwitchClosed(g) {
|
||||
if policy == netplane.UntunnelableDirect {
|
||||
return out
|
||||
}
|
||||
return note(policy,
|
||||
"This setting has no effect while the kill switch is open: with the kill switch open "+
|
||||
"nothing is blocked, so ping, IPTV and VPN passthrough all work — and all of them "+
|
||||
"reach the internet with your real IP address.")
|
||||
"This setting has no effect while the kill switch is open: with the kill switch open the "+
|
||||
"forward chain has no drops at all, so ping, traceroute and raw VPN passthrough "+
|
||||
"(IPsec ESP/AH, PPTP/GRE) all work — and every one of them reaches the internet with "+
|
||||
"your real IP address. IPTV is not part of that: multicast does not pass this router "+
|
||||
"on any setting, which is a separate matter from this one.")
|
||||
}
|
||||
|
||||
switch policy {
|
||||
case netplane.UntunnelableDirect:
|
||||
return note(policy,
|
||||
"Ping, IPTV and VPN passthrough (IPsec/PPTP) work everywhere, but they go straight out "+
|
||||
"with your real IP address instead of through the tunnel — they are the kinds of "+
|
||||
"traffic a tunnel cannot carry.")
|
||||
"Ping and traceroute work everywhere, and so does raw VPN passthrough (IPsec ESP/AH, "+
|
||||
"PPTP/GRE) — but all of it goes straight out with your real IP address instead of "+
|
||||
"through the tunnel, because a tunnel cannot carry this kind of traffic. VPNs that "+
|
||||
"run over UDP (WireGuard, OpenVPN-UDP, IPsec through NAT) are ordinary tunnelled "+
|
||||
"traffic and are unaffected either way. IPTV is not covered by this setting at all: "+
|
||||
"multicast does not pass this router on any of the three, so switching to `direct` "+
|
||||
"will not bring it back.")
|
||||
case netplane.UntunnelableICMP:
|
||||
return note(policy,
|
||||
"Ping and traceroute work everywhere, including addresses you send through the tunnel; "+
|
||||
"the host you ping sees your real IP address. IPTV and VPN passthrough (IPsec/PPTP) "+
|
||||
"work only toward addresses your rules route directly.")
|
||||
default:
|
||||
// This text used to say these things "work only toward addresses your rules
|
||||
// route directly". That was written when a `direct` route final made the
|
||||
// untunnelable drop degenerate into a blanket accept — i.e. when the note was
|
||||
// describing the bug rather than the policy. netplane now blocks what it says
|
||||
// it blocks, so the honest sentence is that none of it works at all, and the
|
||||
// note has to name what is and is NOT affected: "ping does not work" sends an
|
||||
// operator hunting a fault, and the difference between raw ESP and IPsec
|
||||
// through NAT is the difference between "my VPN broke" and "my VPN is fine".
|
||||
return note(netplane.UntunnelableBlock,
|
||||
"Ping, traceroute, IPTV and VPN passthrough work only toward addresses your rules route "+
|
||||
"directly — those already see your real IP address anyway. Toward addresses you send "+
|
||||
"through the tunnel they will not work, because a tunnel cannot carry them and they "+
|
||||
"would otherwise leak your real IP address.")
|
||||
"Ping, traceroute, IPsec/PPTP VPN passthrough and IPTV do not work from your devices at "+
|
||||
"all — not even toward addresses your rules route directly. None of this traffic can "+
|
||||
"travel through a tunnel, so rather than let it out with your real IP address it is "+
|
||||
"dropped. Concretely: ping and Windows tracert fail (on Linux and macOS traceroute "+
|
||||
"sends UDP probes instead, which ARE tunnelled — the hops it prints are the tunnel's "+
|
||||
"path, not your own), and so do raw IPsec (ESP/AH) and PPTP/GRE — a PPTP session will "+
|
||||
"even look connected, because its control channel is TCP and only the payload is "+
|
||||
"dropped. VPNs that run over UDP are NOT affected: WireGuard, OpenVPN-UDP and IPsec "+
|
||||
"through NAT (IKE on UDP 500, NAT-T on UDP 4500) keep working normally. Multicast "+
|
||||
"IPTV does not cross this router under any setting; that one is not this policy.")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -17,7 +17,7 @@ import (
|
||||
func TestCollectWarningsAttributesEntities(t *testing.T) {
|
||||
got := collectWarnings(blockGlobals(),
|
||||
[]string{
|
||||
`ruleset "ads": remote list "https://x/y.srs" is UNREACHABLE right now, so it is NOT applied`,
|
||||
ruleSetNotApplied,
|
||||
`device "kids-tablet": no current IP (ip unset and MAC "aa:bb" not leased), skipped`,
|
||||
`chain "hop": has no hops, target skipped`,
|
||||
`dns_filter enabled but no resolvers configured; filter inert`,
|
||||
@@ -38,14 +38,23 @@ func TestCollectWarningsAttributesEntities(t *testing.T) {
|
||||
if w.Severity != SeverityCritical {
|
||||
t.Errorf("an unapplied blocklist is a protection gap; severity = %q, want critical", w.Severity)
|
||||
}
|
||||
if strings.Contains(w.Message, `ruleset "ads":`) {
|
||||
t.Errorf("the entity prefix must move into Section/Name, not stay in Message: %q", w.Message)
|
||||
// The RULESET-NOT-APPLIED tag stays in the message on purpose: generate
|
||||
// documents it as the operator's grep handle into logread.
|
||||
if !strings.HasPrefix(w.Message, "RULESET-NOT-APPLIED:") {
|
||||
t.Errorf("a tagged warning must keep its greppable tag in the message: %q", w.Message)
|
||||
}
|
||||
}
|
||||
if w, ok := byName["device/kids-tablet"]; !ok {
|
||||
t.Errorf("device warning not attributed; got %+v", got)
|
||||
} else if w.Severity != SeverityWarning {
|
||||
t.Errorf("a skipped device is not a protection gap; severity = %q, want warning", w.Severity)
|
||||
} else {
|
||||
if w.Severity != SeverityWarning {
|
||||
t.Errorf("a skipped device is not a protection gap; severity = %q, want warning", w.Severity)
|
||||
}
|
||||
// The plain `kind "name": message` prefix, by contrast, MOVES into
|
||||
// Section/Name — it carries no information the fields do not.
|
||||
if strings.Contains(w.Message, `device "kids-tablet":`) {
|
||||
t.Errorf("the entity prefix must move into Section/Name, not stay in Message: %q", w.Message)
|
||||
}
|
||||
}
|
||||
if w, ok := byName["chain/hop"]; !ok || w.Severity != SeverityWarning {
|
||||
t.Errorf("chain warning: got %+v", w)
|
||||
@@ -115,7 +124,7 @@ func TestCollectWarningsCapKeepsCriticals(t *testing.T) {
|
||||
for i := 0; i < 200; i++ {
|
||||
noisy = append(noisy, `chain "c": has no hops, target skipped`)
|
||||
}
|
||||
noisy = append(noisy, `ruleset "ads": remote list is UNREACHABLE right now, so it is NOT applied`)
|
||||
noisy = append(noisy, ruleSetNotApplied)
|
||||
|
||||
got := collectWarnings(blockGlobals(), noisy, nil, nil)
|
||||
if len(got) != maxStatusWarnings {
|
||||
@@ -232,6 +241,170 @@ func TestWarningsAgainstRealGenerateOutput(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// ruleSetNotApplied is the message generate/ruleset.go:997 actually produces when
|
||||
// a remote blocklist cannot be fetched — the single most common real fault on a
|
||||
// router in Russia, and the one the panel has no other way to show (an omitted
|
||||
// rule-set produces no row in GET /api/ruleset/status, so the UI is
|
||||
// indistinguishable from "not configured").
|
||||
//
|
||||
// Quoted verbatim, tag and all, because the classifier is a heuristic over free
|
||||
// text and a test written against invented text proves nothing about it. The
|
||||
// version this replaced asserted on `... is UNREACHABLE ... is NOT applied`, a
|
||||
// sentence no producer has ever emitted; it passed for as long as the real
|
||||
// sentence was being graded a plain `warning` under section "generate" with no
|
||||
// name at all.
|
||||
const ruleSetNotApplied = `RULESET-NOT-APPLIED: ruleset "ads" is configured but NOT ACTIVE: ` +
|
||||
`its source "https://big.oisd.nl/domainswild" is unreachable right now, so rule-set "ads" was ` +
|
||||
`omitted and matches NOTHING until it loads (a blocklist blocks nothing; a routing rule is skipped). ` +
|
||||
`Handing an unusable list to the engine would abort engine start and take the LAN down instead. ` +
|
||||
`Retried automatically on the next reconcile (~1 min) — no action needed unless this persists.`
|
||||
|
||||
// TestClassifyRealGenerateTexts grades the sentences generate REALLY emits, by
|
||||
// consequence.
|
||||
//
|
||||
// The defect this pins: severity was inverted. A blocklist that could not be
|
||||
// downloaded — protection the operator configured, not in force — came out
|
||||
// `warning`, because its tag broke the entity regexp and its wording matched no
|
||||
// marker. A typo in the same list's URL came out `critical`, because that one
|
||||
// parsed cleanly into section "ruleset". The panel's alarm banner lights on
|
||||
// critical and on nothing else, so the router shouted about the typo and stayed
|
||||
// quiet about the outage.
|
||||
func TestClassifyRealGenerateTexts(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
text string
|
||||
want string
|
||||
// section/entity attribution, when the panel must be able to deep-link.
|
||||
section, entity string
|
||||
}{{
|
||||
name: "remote blocklist could not be fetched",
|
||||
text: ruleSetNotApplied,
|
||||
want: SeverityCritical,
|
||||
section: "ruleset", entity: "ads",
|
||||
}, {
|
||||
name: "not one DNS filter list could be built",
|
||||
text: "DNS-FILTER-NOT-APPLIED: dns_filter is ON and lists are enabled, but NOT ONE of them " +
|
||||
"could be built right now — nothing is being filtered or allowed. See the per-list " +
|
||||
"warnings above for why. Rebuilt on the next reconcile (~1 min).",
|
||||
want: SeverityCritical,
|
||||
section: "generate",
|
||||
}, {
|
||||
// generate/ruleset.go:1214 — the counterpart the outage used to be graded
|
||||
// BELOW. It stays critical: the consequence is identical (the list is not
|
||||
// loaded and matches nothing), which is the whole point — the inversion is
|
||||
// fixed by lifting the outage, not by lowering the typo.
|
||||
name: "url blocklist with an empty url",
|
||||
text: `ruleset "ads": url source with empty url, skipped`,
|
||||
want: SeverityCritical,
|
||||
section: "ruleset", entity: "ads",
|
||||
}, {
|
||||
// generate/ruleset.go:819 — the list IS still in force, from the copy
|
||||
// compiled earlier. Grading this critical would light the alarm banner on
|
||||
// most reconciles of a healthy router behind a flaky link, and a banner that
|
||||
// is always on is a banner nobody reads when it finally matters.
|
||||
name: "blocklist refresh failed but the compiled copy still blocks",
|
||||
text: `ruleset "ads": could not refresh the list from "https://big.oisd.nl/domainswild" ` +
|
||||
`(dial tcp: i/o timeout); continuing with the copy compiled earlier. Retried on the next reconcile.`,
|
||||
want: SeverityWarning,
|
||||
section: "ruleset", entity: "ads",
|
||||
}, {
|
||||
// generate/route.go:407 — a rule the operator scoped to one device now
|
||||
// applies to the entire network. Whatever it does, it now does to everyone.
|
||||
name: "a rule's source scope collapsed to the whole LAN",
|
||||
text: `rule "kids": none of its source entries can be matched by the engine (interface/zone/MAC ` +
|
||||
`selectors and invalid addresses are dropped), so the rule now applies to EVERY client on ` +
|
||||
`the router instead of that source — check it is still what you want, and use IP ` +
|
||||
`addresses/subnets as the source`,
|
||||
want: SeverityCritical,
|
||||
section: "rule", entity: "kids",
|
||||
}, {
|
||||
// generate/route.go:115 — the rule exists in the UI and routes nothing.
|
||||
name: "a rule whose target is never reached",
|
||||
text: `rule "work": no matcher the engine can evaluate, skipped — its target "group:auto" ` +
|
||||
`never applies and the traffic follows the rules below it and the default`,
|
||||
want: SeverityCritical,
|
||||
section: "rule", entity: "work",
|
||||
}, {
|
||||
// generate/ruleset.go:445 — a redundant field on a list that loads fine.
|
||||
name: "a redundant field on a working blocklist",
|
||||
text: `ruleset "ads": format "binary" is IGNORED for source=geosite — the category decides. Remove it to avoid confusion.`,
|
||||
want: SeverityWarning,
|
||||
section: "ruleset", entity: "ads",
|
||||
}, {
|
||||
// generate/chain.go:108 — a configured path that does not resolve. Its
|
||||
// traffic is blocked fail-closed, so no protection claim is broken.
|
||||
name: "a chain with no hops",
|
||||
text: `chain "hop": has no hops, target skipped`,
|
||||
want: SeverityWarning,
|
||||
section: "chain", entity: "hop",
|
||||
}, {
|
||||
// generate/cache.go:92 — operational, the lists still compile.
|
||||
name: "the compiled lists moved to tmpfs",
|
||||
text: "cache: only 3 MiB free on /overlay (need 8 MiB), using tmpfs /tmp/shater instead — " +
|
||||
"remote rule-sets will be re-downloaded after every reboot, so free some space",
|
||||
want: SeverityInfo,
|
||||
section: "generate",
|
||||
}}
|
||||
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
var got *Warning
|
||||
for _, w := range collectWarnings(blockGlobals(), []string{tc.text}, nil, nil) {
|
||||
if w.Section == "untunnelable" {
|
||||
continue // the always-present policy notice
|
||||
}
|
||||
w := w
|
||||
got = &w
|
||||
}
|
||||
if got == nil {
|
||||
t.Fatalf("the warning was dropped entirely")
|
||||
}
|
||||
if got.Severity != tc.want {
|
||||
t.Errorf("severity = %q, want %q\n text: %s", got.Severity, tc.want, tc.text)
|
||||
}
|
||||
if got.Section != tc.section || got.Name != tc.entity {
|
||||
t.Errorf("attribution = %q/%q, want %q/%q — the panel deep-links on these",
|
||||
got.Section, got.Name, tc.section, tc.entity)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestUnreachableRuleSetFromRealGenerate drives the ACTUAL producer, so this stays
|
||||
// correct if generate rewords or re-tags the message. A url rule-set pointing at a
|
||||
// closed local port fails the reachability probe exactly the way an unreachable
|
||||
// public blocklist does, with no network needed.
|
||||
func TestUnreachableRuleSetFromRealGenerate(t *testing.T) {
|
||||
m := holdModel("closed")
|
||||
m.Rulesets = []model.Ruleset{
|
||||
{Name: "ads", Type: "domain", Source: "url", URL: "http://127.0.0.1:1/blocklist.srs", Format: "binary"},
|
||||
}
|
||||
m.Rules = []model.Rule{
|
||||
{Name: "blockads", Enabled: true, Order: 10, DstRuleset: []string{"ads"}, Target: "block"},
|
||||
}
|
||||
|
||||
_, genWarnings, err := generate.GenerateWithWarnings(m)
|
||||
if err != nil {
|
||||
t.Fatalf("GenerateWithWarnings: %v", err)
|
||||
}
|
||||
t.Logf("real generate warnings: %q", genWarnings)
|
||||
|
||||
var found *Warning
|
||||
for _, w := range collectWarnings(blockGlobals(), genWarnings, nil, nil) {
|
||||
if w.Section == "ruleset" && w.Name == "ads" {
|
||||
w := w
|
||||
found = &w
|
||||
}
|
||||
}
|
||||
if found == nil {
|
||||
t.Fatalf("an unfetchable blocklist produced no warning attributed to it: %q", genWarnings)
|
||||
}
|
||||
if found.Severity != SeverityCritical {
|
||||
t.Errorf("a blocklist that did not load is a protection gap the operator cannot otherwise "+
|
||||
"see; severity = %q, want critical (message: %q)", found.Severity, found.Message)
|
||||
}
|
||||
}
|
||||
|
||||
// blockGlobals is the default policy fixture: kill-switch closed, untunnelable
|
||||
// traffic blocked — i.e. what a stock install runs.
|
||||
func blockGlobals() model.Globals {
|
||||
|
||||
@@ -87,9 +87,19 @@ func TestSessionThenStatus(t *testing.T) {
|
||||
if err := json.NewDecoder(resp.Body).Decode(&out); err != nil {
|
||||
t.Fatalf("decode status json: %v", err)
|
||||
}
|
||||
// running is true because Status() is only ever called from the live daemon.
|
||||
if v, ok := out["running"].(bool); !ok || !v {
|
||||
t.Fatalf("status json missing/false running: %v", out)
|
||||
// `running` reports whether SHATER is running — the daemon up AND its engine
|
||||
// carrying a config (apply.Status). It used to be hard-wired true on the
|
||||
// reasoning that Status() is only called from the live daemon, which made it a
|
||||
// constant the panel then rendered as a green "Online"; this fixture's engine
|
||||
// has never started, so the honest answer here is false. What is asserted is
|
||||
// that the field is present and agrees with engine_running — the two are the
|
||||
// same fact and a client may key on either.
|
||||
v, ok := out["running"].(bool)
|
||||
if !ok {
|
||||
t.Fatalf("status json missing running: %v", out)
|
||||
}
|
||||
if er, ok := out["engine_running"].(bool); !ok || er != v {
|
||||
t.Fatalf("running (%v) and engine_running (%v) must agree: %v", v, out["engine_running"], out)
|
||||
}
|
||||
if _, ok := out["version"]; !ok {
|
||||
t.Fatalf("status json missing version: %v", out)
|
||||
|
||||
Reference in New Issue
Block a user