diff --git a/CLAUDE.md b/CLAUDE.md index 1a4e04e..748ce7d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -236,6 +236,85 @@ Batfish runs in Docker container `batfish` (image `batfish/allinone`), exposing > missing ``/`` question (#80) are unaffected by this and > remain open. + > **UPDATE, 5 September.** The rest of #78 was picked up without waiting + > for Senaka's answers to the four questions now written out in Discussion + > #294, on the reasoning that a decision genuinely blocked on him should + > stay blocked, but nothing else should sit idle in the meantime. That + > split the remaining work into two very different piles. + > + > **The blast radius, fixed.** A single rule failing to convert for ANY + > reason (missing or unsupported ``, an unsupported `` + > value, an unresolved alias, a bad port, an unresolvable network + > reference) used to abort the entire file, not just the interface or rule + > it was on. At the client's real scale — 1,998 XML elements against this + > project's 54-element fixture — one anomaly anywhere meant zero analysis + > of anything. Re-scoped the same way the DHCP-WAN/undeclared-VPN case was: + > the one failing rule is excluded and named, everything else converts + > exactly as if it were never there. This required no guess about what any + > field means — every refusal still names the same cause it always did, + > only its blast radius shrank from "the whole file" to "this one rule". + > + > **``'s semantic question, still untouched.** #80 itself says + > "do not fix this before the client answers" — that instruction stands, + > unconditionally. What DID move: missing `` now gets its own precise + > message, split from present-but-invalid the same way the two interface + > causes were split earlier, because there is no safe default for + > pass/block/reject the way "any" is one for protocol. ``'s + > existing default-to-`"any"` behaviour was not touched at all. + > + > **NAT, reaffirmed rather than left stale.** Checked against the same + > "no guessing" bar the blast-radius fix cleared: it does not clear it. + > There is no version of "convert the NAT rule" that does not invent + > translation semantics this module has never observed. Detection and + > exclusion (#264) is unchanged, and `docs/design/pfsense-nat-support.md` + > records that this was revisited today, not simply never revisited. + > + > A separate idea — disclosing an inferred `` with a comment, + > #80's own preferred Option C, which changes no behaviour, only makes the + > existing silent default visible — was drafted as a question for the team + > rather than built solo, precisely because it touches the one issue + > marked blocked on the client. Not yet decided. + + > **UPDATE, 6 September — the blast-radius fix has a direction, and one + > direction is dangerous.** @SamikaPerera's review on #302 found what the + > entry above did not name: excluding a rule changes what the emitted ACL + > decides, and the two directions are not equally safe. + > + > **Skipping a `block` rule makes the emitted ACL MORE permissive than the + > real device.** Netwise then reports traffic as reachable that the real + > firewall denies — noisy, but safe, because the false finding gets + > investigated and dismissed. + > + > **Skipping a `pass` rule makes the emitted ACL STRICTER than the real + > device**, and this is the direction that matters. A policy rule asserting + > *"this traffic must be denied"* can then report `none` (clean) against + > the emitted ACL, while the real firewall actually permits that traffic — + > a real policy violation, reported as a pass. That is F-4's exact failure, + > reached through the converter instead of through a check: the report says + > "we checked and it is fine" about something nobody actually verified. + > + > Not new IN KIND — #104's interface-level skip made the identical trade, + > accepted at the time. What changed is conspicuousness: a whole excluded + > interface is visible in the skip list as a named gap; a single dropped + > `pass` rule inside an otherwise-normal ACL is not, unless someone reads + > the skip notes and reasons through what they mean for a specific policy + > rule. + > + > **Fixed, not just documented**: `analysis/coverage.py` and + > `analysis/report.py` (PR #302) now carry the skip list through to the + > downloaded report as its own "excluded during conversion" section, and + > the report's coverage statement stops claiming "nothing was skipped" + > when a conversion excluded a rule, even when every check ran clean. That + > closes the disclosure gap Samika also found — before this, the skip list + > reached the user exactly once, at upload, and was gone by the time a + > report was generated. It does not close the underlying modelling gap + > above: a skipped `pass` rule is still excluded, not modelled, and a + > policy check can still report `none` where the real firewall would not. + > Carrying skips into F-1 findings directly (an `error` card per skip, + > visible on the dashboard itself, not only in a downloaded report) was + > raised as the more complete fix and deliberately left for a separate, + > properly scoped change rather than folded into this one -- see #306. + ## 7a. The finding format (F-1) — the one contract **`docs/finding-format.md` is authoritative.** It was agreed by all four team diff --git a/analysis/coverage.py b/analysis/coverage.py index 074a895..6922730 100644 --- a/analysis/coverage.py +++ b/analysis/coverage.py @@ -11,11 +11,46 @@ * status="error" means Netwise could not check something. That row must remain visible even when the main finding sections are skimmed, exported, or sorted elsewhere. + +CONVERSION GAPS ARE A SEPARATE THING FROM A CHECK GAP, AND THEY ARE PASSED IN +SEPARATELY RATHER THAN INFERRED (#302 review, @SamikaPerera) + A finding-based gap means a CHECK could not run. A conversion gap means + part of the SOURCE FILE was never turned into anything a check could see + at all -- today, a pfSense rule `analysis/pfsense_convert.py` could not + convert (#78). Neither this module nor its caller can derive that from + the findings list, because a converter-excluded rule leaves no finding + behind to derive it from -- that is exactly the gap. + + Before this parameter existed, `summarise()` had no way to know a + conversion had dropped anything, so a config with every CHECK running + cleanly reported "Every reported check ran. Nothing was skipped." even + when real rules from the uploaded file were never analysed. Not false + about the checks -- every one of them did run -- but read by a person as + a single claim, and the wrong one: skipping a `pass` rule makes the + emitted config STRICTER than the real device, which can make a policy + check that should report a violation report `none` instead. See + `CLAUDE.md` section 7 for the direction analysis. A reader who trusted + "nothing was skipped" would have no way to know that risk exists for + this specific report. + +`None` MEANS SOMETHING DIFFERENT FROM `()`, AND FROM A NON-EMPTY LIST +(#302 review, @ARSH871-bot) + `web.main._staged_pfsense_skips()` returns three distinct things: `[]` + when it VERIFIED nothing was excluded, a non-empty list when it read + real skip notes, and `None` when a skip record exists but could not be + read -- truncated, wrong type, unparseable. Before this distinction + existed, an unreadable record and a verified-empty one both collapsed to + `[]` here, so a corrupted skip file produced the exact same "nothing was + skipped" claim as a config that genuinely excluded nothing. Passing + `None` through lets this function tell a reader the true thing: not + "nothing was skipped", but "whether anything was skipped could not be + determined" -- and `complete` is `False` in that case for the same + reason it is `False` when something genuinely was skipped. """ from __future__ import annotations -from typing import Any, Dict, Iterable, List, Tuple +from typing import Any, Dict, Iterable, List, Optional, Sequence, Tuple def _label(finding: Dict[str, Any]) -> Tuple[str, str]: @@ -25,9 +60,24 @@ def _label(finding: Dict[str, Any]) -> Tuple[str, str]: ) -def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]: +def summarise( + findings: Iterable[Dict[str, Any]], + conversion_gaps: Optional[Sequence[str]] = (), +) -> Dict[str, Any]: """Return the coverage/certainty view implied by a findings list. + `conversion_gaps` is optional and additive -- omitting it reproduces this + function's exact prior behaviour, so every existing caller keeps working + unchanged. Pass it when the findings came from a converted config and the + converter excluded anything, so the statement below can stop claiming + nothing was skipped when something genuinely was -- just not something a + check could have reported on. + + Pass `None` rather than `()` when the CALLER genuinely does not know + whether anything was excluded -- see this module's own docstring for why + that is a third, distinct state rather than the same thing as "verified + empty". + The result is intentionally plain dicts, matching the rest of the analysis layer's F-1 data. It is derived, not cached; if the findings change, the coverage statement changes with them. @@ -59,6 +109,10 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]: {"check": check, "device": device} for check, device in sorted(checked) ] + conversion_gaps_unreadable = conversion_gaps is None + conversion_gap_rows = ( + [] if conversion_gaps is None else [str(note) for note in conversion_gaps] + ) if not checked and not gaps: # THE THIRD STATE, AND IT IS NOT COMPLETENESS. @@ -80,6 +134,36 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]: f"Findings may be incomplete." ) complete = False + elif conversion_gaps_unreadable: + # THE CASE #302's REVIEW (ROUND TWO, @ARSH871-bot) FOUND. Every + # check ran and none of them reported a blind spot, but the record + # of what a converter may have excluded before any check ran could + # not be read -- truncated, wrong type, unparseable. Complete is + # False here for the same reason it is False when the record names + # a real exclusion below: this report cannot say analysis was + # exhaustive when it does not know whether part of the source file + # was dropped before any check ever saw it. + statement = ( + "Every reported check ran, but the record of what a converter " + "may have excluded before any check ran could not be read. " + "Whether anything was excluded is unknown, so this report " + "cannot claim the analysis is complete." + ) + complete = False + elif conversion_gap_rows: + # THE CASE #302's REVIEW FOUND. Every check ran and none of them + # reported a blind spot -- but that is a claim about the checks, + # not about the file. Complete is False here on purpose: this + # report cannot say analysis was exhaustive when part of the + # source file was excluded before any check ever saw it. + one = len(conversion_gap_rows) == 1 + statement = ( + f"Every reported check ran, but {len(conversion_gap_rows)} part" + f"{'' if one else 's'} of the uploaded configuration could not " + f"be converted and {'was' if one else 'were'} not included in " + f"this analysis." + ) + complete = False else: statement = "Every reported check ran. Nothing was skipped." complete = True @@ -89,4 +173,6 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]: "statement": statement, "checked": checked_rows, "gaps": gaps, + "conversion_gaps": conversion_gap_rows, + "conversion_gaps_unreadable": conversion_gaps_unreadable, } diff --git a/analysis/pfsense_convert.py b/analysis/pfsense_convert.py index f42f2a8..ff5184d 100644 --- a/analysis/pfsense_convert.py +++ b/analysis/pfsense_convert.py @@ -195,6 +195,11 @@ "likely a named alias group" ), "unsupported_rule_type": "a rule's is not pass, block or reject", + "missing_rule_type": ( + "a rule's element is absent -- cannot tell whether it is " + "meant to pass, block or reject, and guessing either direction " + "would misstate what the rule does" + ), "unsupported_protocol": ( "a rule's is not tcp, udp, icmp or any -- combined forms " "like 'tcp/udp' need two Cisco ACL lines and are not handled" @@ -730,7 +735,13 @@ def _check_rule_order_is_unambiguous( def _rule_to_acl_line(rule_el: ET.Element, interfaces: Dict[str, Dict[str, str]]) -> str: """Convert one PF Sense element into one Cisco extended-ACL line.""" pf_type = _text(rule_el, "type") - action = _PFSENSE_TO_CISCO_ACTION.get(pf_type or "") + # Absent gets its own message, distinct from present-but-invalid (#78 + # item 4) -- " is None" was the same confusing string either way, + # and the two are different questions: one is a value this module does + # not recognise, the other is a value that was never written at all. + if pf_type is None: + raise PfSenseConversionError(REFUSALS["missing_rule_type"]) + action = _PFSENSE_TO_CISCO_ACTION.get(pf_type, None) if action is None: raise PfSenseConversionError( f"{REFUSALS['unsupported_rule_type']}: is {pf_type!r}" @@ -974,15 +985,52 @@ def convert(xml_path: Union[str, Path]) -> ConversionResult: # module still refuses rather than guesses, because the general case # -- some rules quick, some not, in no particular pattern -- does not # reduce to a single reordering the way the all-or-nothing cases do. + # SKIPPED, NOT REFUSED (#78 item 4, re-scoped the same way item 1/2 were + # in #104): a rule that _rule_to_acl_line() cannot convert -- missing or + # unsupported , an unsupported value, a named alias + # where an address or port is expected, an out-of-range port, an + # unresolvable network reference -- used to abort THIS ENTIRE FILE, not + # just the interface it was on or the rule itself. At the client's real + # scale (1,998 XML elements against this project's 54-element fixture, + # per CLAUDE.md section 7), one anomaly anywhere meant zero analysis of + # anything. That is the same shape #104 fixed for an unmodellable + # interface, just one level deeper -- so it gets the same fix: exclude + # the one rule, name the real reason in `skipped`, convert everything + # else exactly as if it were never there. + # + # This changes NOTHING about what any field is interpreted to mean -- + # every REFUSALS message below still names the same cause it always did. + # It only shrinks the blast radius of that cause from "the whole file" + # to "this one rule". In particular this does NOT touch 's + # existing default-to-"any" behaviour, which is issue #80's own open + # question (blocked on the client) and is not this change's to answer. acl_lines_by_role: Dict[str, List[str]] = {} for role, rules in rules_by_role.items(): - if not any(_is_quick(r) for r in rules): - rules = list(reversed(rules)) - acl_lines_by_role[role] = [_rule_to_acl_line(r, interfaces) for r in rules] - continue - lines_for_role = [_rule_to_acl_line(r, interfaces) for r in rules] - _check_rule_order_is_unambiguous(rules, interfaces, lines_for_role) - acl_lines_by_role[role] = lines_for_role + no_quick_rules_at_all = not any(_is_quick(r) for r in rules) + # Position in a skip note is counted from ORIGINAL document order, + # before any reversal -- so the number matches what a human reading + # the XML top to bottom would count, not this function's internal + # processing order. + original_order = rules + ordered = list(reversed(rules)) if no_quick_rules_at_all else rules + + kept_rules: List[ET.Element] = [] + kept_lines: List[str] = [] + for r in ordered: + try: + kept_lines.append(_rule_to_acl_line(r, interfaces)) + except PfSenseConversionError as error: + position = original_order.index(r) + 1 + skipped.append( + f"rule {position} of {len(original_order)} on {role!r} " + f"could not be converted: {error} -- skipped, not converted" + ) + continue + kept_rules.append(r) + + if not no_quick_rules_at_all: + _check_rule_order_is_unambiguous(kept_rules, interfaces, kept_lines) + acl_lines_by_role[role] = kept_lines lines: List[str] = [f"hostname {hostname}", "!"] for role, info in interfaces.items(): @@ -1004,7 +1052,15 @@ def convert(xml_path: Union[str, Path]) -> ConversionResult: for role in interfaces: acl_name = _acl_name(role, rule_bearing_roles=modelled_roles) lines.append(f"ip access-list extended {acl_name}") - if role in acl_lines_by_role: + # TRUTHINESS, NOT MEMBERSHIP (#78 item 4). A role can now be a key in + # acl_lines_by_role with an EMPTY list -- every one of its rules + # failed to convert and was skipped -- which used to be impossible: + # before the per-rule skip above, a role either had every rule + # convert or the whole file aborted, so a present key always meant + # real content. `role in acl_lines_by_role` alone would then emit an + # ACL with no lines in it at all, not even a fail-closed deny -- the + # exact fail-OPEN inversion the deny-all fallback exists to prevent. + if acl_lines_by_role.get(role): lines.extend(f" {line}" for line in acl_lines_by_role[role]) else: lines.append(" deny ip any any") diff --git a/analysis/report.py b/analysis/report.py index a1eac34..c56c335 100644 --- a/analysis/report.py +++ b/analysis/report.py @@ -321,15 +321,26 @@ def _section_html(title: str, lede: str, items: List[Dict], css_class: str, return f'{head}

{html.escape(lede)}

{body}' -def _coverage_html(findings: Sequence[Dict[str, Any]]) -> str: +def _coverage_html( + findings: Sequence[Dict[str, Any]], + conversion_gaps: Optional[Sequence[str]] = (), +) -> str: """The first-class coverage statement (#235), derived from status values. It is not a fourth finding section. It is the reading aid that says whether the finding sections are complete, and if not, exactly which blind spots remain. Removing the gap list is the mutation #235 names: the report would still have findings, but no longer name what was not checked. + + `conversion_gaps` (#302 review) is rendered as its OWN list, never merged + into `gaps` -- a check that could not run and a rule a converter excluded + before any check ran are different claims, and merging them would lose + which one happened. Passing `None` (#302 review, round two) is a THIRD + claim again, distinct from an empty list -- see `coverage.summarise()`'s + own docstring -- and is rendered as its own warning line, not silently + folded into "nothing was skipped". """ - summary = coverage.summarise(findings) + summary = coverage.summarise(findings, conversion_gaps) css = "complete" if summary["complete"] else "incomplete" esc = html.escape @@ -372,14 +383,38 @@ def _coverage_html(findings: Sequence[Dict[str, Any]]) -> str: f'

Checked: {esc(names)}.

' ) + if summary["conversion_gaps_unreadable"]: + parts.append( + '

The record of what a converter ' + "excluded before analysis could not be read. Whether " + "anything was excluded is unknown.

" + ) + elif summary["conversion_gaps"]: + parts.append( + '

Excluded during conversion, ' + "before any check ran:

" + ) + parts.append("
    ") + for note in summary["conversion_gaps"]: + parts.append(f"
  • {esc(note)}
    • ") + parts.append("
") + parts.append("") return "".join(parts) def render_html(findings: Sequence[Dict[str, Any]], source: Optional[str] = None, - generated_at: Optional[str] = None) -> str: - """A complete, self-contained HTML report. No external files, no scripts.""" + generated_at: Optional[str] = None, + conversion_gaps: Optional[Sequence[str]] = ()) -> str: + """A complete, self-contained HTML report. No external files, no scripts. + + `conversion_gaps` (#302 review) names anything a converter (pfSense's, + today) excluded before this findings list ever existed -- optional, and + omitting it reproduces this function's exact prior output. `None` (#302 + review, round two) is a third, distinct state -- "we do not know whether + anything was excluded" -- see `coverage.summarise()`'s own docstring. + """ s = _sections(findings) when = generated_at or _now() subject = source or "an uploaded configuration" @@ -396,17 +431,44 @@ def render_html(findings: Sequence[Dict[str, Any]], ) # "Nothing was skipped" is a COVERAGE claim, and it is only true when - # something was actually reported. With an empty findings list it says - # nobody-looked in the words of everything-ran -- the same F-4 confusion - # the coverage box above was fixed for, in a sentence that predates it. - # One claim, two places; both now read from the same condition. + # something was actually reported AND nothing was excluded before any + # check ran (#302 review -- conversion_gaps is exactly that second + # condition, which this sentence did not know about before). With an + # empty findings list it says nobody-looked in the words of + # everything-ran -- the same F-4 confusion the coverage box above was + # fixed for, in a sentence that predates it. One claim, two places; both + # now read from the same conditions. + # + # `conversion_gaps is None` MUST BE CHECKED BEFORE THE PLAIN TRUTHINESS + # TEST BELOW (#302 review, round two, @ARSH871-bot). `None` and `()` are + # both falsy, so `if conversion_gaps:` alone cannot tell "we verified + # nothing was excluded" from "we do not know" -- and folding the second + # into the `elif findings:` branch below would print "Every check ran. + # Nothing was skipped." while the actual answer is unknown, the exact + # false-completeness claim this round of review exists to remove. + if conversion_gaps is None: + blind_empty_text = ( + "Every check that ran found nothing, but the record of what a " + "converter may have excluded before any check ran could not be " + "read -- see Coverage and certainty above." + ) + elif conversion_gaps: + blind_empty_text = ( + "Every check that ran found nothing, but part of the uploaded " + "configuration was excluded before any check saw it -- see " + "Coverage and certainty above." + ) + elif findings: + blind_empty_text = "Every check ran. Nothing was skipped." + else: + blind_empty_text = "No check reported a result, so nothing is known either way." + blind = _section_html( "Could not check", "These checks did not run. Nothing is known about what they cover. " "This is not a pass.", s["blind"], "blind", - ("Every check ran. Nothing was skipped." if findings - else "No check reported a result, so nothing is known either way."), + blind_empty_text, warn=True) problems = _section_html( @@ -429,7 +491,7 @@ def render_html(findings: Sequence[Dict[str, Any]], f"

Netwise analysis report

" f'

{html.escape(subject)} · generated ' f'{html.escape(when)}

' - f"{counts}{_coverage_html(findings)}{blind}{problems}{clean}" + f"{counts}{_coverage_html(findings, conversion_gaps)}{blind}{problems}{clean}" '