[multicast] prevent VLAN translation via match key enforcement

Fixes #107.

Stacked on #189.

This adds VLAN-aware NAT ingress matching to prevent cross-VLAN translation.
Previously, a packet arriving with VLAN 100 destined to a multicast group
configured for VLAN 200 would be NAT encapsulated and forwarded, effectively
translating the packet to the wrong customer's network.

NAT ingress table matching (mcast_nat.rs, mod.rs):
- Add Ipv4VlanMatchKey and Ipv6VlanMatchKey that match on destination address,
  VLAN header validity, and VLAN ID
- For groups with VLAN, install two entries: untagged (for decapsulated Geneve
  from underlay) and correctly tagged (for customer packets)
- Packets with the wrong VLAN miss both entries and are not NAT encapsulated

Multicast router VLAN handling (sidecar.p4):
- Strip incoming VLAN tag before routing lookup in MulticastRouter4/6
- forward_vlan action re-adds the group's configured VLAN on egress
- Prevents unintended VLAN translation at the routing stage

Rollback changes:
- Remove dead NAT rollback branches for internal groups (no NAT entries)
- Add rollback support for VLAN changes in NAT and route tables

Counter fix:
- The underlay multicast counter condition was unreachable for packets tagged
  MULTICAST_TAG_UNDERLAY_EXTERNAL that were not decapped. The check for
  == MULTICAST_TAG_UNDERLAY excluded these packets, causing them to fall
  through to the external counter.

Pull Request: #194

Author: zeeshanlakhani

dpd-client/tests/integration_tests/mcast.rs

@@ -612,8 +612,16 @@ control NatIngress (
 	}
 
 	// Separate table for IPv4 multicast packets that need to be encapsulated.
+	// Groups without VLAN match untagged only. Groups with VLAN match both
+	// untagged (for decapsulated Geneve from underlay) and correctly tagged.
+	// Packets with wrong VLAN miss and are not NAT encapsulated.
+	// When hdr.vlan.isValid()==false, vlan_id matches as 0.
 	table ingress_ipv4_mcast {
-		key = { hdr.ipv4.dst_addr : exact; }
+		key = {
+			hdr.ipv4.dst_addr : exact;
+			hdr.vlan.isValid() : exact;
+			hdr.vlan.vlan_id : exact;
+		}
 		actions = { mcast_forward_ipv4_to; }
 		const size = IPV4_MULTICAST_TABLE_SIZE;
 		counters = mcast_ipv4_ingress_ctr;
@@ -630,8 +638,16 @@ control NatIngress (
 	}
 
 	// Separate table for IPv6 multicast packets that need to be encapsulated.
+	// Groups without VLAN match untagged only. Groups with VLAN match both
+	// untagged (for decapsulated Geneve from underlay) and correctly tagged.
+	// Packets with wrong VLAN miss and are not NAT encapsulated.
+	// When hdr.vlan.isValid()==false, vlan_id matches as 0.
 	table ingress_ipv6_mcast {
-		key = { hdr.ipv6.dst_addr : exact; }
+		key = {
+			hdr.ipv6.dst_addr : exact;
+			hdr.vlan.isValid() : exact;
+			hdr.vlan.vlan_id : exact;
+		}
 		actions = { mcast_forward_ipv6_to; }
 		const size = IPV6_MULTICAST_TABLE_SIZE;
 		counters = mcast_ipv6_ingress_ctr;
@@ -1311,7 +1327,9 @@ control MulticastRouter4(
 	apply {
 		// If the packet came in with a VLAN tag, we need to invalidate
 		// the VLAN header before we do the lookup.  The VLAN header
-		// will be re-attached if set in the forward_vlan action.
+		// will be re-attached if set in the forward_vlan action (or
+		// untagged for groups without VLAN). This prevents unintended
+		// VLAN translation.
 		if (hdr.vlan.isValid()) {
 			hdr.ethernet.ether_type = hdr.vlan.ether_type;
 			hdr.vlan.setInvalid();
@@ -1449,7 +1467,9 @@ control MulticastRouter6 (
 	apply {
 		// If the packet came in with a VLAN tag, we need to invalidate
 		// the VLAN header before we do the lookup.  The VLAN header
-		// will be re-attached if set in the forward_vlan action.
+		// will be re-attached if set in the forward_vlan action (or
+		// untagged for groups without VLAN). This prevents unintended
+		// VLAN translation.
 		if (hdr.vlan.isValid()) {
 			hdr.ethernet.ether_type = hdr.vlan.ether_type;
 			hdr.vlan.setInvalid();

dpd/p4/sidecar.p4

@@ -2,7 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/
 //
-// Copyright 2025 Oxide Computer Company
+// Copyright 2026 Oxide Computer Company
 
 /// This module contains the support for reading the indirect counters defined
 /// by the p4 program.  While direct counters are attached to an existing table,
@@ -284,6 +284,7 @@ enum DropReason {
     GeneveOptionsTooLong,
     GeneveOptionMalformed,
     GeneveOptionUnknown,
+    VlanMismatch,
 }
 
 impl TryFrom<u8> for DropReason {
@@ -317,6 +318,7 @@ impl TryFrom<u8> for DropReason {
             23 => Ok(DropReason::GeneveOptionsTooLong),
             24 => Ok(DropReason::GeneveOptionMalformed),
             25 => Ok(DropReason::GeneveOptionUnknown),
+            26 => Ok(DropReason::VlanMismatch),
             x => Err(format!("Unrecognized drop reason: {x}")),
         }
     }
@@ -343,8 +345,8 @@ fn reason_label(ctr: u8) -> Result<Option<String>, String> {
         DropReason::Ipv4TtlExceeded => "ipv4_ttl_exceeded".to_string(),
         DropReason::Ipv6TtlInvalid => "ipv6_ttl_invalid".to_string(),
         DropReason::Ipv6TtlExceeded => "ipv6_ttl_exceeded".to_string(),
-        DropReason::Ipv4Unrouteable => "ipv6_unrouteable".to_string(),
-        DropReason::Ipv6Unrouteable => "ipv4_unrouteable".to_string(),
+        DropReason::Ipv4Unrouteable => "ipv4_unrouteable".to_string(),
+        DropReason::Ipv6Unrouteable => "ipv6_unrouteable".to_string(),
         DropReason::NatIngressMiss => "nat_ingress_miss".to_string(),
         DropReason::MulticastNoGroup => "multicast_no_group".to_string(),
         DropReason::MulticastInvalidMac => "multicast_invalid_mac".to_string(),
@@ -362,6 +364,7 @@ fn reason_label(ctr: u8) -> Result<Option<String>, String> {
             "geneve_option_malformed".to_string()
         }
         DropReason::GeneveOptionUnknown => "geneve_option_unknown".to_string(),
+        DropReason::VlanMismatch => "vlan_mismatch".to_string(),
     };
     Ok(Some(label))
 }

dpd/src/counters.rs

@@ -351,6 +351,7 @@ pub(crate) fn add_group_external(
         scoped_underlay_id.id(),
         nat_target,
         group_info.sources.as_deref(),
+        group_info.external_forwarding.vlan_id,
     );
 
     // Configure external tables and handle VLAN propagation
@@ -656,8 +657,12 @@ pub(crate) fn modify_group_external(
 
     // Create rollback context for external group update
     let group_entry_for_rollback = group_entry.clone();
-    let rollback_ctx =
-        GroupUpdateRollbackContext::new(s, group_ip, &group_entry_for_rollback);
+    let rollback_ctx = GroupUpdateRollbackContext::new(
+        s,
+        group_ip,
+        &group_entry_for_rollback,
+        new_group_info.external_forwarding.vlan_id,
+    );
 
     // Pre-compute normalized sources for rollback purposes
     let normalized_sources = normalize_sources(new_group_info.sources.clone());
@@ -693,10 +698,9 @@ pub(crate) fn modify_group_external(
     updated_group.int_fwding.nat_target = Some(nat_target);
 
     let old_vlan_id = updated_group.ext_fwding.vlan_id;
-    updated_group.ext_fwding.vlan_id = new_group_info
-        .external_forwarding
-        .vlan_id
-        .or(updated_group.ext_fwding.vlan_id);
+    // VLAN is assigned directly -> Some(x) sets VLAN, None removes VLAN
+    updated_group.ext_fwding.vlan_id =
+        new_group_info.external_forwarding.vlan_id;
     updated_group.sources = normalize_sources(
         new_group_info.sources.clone().or(updated_group.sources),
     );
@@ -789,9 +793,11 @@ pub(crate) fn modify_group_internal(
         s,
         group_ip.into(),
         &group_entry_for_rollback,
+        // Internal groups don't have VLANs, so `attempted_vlan_id` is None.
+        None,
     );
 
-    // Internal groups don't update sources - always `None`
+    // Internal groups don't update sources, so always `None`
     debug_assert!(
         group_entry.sources.is_none(),
         "Internal groups should not have sources"
@@ -1182,7 +1188,7 @@ pub(super) fn sources_contain_any(sources: &[IpSrc]) -> bool {
 /// This ensures uniqueness across the group's lifecycle and prevents
 /// tag collision when group IPs are reused after deletion.
 fn generate_default_tag(group_ip: IpAddr) -> String {
-    format!("{}:{}", Uuid::new_v4(), group_ip)
+    format!("{}:{group_ip}", Uuid::new_v4())
 }
 
 /// Multiple representations map to "allow any source" in P4:
@@ -1357,12 +1363,17 @@ fn configure_external_tables(
     add_source_filters(s, group_ip, group_info.sources.as_deref())?;
 
     // Add NAT entry
+    let vlan_id = group_info.external_forwarding.vlan_id;
     match group_ip {
         IpAddr::V4(ipv4) => {
-            table::mcast::mcast_nat::add_ipv4_entry(s, ipv4, nat_target)?;
+            table::mcast::mcast_nat::add_ipv4_entry(
+                s, ipv4, nat_target, vlan_id,
+            )?;
         }
         IpAddr::V6(ipv6) => {
-            table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat_target)?;
+            table::mcast::mcast_nat::add_ipv6_entry(
+                s, ipv6, nat_target, vlan_id,
+            )?;
         }
     }
 
@@ -1688,39 +1699,46 @@ fn update_external_tables(
         add_source_filters(s, group_ip, new_sources.as_deref())?;
     }
 
+    let old_vlan_id = group_entry.ext_fwding.vlan_id;
+    let new_vlan_id = new_group_info.external_forwarding.vlan_id;
+
     // Update NAT target - external groups always have NAT targets
-    if Some(new_group_info.internal_forwarding.nat_target.ok_or_else(|| {
-        DpdError::Invalid("external groups must have NAT target".to_string())
-    })?) != group_entry.int_fwding.nat_target
+    // Also handles VLAN changes since VLAN is part of the NAT match key
+    let new_nat_target =
+        new_group_info.internal_forwarding.nat_target.ok_or_else(|| {
+            DpdError::Invalid(
+                "external groups must have NAT target".to_string(),
+            )
+        })?;
+
+    if Some(new_nat_target) != group_entry.int_fwding.nat_target
+        || old_vlan_id != new_vlan_id
     {
         update_nat_tables(
             s,
             group_ip,
-            Some(new_group_info.internal_forwarding.nat_target.ok_or_else(
-                || {
-                    DpdError::Invalid(
-                        "external groups must have NAT target".to_string(),
-                    )
-                },
-            )?),
+            Some(new_nat_target),
             group_entry.int_fwding.nat_target,
+            old_vlan_id,
+            new_vlan_id,
         )?;
     }
 
-    // Update VLAN if it changed
-    if new_group_info.external_forwarding.vlan_id
-        != group_entry.ext_fwding.vlan_id
-    {
+    // Update route table VLAN if it changed
+    // Route tables use simple dst_addr matching but select forward vs forward_vlan action
+    if old_vlan_id != new_vlan_id {
         match group_ip {
             IpAddr::V4(ipv4) => table::mcast::mcast_route::update_ipv4_entry(
                 s,
                 ipv4,
-                new_group_info.external_forwarding.vlan_id,
+                old_vlan_id,
+                new_vlan_id,
             ),
             IpAddr::V6(ipv6) => table::mcast::mcast_route::update_ipv6_entry(
                 s,
                 ipv6,
-                new_group_info.external_forwarding.vlan_id,
+                old_vlan_id,
+                new_vlan_id,
             ),
         }?;
     }
@@ -1811,7 +1829,11 @@ fn delete_group_tables(
             // (which have NAT targets).
             if group.int_fwding.nat_target.is_some() {
                 remove_ipv4_source_filters(s, ipv4, group.sources.as_deref())?;
-                table::mcast::mcast_nat::del_ipv4_entry(s, ipv4)?;
+                table::mcast::mcast_nat::del_ipv4_entry(
+                    s,
+                    ipv4,
+                    group.ext_fwding.vlan_id,
+                )?;
             }
 
             delete_group_bitmap_entries(s, group)?;
@@ -1825,7 +1847,11 @@ fn delete_group_tables(
             // NAT targets). Internal groups don't have source filtering.
             if group.int_fwding.nat_target.is_some() {
                 remove_ipv6_source_filters(s, ipv6, group.sources.as_deref())?;
-                table::mcast::mcast_nat::del_ipv6_entry(s, ipv6)?;
+                table::mcast::mcast_nat::del_ipv6_entry(
+                    s,
+                    ipv6,
+                    group.ext_fwding.vlan_id,
+                )?;
             }
 
             delete_group_bitmap_entries(s, group)?;
@@ -1863,31 +1889,47 @@ fn update_nat_tables(
     group_ip: IpAddr,
     new_nat_target: Option<NatTarget>,
     old_nat_target: Option<NatTarget>,
+    old_vlan_id: Option<u16>,
+    new_vlan_id: Option<u16>,
 ) -> DpdResult<()> {
     match (group_ip, new_nat_target, old_nat_target) {
-        (IpAddr::V4(ipv4), Some(nat), Some(_)) => {
-            // NAT to NAT - update existing entry
-            table::mcast::mcast_nat::update_ipv4_entry(s, ipv4, nat)
+        (IpAddr::V4(ipv4), Some(new_nat), Some(old_nat)) => {
+            // NAT to NAT - update existing entry (handles VLAN changes internally)
+            table::mcast::mcast_nat::update_ipv4_entry(
+                s,
+                ipv4,
+                new_nat,
+                old_nat,
+                old_vlan_id,
+                new_vlan_id,
+            )
         }
-        (IpAddr::V6(ipv6), Some(nat), Some(_)) => {
-            // NAT to NAT - update existing entry
-            table::mcast::mcast_nat::update_ipv6_entry(s, ipv6, nat)
+        (IpAddr::V6(ipv6), Some(new_nat), Some(old_nat)) => {
+            // NAT to NAT - update existing entry (handles VLAN changes internally)
+            table::mcast::mcast_nat::update_ipv6_entry(
+                s,
+                ipv6,
+                new_nat,
+                old_nat,
+                old_vlan_id,
+                new_vlan_id,
+            )
         }
         (IpAddr::V4(ipv4), Some(nat), None) => {
             // No NAT to NAT - add new entry
-            table::mcast::mcast_nat::add_ipv4_entry(s, ipv4, nat)
+            table::mcast::mcast_nat::add_ipv4_entry(s, ipv4, nat, new_vlan_id)
         }
         (IpAddr::V6(ipv6), Some(nat), None) => {
             // No NAT to NAT - add new entry
-            table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat)
+            table::mcast::mcast_nat::add_ipv6_entry(s, ipv6, nat, new_vlan_id)
         }
         (IpAddr::V4(ipv4), None, Some(_)) => {
             // NAT to no NAT - delete entry
-            table::mcast::mcast_nat::del_ipv4_entry(s, ipv4)
+            table::mcast::mcast_nat::del_ipv4_entry(s, ipv4, old_vlan_id)
         }
         (IpAddr::V6(ipv6), None, Some(_)) => {
             // NAT to no NAT - delete entry
-            table::mcast::mcast_nat::del_ipv6_entry(s, ipv6)
+            table::mcast::mcast_nat::del_ipv6_entry(s, ipv6, old_vlan_id)
         }
         _ => Ok(()), // No change (None → None)
     }
@@ -1934,33 +1976,49 @@ fn update_internal_group_bitmap_tables(
 /// Only updates the external bitmap entry since that's the only bitmap
 /// entry created during group configuration. The underlay replication
 /// is handled separately via the ASIC's multicast group primitives.
+///
+/// # Arguments
+///
+/// * `s` - Switch instance for table operations.
+/// * `group_ip` - IP address of the multicast group.
+/// * `external_group_id` - ID of the external multicast group for bitmap updates.
+/// * `members` - Group members used to recreate port bitmap.
+/// * `current_vlan_id` - VLAN currently in the table (may be the attempted new VLAN).
+/// * `target_vlan_id` - VLAN to restore to (the original VLAN).
 fn update_fwding_tables(
     s: &Switch,
     group_ip: IpAddr,
     external_group_id: MulticastGroupId,
     members: &[MulticastGroupMember],
-    vlan_id: Option<u16>,
+    current_vlan_id: Option<u16>,
+    target_vlan_id: Option<u16>,
 ) -> DpdResult<()> {
     match group_ip {
-        IpAddr::V4(ipv4) => {
-            table::mcast::mcast_route::update_ipv4_entry(s, ipv4, vlan_id)
-        }
-        IpAddr::V6(ipv6) => {
-            table::mcast::mcast_route::update_ipv6_entry(s, ipv6, vlan_id)
-                .and_then(|_| {
-                    // Update external bitmap for external members
-                    // (only external bitmap entries exist; underlay replication
-                    // uses ASIC multicast groups directly)
-                    let external_port_bitmap =
-                        create_port_bitmap(members, Direction::External);
-                    table::mcast::mcast_egress::update_bitmap_entry(
-                        s,
-                        external_group_id,
-                        &external_port_bitmap,
-                        vlan_id,
-                    )
-                })
-        }
+        IpAddr::V4(ipv4) => table::mcast::mcast_route::update_ipv4_entry(
+            s,
+            ipv4,
+            current_vlan_id,
+            target_vlan_id,
+        ),
+        IpAddr::V6(ipv6) => table::mcast::mcast_route::update_ipv6_entry(
+            s,
+            ipv6,
+            current_vlan_id,
+            target_vlan_id,
+        )
+        .and_then(|_| {
+            // Update external bitmap for external members
+            // (only external bitmap entries exist, underlay replication
+            // uses ASIC multicast groups directly)
+            let external_port_bitmap =
+                create_port_bitmap(members, Direction::External);
+            table::mcast::mcast_egress::update_bitmap_entry(
+                s,
+                external_group_id,
+                &external_port_bitmap,
+                target_vlan_id,
+            )
+        }),
     }
 }