fix(flow-filter): verify a masquerade destination, do not resolve it - #1689
Closed
daniel-noland wants to merge 8 commits into
Closed
Conversation
The context property tests stop at a `LookupResult`. The step after it -- turning that result into the destination and NAT flags every downstream NF acts on -- had only hand-written coverage, so the mapping was never checked against a configuration nobody chose. `nf_metadata_matches_config_oracle` closes that hop. It runs generated packets through the real NF and compares the stamped metadata against a prediction derived from `context::fuzz::oracle_lookup` -- the same oracle the context suite uses, now `pub(crate)` so the config's meaning of a route stays stated in one place. Only the `LookupResult` -> metadata step is restated. Scope is flowless packets: bypass, invalidation and the reply paths are not functions of the configuration and are covered elsewhere. Verified by mutation: swapping the static src/dst flags, dropping the masquerade-destination check, attaching the flow key whenever NAT is stateful rather than only alongside static NAT, and dropping a source miss with the wrong `DoneReason` all fail the test. Along the way, two fixes the oracle forced out: `ManifestSpec::strip_nat` was over-strict. Config forbids stateful NAT only opposite *stateful* NAT -- `validate_nat_combinations` says outright that "no NAT or static NAT only is compatible with all other modes on the other side", and `static_nat_plus_masquerade_context` in this very file relies on it. The generator stripped *all* NAT from the far side, so masquerade-opposite-static-NAT peerings never appeared in any generated overlay. That is the only shape producing a route that needs both stateful and static NAT, which is the sole case where the NF retains a flow key -- so the flow-key path was unreachable to every property test in the crate. Downgrading to static NAT instead of flattening to plain makes it reachable: the new test now records ~54k flow-key routes per 30s run, and coverage counters assert it stays that way. The comment above `validate_nat_combinations` claimed it rejects "NAT (static or stateful)" on the far side, which contradicts the table and the code directly beneath it. That reading is the most likely source of the generator's mistake, so correct it. Also adds v6 UDP and ICMPv6 packet builders, so a v6 probe is realized as the packet it describes. ICMPv6 carries a different next header than ICMPv4; the probe is adjusted to match whatever the built packet really carries, so the oracle is never asked about a packet that could not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ader stacks `classify` is the only place this NF reaches into a packet's headers, and every test above it fed exactly four hand-built shapes: v4 TCP, UDP, ICMP, and a bare Ethernet frame. No VLAN tag, no IPv6 extension chain, no fragment, no authentication header, and none of the fuzzed remainder of any header's fields. `net` already ships a bolero header-stack generator; nobody had pointed it at this NF. `arbitrary_header_stacks_uphold_the_config_contract` runs ten stack shapes through the real NF and asserts the stamped metadata equals what the config oracle predicts for the key that stack presents. Addresses are steered into the configured prefixes (a uniformly random address never lands in one, and the run would collapse into destination misses) while every other field stays fuzzed. What the test claims is bounded, and the doc comment says so: `probe_from_packet` extracts the key with the same accessors `classify` uses, so this is not an independent check of that extraction. Its value is that no generated stack panics, the fail-closed invariants hold on shapes nobody picked, and the route -> metadata mapping holds for keys only exotic stacks produce. Coverage counters insist those keys are reached: a typical 1s run sees ~1600 portless IP packets and ~4400 non-transport protocol numbers, neither reachable from the four hand-built shapes. One behaviour it turned up -- an IPv6 extension header masking the transport protocol -- is surprising enough to deserve stating rather than leaving for a reader to infer from a passing property test, so it is pinned separately alongside the other such edges. Requires net's "bolero" feature in dev-dependencies for the generator. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…tables as fans
Every expose the generator emitted was a single block -- one `/24`, one
`/32`. Real exposes carry `not` / `not_as` exclusions, which validation
subtracts and normalizes away, so a `ValidatedExpose` reaching the table
builder is normally a *fan* of disjoint prefixes of differing lengths. The
generator never produced one, and the whole crate leans on that shape:
`rule_priority` orders purely by prefix length.
Add an `ExcludeSel` to each generated expose. Punching a single host out of
a `/24` expands it to prefixes of every length from `/25` to `/32` at once,
which is as hard as the length ordering gets.
Constraints the exclusions have to respect, all encoded and commented:
- Port forwarding forbids exclusions outright, so only the masquerade
half of the Masquerade*PortFw pairs takes one.
- Static NAT requires equal address counts on both sides, so exclusions
are applied symmetrically to the private and public blocks.
- Every variant stays in the block's upper half, because
`derive_routing_probes` aims its guaranteed-routing probes at host `.1`
and `FW_HOST` sits in the lower half -- so neither the derived probes
nor the deliberate nested-overlap cases are disturbed.
`exclusions_reach_the_config_as_multi_length_prefix_fans` asserts the fan
actually materializes (widest spread must reach 8), because an exclusion
that validation collapsed back into its block would leave every test above
it quietly passing on the single-prefix exposes it had before.
Value shown by mutation rather than argued: truncating `RuleSet::from_overlay`
to lower only the *first* prefix of each expose is caught in ~100 iterations
with exclusions on, and passes cleanly with them off. That is the same class
of blind spot the ACL cross-product had -- when every input has one element,
a lowering that drops all but the first is the identity.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r state All three are behaviours the fuzz suites hit thousands of times per run and none could describe: a property test says "the NF agreed with the config", not "and here is what the config means". All three read like bugs until explained, which is exactly why they should be tests rather than folklore. `ipv6_extension_header_masks_the_transport_protocol`: `Net::next_header()` reports the IPv6 header's own next-header field, so any extension header puts *its* number there -- while `try_transport()` still walks the chain and finds the real ports. TCP behind a hop-by-hop header therefore presents `(proto = HOPOPT, ports = the TCP ports)`. A protocol-restricted expose lowers to an exact match on the protocol byte and cannot match it, so the traffic is dropped rather than forwarded. Fail-closed, but a functional gap: legitimate TCP over IPv6 carrying extension headers cannot reach a TCP-restricted port-forwarding destination. The test pins both the mechanism and the consequence, and shows the same packet routing through an unrestricted expose to confirm the protocol byte is what excluded it. `portless_packet_cannot_match_a_port_restricted_expose`: a packet with no usable transport ports looks up with port 0, and config forbids port 0 in an expose's ranges, so a port-restricted expose can never match one. Not exotic -- ICMP, non-first fragments, and (by the above) anything behind an IPv6 extension header all present portless keys. Fail-closed, but it makes port-forwarded destinations unreachable to that traffic, including the ICMP errors path MTU discovery needs. The test shows the same packet routing through an unrestricted expose, so the port is demonstrably what excluded it. `flow_from_a_newer_generation_is_honored_for_bypass`: `dst_vpcd_from_valid_flow` rejects only `flow_genid < genid`, so a flow tagged with a generation this worker has not yet observed short-circuits the tables. That is the transient its comment describes -- the control plane has stamped flows with the new generation before this worker reads it, and treating them as stale would tear down every flow the new config just blessed. It was the one direction of the genid comparison nothing covered. The test aims at a destination the tables do not cover, so honouring the flow is observable rather than incidental. The first was found by the adversarial-header suite added two commits back; grouping it here keeps the three fail-closed edges stated in one place, and the portless test's doc comment already cross-references it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`table_name` builds `flow_filter_{base}_{seq}` with a process-global decimal
counter, and rte_acl rejects a context name over 31 bytes:
dpdk build: "invalid ACL context name: ACL context name is too long
(32 > 31 bytes)"
Today's bound holds only by accident of how the names happen to be spelled.
The 12-byte prefix plus the longest current base (`remote_v4`, 9 bytes)
leaves nine digits, so the ceiling is ~10^9 and effectively unreachable --
but it scales with the base, and a base four bytes longer would cut it by
four orders of magnitude, to a counter value a long-lived dataplane really
does reach. The counter advances once per table built, so a process that
rebuilds its tables on every configuration change gets there; and when it
does, *every* subsequent table build fails. Reconfiguration stops working
with the last good tables left in place.
Make the bound hold for every value the counter can take rather than for the
values today's names happen to permit. A 3-byte prefix, a base capped at 9
bytes, and a hexadecimal counter (16 bytes at most for a `u64`) give at most
29 bytes. `table_names_fit_the_rte_acl_limit_for_every_counter_value`
asserts this against `u64::MAX` rather than against the counter's current
value, which is the property the decimal scheme lacked, and a `debug_assert`
catches an over-long base at its call site.
This lands ahead of the masquerade table added later in this series, whose
base is longer than any existing one -- so the constraint is stated before
anything leans on it, rather than after.
Context names are internal to rte_acl's registry -- nothing renders or
persists them -- so renaming is safe.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`FlowFilterContext::lookup` is the readable per-packet oracle the property suites run against `lookup_batch`. Its v4 and v6 arms were the same thirty lines twice over -- stage 1 (destination -> `Verdict`), then stage 2 (source -> source NAT) for a hit -- differing only in which pair of typed tables they reached into. Factor that body into `lookup_one<I>`, taking the `Query<I>` the batched path already builds for exactly these fields. No behaviour change: the two arms were identical before and call one function after, and the suite that cross-checks this path against `lookup_batch` is unchanged. This is a prelude. The next commit adds a third lookup stage, and doing it against two copies would state the new sequence twice and leave the two free to drift -- in the one place whose whole job is to be the simple statement the batched path is checked against. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Config models the destination -> VPC relation as one-to-many. In
`config/src/external/overlay/vpcrouting.rs`, `VpcRouteTable` is
`destination prefix -> VpcRouteSet`, and `VpcRouteSet` is a `Vec`:
struct VpcRouteSet(Vec<VpcRoute>);
`VpcRoute::can_overlap` deliberately exempts masquerade/masquerade, so two
peers may masquerade behind one public range and both claim the same
destination prefix.
Stage 1 of the flow-filter lowers that relation into a *function*:
`RemoteKey -> Verdict`, and `Verdict` holds exactly one `dst_vpcd`. Two
peers sharing a range therefore produce two rules with identical match sets
and identical priorities differing only in the destination VPC, and the
collapse is settled by tie-break order -- stable-sort position under the
reference backend, and formally unspecified under rte_acl, so the two
backends need not even agree.
`apply_route` then required the packet's flow to agree with that arbitrary
winner, so every masquerade reply toward the *losing* peer was dropped and
its flow cancelled. Deterministic, but arbitrary: one of the two peers was
simply broken.
The root cause is that a masquerade public address is not a routable
identifier. It names a NAT pool. It can never accept a new connection, so
the only legitimate traffic to it is reply traffic, which by construction
rides a flow -- and the flow is the only thing that can distinguish two
connections to the same address. State is primary for this traffic class;
the table can only ever be a plausibility check.
So stop asking the table an unanswerable question. "Which VPC owns
20.0.0.5?" has no unique answer. "Does vpc3 masquerade 20.0.0.5 toward me?"
does. Stage 3 (`MasqueradeKey`) carries `dst_vni` as an *input*: the flow
supplies the candidate, the table says whether the configuration agrees.
- `LookupInput` carries `flow_dst_vpcd`, so verification stays on the
batched path with the other stages rather than becoming a per-packet
lookup in the NF.
- `LookupResult::MasqueradeDestination(Option<VpcDiscriminant>)` replaces
`Route` with a masquerade `dst_nat`. `Some` means verified; `None` means
no candidate, or one config rejects. There is no source NAT to report --
such a packet rides a flow that already carries its own state.
- Stage 2 and stage 3 are mutually exclusive, so each rte_acl call still
asks exactly one question and the burst still makes three calls.
The property oracle changes shape to match: a stage-1 match becomes
`StageOne`, whose `Masquerade` variant deliberately carries no VPC. That is
the same claim the tables now make, stated where the oracle can hold it --
and it is what lets two overlapping masquerade exposes compare *equal*, so
`consider` can accept that tie as benign while still failing on a tie whose
candidates disagree. Without it the oracle would have to assert that no two
exposes ever tie, which is exactly the thing config permits.
Note what this does NOT weaken. An earlier proposal was to take the
destination straight from the flow; that is wrong, and it is why this is a
verification and not a substitution. The old equality check was doing double
duty -- a broken disambiguator *and* a working staleness check -- so
removing it outright would have let a flow naming a since-unpeered VPC
forward traffic there. Asking the configuration answers both at once:
`masquerade_reply_is_refused_when_the_flow_names_the_wrong_peer` pins the
staleness half, and the pre-existing `masquerade_reply_with_mismatched_flow_
destination_is_filtered` now passes for a stronger reason (config does not
agree that VPC owns the address, rather than a tie-break disagreeing).
`masquerade_replies_reach_both_peers_sharing_a_public_range` is the
regression test: both peers' replies now work, where one was always dropped.
Reverting the verification to the old equality rule fails it.
Two test-visible consequences of the model change, both intended:
- `dst_side_nat_modes` asserted a masquerade destination "resolves as a
marker" with a destination VPC. It no longer resolves at all; the test
now pins all three verification outcomes.
- Derived routing probes at a masquerade destination never route, so they
are split into `masquerade_probes` carrying the correct candidate and
asserted to verify. That keeps stage 3's *hit* path exercised -- a
verification table matching nothing would satisfy every "must drop"
assertion in the suite (~13k masquerade destinations per 30s run).
Cross-peering overlaps are still absent from the generated overlays, so the
property suite does not yet catch this class on its own; that follows in the
next commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The previous fix made a masquerade destination something the tables verify
against the flow's candidate rather than resolve from the address. Nothing
in the generated overlays exercised why: the prefix pool gives every expose
its own block, so two peers never shared a masquerade range and every
destination stayed unambiguous. Mutating the verification back to the old
"flow must equal stage 1's arbitrary winner" rule was caught only by the
hand-written regression test.
`OverlaySpec::shared_masquerade_pool` builds the shape. Plain-masquerade
exposes translate into one range shared across the whole overlay, so peers
of the same VPC advertise overlapping masquerade destinations -- the
one-to-many relation config permits and `VpcRouteSet` already models.
Two things this needed, each a lesson about the generator:
- Left to chance the overlap appeared in well under 1% of overlays, since
it needs two peerings of one VPC to *both* draw a plain masquerade
expose. Within the mode it is now arranged: each peering's remote side
leads with masquerade and the local side is stripped of stateful NAT
(config forbids it on both sides, and normalization would otherwise undo
the masquerade). ~14k ambiguous destinations per 60s run.
- No probe could reach the shared pool. It sits outside the `0..blocks`
range probe selectors are reduced into, so the ambiguous destinations
were built and then never looked up. `ProbeSpec::dst_shared_masquerade`
aims at it.
The oracle needs nothing further: the preceding commit already dropped the
VPC from `StageOne::Masquerade`, so two overlapping masquerade exposes
compare equal and `consider` treats the tie as benign rather than as the
ambiguity it fails on.
The combined-masquerade variants keep their own block: they exist to pin the
nested and equal-length overlaps, which moving their public side would
dissolve.
Mutation results, all four combinations:
- both lookup paths reverted -> `reference_lookup_matches_config_oracle`
fails in ~400 iterations, and the rte_acl differential fails
- batched path only -> `batched_lookup_matches_single_lookup` fails, which
is what keeps the two paths from drifting apart
- either -> the hand-written regression test fails
Before this commit only the last of those held.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
daniel-noland
added
dont-merge
Contributor
|
Superceded by #1691 |
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
2 participants
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.
ai generated, undergoing extra review by human coauthor, don't review yet