config: reject login classes carrying unenforced deny regexes - #6838
config: reject login classes carrying unenforced deny regexes#6838psaab wants to merge 10 commits into
Conversation
A custom `system login class` accepts four fine-grained regex
sub-statements. xpf's runtime RBAC gate (pkg/cli/permissions.go
checkPermission) is coarse — it matches a required permission against the
class's mapped bucket set and consults NONE of the four. All four were
therefore "recognized, not enforced": the config committed and the regex
configured nothing.
For the restrictive half that is fail-open. An operator writing
set system login class limited permissions all
set system login class limited deny-commands "request system zeroize"
got a class that committed cleanly and could still zeroize the box —
a config asserting the verb was denied, on a box where it was allowed.
Accepting a security restriction as inert state is strictly worse than
refusing it, so the strict commit / commit-check path now hard-rejects.
The four leaves are NOT symmetric, and this deliberately covers only two
of them. deny-commands / deny-configuration SUBTRACT from the permission
bits, so dropping them leaves the denied verbs allowed — the class ends
up more permissive than the config states. allow-commands /
allow-configuration ADD to the permission bits (Junos: usable "in
addition to" what the bits allow), so dropping them yields a subset of
what the operator wrote — less access, never more. That is fail-closed,
so they keep the #4304 accept-with-advisory treatment; rejecting them
would break configs that are already safe. Junos precedence cuts the
same way: allow-commands wins over deny-commands, so a class pairing
them is a deny-with-exceptions and rejecting on deny-presence alone
still catches it.
The gate keys off leaf PRESENCE, recorded at parse into a new
LoginClass.DenyLeavesPresent, not off the field value. `deny-commands ""`
and a valueless `deny-commands` both flatten to the empty string and are
indistinguishable from an absent leaf, so a `!= ""` test would accept
them — yet an empty POSIX regex matches every command, i.e. denies
everything, the most restrictive thing an operator can write and the
most dangerous to silently drop. Recording presence in the existing
parse switch also inherits its dual-AST-shape coverage: the leaf carries
its name in Keys[0] in both the flat-set and hierarchical-block shapes.
The tolerant load / peer-sync path does more than warn. #1960 says an
already-persisted or peer-synced config must still boot, so the
rejection cannot simply be re-run there — but a bare warning would leave
the runtime fail-open exactly where it started, since such a config
still reaches the RBAC gate with its full permission set and the deny
dropped. So that path folds the class to view-only: the operator asked
for strictly less than `permissions` grants, we cannot compute how much
less, and we resolve the ambiguity in the restrictive direction. This is
the same least-privilege fold mapJunosPermissions already applies to
Junos tokens with no precise coarse equivalent. It cannot brick the box
— resolveClassPerms consults the built-in classes first and the fold
only ever touches a custom class, so console super-user access is
untouched. viewOnlyFold is explicitly non-widening: a class that granted
nothing keeps granting nothing, rather than being handed view access by
an unconditional assignment.
The #4304 advisory loses its deny branch rather than having it reworded.
It is unreachable on the strict path now, and on the tolerant path it
would be false: the class has been folded to MORE restrictive, so the
old "MORE PERMISSIVE" text would tell the operator the opposite of what
happened. The advisory also now reports the effective permission set
rather than recomputing it, so a folded class is not described as still
holding permissions it lost; the two are identical for every unfolded
class, so no existing output changes.
This is the fail-closed half only. Enforcing the regexes across every
operational-command and configuration dispatch point needs a
matching-semantics decision and stays open; advances #5831.
Validation: `go test ./pkg/config/... ./pkg/cli/...` and the full
`go test ./...` pass. Each guard was mutation-proved — reverted
individually, confirmed to fail from an assertion (build and vet clean
under the mutation), restored and re-run green — including the
quoted-empty edge, the non-widening fold, and a positive control
asserting that valid classes and the additive allow-* half still commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
The #5831 tolerant-path fold collapsed a custom login class carrying an unenforceable deny-commands / deny-configuration to view-only. That resolves the un-enforceable restriction in the right direction but can strand the box. pkg/daemon/daemon_run.go assigns the CONFIGURED login class to any OS user whose name matches a `system login user` entry, and `root` is a name the username validator accepts — account provisioning skips root, the CLI class assignment does not. So this previously-accepted config set system login class noc-admin permissions [ view configure ] set system login class noc-admin deny-configuration "security policies" set system login user root class noc-admin binds the console operator to noc-admin. Before the gate the deny leaf was inert and the class held view + configure; after a view-only collapse it holds view alone, so root cannot enter `configure` to delete the offending statement — while validateLoginClassDenyStrict rejects every commit until it IS deleted. A configure-only class collapsed to the empty permission set, which is worse. Recovery needed an out-of-band shell. The old no-brick argument ("resolveClassPerms consults the built-ins first, so console super-user access is untouched") does not hold: built-ins-first lookup only decides which table answers for a class NAME, not that any actual login is bound to a built-in. And #1960's rule covers this shape — a config that LOADS but revokes the access needed to repair itself is the same failure with extra steps. Implementation. viewOnlyFold becomes repairableFloorFold: the folded set is {PermView, PermConfig} intersected with what the class already held. PermAll contributes both (checkPermission returns nil on PermAll for every required permission, so it subsumes them) and is itself dropped. The floor is an intersection, not a grant — a class holding neither bucket still folds to the empty set, so a restriction gate can never become a privilege escalation. PermConfig is exactly what requiredPermission demands for `configure`, and config mode applies no further per-statement gate, so it is precisely the self-repair channel. The fold keeps its teeth where it can: PermClear, PermControl, PermMaint and PermAll all go, so the motivating example (`permissions all` + `deny-commands "request system zeroize"`) genuinely loses zeroize. Stated plainly in the code and the operator doc: for deny-configuration this enforces nothing, because xpf's coarse model has only all-or-none configuration and "none" is the state that strands repair. The strict gate is the forcing function, and the warning now names the post-fold set, the retained configure, that limit, and the fact that commits stay blocked until the statement is removed. Dropping Clear/Control/Maint even when only deny-configuration is present is deliberate over-restriction in a dimension the operator did not restrict: one rule that always errs restrictive is easier to reason about than a per-leaf branch, and unlike configure those verbs are recoverable from the CLI. Also corrected four claims that still described pre-PR behaviour: docs/config-schema.md and pkg/config/schema_system.go said deny leaves get a "MORE PERMISSIVE" compile advisory (strict now rejects, tolerant folds); loginClassAdvisoryWarnings' doc said the advisory covers allow/deny regexes after deny reporting was removed; docs/system-login.md and compiler_login_deny.go claimed the fold cannot lock an operator out. docs/system-login.md now carries the worked counter-example and the step-by-step recovery path. Validation. TMPDIR=/tmp go test ./pkg/config/... ./pkg/cli/... ./pkg/configstore/... ./pkg/daemon/... — all green, exit 0. gofmt and go vet clean on every touched file. Eight mutations, each reverted after measurement (files snapshotted and restored byte-for-byte, sha256-verified); every one produced an ASSERTION failure with go vet clean, never a build break: - exact pre-PR viewOnlyFold body restored: RED in both packages — pkg/config "fold STRANDED the box: class \"noc-admin\" held `configure` before the fold and lost it ([0])", the configure-only case reporting the empty set, and pkg/cli "STRANDED: the console class lost `configure` to the fold (permission denied: \"configure\" requires a higher login class)". This is the parent-RED proof that the new tests bind the fix. - keepConfig folded into keepView (view-only collapse): same RED set. - floor returned as a literal {PermView, PermConfig}: RED on the three non-widening arms plus pkg/cli BelowFloorIsNotRaised and EmptyStaysEmpty. - fold made a no-op: RED on ToleratedButFolded, four NeverWidens arms, and pkg/cli BelowFloorIsNotRaised. - strict gate short-circuited to nil: RED on all six RejectedAtCommit arms, the hierarchical-shape test, and DenyCommandsRejected. - presence recording degraded to a value test: RED on all four quoted-empty / valueless arms and PresenceSurvivesEmptyValue. - warning's "still NOT in force" limit reworded to claim enforcement: RED on FoldWarningStatesTheRecoveryPath. - gate widened onto the additive allow-commands leaf: RED on GateIsScopedToRestrictiveLeaves and two ValidStillCommits arms. No cluster or smoke validation was run for this change; it is compile-time and CLI-gate only. Advances #5831; per-command deny enforcement stays open there.
The repair floor retains {PermView, PermConfig}. A deny AIMED AT A
RETAINED BUCKET is therefore unenforceable AND unrestricted — the fold
compensates for the buckets it drops and does nothing for the two it
keeps. Both retained buckets are reachable deny targets:
- PermConfig gates `configure`, so `deny-configuration <anything>` is
a complete no-op;
- PermView gates `show` / `ping` / `traceroute` / `monitor`
(requiredPermission), so `deny-commands "show interfaces"` is
equally a complete no-op.
The previous warning carved out CONFIGURATION alone, so the view-level
shape was described as restricted when it was not. The claim was broader
than the behaviour, which is the defect class #5831 exists to fix.
The floor created that gap, and the alternative was weighed rather than
assumed. Tightening to {PermConfig} would make view-level denies bite
and is technically safe for repair — config mode applies no
per-statement gate, so `configure` alone completes the fix. It is
rejected because the cost lands on the wrong configs: a class whose deny
targets `request` would lose ALL of show/ping/monitor, and a view-only
class would fold to nothing. Trading a large silent operational
downgrade for partial enforcement of a control xpf cannot honor at any
granularity is the worse deal.
So the fold is unchanged and the CLAIM is corrected. The operator
warning now renders from the RETAINED set (loginClassDenyFoldWarning)
rather than prose, so it cannot drift wider than the behaviour again: it
names the retained levels, states they are COMPLETELY UNRESTRICTED
including anything the statement names, and says plainly that the fold
reduces blast radius rather than enforcing the statement. The
enforcement that exists is validateLoginClassDenyStrict refusing every
commit until the statement is removed. Per-command RBAC stays on #5831.
The same correction lands in repairableFloorFold's doc comment, the
docs/system-login.md warning block, and docs/config-schema.md — which
claimed "every operational-verb bucket the deny could target is
dropped", false because `view` is one and is retained.
Permission expectations in tests are now compared as RENDERED sets
(describePerms) rather than raw slices. PermView is the iota ZERO value,
so an expectation whose first element is PermView can be satisfied by an
uninitialised element; the rendered form is non-default and a spurious
zero shows up as a duplicate "view".
Validation. TMPDIR=/tmp go test ./pkg/config/... ./pkg/cli/...
./pkg/configstore/... ./pkg/daemon/... — all six packages ok, exit 0.
gofmt and go vet clean on every touched file.
Five mutations, each snapshotted and restored byte-for-byte with
sha256 verification, scored from real go test exit codes. All produced
ASSERTION failures with `go vet` exit 0 — never a build break:
- r1's CONFIGURATION-only carve-out restored: RED on all four warning
shapes. This is the parent-RED for the finding.
- retained set HARD-CODED to "{configure,view}" while keeping the
honest prose: RED on the per-class case only — scope-correct.
- "COMPLETELY UNRESTRICTED" removed: RED on all four shapes.
- enforcement re-claimed: RED on all four shapes.
- fold returns a zero-filled slice of the right length: RED across
both packages including the two pkg/cli runtime tests.
The hard-coded-set mutation initially SURVIVED. The assertion anchored
on the bare rendered set, and the message also renders the PRE-fold set
— for a class already at the floor the two strings are identical, so it
passed while the claim slot was a literal. The assertion now anchors on
"gated at <set> is", which is unique to the claim, plus the folded-to
slot so the two renderings cannot drift apart. A test that passes for
the reason you predicted is not evidence until a mutation shows it can
fail for one you did not.
A new pkg/cli runtime test pins the retained-bucket no-op as a tested
property rather than a comment: `deny-commands "show interfaces"` still
permits `show interfaces`, `ping`, `traceroute`, `monitor` and
`configure`, while `request system zeroize` stays denied. It documents
the gap deliberately so a future change that believes it closed the gap
must prove it there rather than in prose.
Advances #5831; per-command deny enforcement stays open there.
# 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. Advances #6838.
# Conflicts: # _Log.md
Independent hostile review at
|
The tolerant #5831 fold resolved the class to mutate by NAME through a last-wins `map[string]*LoginClass`, but the runtime reader -- pkg/cli/permissions.go resolveClassPerms -- returns the FIRST match. A config spelling `class limited` twice therefore folded an object the runtime never consults. Measured end to end through the real peer-sync ingress (Store.SyncApply -> ActiveConfig -> checkPermission) with `user root class limited`: the resolved block kept PermAll, `request system zeroize` -- the exact verb deny-commands named -- was ALLOWED, showConfigRedacted() was false so IKE PSKs and SNMP communities rendered in cleartext, and the operator warning asserted a reduction that had not happened to the block in use. Both orderings failed open, and pointer identity alone closes only one of them. With the deny on the SECOND block, folding exactly the offending object is correct by identity and useless in effect: the runtime reads the FIRST block, which never carried a deny leaf and so keeps `permissions all`. The fold therefore carries the *LoginClass pointers on loginClassDenyRejection AND narrows every block sharing the name. That makes the outcome independent of the reader's tie-break rule, which is the property that has to hold; matching first-match would only be a proxy for it and would rot silently the day resolveClassPerms changed its pick. Folding a non-offending sibling cannot over-restrict it -- repairableFloorFold is an intersection, so a sibling is only ever narrowed to what it already held. The warning reports the union on each side, since the config does not say which block answers. Both halves are bound separately: restoring the name map reds `deny-on-first`, and folding only the offender reds `deny-on-second`. Repairs two #6662 packed-gate tests that the master merge left RED. Their fixtures carried deny leaves the new strict gate refuses, and TestLoginNestedCompilesCorrectly_6662 additionally asserted the MORE PERMISSIVE advisory this PR deleted. The deny carry-through those fixtures were buying was not dropped with them -- it moved to TestLoginCarriesDenyLeavesBothShapes_6662, which drives both AST shapes through the tolerant path. Also binds the previously unguarded advisory line (reverting `range lc.MappedPermissions` to a fresh mapJunosPermissions call left the failure set byte-identical to baseline, so it could regress to lying about a folded class with the suite green), and replaces the per-leaf `case` arm that recorded DenyLeavesPresent with a loginClassLeafRestrictive table the compiler consults directly. Adding a row is now the only edit needed to gate a new restrictive leaf; a two-direction canary pins the table against the schema, so a `class` leaf added without a classification reds the suite instead of defaulting to unrestricted. Junos has several xpf does not model yet (deny-hidden-commands, access-start/access-end, allowed-days), each a fresh instance of the same fail-open once the schema parses it. Three stale references to the deleted MORE PERMISSIVE advisory corrected, in the packed-gate rejection string an operator sees, its rationale comment, and docs/system-login.md. Validation: go build ./..., go vet ./..., and go test ./... all green (59 packages ok, zero FAIL). Each fix's revert observed RED individually: name map -> deny-on-first; offender-only fold -> deny-on-second; fresh mapJunosPermissions -> the advisory test; an unclassified schema leaf -> the key-set canary; hardcoded case arms with a classified leaf -> the enforcement canary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Fold r3 delivered at
|
| deny on | Classes[0] | Classes[1] | resolves | zeroize |
|---|---|---|---|---|
| first | [5]=PermAll, deny — unfolded |
[0 3], folded (no-op) |
[5] |
ALLOWED |
| second | [5]=PermAll, no deny — untouched |
[0 3], deny, folded correctly |
[5] |
ALLOWED |
In the second arm the name map already lands on the right object. Pointer
identity fixes the lookup and the box still fails open, because the block the
runtime reads never carried a deny leaf and keeps permissions all.
So the fold carries the *LoginClass pointers and narrows every block
sharing the name. That makes the outcome independent of resolveClassPerms'
tie-break rule rather than matching it — matching first-match would be a proxy
for the property and would rot the day the reader's pick changed. Folding a
non-offending sibling cannot over-restrict it: repairableFloorFold is an
intersection. The warning now reports the union on each side, since the config
does not say which block answers.
Both halves bound independently, each red on its own revert:
- restore the name-keyed last-wins map →
deny-on-firstreds (both arms, in fact) - keep pointers, fold only the offender →
deny-on-secondreds,deny-on-firstpasses
Second instance: none. The diff adds exactly two map[string]. The other
is seen, a leaf-name dedup set inside one class with no runtime reader. I
re-grepped MappedPermissions myself rather than taking the bounded blast
radius on trust — pkg/cli/permissions.go:35 is still the only non-test reader.
B2 (blocking) — CLOSED, without weakening the controls
Both #6662 tests pass. The deny leaves came out of the fixtures and the
MORE-PERMISSIVE assertion is gone, but the carry-through those fixtures were
buying was not dropped with them — measured first that the deny values
now have no production reader at all (their only consumer was the advisory this
PR deleted), then moved the coverage to TestLoginCarriesDenyLeavesBothShapes_6662,
which drives both AST shapes through the tolerant path and asserts value
and presence. The packed/nested control is intact; one property became two.
M1 / M2 / M3 — CLOSED
- M1 — fixed, plus two more instances of the same staleness the sweep
turned up: the rationale comment atcompiler_system_login_gates.go:44-49
anddocs/system-login.md:508. All three described an advisory that no
longer exists. - M2 — bound by
TestLoginClassAdvisoryReportsPostFoldPermissions. Revert
the line-pair → the advisory reports{super-user}for a class folded to
{configure,view}, and the test reds. - M3 — went further than a canary.
DenyLeavesPresentis no longer
recorded from a per-leafcasearm; it is driven by a
loginClassLeafRestrictivetable, so classifying a leaf is the only edit
needed to gate it. Two canaries: a key-set pin against the schema in both
directions, and an enforcement test that the table actually drives the
compiler. Demonstrated as a two-state proof — addingdeny-hidden-commands
to schema+table alone gates it correctly; severing the wiring back to
hardcoded arms with the same schema+table reds the enforcement canary.
DO-NOT-TOUCH list respected
Lockout half, commit-confirmed rollback target, the four-row doc asymmetry
table, custom-class accept-with-advisory, inactive: pruning — all untouched.
#6966 left alone; it is pre-existing and belongs to that issue.
_Log.md's superseded first entry is annotated in place, not rewritten.
Validation: go build ./..., go vet ./..., go test ./... all green at
f9db091ef on a clean tree — 61 packages ok, zero FAIL. (The pkg/ddns
port-collision flake did not recur.)
Fold r3 at
|
| deny on | Classes[0] | Classes[1] | resolves | zeroize |
|---|---|---|---|---|
| first | [5]=PermAll, deny — unfolded |
[0 3], folded (no-op) |
[5] |
ALLOWED |
| second | [5]=PermAll, no deny — untouched |
[0 3], deny, folded correctly |
[5] |
ALLOWED |
In the deny-on-second arm the last-wins map already lands on the right object.
Anchoring on the pointer fixes the lookup and the box still fails open, because
the block the runtime reads never carried a deny leaf and keeps permissions all.
So the fold carries the *LoginClass pointers and narrows every block sharing
the name. The rationale is the one I have been pushing all campaign, and the lane
reached it independently:
this makes the outcome independent of
resolveClassPerms' tie-break rule
instead of matching it — matching first-match would be a proxy for the property
and would rot silently the day the reader's pick changed.
Folding a non-offending sibling cannot over-restrict it (repairableFloorFold is
an intersection), and the warning reports the union on each side since the config
does not say which block answers.
Two reverts, proving the halves are independently necessary:
restore the name-keyed last-wins map -> deny-on-first RED (FAIL-OPEN: request system zeroize ALLOWED, cleartext secrets, false warning), reds both arms. Keep
pointers but skip classes with no deny leaves -> deny-on-second RED while
deny-on-first PASSES.
No second instance of the name-vs-pointer divergence: the diff adds exactly two
map[string], and the other is a leaf-name dedup set with no runtime reader. The
lane re-grepped MappedPermissions itself rather than taking my bounded blast
radius on trust.
B2 — closed without weakening the controls
Before stripping anything it measured whether the deny assertions bought real
coverage: the deny values now have no production reader at all — their only
consumer was the advisory this PR deleted, and the gate reads DenyLeavesPresent.
So dropping them costs no behavioural coverage, only parse fidelity, which moved
to a new TestLoginCarriesDenyLeavesBothShapes_6662 driving both AST shapes
through the tolerant path. Packed/nested control intact; one property became two.
M1-M3 — closed, and M3 went past what I asked
M1 fixed plus two more instances the sweep found (compiler_system_login_gates.go:44-49
and docs/system-login.md:508) — all three described the deleted advisory.
M2 bound; reverting to range mapped reds with the advisory reporting
{super-user} for a class folded to {configure,view}.
M3 is not a canary. DenyLeavesPresent is no longer recorded from a per-leaf
case arm — it is driven by a loginClassLeafRestrictive table the compiler
consults, so classifying a leaf is now the only edit needed to gate it. Proven
as a two-state demonstration: adding deny-hidden-commands to schema+table alone
gates it correctly; severing the wiring back to hardcoded arms with the same
schema+table reds the enforcement canary; adding it to the schema without
classifying it reds the key-set canary on exactly the named leaf. That converts a
drift detector into a structure where the drift is not expressible.
Flagged for the re-gate
Grouping by name changed the order validateLoginClassDenyStrict reports for a
pathological config where two names both carry denies and one name's blocks
straddle the other — now first-appearance-of-name rather than first-offending-block.
Both deterministic, documented in the collect function, and no test covered that
shape either way. Volunteered rather than left to be found.
Gate remains 2-of-3 on this PR: Codex cannot review this code (two legs killed by
the content filter, the second under a two-file scope at 191k tokens).
Re-gate at
|
The #5831 tolerant fold shipped a new operator-facing claim that is false for a class NAME shadowing a system-defined one. For class super-user { permissions all; deny-commands "request system zeroize"; } the fold rewrote the block and warned "The class is folded from {super-user} to {configure,view}", while pkg/cli resolveClassPerms consults config.LoginClassPermissions FIRST and so returned the built-in [PermAll]: `request system zeroize` allowed, secrets rendered in cleartext. That is the #6838 B1 defect one spelling further out — an operator-facing assertion of a narrowing the runtime never applied. It is measurably NOT a regression. The same probe run against origin/master (edefb75) allows zeroize and renders cleartext too, so the runtime delta is zero; the strict path rejects the config through the #6701 shadow gate either way. It is fixed here because this PR's whole subject is operator-facing claims being true, and the new sentence contradicts the #6701 warning sitting beside it in the same list, which states that such a definition "is INERT, so any narrowing it expresses is silently not applied while the commit advisory reports that it was". So the fold now skips a name found in LoginClassPermissions. Shadowing is a property of the NAME, which the whole cohort shares, and the table read is the same static one the runtime itself consults first — not the map-over-the-config's-own-blocks that B1 removed, which could disagree with the reader's tie-break. The knock-on matters as much as the warning. lc.MappedPermissions has exactly two production readers and the second is the #4304 commit advisory, which reports the EFFECTIVE set: folding a shadowing class had turned its advisory from "mapped to {super-user}" — true, the built-in does grant everything — into "mapped to {configure,view}", i.e. into the precise sentence #6701 warns is false. Skipping leaves both claims where #6701 found them. Nothing is emitted in their place: #6701 already states the truth for this shape, and whether an inert definition deserves more than a warning is #6701's question, not this gate's. Also binds three message-rendering lines the cohort fold added or made load-bearing, each of which could be deleted with the full pkg/config and pkg/cli suites still green: the `seen` leaf-name dedup (only reachable once the cohort's leaves were concatenated), sort.Strings(leaves), and unionPerms' dedup scan. One two-block fixture covers all three — the blocks duplicate a leaf, record their leaves out of sorted order, and hold overlapping permission sets — so each revert degrades the same rendered message on a different clause. They change no permission and no accept/reject outcome, but an operator message is this gate's entire product, and "deny-commands / deny-configuration / deny-configuration" reads like a parser bug in the operator's own config. And pins the strict gate's report-one CHOICE. The rule is first appearance of the class NAME, not of the first offending BLOCK; only an alpha/beta/alpha straddle tells the two apart. Both are safe — both reject, and the message names class and leaf — but it is an operator-facing message that no test constrained. Validation: go build ./... , go vet ./... and go test ./... all rc=0 over the whole tree. Five independent revert proofs, each observed RED: removing the shadowing guard reds three separate assertions (narrowed MappedPermissions, the false fold claim, the narrowed advisory); removing sort.Strings(leaves), the `seen` dedup, or unionPerms' dedup each reds the rendered-message comparison; reporting rejections[len-1] instead of rejections[0] reds the ordering pin with `beta`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Conflict confined to _Log.md, resolved as a pure union: both sides insert
a block of new entries at the same anchor and neither deletes a line, so
the resolution removes exactly the three conflict markers and keeps both
blocks whole. docs/system-login.md auto-merged — master's insertion lands
at base line 262 and this branch's earliest change is base line 285, so
the hunks do not overlap. Zero production Go files are touched by both
sides.
Verified structurally rather than by eye, with the totals predicted before
resolving and matched after:
_Log.md lines 79188 predicted, 79188 actual
docs/system-login.md lines 1542 predicted, 1542 actual
per-heading multiset under inclusion-exclusion,
count(result) == count(ours) + count(theirs) - count(base),
holds for all 1581 distinct _Log.md headings and all 31
docs/system-login.md headings; no heading has a negative
prediction, i.e. nothing was dropped on either side
result differs from each parent by exactly the other's changes:
ours->result == base->theirs (_Log.md +1963/-0, doc +77/-0) and
theirs->result == base->ours (_Log.md +95/-0, doc +201/-10)
A set comparison was deliberately not used: `## 2026-07-03` alone appears
84 times in the base file, so a set view of the headings reports phantom
losses. Anchored marker sweep (^<{7}, ^={7}$, ^>{7}) over all tracked
files returns nothing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
#5561 (merged to master as part of #6645) moved the built-in-first login class lookup out of `pkg/cli resolveClassPerms` and into `config.ResolveClassPermissions`, now shared by the CLI adapter and the REST control surface's `pkg/authz` gate. Every comment this round added named the old site. Precedence itself is unchanged — ResolveClassPermissions still consults LoginClassPermissions first — so the fix's premise holds on both surfaces, and the new test asserts it rather than assuming it: it fails with "premise changed" if a built-in ever stops answering for a shadowing name. The reader count is also unchanged at two (ResolveClassPermissions plus the #4304 advisory), so the argument that the advisory is the only reader a shadowing name can reach survives; only the name of the first one moved. Correcting these matters more than usual here: this PR's subject is operator-facing claims being true, and a citation into a function that no longer does the thing cited is the same failure one layer down. Validation: go build ./... , go vet ./... , go test ./... all rc=0 on the merge result (62 packages ok, zero FAIL). All five revert proofs re-run against the merged tree and each still reds — the shadowing guard, the three message-rendering lines, and the strict gate's report-one ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Round 4 at
|
| PR head | origin/master | |
|---|---|---|
| resolved perms | [PermAll] |
[PermAll] |
request system zeroize |
ALLOWED | ALLOWED |
showConfigRedacted() |
false | false |
| MappedPermissions | [view,configure] (folded) |
[super-user] |
Runtime delta zero, message-only defect. My ruling holds.
But there was a second false claim I had not found, and it invalidated the fix I
proposed. The fold's mutation of MappedPermissions is also read by the #4304
commit advisory. On master that advisory said mapped to ... {super-user} — true,
since the built-in grants everything. This PR turned it into {configure,view}.
That is literally the sentence #6701's warning next to it calls out: "silently
not applied while the commit advisory reports that it was."
So re-wording the fold warning alone — the option I offered — would have left the
contradiction standing in the other message. The lane took the skip option
instead: if _, builtin := LoginClassPermissions[r.class]; builtin { continue }.
Both claims revert to exactly what #6701 already reports, the tolerant path for a
shadowing name is now byte-identical to master, and nothing new is emitted in their
place. The skip is per-NAME, so a non-shadowing class in the same config still folds.
Revert (delete the 3 lines) reds on three independent assertions — the narrowed
MappedPermissions, the false fold claim, and the narrowed advisory — plus a scope
control that reds if the guard is widened into a blanket disable.
That is the right shape of finding: I offered two options, one of which was wrong
for a reason I could not see, and the lane established which by measuring rather
than picking.
MINOR-2 — "one assertion covers all three" confirmed by measurement
One fixture: two limited blocks both carrying deny-configuration, recorded out
of sorted order, with overlapping permission sets. Three separate reverts, each
observed RED on that single comparison — dropped sort gives
deny-configuration / deny-commands; dropped seen dedup gives a repeated
deny-configuration; dropped unionPerms dedup gives
{clear,configure,view,view}. Nothing extra needed.
NIT pinned, and the merge forced a correction the lane made itself
Ordering pinned on the straddle shape; reverting to rejections[len-1] reds naming
beta.
Master's #5561 — the PR I merged earlier today — moved the built-in-first lookup
out of pkg/cli resolveClassPerms into config.ResolveClassPermissions, now shared
with the REST surface's pkg/authz gate. Every comment written this round cited
the old site. Precedence is unchanged and the reader count is still two, so the
fix's premise survives — but the citations were retargeted across four files, and
the premise is now asserted rather than assumed: the test fails with "premise
changed" if a built-in ever stops answering for a shadowing name. All five reverts
were re-run against the merged tree and each still reds.
Converting an assumption into a guard, prompted by a merge that could have silently
invalidated it, is exactly the response I want to that situation.
Union proof — and a correction to MY stated invariant
Only _Log.md conflicted; docs/system-login.md auto-merged (master inserts at
base line 262, this branch's earliest change is base line 285 — no overlap), so the
doc conflict my brief predicted did not materialise at this master tip.
Predicted before resolving, matched after: _Log.md 79188 lines, doc 1542.
Per-heading multiset under inclusion-exclusion holds for all 1581 _Log.md
headings and all 31 doc headings, with no negative predictions. Set comparison
deliberately not used.
My "zero lines deleted both directions" invariant is wrong as stated. It holds
for _Log.md, but on docs/system-login.md this branch legitimately deletes 10
lines from its own earlier rounds. The union is still correct — property 3 accounts
for those 10 exactly: ours->result equals base->theirs (+1963/-0 and +77/-0),
and theirs->result equals base->ours (+95/-0 and +201/-10).
So zero-deletions is a special case that applies only when both sides are pure
insertions. The general invariant is the differs-by-exactly-the-other's-CHANGES
one, which handles deletions correctly. Recorded.
Gates, whole ./..., on the merge result
go build rc=0, go vet rc=0, go test ./... rc=0, 62 packages, zero FAIL,
re-run after the citation commit. gofmt -l clean on every touched file. No smoke
owed — Go control-plane only. #6966 and #6992 untouched.
Gate stands at 2-of-3: Codex cannot review this PR.
1 participant
Advances #5831 — the fail-closed half only. Per-command deny
enforcement stays open on #5831.
The defect
pkg/cli/permissions.gocheckPermissionis coarse: it matches a requiredpermission against the class's mapped bucket set and consults none of the
four regex sub-statements a custom
system login classaccepts. All four were"recognized, not enforced". So
committed cleanly and could still zeroize the box — a config asserting the verb
was denied, on a box where it was allowed. A security restriction accepted as
inert state is strictly worse than a refused one.
Confirmed on
ad9591177:pkg/clihas zero references to any of the fourfields. One correction to the issue's framing —
compiler_system.godoes morethan parse them:
loginClassAdvisoryWarningsalready emitted aWARNING ... MORE PERMISSIVEadvisory (#4304 FIX 2). So this is not "silently ignored", itis "warned about and still ignored". This PR upgrades that warning to a
rejection.
Directionality — the four fields are NOT symmetric
allow-commands,allow-configurationdeny-commands,deny-configurationIgnoring an additive grant cannot widen anything, so rejecting the allow-*
half would break configs that are already safe. Junos precedence cuts the same
way:
allow-commandswins overdeny-commands, so a class pairing them isa deny-with-exceptions — rejecting on deny-presence alone still catches it, and
no allow leaf makes a deny leaf safe to drop.
The gate keys off PRESENCE, not value
deny-commands ""and a valuelessdeny-commandsboth flatten to the emptystring — indistinguishable from an absent leaf, so a
!= ""test accepts them.But an empty POSIX regex matches every command, i.e. denies everything — the
most restrictive thing an operator can write and the most dangerous to drop.
Presence is recorded at parse into
LoginClass.DenyLeavesPresent, inside theexisting switch, which inherits its dual-AST-shape coverage (the leaf carries
its name in
Keys[0]in both the flat-set and hierarchical-block shapes; bothare tested).
Tolerant path: fold to the REPAIR FLOOR
#1960 says a persisted or peer-synced config must still boot, so the rejection
cannot simply be re-run. But a bare warning would leave the runtime fail-open
exactly where it started — such a config still reaches the RBAC gate with its
full permission set and the deny dropped. Warning about a hole is not closing
it. So that path resolves the un-enforceable restriction in the restrictive
direction — bounded by repairability:
Two properties, and the design tension lives between them:
held
PermAll(which subsumes both —checkPermissionreturns nil onPermAllfor every required permission). A class holding neither foldsto the empty set, so this is an intersection, not a grant. That is the
whole difference between a restriction gate and a privilege escalation.
PermConfigis exactly whatrequiredPermissiondemands forconfigure, and config mode applies nofurther per-statement gate (
checkPermissionhas a single call site, on theoperational dispatch path). So
PermConfigis the self-repair channel inthe coarse model.
Everything above the floor is dropped —
PermClear,PermControl,PermMaintand
PermAllitself — so the motivating example (permissions all+deny-commands "request system zeroize") genuinely loses zeroize.Why NOT a view-only collapse — it strands the box
An earlier revision of this PR folded to view-only. That is unrecoverable.
pkg/daemon/daemon_run.goassigns the configured class to any OS user whosename matches a
system login user(if u.Name == osUser { shell.SetUserClass(u.Class) }), androotis a name the username validatoraccepts — account provisioning skips root, the CLI class assignment does not.
So this previously-accepted config binds the console operator to a custom
class:
Before upgrade
noc-adminholds view + configure (the deny is inert). Under aview-only collapse it holds view alone — so root cannot enter
configuretodelete the offending statement, while the strict gate rejects every commit
until it is deleted. A configure-only class collapses to the empty
permission set, which is worse. Recovery would need an out-of-band shell.
The old no-brick argument ("
resolveClassPermsconsults the built-ins first, soconsole super-user access is untouched") does not hold: built-ins-first
lookup only decides which table answers for a class name, not that any actual
login is bound to a built-in. And #1960 covers this shape — a config that
loads but revokes the access needed to repair itself is the same failure
with extra steps.
Note what the operator actually wrote above:
permissions [view configure]plusdeny-configurationmeans configure everything except security policies.Removing all configure is more restrictive than they asked for, in the one
direction that is unrecoverable.
deny-configurationxpf's coarse model has exactly two configuration states — all or none — and
"none" is the state that strands repair. So the tolerant path keeps "all": a
class carrying
deny-configurationcan still edit the configuration it names,until the statement is removed.
This is an honest fail-open with a stated boundary, not an unnoticed one:
the strict gate rejects it outright at commit, so no new config can enter this
state.
places on CONFIGURATION is therefore still NOT in force").
repairableFloorFold's doc comment and a[!WARNING]block in
docs/system-login.md.Per-path configuration RBAC — what Junos actually implements, and what would let
this be enforced properly — stays on #5831.
Recovery path
For an operator who already committed such a config:
viewif it had it).configure(retained by the fold).delete system login class <name> deny-configuration(and/ordeny-commands).commit— the first commit the strict gate lets through.Nothing else about the box can be committed until that statement is gone. That
is the forcing function, not an accident.
Behaviour change worth calling out
A deployment running today with
permissions all+ a deny leaf will see itscustom-class users drop to
{view, configure}after upgrade — losingclear,request, the destructive maintenance verbs, and cleartext secret rendering.That is the fail-closed direction and is what the operator asked for (less than
all), with a loud warning naming the class and the post-fold set.Dropping Clear/Control/Maint even when only
deny-configurationis present isdeliberate over-restriction in a dimension the operator did not restrict: one
rule that always errs restrictive is easier to reason about than a per-leaf
branch, and unlike
configurethose verbs are recoverable from the CLI. Thejustification is in the code at the fold.
The #4304 advisory loses its deny branch
Rather than being reworded: it is unreachable on the strict path now, and on the
tolerant path it would be misleading — the class has been folded more
restrictive on the half
deny-commandstargets, so the old "MORE PERMISSIVE"text would tell the operator the opposite of what happened. (Configuration is
the one half where the restriction genuinely does not bind, and the fold's own
warning says that precisely, which a blanket advisory cannot.) The advisory also
now reports the effective permission set instead of recomputing it (identical
for every unfolded class, so no existing output changes).
Validation
TMPDIR=/tmp go test ./pkg/config/... ./pkg/cli/... ./pkg/configstore/... ./pkg/daemon/...— all six packages ok, exit 0.gofmtandgo vetcleanon every touched file (three pre-existing gofmt-unclean files in
pkg/configwere verified unclean on
origin/mastertoo and left alone).No cluster or smoke validation was run — this change is compile-time and
CLI-gate only.
Mutation proofs
Eight mutations. Each file snapshotted byte-for-byte, mutated, measured, then
restored verbatim and sha256-verified. Every one produced an assertion
failure with
go vetclean — never a build break. Scored from realgo testexit codes, never a piped command.
viewOnlyFoldbody restoredfold STRANDED the box: class "noc-admin" held \configure` before the fold and lost it ([0]), the configure-only case reporting[], and inpkg/clikeepConfigfolded intokeepView(view-only collapse){PermView, PermConfig}BelowFloorIsNotRaised+EmptyStaysEmptyToleratedButFolded, fourNeverWidensarms,BelowFloorIsNotRaisednilRejectedAtCommitarms + hierarchical shape +DenyCommandsRejectedFoldWarningStatesTheRecoveryPathallow-commandsleafGateIsScopedToRestrictiveLeaves+ twoValidStillCommitsarmsM1b is the parent-RED proof that the repair-floor tests bind the fix — the
verbatim pre-PR body, not an approximation. (An approximate view-only collapse
also went RED, but it was more forgiving than the original: it handed a
configure-only class
[view]where the real one gives[]. The verbatim run isthe one reported.)
M5 is the scope proof: the guard fails exactly at the edge a value-based
gate would miss, and nowhere else. M7 is the positive control — valid
classes and the additive allow-* half must still commit, so the rejection tests
cannot be passing by rejecting everything.
Tests
pkg/cli/permissions_login_deny_fold_5831_test.gois the runtime half, driventhrough
Store.SyncApply(the real HA peer-sync ingress, sharingcompileTreeLenientwith boot-from-persisted-DB) and then throughresolveClassPerms→checkPermission. It pins that the folded class reachesconfigureand still loses clear/request/zeroize/cleartext-secrets; that abelow-floor class is not raised to the floor; that an empty class stays
empty; and one test specifically kills the old built-ins-first argument by
proving a custom-named class is answered by the custom table with its folded
permissions.
Docs:
docs/system-login.mdcarries the directionality table, thepresence-vs-value rule, the repair-floor definition, the worked stranding
counter-example, the stated
deny-configurationlimit, and the numberedrecovery procedure.
docs/config-schema.mdrecords the strict/tolerant split.🤖 Generated with Claude Code
https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi