Repository navigation
ADR-0013: supersede 0002 with the rule set as implemented - #227
Merged
Merged
Conversation
#104 asked for a superseding ADR naming the third inter-VLAN rule and stating the current total. Producing it meant reading the enforced ruleset rather than recounting the prose, and the total turned out to be the wrong thing to ask for. The third rule is `10.0.99.20 -> 10.0.30.10:161/udp`, SNMP-Exporter scraping the iLO, added with the monitoring stack and never recorded because it was understood as wiring up a scrape target rather than as a change to the segmentation design. But counting rules cannot describe this firewall. What a segment reaches is decided by the ordered interaction of its blocks, its catch-all, and which interface a rule sits on: * Default deny holds for 99, 30, 40, 20 and 10 — each blocks every other segment explicitly before its egress rule. * It does not hold for Hicks. Hicks blocks 40, 20 and 10 and then passes to `any`, so it reaches all of Winterfell and all of ImaginationLAN on every protocol and port. No rule grants that and no rule denies it. network.md has recorded it correctly the whole time ("Internet, 99, 30"); ADR-0002 and security.md described something narrower than what is deployed. * Nor for the switch LAN, which still carries pfSense's stock "Default allow LAN to any" and reaches every segment while network.md said "Nothing". * One explicit rule matches nothing: "Allow Hicks access to ImaginationLAN" sits on the ImaginationLAN interface, where Hicks traffic never arrives. The access is real; it comes from the catch-all above it. ADR-0008's two rules are not part of this — they are tracked as decided-but-not-built in #102 and the firewall is correctly in the pre-0008 state. Verified from 10.0.99.20: 10.0.50.1, 10.0.30.1, 10.0.20.1 and 10.0.10.1 all unreachable, both SNMP exceptions polling healthily, no floating rules, so per-interface first-match is the whole story. ADR-0002 is marked Superseded and otherwise untouched, per ADR-0001. Its reasoning is unchanged; only its claim about the rule set is retired. README and security.md now point at the list instead of restating a total, and README's ADR count was already wrong in two places (eleven and twelve, against twelve files). The ineffective rule and the two broad paths are left as they are. Narrowing Hicks means enumerating what it legitimately needs and would break management access and the lab if got wrong; that is a decision of its own, not a documentation fix. Closes #104 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are still factual consistency issues in the updated documentation (e.g., README runbook count and ambiguous/inconsistent port specs in ADR-0013) that should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a new ADR that supersedes ADR-0002 by documenting inter-segment reachability as actually implemented (including implicit access via catch-all rules), and updates surrounding documentation to reference the new list-based source of truth instead of repeating a potentially misleading “rule count”.
Changes:
- Add ADR-0013 describing segment reachability per source segment (including implicit paths and an ineffective rule).
- Mark ADR-0002 as superseded (keeping original reasoning intact) and update README/security/network docs to point to ADR-0013.
- Move #104 to Done in the roadmap and fix README’s ADR count references to 13.
File summaries
| File | Description |
|---|---|
| README.md | Updates highlights to link to ADR-0013 and corrects ADR count references. |
| docs/security.md | Replaces “count of exceptions” prose with a list-based reference to ADR-0013 and clarifies where default deny does/doesn’t hold. |
| docs/roadmap.md | Moves #104 to Done with a detailed rationale summary. |
| docs/network.md | Corrects LAN reachability to match deployed pfSense behavior and adds a clarifying footnote. |
| docs/adr/0013-segment-access-as-implemented.md | New ADR documenting reachability as implemented and superseding ADR-0002’s rule-count claim. |
| docs/adr/0002-vlan-segmentation-strategy.md | Marks ADR-0002 as superseded by ADR-0013 while leaving original decision text unchanged. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+67
to
+70
| | 99 | `10.0.99.20 → 10.0.30.10:161-162/udp` | SNMP-Exporter scrapes the iLO — **this is the third rule #104 was about** | | ||
| | 99 | `10.0.99.20 → 10.7.7.2:161-162/udp` | SNMP-Exporter scrapes the switch | | ||
| | 30 | `10.0.30.10 → 10.0.99.20/udp` | the iLO's return path | | ||
| | LAN | `10.7.7.2 → 10.0.99.20:161-162/udp` | the switch's return path | |
Comment on lines
+73
to
75
| - **Documented decisions and runbooks.** Thirteen ADRs covering what was chosen | ||
| and what was rejected — including the costs accepted knowingly; twelve | ||
| runbooks for the operations that are easy to get wrong at 1am. |
ADR-0013 recorded that Hicks reaches all of Winterfell and ImaginationLAN and that the switch LAN reaches every segment. Recording a finding and tracking nothing about it is the state roadmap.md exists to prevent — an accepted gap with no issue behind it reads, six months later, as one somebody decided was fine. Both are postures rather than defects, so neither belongs in the ADR as a decision. #228 is the one that needs deciding before #102 adds rules to the same segment; #229 is lower risk because getting it wrong costs SNMP polling of neo, and that is monitored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #104.
The issue asked for a superseding ADR naming the third inter-VLAN rule and stating the current total. Producing it meant reading the enforced ruleset instead of recounting the prose — and the total turned out to be the wrong thing to ask for.
The third rule
10.0.99.20 → 10.0.30.10:161/udp— SNMP-Exporter scraping the ProLiant's iLO. Added with the monitoring stack, never recorded, because it was understood as wiring up a scrape target rather than as a change to the segmentation design.Why counting was the wrong instrument
What a segment reaches is decided by the ordered interaction of its block rules, its catch-all, and which interface a rule sits on:
any. There is no block for 30 or 99, so Hicks reaches all of both, on every protocol and port. No rule grants that and no rule denies it.network.mdhas had this right the whole time ("Internet, 99, 30"); ADR-0002 andsecurity.mddescribed something narrower than what is deployed.10.7.7.0/24still carries pfSense's stock Default allow LAN to any and reaches every segment, whilenetwork.mdsaid it reaches "Nothing".vlan50 → vlan30) sits on the ImaginationLAN interface, where Hicks traffic never arrives — pfSense evaluates on the interface a packet enters. The access is real; it comes from the catch-all above it.ADR-0008's two rules are not part of this. They are tracked as decided-but-not-built in #102, and the firewall is correctly in the pre-0008 state — so its "five" was never a contradiction.
Verification
pfctl -sronmorpheuswith interface tables resolved, cross-read againstconfig.xml. No floating rules, so per-interface first-match (quick) evaluation is the whole story. From10.0.99.20:10.0.50.1,10.0.30.1,10.0.20.1,10.0.10.1all unreachable; both SNMP exceptions (shivaon VLAN 30,neoon LAN) polling healthily.Changes
docs/adr/0013-segment-access-as-implemented.md— reachability per source segment, plus the six explicit passes, the two implicit paths, and the rule that matches nothing.Supersededand otherwise untouched, per ADR-0001. Its reasoning is unchanged; only its claim about the rule set is retired.README.md,docs/security.md— point at the list instead of restating a total. README's ADR count was already wrong in two places ("Eleven" at :71, "12" at :168, against 12 files); both now say 13.docs/network.md— the LAN row said "Nothing"; now footnoted with what the ruleset actually does.docs/roadmap.md— Write a superseding ADR: 0002 says two inter-VLAN rules, there are three #104 to Done.Deliberately not done
The ineffective rule and the two broad paths are left in place. Narrowing Hicks means enumerating what it legitimately needs, and would break management access and the lab if got wrong — that is a decision of its own, not a documentation fix. Deleting the dead rule while writing the document explaining why it does nothing is how the next surprise gets built.
make validatepasses.🤖 Generated with Claude Code