Repository navigation
[dpd,dpd-api,dpd-types] make external multicast constraints self-enforcing - #396
zeeshanlakhani wants to merge 24 commits into
Conversation
This work constrains group and SSM source validation based on a variety of specs, linked throughout the PR. Previously, DPD was accepting groups and (S, G) sources that no packet could actually exercise, including ones that can never leave a host/link or, worse, sources a switch can't forward. Here's the list (the first classifier one was the most obvious): | address | verdict | spec | |--------------------------------|---------|-------------------------------| | ff3x::/32 in ff30::/12 only | is SSM | RFC 4607 §1; RFC 3306 is ASM | | ff3x:: group ID > 0x40000000 | accept | RFC 4607 §4.3 | | v6 scopes other than 4/5/8/e | reject | RFC 7346; no Ethernet realm | | class E (240.0.0.0/4) source | reject | RFC 6890 "Source: False" | | 0.0.0.0/8 source | reject | RFC 6890 "Forwardable: False" | | 100.64.0.0/10 source | accept | RFC 6598, operator-internal | | source/group family mismatch | reject | (local choice) | | 232.0.0.0 (SSM null) | reject | RFC 4607 §4.3 | | v4-mapped/compat v6 source | reject | RFC 4291 §2.5.5, aliases IPv4 |
The external-group docs say “IPv4 and non-admin-local IPv6 multicast traffic,” but external groups can use admin-local IPv6 too. The exception, however, is the `ff04::/64` underlay range, which is reserved for the underlay mcast group mapped from the external mcast group. This makes the information explicit. The old wording is in a blessed spec, so this needs a new API version: `MCAST_EXTERNAL_SCOPE_DOCS`.
…rcing This began as a normal, ol' doc fix. The external multicast group API endpoints had documented scope that didn't match the handlers. Group and source validation already ran in the handlers, but this moves the checks into the request types themselves, at the deserialization boundary, and revisits the address rules for consistency. This includes relaxing the IPv6 scope rules from the earlier validation branch, preserving flexibility vs forcing some kind of extra Oxide-driven policy. Then, this review question came along: > Could these documented constraints also be made self-enforcing via > newtypes? Your decision whether doing so is worth the API churn and > code complexity, but imo it's worth considering. My response to this grew into newtypes for group addresses, sources, and NAT targets; plus, ASM/SSM request variants. The rollback work and cleanup account for much of this diff. Creation requests now check group/source constraints during deserialization. Updates validate addresses at parse time; source-family and SSM validations still run against the group address within the handler(s). Group addresses (typed as `ExternalMulticastIp`): | Group addr / condition | Outcome | Spec / qualification | | --- | --- | --- | | Not multicast | Reject | Group addresses must be multicast. | | `224.0.0.0` | Reject | RFC 1112 §4: "guaranteed not to be assigned to any group", not a MUST NOT; rejected because it can never name a group. | | `232.0.0.0` | Reject | RFC 4607 §4.3: SSM null address. | | `232.0.0.1`–`232.0.0.255` | Accept | RFC 4607 §4.3: reserved for IANA allocation. | | `232/8` | SSM | RFC 4607 §1. | | `ff0x::`, with all-zero group ID | Reject | RFC 4291 §2.7.1: "reserved and shall never be assigned to any multicast group"; rejected on the same reasoning as `224.0.0.0`. | | IPv6 scope `0` | Reject | RFC 4291 §2.7: reserved; packets must be discarded. | | IPv6 scopes `1`, `2` | Reject | RFC 4291 §2.7: interface-local and link-local traffic cannot be forwarded past those scopes. | | IPv6 scopes `3`–`f` | Accept, subject to other checks | RFC 4291 §2.7; RFC 7346. Scope `f` is reserved; received packets are treated as global scope. | | `ff04::/64` | Reject | Oxide underlay reservation. | | Rest of `ff04::/16` | Accept, subject to other checks | Outside the reserved `/64`. | | Flags ~ `R=0`, `P=T=1`, `plen=0`, either `X` value | SSM | RFC 4607 §1; RFC 7371 §4.1.2: `ff3x` and `ffbx` flag patterns; reserved fields ignored. | | IPv6 SSM with nonzero network prefix | Accept | RFC 4607 §1: a system should treat all of `ff3x::/32` as SSM. RFC 3306 §6 requires a zero prefix only of the node that's forming the address. | | `ff3x::4000:0` | Reject | RFC 4607 §4.3: reserved SSM null address. | | IPv6 SSM group IDs `0`–`3fff:ffff` | Accept | RFC 4607 §1: invalid as SSM addresses (assigned with `P=T=0`, RFC 3307 §4.1), but a router *may* drop them, so we don't reject them at the API. | | IPv6 SSM group IDs `4000:1`–`7fff:ffff` | Accept | RFC 3307 §4.2; RFC 4607 §4.3: IANA allocation range. | | IPv6 SSM group IDs `8000:0`–`ffff:ffff` | Accept | RFC 3307 §4.3.1, §4.3.2 and RFC 10028 §3: dynamic range where host SSM allocation is `f000:0`–`fcff:ffff`. | | Flags ~ `P=1`, `T=0` | Reject | RFC 7371 §4.1.1: `P` requires `T`. | | Flags ~ `R=1` without both `P` and `T` | Reject | RFC 7371 §4.2.2: `R` requires `P` and `T`. | | Flags ~ `R=1` with `plen` outside range `1`–`64` | Reject | RFC 3956 §4: embedded-Rendezvous Point (RP) prefix-length bounds. | | Flags ~ `R=1` with `RIID=0` (RP Interface ID) | Reject | RFC 3956 §6.3: zero RIID is excluded. | | Flags ~ `R=1` with derived RP in `::/16`, `fe80::/10`, or `ff00::/8` | Reject | RFC 3956 §4: unusable embedded RP address. | | Prefix-based `ff2` bits | Ignored | RFC 7371 §4.1.1, §4.2.1: reserved bits ignored on receipt. | | Prefix-based, `R=0`: `rsvd` bits | Ignored | RFC 3956 §3: reserved bits ignored on receipt. | | Flags ~ `R=1`: low `rsvd` nibble becomes `RIID` | Checked | RFC 7371 §4.2.2; RFC 3956 §6.3: reject `RIID=0`. | | IANA assignment or an embedded-prefix scope comparison | Not checked | No registry lookup or scope comparison with the embedded prefix. | Exact sources (typed as `ExactSource`) and request constraints: | Exact source / condition | Outcome | Spec / qualification | | --- | --- | --- | | Multicast source | Reject | RFC 1112 §6.2; RFC 4291 §2.7: multicast addresses cannot be packet sources. | | `0.0.0.0`, `::` | Reject | RFC 1812 §5.3.7 and RFC 6890 for `0.0.0.0`, as with `0.0.0.0/8` specified below; RFC 4291 §2.5.2: an unspecified IPv6 source is never forwarded. | | `127/8`, `::1` | Reject | RFC 1122 §3.2.1.3; RFC 4291 §2.5.3: loopback sources cannot be forwarded outside the host. | | `255.255.255.255` | Reject | RFC 1122 §3.2.1.3: broadcast addresses cannot be sources. | | `169.254/16`, `fe80::/10` | Reject | RFC 3927 §2.7; RFC 4291 §2.5.6: link-local traffic cannot be forwarded off-link. | | `0.0.0.0/8` | Reject | RFC 1122 §3.2.1.3 permits initialization use; RFC 1812 §5.3.7 recommends against forwarding these sources. | | `240/4` | Reject | RFC 1112 §4: reserved Class E space; the special-purpose registry marks `Source=False` (RFC 6890 §2.2.2). | | `::ffff:0:0/96`, `::/96` | Reject | RFC 6890 §2.2.3 marks the mapped form `Source=False`; RFC 4291 §2.5.5.1 deprecates the compatible form. This matches OPTE. | | `100.64/10` | Accept | RFC 6598 §4: shared address space is usable within an operator's network. | | Source family differs from group family | Reject | Same-family `(S,G)` check; matches OPTE. | | SSM create or supplied update source list: no exact source, or `Any` | Reject | RFC 4607 §2: SSM requires a source-specific channel. | Includes: - Exact-source rejection set and same-family checks match OPTE `Include` subscriptions. - In the new API version, omitted sources on the update/modification preserve the source filter, while the older endpoints convert this omission to `Any` (wildcard), which SSM will reject. - External groups use caller-supplied addresses. No IANA-assignment lookup or embedded-prefix scope check occurs there. - Regarding rollback, `DpdError::Unwind` now collects multiple rollback failures. HTTP errors preserve the initial status and use a special `ROLLBACK_FAILURE_ERROR_CODE` in propagation. Restoration can re-add missing bitmap, replication, and route entries. - `chaos`-feature tests inject ASIC failures and check usage counts after rollback, including failures during the rollback op itself. - `dpd-client` now maps `ExternalMulticastIp` and `UnderlayMulticastIpv6` to the `dpd-types` newtypes (via progenitor `replace`), allowing clients to reject bad addresses when parsing, before a request goes out.
…cleanup Our SSM validation now matches oxidecomputer/dendrite#396, which is the updated read on the RFCs for both group and source checking. We move the interface lookups onto `IfAddrs`/`IfAddr`, and add tests for source parsing and the interface-name handling. Also, we do a full run over the documentation to make this match up with the current stack of PRs going into prod.
We (now) reject a source-less join on an address classified as SSM at CLI validation, and require `--multicast-iface` alongside `--multicast-source` for an SSM join to always pin to a specific interface. Classification uses 232.0.0.0/8 for IPv4, and `ff3x::/32` or `ffbx::/32` with a zero prefix length for IPv6 (RFC 4607 §1, RFC 7371 §4.1.2). We also check SSM destinations the way dendrite does as of oxidecomputer/dendrite#396. Only the SSM null addresses (232.0.0.0, ff3x::4000:0, RFC 4607 §4.3) and IPv6 scopes 0-2 get rejected (dendrite loosened its earlier, stricter rules, for more flexibility and a closer RFC read). `--multicast-source` has to be an address that can actually send forwarded traffic (e.g., no broadcast, link-local, 0/8, 240/4, or v4-mapped/compatible v6). Otherwise, the join would just sit there receiving nothing, and that looks like a network regression. The SMF manifest's start method had pointed at `mgd_method_script.sh`, a copy-paste path nothing installs to, so the service flapped between incomplete and running. It now points at `method_script.sh` (matching `package-manifest.toml`), and the script is reworked to spawn one joiner per configured multicast group (SSM encoded as `group@src1,src2`). Multicast joins are pinned through a single `multicast_iface` property. The binary accepts an interface name or bound IP address and resolves it to the IPv4 address or IPv6 interface index required for each join. Interface- and link-local IPv6 groups also use this index as their bind scope. `--scope` is now solely for unicast, and the `ipv6_scope` property has been removed. The service idles when no groups are configured. If any joiner exits, the service restarts through the `ctrun` contract and every membership is re-joined.
9af13a9 to
dff8591
Compare
This also includes `PartialEq` and `Eq` derives on multicast error enums, allowing tests to use `assert_eq!` on the full error.
dff8591 to
6366b61
Compare
This is the first of a series of recut PRs split from #675/#696. Validated address types now live in client-common (`Vni`, `OverlayMulticast`, `UnderlayMulticastIpv6`). We'll eventually get to the great big shared oxnet typed world :)! MRIB route types in mg-api-types-versions point toward the upcoming v13. While the API version itself is not declared here, we'll be working with this version throughout the upcoming split set. Group validation is essentially ported from dendrite oxidecomputer/dendrite#396: RFC-based (and flexible) ruleset(s) plus the internal, Oxide-specific `ff04::/64` underlay reservation. Proptests can be found in `impls/mrib/` and import `crate::latest::mrib`. The proptest suite is differential: `spec_*` fns encode the RFC rules and the Oxide underlay reservation, and the properties check that the constructors agree, error variant(s) included. Includes: - Fixed: in test-proptest.sh, the `rdb`/`bgp` blocks copied regressions to paths that the output rule never could match, and copies were skipped on failures as well; the output rule is now `**/*` over per-crate subdirs. - Running client-common unit tests in CI.
This is the first of a series of recut PRs split from #675/#696. Validated address types now live in client-common (`Vni`, `OverlayMulticast`, `UnderlayMulticastIpv6`). We'll eventually get to the great big shared oxnet typed world :)! MRIB route types in mg-api-types-versions point toward the upcoming v13. While the API version itself is not declared here, we'll be working with this version throughout the upcoming split set. Group validation is essentially ported from dendrite oxidecomputer/dendrite#396: RFC-based (and flexible) ruleset(s) plus the internal, Oxide-specific `ff04::/64` underlay reservation. Proptests can be found in `impls/mrib/` and import `crate::latest::mrib`. The proptest suite is differential: `spec_*` fns encode the RFC rules and the Oxide underlay reservation, and the properties check that the constructors agree, error variant(s) included. Includes: - Fixed: in test-proptest.sh, the `rdb`/`bgp` blocks copied regressions to paths that the output rule never could match, and copies were skipped on failures as well; the output rule is now `**/*` over per-crate subdirs. - Running client-common unit tests in CI.
While adding multicast group/source checks at deserialization time in
Maghemite, we realized it'd be preferable to share the validators and
source types from `dpd-types-versions`. This also lets us drop the
associated `dropshot` and `omicron-common` deps.
A few of these types may eventually belong in oxnet (commented), but most
do not. These stay here, and the validation rules get versioned and
imported like any other API type. The versioning of certain types,
upstream, is a crucial part of this effort.
Also includes:
- This places the 24-bit Vni max bound in updated schema. This needs
versioned NAT and attached-subnet types.
- `InvalidIpv6Ssm` has been folded into `ReservedSsmNull`.
- Introduces proptests covering the group, source, tag and VNI
validators against oracles written from the RFCs
- This also adds a buildomat job `test-proptest.sh` to run proptests @
`PROPTEST_CASES=1000000`, akin to Maghemite.
- doc fixes: non-linked-text RFC citations on schema types
- api changes:
- v1 `Vni::new` now returns an `Result` (vs an `Option`).
- `mcast::Error` -> `UnderlayMulticastError` and `*ParseError` splits.
… `testing` feat Maghemite had its own copies of the source/underlay/VNI generators and the ASM/SSM group strategies, which re-encoded dendrite's rules. Now, we transport tht work here behind a `testing` feature gate.
bff09af to
6708a26
Compare
cfzimmerman
left a comment
There was a problem hiding this comment.
This PR is large enough that I didn't close-review everything. Please lmk if you'd like me to take a look at any particular changes or if you decide to break this into separate PRs.
Otherwise, I'll approve with the understanding that we'll talk through some of the open questions. And since this is the last open PR I see, here are my thoughts on the stack in total:
You have done tremendous work for multicast and have fixed some legit dendrite tech debt along the way. While there's feedback along the margins, this is fantastic, and the goal imo is to validate and merge on a reasonable timeline.
Your CI tests already establish solid confidence. I think the remaining pieces are system-level validation. For both voxel and a rack(lette), we should probably test both multicast and existing network functionality pretty aggressively. Perhaps have an agent write up a torture test suite looking for regressions and bugs? The goal is just to smoke out any possible regressions or subtle issues.
Finally, I'd like to plan out how this will work with R24 and other dendrite PRs. In my understanding, once this merges, we can't uprev omicron's dendrite for other projects until the omicron side of multicast merges. Is that correct? If so, then we should probably synchronize all the dependent multicast merges, right? Otherwise an important bug or security fix could get stranded behind landing multicast in omicron. Please lmk if I'm misunderstanding this.
I appreciate that getting this all merged will require a lot of coordination. Please lmk if you want to discuss or need support landing this stack.
Includes: - Collapses `Ipv4Nat`/`Ipv6Nat` into `IpNat<T>`, while keeping schema structure, which led to the removal of dpd's `Reservation` assoc type. - 224.0.0.0/24 is rejected again as `LocalIpv4Scope`.
This comes from my extensive comment in #396 (comment). When an update fails partway through, dpd tries to restore the previous forwarding config before returning the error. We still hold the previous config and the mcast lock at that point, so if the restore goes through, existing traffic recovers right away instead of waiting on the next Nexus/ddmd pass. That matters for join/leave semantics: one instance's failed join shouldn't cut traffic to members already subscribed, and a failed leave should resort back to membership that was actually programmed (not something in between). A single membership update touches PRE membership, the decap bitmap, and the replication entry. External retargeting can also hit source filters, routing/NAT, and the bitmap VLANs on the old and new underlay groups. It's a lot. The external group shares its underlay group's ASIC IDs (1:1), the writes have an ordering, and references constrain when things can be freed. set/clear makes each write safe to repeat, but if step 4 of 6 fails we still have to decide what happens to 1-3. Nexus's mcast RPW owns group config and ddmd owns membership from peer subscriptions. Both keep their wanted state and retry; the error codes just let them tell failures apart if/when they want to. If the unwind itself fails, it's best effort: it keeps going through the remaining independent undo steps, logs, and returns a rollback-failure error. We keep the group IDs and any NAT target reservation and mark the group for repair. The next request that touches it repairs it (ASIC membership readback for underlay groups where the backend supports it). If repair is blocked, GETs and external writes return 503 (repair pending, w/ Retry-After); list still answers from stored config, and delete always works.
This began as a normal, ol' doc fix (I've kept the history here, including #364, which has been closed). The external multicast group API endpoints had documented scope that didn't match the handlers. Group and source validation already ran in the handlers, but this moves the checks into the request types themselves, at the deserialization boundary, and revisits the address rules for consistency. This includes relaxing the IPv6 scope rules from the earlier validation branch, preserving flexibility vs forcing some kind of extra Oxide-driven policy.
Then, this review question came along:
My response to this grew into newtypes for group addresses, sources, and NAT targets; plus, ASM/SSM request variants. The rollback work
and cleanup account for much of this diff.
Creation requests now check group/source constraints during
deserialization. Updates validate addresses at parse time; source family and SSM validations still run against the group address within the handler(s).
Group addresses (typed as
ExternalMulticastIp):224.0.0.0232.0.0.0232.0.0.1–232.0.0.255232/8ff0x::, with all-zero group ID224.0.0.0.01,23–ffis reserved; received packets are treated as global scope.ff04::/64ff04::/16/64.R=0,P=T=1,plen=0, eitherXvalueff3xandffbxflag patterns; reserved fields ignored.ff3x::/32as SSM. RFC 3306 §6 requires a zero prefix only of the node that's forming the address.ff3x::4000:00–3fff:ffffP=T=0, RFC 3307 §4.1), but a router may drop them, so we don't reject them at the API.4000:1–7fff:ffff8000:0–ffff:fffff000:0–fcff:ffff.P=1,T=0PrequiresT.R=1without bothPandTRrequiresPandT.R=1withplenoutside range1–64R=1withRIID=0(RP Interface ID)R=1with derived RP in::/16,fe80::/10, orff00::/8ff2bitsR=0:rsvdbitsR=1: lowrsvdnibble becomesRIIDRIID=0.Exact sources (typed as
ExactSource) and request constraints:0.0.0.0,::0.0.0.0, as with0.0.0.0/8specified below; RFC 4291 §2.5.2: an unspecified IPv6 source is never forwarded.127/8,::1255.255.255.255169.254/16,fe80::/100.0.0.0/8240/4Source=False(RFC 6890 §2.2.2).::ffff:0:0/96,::/96Source=False; RFC 4291 §2.5.5.1 deprecates the compatible form. This matches OPTE.100.64/10(S,G)check; matches OPTE.AnyIncludes:
Includesubscriptions.Any(wildcard), which SSM will reject.DpdError::Unwindnow collects multiple rollback failures. HTTP errors preserve the initial status and use a specialROLLBACK_FAILURE_ERROR_CODEin propagation. Restoration can re-add missing bitmap, replication, and route entries.chaos-feature tests inject ASIC failures and check usage counts after rollback, including failures during the rollback op itself.dpd-clientnow mapsExternalMulticastIpandUnderlayMulticastIpv6to thedpd-typesnewtypes (via progenitorreplace), allowing clients to reject bad addresses when parsing, before a request goes out.