On 2/4/26 4:20 PM, Dumitru Ceara wrote: > On 2/3/26 8:34 AM, Ales Musil via dev wrote: >> The RFC defines a Virtual Router Redundancy Protocol [0], in order >> for that protocol to work the workload might "spoof" MAC address >> within ARP or ND request/response. This wasn't allowed as the port >> security is specifically designed against spoofing and checks if >> the port security MAC address is the same for source of ARP/ND >> and the inner source/target address. To make the port security >> compliant add an special literal which when specified will allow >> user to add any/all MAC addresses defined by VRRPv3. The traffic >> from and to those additional MAC addresses will be allowed as >> well as permutations of ARP/ND inner MACs combined with the >> physical MAC as a source. >> >> [0] https://datatracker.ietf.org/doc/html/rfc9568 >> Reported-at: https://issues.redhat.com/browse/FDP-2979 >> Signed-off-by: Ales Musil <[email protected]> >> Acked-by: Jacob Tanenbaum <[email protected]> >> --- >> v2: Rebase on top of latest main. >> Update the RFC url. >> Add Jacob's ack. > > Hi Ales, > > Thanks for the patch! It seems correct to me, I just had some minor > findings below. > > I'm fine if you address them while merging the patch if there are no > further comments. It would be nice to also hear Ilya's opinion first > though as he was part of the discussions on the previous versions.
Thanks, Ales and Dumitru! I think, this change looks good in general, I didn't read the code too closely, but I have just one comment below. <snip> >> diff --git a/ovn-nb.xml b/ovn-nb.xml >> index e9ca27413..a0a465be4 100644 >> --- a/ovn-nb.xml >> +++ b/ovn-nb.xml >> @@ -2057,6 +2057,15 @@ >> addresses within an element may be space or comma separated. >> </p> >> >> + <p> >> + There is also special prefix "VRRPv3" that allows specification of > > Nit: "This also supports the special prefix ..." > >> + single physical MAC and multiple VRRPv3 MAC addresses. The format >> + allows the same definition of multiple IP addresses associated >> with > > Nit: "same" sounds weird here. Maybe "As for non VRRPv3 entries, > multiple IP addresses can be associated with the specified MACs"? May also need to qualify which MACs, as it is not clear how the physical MAC plays here. More on that below... > >> + the MACs specified. "VRRPv3" without single physical MAC >> translates >> + allowing traffic for the whole "VRRPv3" range of MACs. See more in >> + the examples. >> + </p> >> + >> <p> >> This column is provided as a convenience to cloud management >> systems, >> but all of the features that it implements can be implemented as >> ACLs >> @@ -2102,6 +2111,30 @@ >> 255.255.255.255, and any address in 224.0.0.0/4. The host may >> not >> send or receive any IPv6 (including IPv6 Neighbor Discovery) >> traffic. >> </dd> >> + >> + <dt><code>"VRRPv3 <PHYSICAL_MAC>"</code></dt> >> + <dd> >> + The host may send traffic from and receive traffic to the >> + specified physical MAC addresses, and to all VRRPv3 MAC >> addresses. > > I'd specify the VRRPv3 MAC formats here explicitly, i.e., > 00-00-5E-00-01-XX and 00-00-5E-00-02-XX. > >> + This also affects the permutations of inner MAC for ARP/ND. The > > "the permutations" doesn't sound explicit enough to me. I'd just skip > it and only leave the sentence below, I think. > >> + ARP/ND can be sent with physical MAC as a source and the inner >> + SHA/TLL/SLL can be the physical MAC or any VRRPv3 MAC. >> + </dd> >> + >> + <dt><code>"VRRPv3 <PHYSICAL_MAC> >> + <VRRPV3_MACv4_1>/[<MASK1>] >> + <VRRPV3_MACv4_N> >> + <VRRPV3_MACv6_1>/[<MASK2>] >> + <VRRPV3_MACv6_N>"</code></dt> >> + <dd> >> + The host may send traffic from and receive traffic to the >> + specified physical MAC addresses, and to all VRRPv3 MAC >> addresses >> + specified in the option. Also note that the MAC addresses can >> + be provided with a mask between /40 and /48 prefix. >> + This also affects the permutations of inner MAC for ARP/ND. The > > Same here. > >> + ARP/ND can be sent with physical MAC as a source and the inner >> + SHA/TLL/SLL can be the physical MAC or provided VRRPv3 MAC. >> + </dd> >> </dl> >> </column> >> </group> <snip> >> +AS_BOX([With MAC only port security, VRRPv3=any]) >> +reset_pcap_and_expected >> +check ovn-nbctl --wait=hv lsp-set-port-security lsp1 "VRRPv3 >> 00:00:00:00:10:01" >> + >> +test_arp no >> +test_nd no >> + >> +OVN_CHECK_PACKETS([hv1/vif1-tx.pcap], [vif1.expected]) >> +OVN_CHECK_PACKETS([hv1/vif2-tx.pcap], [vif2.expected]) >> + >> +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_CHK_IN_PORT_SEC | >> ofctl_strip_all | sort | grep -v NXST_FLOW], [0], [dnl >> + table=OFTABLE_CHK_IN_PORT_SEC, priority=80,reg14=0x1,metadata=0x1 >> actions=load:0x1->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC, >> priority=90,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01 >> actions=resubmit(,OFTABLE_CHK_IN_PORT_SEC_ND) So, this allows any traffic from the physical MAC, even though a regular port security is not configured, only the VRRP one. So, if we add the regular port security "00:00:00:00:10:01", we'll get a duplicate OF rule. Not sure if it's a problem, but just something to note. But also, port security "00:00:00:00:10:01 192.168.0.1" will be ineffective in combination with "VRRPv3 00:00:00:00:10:01", as the vrrp one will allow any traffic from 00:00:00:00:10:01, not only vrrp-related. Should this rule be restricted to ARP/ND and have all the regular traffic handled by standard port security? >> + table=OFTABLE_CHK_IN_PORT_SEC, >> priority=90,reg14=0x1,metadata=0x1,dl_src=00:00:5e:00:01:00/ff:ff:ff:ff:ff:00 >> actions=resubmit(,OFTABLE_CHK_IN_PORT_SEC_ND) >> + table=OFTABLE_CHK_IN_PORT_SEC, >> priority=90,reg14=0x1,metadata=0x1,dl_src=00:00:5e:00:02:00/ff:ff:ff:ff:ff:00 >> actions=resubmit(,OFTABLE_CHK_IN_PORT_SEC_ND) These two rules are fine though, as they should allow routed ip traffic. >> + table=OFTABLE_CHK_IN_PORT_SEC, priority=95,arp,reg14=0x1,metadata=0x1 >> actions=resubmit(,OFTABLE_CHK_IN_PORT_SEC_ND) >> +]) >> +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_CHK_IN_PORT_SEC_ND | >> ofctl_strip_all | sort | grep -v NXST_FLOW], [0], [dnl >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, priority=80,arp,reg14=0x1,metadata=0x1 >> actions=load:0x1->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=80,icmp6,reg14=0x1,metadata=0x1,nw_ttl=255,icmp_type=135 >> actions=load:0x1->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=80,icmp6,reg14=0x1,metadata=0x1,nw_ttl=255,icmp_type=136 >> actions=load:0x1->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=90,arp,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01,arp_sha=00:00:00:00:10:01 >> actions=load:0->NXM_NX_REG10[[12]] Same here. This one allows ARP for the physical MAC and any SIP, so the VRRP port security record allows for spoofing any IP address, even if we have a regular port security that restricts it. >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=90,arp,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01,arp_sha=00:00:5e:00:01:00/ff:ff:ff:ff:ff:00 >> actions=load:0->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=90,icmp6,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01,nw_ttl=255,icmp_type=135,icmp_code=0,nd_sll=00:00:00:00:00:00 >> actions=load:0->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=90,icmp6,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01,nw_ttl=255,icmp_type=135,icmp_code=0,nd_sll=00:00:00:00:10:01 >> actions=load:0->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=90,icmp6,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01,nw_ttl=255,icmp_type=135,icmp_code=0,nd_sll=00:00:5e:00:02:00/ff:ff:ff:ff:ff:00 >> actions=load:0->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=90,icmp6,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01,nw_ttl=255,icmp_type=136,icmp_code=0,nd_tll=00:00:00:00:00:00 >> actions=load:0->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=90,icmp6,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01,nw_ttl=255,icmp_type=136,icmp_code=0,nd_tll=00:00:00:00:10:01 >> actions=load:0->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_IN_PORT_SEC_ND, >> priority=90,icmp6,reg14=0x1,metadata=0x1,dl_src=00:00:00:00:10:01,nw_ttl=255,icmp_type=136,icmp_code=0,nd_tll=00:00:5e:00:02:00/ff:ff:ff:ff:ff:00 >> actions=load:0->NXM_NX_REG10[[12]] >> +]) >> +AT_CHECK([ovs-ofctl dump-flows br-int table=OFTABLE_CHK_OUT_PORT_SEC | >> ofctl_strip_all | sort | grep -v NXST_FLOW], [0], [dnl >> + table=OFTABLE_CHK_OUT_PORT_SEC, priority=80,reg15=0x1,metadata=0x1 >> actions=load:0x1->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_OUT_PORT_SEC, >> priority=85,reg15=0x1,metadata=0x1,dl_dst=00:00:00:00:10:01 >> actions=load:0->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_OUT_PORT_SEC, >> priority=85,reg15=0x1,metadata=0x1,dl_dst=00:00:5e:00:01:00/ff:ff:ff:ff:ff:00 >> actions=load:0->NXM_NX_REG10[[12]] >> + table=OFTABLE_CHK_OUT_PORT_SEC, >> priority=85,reg15=0x1,metadata=0x1,dl_dst=00:00:5e:00:02:00/ff:ff:ff:ff:ff:00 >> actions=load:0->NXM_NX_REG10[[12]] >> +]) >> + <snip> Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
