screen: expose unresolved profile references in status and metrics (advances #5806) - #6839
Open
psaab wants to merge 8 commits into
Open
screen: expose unresolved profile references in status and metrics (advances #5806)#6839psaab wants to merge 8 commits into
psaab wants to merge 8 commits into
Conversation
A zone can reference a screen ids-option profile that is not defined.
Strict commit rejects that outright, but tolerant startup/recovery of an
older or externally modified active.json, HA config-sync from a
schema-skewed peer, and rolling-upgrade intervals all downgrade it to a
warning. The dataplane then enforces NONE of that zone's screen checks —
no LAND, fragment, source-route, SYN/ICMP/UDP flood, scan/sweep or
session-limit — while the active configuration still says a screen is
attached.
The missing-profile set already crosses the wire as
ConfigSnapshot.ScreenMissingProfiles and drives a rate-limited runtime
WARN in the Rust screen runtime. But that WARN was the ONLY signal:
nothing in pkg/api, pkg/grpcapi or pkg/cli referenced the set at all, and
a warning can be missed entirely during failover noise. The issue states
the requirement directly — one warning is not the sole signal.
Three surfaces now report it, all derived from ONE computation.
ScreenMissingProfileRefs is exported as the shared SSOT: a thin wrapper
over the existing snapshot builder, so the metric and the status block
report the exact set that was published to the helper rather than a
re-derived predicate that could drift from it. Config-derived is the
correct truth source here rather than a desired-versus-applied gap: the
defect is precisely that the ACTIVE CONFIG claims a screen while nothing
enforces it, so the config is the authoritative statement of the
unresolved reference.
xpf_screen_unresolved_profile_zones{zone,profile,disposition} is a gauge
emitted BEFORE the dataplane gate, mirroring its config-derived sibling
collectHostInboundAddresslessZones — a config-only or degraded boot is
exactly when an unenforced security control must stay visible. The series
is present only while a reference is unresolved, so max_over_time alerts
on any zone that was ever left unscreened.
Both `show security screen` renderers, local CLI and gRPC, now print an
"Unresolved screen profile references:" block. Placement is the load-
bearing detail: both renderers early-return "No screen profiles
configured" when cfg.Security.Screen is empty, and that is EXACTLY the
tolerant-load shape that strands a reference — the profile definitions
are gone, the zone still claims one, and the operator was being told in
as many words that nothing was configured. The block is therefore emitted
before that return.
The disposition label and the status sentence report what the dataplane
does with such a zone TODAY: not-enforced-pass, the verdict stays
ScreenVerdict::Pass and traffic is forwarded unscreened. That is a
statement of current behaviour, deliberately NOT a resolution of the
fail-closed-versus-pass posture. That posture is a genuine availability-
versus-security decision — a runtime fail-closed posture is itself an
availability brick under the #1960 no-brick rationale, and these tolerant
paths are the only ones that reach the state — and it remains open. It
was deferred to "the /research half of #3082", but #3082, #3908 and #3066
are all closed, so #5806 is its only surviving owner.
Validation: two mutations, each preceded by a 0-error go build + go vet so
every RED is an assertion. Moving the collector below the
`dp == nil || !dp.IsLoaded()` gate empties the series on a config-only
boot. Moving the status block inside the empty-Screen else branch
restores the bare "No screen profiles configured". Each was restored and
touched, and both returned to green. Every guard is paired with a
negative control asserting a RESOLVED reference emits and renders
nothing, so none can pass by firing unconditionally.
The pkg/api fixture reaches the defect through the real tolerant path
rather than simulating it. Strict commit rejects the dangling reference
(verified firsthand), so the test commits a valid config, externally
modifies the persisted active.json so the profile definition no longer
parses while the zone's reference survives, and re-loads it through
store.Load() — reachability path 1 from the issue. The fixture asserts
its own assumption about the persisted shape, so it fails loudly rather
than silently testing nothing if that shape ever changes.
go test ./... exits 0. gofmt is clean on every touched file; the five
pre-existing gofmt violations under pkg/dataplane/userspace/*_test.go are
untouched by this branch.
advances #5806
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
… label Fold of three review corrections on the #5806 visibility work. Wording. "Traffic is forwarded UNSCREENED" overstated the behaviour. It reads as a permit, and an operator could take it to mean the firewall is passing traffic it would otherwise deny. What is actually true is narrower: the screen checks are skipped, and nothing else about the packet's treatment changes — zone security policy still evaluates it normally. The disposition now reads "the profile reference does not resolve, so no screen checks are applied to this zone; policy evaluation is unaffected", which is both accurate and posture-neutral. Derive-or-anchor. The skip decision is NOT derivable from the Go emitter: it lives in the Rust runtime, where screen/mod.rs returns ScreenVerdict::Pass on the None branch, and every Go mention of it is a comment asserting it. That is precisely the shape that goes stale silently — true when written, believed by a later reader after it stopped being true. Since the string must be a constant, it now carries a literal #5806 anchor, and the comment explains that the anchor is load-bearing rather than decorative: when the enforcement posture is settled, a grep for the issue number has to land on every place asserting today's behaviour. Two tests assert the anchor survives into the metric HELP text and into the rendered status block. Label to HELP. The disposition was a metric label. It is a global statement about the current implementation, identical for every series, so it carried no information as a label — and a prose label value would have handed us unbounded cardinality the day someone made it vary. The label set is now exactly {zone, profile}, the two things that actually differ per series, and the disposition rides in the descriptor's HELP text plus one trailing status line. Both read the same exported ScreenUnresolvedDisposition constant, so the metric and the CLI/gRPC block cannot drift into describing the behaviour differently. The ScreenMissingProfileRefs doc comment now states the SSOT reuse as the contract rather than an implementation convenience: going through it is what makes it impossible for a surface to report a different set than the dataplane was actually told about. It also records why config-derived is correct here and was wrong in #6828 — there an authoritative zero was published from config while a fence was actively dropping; here the defect IS a property of the configuration. Validation: every mutation was re-gated after the fold, not just the new ones, because a fold can introduce a regression. Each was preceded by a 0-error build so the RED is an assertion. Moving the collector below the dataplane gate still empties the config-only-boot series. Moving the status block inside the empty-Screen else branch still restores the bare "No screen profiles configured". Restoring the old overstated wording (which also drops the anchor) reddens three guards across all three packages: the HELP anchor, the "policy evaluation is unaffected" check, and the rendered-text anchor. Reintroducing the disposition label reddens the label-set guard. Each file was restored and touched, and every case returned to green. One hazard is recorded in _Log.md: restoring a mutated file with `git checkout --` reverts it to HEAD, which silently discarded this fold's own uncommitted changes to metrics_counters.go. It was caught by grepping the restored file instead of trusting the restore. Restore from the pre-mutation snapshot, not from HEAD, when the file already carries uncommitted work. go test ./... exits 0; build, vet and gofmt clean. advances #5806 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Five gate findings, all of which were that a claim existed without a guard behind it. Policy evaluation is unaffected — now bound, not downgraded. The status text and metric HELP assert that an unresolved screen-profile reference skips the screen checks and leaves policy evaluation alone. Nothing proved the second half, and it is not provable from Go: the decision lives in the Rust runtime. It is now bound where it is decidable. A new test drives the real stage_screen_check and asserts that a zone with an unresolved reference yields StageOutcome::Continue — the packet is neither dropped nor is its descriptor consumed — which is precisely what "the poll loop goes on to evaluate zone policy" means at that seam. Making the None branch Drop instead of Pass turns it red. That test also pins a real gap it uncovered, which is not a property of this change. stage_screen_check short-circuits on !screen.has_profiles(), and has_profiles() is !self.zones.is_empty() — the RESOLVED map only. So when the security screen stanza is absent ENTIRELY, which is exactly the tolerant-load shape that strands a reference, the stage returns before maybe_warn_missing_profile can run and the rate-limited runtime WARN never fires at all. That WARN is the signal #3082 shipped and the one #5806 treats as already existing. In that case the config-derived metric and status block added here are the only signal there is. Asserted, so the gap is pinned rather than described. The local-CLI renderer had no test. Deleting its emit passed the entire suite — only the gRPC renderer was covered. It now has an integration test built on the same real tolerant-load fixture, capturing stdout, plus a negative control. The shared constant did not bind as an SSOT. The old tests checked that the sentence appears in the metric HELP and in the rendered block, which permits exactly what the constant exists to prevent: replacing both uses with identical duplicated literals passes every one of them while leaving the two surfaces free to drift. The new guard asserts common source instead of equal values — the sentence must exist as a literal exactly once, its const definition, with consumers reaching it by identifier. Duplicating the literal passes every old test and fails only this one. The snapshot publication path had no guard, even though the whole SSOT argument rests on the snapshot actually carrying the set. Nulling ScreenMissingProfiles in buildSnapshot previously passed everything. The pkg/api test file now carries a header naming which cases bind the fix and which are negative controls, so the case count is not over-trusted. Validation: four new mutations, each preceded by a 0-error build. The first attempt at the local-CLI mutation broke the build on an unused import, which would have been a false red; it was redone so the code still compiles and the red comes from an assertion. The duplication mutation is the four-cell proof — it escapes every old guard and is caught only by the new one. go test ./... exits 0; cargo test --bins --test-threads=1 reports 4235 passed, 0 failed. Formatting note recorded in _Log.md: rustfmt on the single touched .rs file still reformatted about 97 pre-existing lines across 46 hunks. The additions were re-applied onto the pristine file instead, giving 171 insertions and 0 deletions in 2 hunks. "Format only the files you touched" is not sufficient on this tree. advances #5806 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Six review findings, two of them major. The publication guard could not run. buildSnapshot enumerates ip-rules through real netlink, which fails "operation not permitted" in a restricted sandbox before the screen assertion is ever reached, so the guard added for the snapshot path silently never executed. Verified the mechanism directly rather than on report: stubbing ruleListFn to return an error reproduces "route snapshot: list ip-rules for family 2: operation not permitted". The in-tree stubRuleListHermetic is now wired. The same guard was also weaker than its claim. It compared only the Go struct, so any break after the struct is built — a wire-tag rename, Rust field drift — passed it, even though the whole SSOT argument rests on the dataplane being told the same set. It now marshals the snapshot and checks the screen_missing_profile_zones key and its zone/profile members, which are the names the Rust decoder actually reads. The policy test accepted a no-op on the thing it tested. Two shapes. It accepted any Continue(_), which includes ScreenCheckOutcome:: SynCookieChallenge — an outcome that continues but answers the packet with a challenge rather than carrying it on to policy, so it does not satisfy the claim; the assertion is now the exact Continue(ScreenCheckOutcome::Pass). And the missing-only subtest expected warns == 0, which is also the failure default if the missing-profile threading were deleted or update_missing_profiles were a no-op, so it passed whether the mechanism worked or did not exist. It now arms the gate afterwards on the same state — adding a resolved profile for an unrelated zone, leaving the lan reference untouched — and requires the warning to appear. That pair separates "the gate suppresses a working mechanism" from "there is no mechanism". Three claims corrected. A helper insertion had orphaned source_route_screen's doc comment onto missing_only_screen, leaving a source-route description above a function that contains only a missing reference. The comment describing StageOutcome::Continue as necessarily unconsumed is contradicted by the SYN-cookie arm; it now states that as the reason the exact variant is asserted. And the "ONLY signal" wording overstated: other reporting exists — tolerant-load configuration warnings and daemon logging — so the defensible claim is that the runtime dataplane WARN specifically cannot fire. Narrowed in pkg/api/metrics.go, the Rust test comment and docs/feature-coverage.md, and corrected on #6860 and #5806 so the record does not keep the stronger version. Test acceptance. The CLI tests assert structure rather than token containment: the heading exists, exactly one row sits under it carrying both the zone and the profile, and the disposition appears exactly once after the rows. The CLI silence control no longer rests on absence alone, since an aborted render also contains no heading — it requires the healthy inventory to render. The source-identity guard counts occurrences rather than files, because a duplicate in the same file is as drift-prone as one in another, and adds a split-concatenation check, since a literal broken across + chunks defeats a plain substring scan. Validation: four new mutations plus a re-gate of the prior set, each preceded by a 0-error build. Making update_missing_profiles a no-op — the exact escape identified in review — is now red where it previously passed. Renaming the wire tag while the Go struct stays correct reddens the publication guard. Dropping the heading while keeping the rows reddens the CLI structural guard. On re-gate, moving the collector below the dataplane gate is still red; the old-wording mutation now reddens four packages rather than three, because the CLI guard picked it up too; the CLI emit and publication mutations are still red. Each was restored and touched, green after every one. One narrowing is argued rather than demonstrated, and is marked as such in _Log.md: producing a SynCookieChallenge for a missing-profile zone would require contriving production code that cannot reach it, so the escape the Continue(Pass) assertion closes is derived from the enum, not shown by mutation. go test ./... exits 0; cargo test --bins --test-threads=1 reports 4235 passed, 0 failed. advances #5806 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
# Conflicts: # _Log.md
Resolve the sole conflict, _Log.md, by union: every entry from both sides is retained and none is rewritten. The file is no longer append-ordered, so line counts and prefix checks say nothing useful about the result; the resolution was verified structurally instead, by confirming that each pre-merge side diffs into the merged file with add-hunks only and no changed-or-deleted hunk on either side. Every other path merged without conflict. Because a clean textual auto-merge can still break compilation when a signature moves on one side, that was confirmed by building rather than by inspection: go build ./... clean on the merged tree, and cargo check --all-targets clean for the Rust crate. Advances #6839.
# Conflicts: # _Log.md
# Conflicts: # _Log.md
Owner
Author
First independent review, @
|
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
Drives the posture-independent half of #5806 — acceptance criterion 4,
"Runtime status and Prometheus expose the unresolved reference and enforcement
disposition; one warning is not the sole signal."
It deliberately does not pick the fail-closed-vs-pass posture. That is a real
availability-versus-security decision and it stays open under #5806.
STEP-0 against current
origin/master(ad95911)Criterion 4 is genuinely unmet. The missing-profile set already crosses the
wire as
ConfigSnapshot.ScreenMissingProfilesand drives a rate-limited runtimeWARN — but
git grep -ln "screen_missing_profile_zones\|ScreenMissingProfileZones"returns 9 files, all of them either
pkg/dataplane/userspace/protocol.go(thewire field) or
userspace-dp/**(the consumer + tests). Nothing inpkg/api,pkg/grpcapi, orpkg/cli. So the WARN is the sole signal, which isexactly what the criterion rules out.
This refines the earlier scoping note on the issue, which said "the observability
half already shipped" — true of the WARN, not of status or metrics.
The posture is orphaned, not an oversight.
userspace-dp/src/screen/mod.rs:48-52defers it to "the /research half of #3082"; #3082, #3908 and #3066 are all
CLOSED, so #5806 is its only surviving owner. Untouched here.
What this adds
One SSOT.
ScreenMissingProfileRefsis exported as a thin wrapper over theexisting snapshot builder, so the metric and both status renderers report the
exact set published to the helper rather than a re-derived predicate that
could drift. Config-derived is the right truth source here, not a
desired-vs-applied gap: the defect is "the active config claims a screen and
nothing enforces it", so the config is the authoritative statement of the
unresolved reference.
Prometheus.
xpf_screen_unresolved_profile_zones{zone,profile,disposition},emitted before the dataplane gate — mirroring its config-derived sibling
collectHostInboundAddresslessZones, whose doc comment states the same"same builder the daemon logs from" discipline. A config-only / degraded boot is
exactly when an unenforced security control must stay visible. Present only
while a reference is unresolved, so
max_over_time(...)alerts on any zone everleft unscreened.
Status, both renderers. An
Unresolved screen profile references:block inlocal-CLI and gRPC
show security screen.On
disposition. The label and the status sentence report what the dataplanedoes today (
not-enforced-pass— the verdict staysScreenVerdict::Pass,traffic is forwarded unscreened). A factual statement of current behaviour, not a
decision about the future posture. If you would rather that wording wait until
the posture is settled, it is one constant and one sentence to drop.
Proof — two mutations, both RED on an ASSERTION
go build+go vetreturned 0 errors under each mutation first.dp == nil || !dp.IsLoaded()gateTestScreenUnresolvedProfileZonesEmittedOnTolerantLoad—series = map[]Screenelse branchTestShowScreenReportsUnresolvedReferenceWithNoProfilesDefinedEach restored and
touched; both returned to green.Every guard is paired with a negative control — a resolved reference must
emit no series and render no block — so none of them can pass by firing
unconditionally. Plus a nil-store no-panic case for the pre-dataplane-gate path.
The fixture reaches the defect through the real path
Strict commit rejects the dangling reference (verified firsthand:
commit check failed: security zone "trust" references undefined screen profile "no-such-profile"), so the state cannot simply be committed. The test thereforecommits a valid config, externally modifies the persisted
active.jsonsothe profile definition no longer parses (the
ids-optionpath token occursexactly once and is renamed) while the zone's reference survives, then
re-loads through
store.Load()— reachability path 1 from the issue, and thesame shape an HA sync from a schema-skewed peer produces.
The fixture asserts its own assumption (
ids-optioncount == 1) so it failsloudly rather than silently testing nothing if the persisted shape ever changes.
Gates
go test ./...— exit 0go test ./pkg/api/... ./pkg/grpcapi/... ./pkg/cli/... ./pkg/dataplane/...— exit 0go build/go vet/gofmt -lclean on every touched file. Fivepre-existing
gofmtviolations underpkg/dataplane/userspace/*_test.goareuntouched by this branch (confirmed unmodified).
Docs
docs/feature-coverage.md's Screen/IDS row now documents the three surfaces, theshared SSOT, and — explicitly — that
dispositiondescribes current behaviourrather than resolving the posture.
Not in scope
Acceptance criteria 1, 2, 3 and 5 (fence vs reject-snapshot vs quarantine,
cross-node determinism, transactional ordering with snapshot generation,
automatic recovery) all depend on the posture decision and are left to it.
Hence
Advances #5806, notCloses.Advances #5806.
🤖 Generated with Claude Code
https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi