config: close 3 more leaf-complete subtrees to closed-world (#4313) - #4646
Merged
Conversation
…4313) Flip the Phase-2 ESP crypto container `security ipsec proposal <name>` to closedWorld:true, the sibling of the already-closed Phase-1 IKE proposal (#4623). This is another per-subtree closure of the #4313 opt-in schema gap: an unmodeled Junos keyword under an opt-in subtree commits clean and is silently dropped by the compiler. Silent-drop here FAILS OPEN on crypto exactly like the IKE proposal: a fat-fingered `encryption-algorith` or `authentication-algoritm` used to commit clean, the compiler then found no such child, and the ESP SA negotiated WITHOUT the operator's chosen cipher/hash — a silent downgrade believed to be aes-256 while it was not. The flip rejects the typo at strict commit; the tolerant Load/SyncApply path downgrades it to a warning (compileTreeLenient, #1960), so a stored or peer-synced config is never bricked. Leaf-completeness audit (gates the flip; a blanket flip would false-reject valid-but-unmodeled leaves, the #4191 class): the full Junos grammar is exactly protocol, encryption-algorithm, authentication-algorithm, dh-group, lifetime-seconds, lifetime-kilobytes, and description. The first five were already modeled; this change models the two that were missing so the closed subtree does not false-reject a valid proposal carrying them: - lifetime-kilobytes — the ESP volume-based rekey knob that DISTINGUISHES the Phase-2 proposal from the Phase-1 IKE proposal (Phase-1 has none, which is why description alone completed it). It is captured (IPsecProposal.LifetimeKilobytes) but accepted-only: the strongSwan renderer emits rekey_time (seconds) and no rekey_bytes, so volume-based rekey is not yet enforced. ValidateConfig emits an accepted-only advisory (compiler_validate_warn.go) so an operator is not silently misled into believing the SA rekeys on bytes — strictly better than the pre-flip silent commit-and-drop. - description — cosmetic, scalar (a multi-word value must be quoted), compiler-ignored (the IPsec proposal loop has no case for it). The compiler (compiler_ipsec.go, the IPsec proposal loop) reads a subset of the modeled set (protocol / encryption-algorithm / authentication-algorithm / dh-group / lifetime-seconds / lifetime-kilobytes; description is ignored), so closing carries no false-reject risk. Every modeled leaf carries its value on the same statement line (no nested value block in Junos), so closed-world never descends into an AST child of a value leaf in either parser shape. Validation: schema_closedworld_ipsec_proposal_4313_test.go — an unmodeled keyword and each crypto/volume-rekey typo are REJECTED at strict commit (RED on revert of the closedWorld flag, verified by toggling it false); every modeled leaf (including the two added) commits clean, on its own AND combined into a fully-specified proposal (no false-reject); the lenient Load/SyncApply path warns-not-bricks; and the lifetime-kilobytes advisory is asserted. go test ./pkg/config/... green; gofmt/vet/build clean; golden_4406 unaffected (the two new leaves are unused by the corpus). Docs: docs/config-schema.md records the flip and the completeness audit, and removes security ipsec proposal from the "remaining per-subtree flips" list now that both missing leaves are modeled. Addresses #4313. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Flip `security nat nat64` and `security nat natv6v4` to closedWorld:true, continuing the per-subtree closure of the #4313 opt-in schema gap (an unmodeled Junos keyword under an opt-in subtree commits clean and is silently dropped by the compiler). Both are xpf-NATIVE stanzas — Junos does NAT64 via source/destination NAT plus `then static-nat inet`, not this spelling — so their grammar IS exactly what xpf models and compiles. There is no external Junos superset to false-reject (the #4191 class), which makes them leaf-complete by construction: - security nat nat64: the container's only child is `rule-set`, and a rule-set's only children are `prefix` and `source-pool` (both modeled value leaves whose value rides on the same statement line). The compiler (compileNAT64) reads ONLY those two and the struct (NAT64RuleSet) holds ONLY Prefix + SourcePool. closedWorld is set on the nat64 container so it inherits down (the childClosed fold): a typo at the nat64 level (`rulset`) OR under a rule-set (`prefx` / `source-pol`) is rejected. Silent-drop was a real footgun — a typo'd `prefx` left NAT64RuleSet.Prefix empty, validateNAT64PrefixStrict skipped the rule (Prefix == "" -> continue), and NAT64 translation silently did nothing, so IPv6-only clients lost IPv4 reachability with no error. - security nat natv6v4: its entire grammar is the single flag `no-v6-frag-header` (modeled); the compiler reads ONLY that keyword and the struct (NATv6v4Config) holds ONLY NoV6FragHeader. A typo (`no-v6-frag-heder`) previously committed clean and silently left the IPv6 fragment header in translated packets. As with every flip the reject fires only on the strict operator commit path; the tolerant Load/SyncApply path downgrades it to a warning (compileTreeLenient, #1960), so a stored or peer-synced config is never bricked. Validation: schema_closedworld_nat64_4313_test.go and schema_closedworld_natv6v4_4313_test.go — a typo at each closed level is REJECTED at strict commit (RED on revert of the closedWorld flag, verified by toggling it false); every modeled leaf commits clean (no false-reject); the lenient path warns-not-bricks. go test ./pkg/config/... green; gofmt/vet/build clean. Docs: docs/config-schema.md records both flips and their by-construction completeness. _Log.md updated. Addresses #4313. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the #4313 campaign by flipping three additional leaf-complete (or made leaf-complete) subtrees to closedWorld:true, so unknown/typo’d keywords are rejected on the strict commit path instead of being silently dropped by the compiler.
Changes:
- Close
security ipsec proposal,security nat nat64, andsecurity nat natv6v4to closed-world validation inschema_security.go. - Model/capture
security ipsec proposal ... lifetime-kilobytes(plus advisory-only warning viaValidateConfig) to make the IPsec proposal subtree leaf-complete before closing it. - Add RED-on-revert tests for strict reject + lenient “does not brick” behavior; update
docs/config-schema.mdand_Log.md.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/config/types_security.go | Adds IPsecProposal.LifetimeKilobytes to capture the Junos leaf for leaf-completeness + advisory surfacing. |
| pkg/config/schema_security.go | Sets closedWorld:true for security nat nat64, security nat natv6v4, and security ipsec proposal; models missing IPsec proposal leaves. |
| pkg/config/compiler_ipsec.go | Captures lifetime-kilobytes during IPsec proposal compilation. |
| pkg/config/compiler_validate_warn.go | Emits an accepted-only advisory when lifetime-kilobytes is configured. |
| pkg/config/schema_closedworld_ipsec_proposal_4313_test.go | Adds strict-reject / no-false-reject / lenient-no-brick / advisory coverage for IPsec proposal. |
| pkg/config/schema_closedworld_nat64_4313_test.go | Adds strict-reject / no-false-reject / lenient-no-brick coverage for NAT64 closed-world. |
| pkg/config/schema_closedworld_natv6v4_4313_test.go | Adds strict-reject / no-false-reject / lenient-no-brick coverage for NATv6v4 closed-world. |
| docs/config-schema.md | Documents the new closed-world flips and the associated completeness audits. |
| _Log.md | Records the write/edit action per repo logging rules. |
Comment on lines
+49
to
+52
| bad := strings.Fields(typo)[0] | ||
| if !strings.Contains(err.Error(), bad) || !strings.Contains(err.Error(), "closed-world") { | ||
| t.Fatalf("error must name the typo %q and the closed-world subtree, got: %v", bad, err) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #4313 (closes 3 more leaf-complete subtrees; the systematic per-subtree closure continues).
Extends the per-subtree closed-world flips of #4578 (master-password) and #4623 (Phase-1 IKE proposal). The #4313 root cause is the opt-in schema gate (
schema_walk.go): an unmodeled Junos keyword under an opt-in subtree resolves to a nil schema child, commits clean, and is silently dropped by the compiler. A blanket flip is infeasible (it would break the deliberately-lenient accept-with-advisory knobs #2078/#4231 and false-reject valid-but-unmodeled Junos — the #4191 class), so the driveable path is a per-subtreeclosedWorld:trueflip where the subtree is leaf-COMPLETE.Three more subtrees, each after a leaf-completeness audit, one commit per subtree:
1.
security ipsec proposal(Phase-2 ESP crypto) — the sibling of the closed Phase-1 IKE proposal. Silent-drop FAILS OPEN on crypto: a fat-fingeredencryption-algorithused to commit clean and the ESP SA negotiated WITHOUT the operator's cipher. The subtree was INCOMPLETE (Junos allowsdescriptionandlifetime-kilobytes, both unmodeled), so this commit models both FIRST —lifetime-kilobytescaptured (IPsecProposal.LifetimeKilobytes) but accepted-only with aValidateConfigadvisory (the renderer emitsrekey_time, notrekey_bytes), and cosmetic compiler-ignoreddescription— THEN flips the container. The compiler reads a subset of the modeled set, so no false-reject.2.
security nat nat64— an xpf-native stanza (Junos does NAT64 differently), leaf-complete by construction: onlyrule-set→prefix/source-pool, whichcompileNAT64+NAT64RuleSetare the sole readers/holders of.closedWorldon the container inherits down. A typo'dprefxleftPrefixempty,validateNAT64PrefixStrictskipped the rule, and NAT64 silently did nothing — IPv6-only clients lost IPv4 reachability with no error.3.
security nat natv6v4— an xpf-native flag stanza whose entire grammar isno-v6-frag-header. A typo silently left the IPv6 fragment header in translated packets.Each flip fires only on the strict operator commit path; the tolerant Load/SyncApply path downgrades it to a warning (
compileTreeLenient, #1960), so a stored or peer-synced config is never bricked.Validation (per subtree, RED on revert):
schema_closedworld_ipsec_proposal_4313_test.go,schema_closedworld_nat64_4313_test.go,schema_closedworld_natv6v4_4313_test.go— a typo'd/unmodeled keyword under each newly-closed subtree is REJECTED at strict commit naming the keyword + "closed-world" (verified RED by toggling each flag false); every modeled leaf still commits clean (no false-reject; the ipsec test exercises the full leaf set including the two added leaves); the lenient path warns-not-bricks.go test ./pkg/config/...green; gofmt/vet/build clean; golden_4406 unaffected. Docs updated indocs/config-schema.md(each flip + its completeness audit;security ipsec proposalremoved from the remaining-flips list).🤖 Generated with Claude Code
https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi