On 2/11/26 10:37 AM, Ales Musil 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 a 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]> > --- > v4: Rebase on top of latest main. > Update the RFC url. > Add Jacob's ack. > v5: Rebase on top of latest main. > Address nits pointed out by Dumitru. > Add extra test for invalid VRRPv3 MAC. > Update the wording in documentation. > Remove acks as the code changed. > Do not populate flows for physical MAC when VRRP is specified. > v6: Rebase on top of latest main. > Update the documentation formatting. > Update the behavior when masked portion of MAC is non-zero. > Address some nits. > Update testing. > Remove all acks as the patch has changed. > v7: Update the tests. > Update the documentation according to Ilya's suggestion. > v8: Rebase on top of latest main. > Address nits. > Adjust the test. > --- > NEWS | 3 + > controller/lflow.c | 931 +++++++++++++++++++++++++++++---------------- > ovn-nb.xml | 55 +++ > tests/ovn.at | 854 +++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 1520 insertions(+), 323 deletions(-)
Thanks, Ales! There are potentially few things that could be improved in the regular port-security and by extension in the new VRRP one, but that's out of scope for this patch. This implementation seems correct from the VRRP point of view, and in line with how the regular entries are handled. I have a couple of tiny nits below, but otherwise, Acked-by: Ilya Maximets <[email protected]> > > diff --git a/NEWS b/NEWS > index bb550fe59..c566ebfc8 100644 > --- a/NEWS > +++ b/NEWS > @@ -101,6 +101,9 @@ Post v25.09.0 > - Add "distributed" option for load balancer, that forces traffic to be > routed only to backend instances running locally on the same chassis > it arrives on. > + - Add support for special port_security prefix "VRRPv3". This prefix > allows > + CMS to allow all required traffic for a VRRPv3 virtual router behind > LSP. > + See ovn-nb(5) man page for more details. > > OVN v25.09.0 - xxx xx xxxx > -------------------------- > diff --git a/controller/lflow.c b/controller/lflow.c > index b6be5c630..eb96a9597 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c <snip> > @@ -2714,99 +2854,118 @@ build_in_port_sec_ip6_flows(const struct > sbrec_port_binding *pb, > * 'in_port_sec_nd' table. */ > static void > build_in_port_sec_nd_flows(const struct sbrec_port_binding *pb, > - struct lport_addresses *ps_addr, > - struct match *m, struct ofpbuf *ofpacts, > + struct eth_addr phys_mac, > + const struct vector *ip6_addrs, > + const struct vector *vrrp6_addrs, > + bool is_vrrp, struct match *m, > + struct ofpbuf *ofpacts, > struct ovn_desired_flow_table *flow_table) > { > build_port_sec_allow_action(ofpacts); > + reset_match_for_port_sec_flows(pb, MFF_LOG_INPORT, m); > + match_set_dl_src(m, phys_mac); > + match_set_dl_type(m, htons(ETH_TYPE_IPV6)); > + match_set_nw_proto(m, IPPROTO_ICMPV6); > + match_set_icmp_code(m, 0); > + match_set_nw_ttl(m, 255); > > /* Add the below logical flow equivalent OF rules in 'in_port_sec_nd' > * table. > * priority: 90 > - * match - "inport == pb->port && eth.src == ps_addr.ea && > - * icmp6 && icmp6.code == 135 && icmp6.type == 0 && > - * ip6.tll == 255 && nd.sll == {00:00:00:00:00:00, ps_addr.ea}" > + * match - "inport == pb->port && eth.src == phys_mac && > + * icmp6 && icmp6.type == 135 && icmp6.code == 0 && > + * ip6.tll == 255 && > + * nd.sll == {00:00:00:00:00:00, phys_mac, vrrp6_addrs}" > * action - "port_sec_failed = 0;" > */ > - reset_match_for_port_sec_flows(pb, MFF_LOG_INPORT, m); > - match_set_dl_type(m, htons(ETH_TYPE_IPV6)); > - match_set_nw_proto(m, IPPROTO_ICMPV6); > - match_set_dl_src(m, ps_addr->ea); > - match_set_nw_ttl(m, 255); > + > match_set_icmp_type(m, 135); > - match_set_icmp_code(m, 0); > > match_set_arp_sha(m, eth_addr_zero); > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > pb->header_.uuid.parts[0], m, ofpacts, > &pb->header_.uuid); > > - match_set_arp_sha(m, ps_addr->ea); > - ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > - pb->header_.uuid.parts[0], m, ofpacts, > - &pb->header_.uuid); > + if (!is_vrrp) { > + match_set_arp_sha(m, phys_mac); > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > + pb->header_.uuid.parts[0], m, ofpacts, > + &pb->header_.uuid); > + } > + > + struct masked_eth_addr *mmac; > + VECTOR_FOR_EACH_PTR (vrrp6_addrs, mmac) { > + match_set_arp_sha_masked(m, mmac->addr, mmac->mask); > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > + pb->header_.uuid.parts[0], m, ofpacts, > + &pb->header_.uuid); > + } > > + match_set_arp_sha_masked(m, eth_addr_zero, eth_addr_zero); > match_set_icmp_type(m, 136); > - match_set_icmp_code(m, 0); > - if (ps_addr->n_ipv6_addrs) { > + if (!vector_is_empty(ip6_addrs)) { > /* Add the below logical flow equivalent OF rules in 'in_port_sec_nd' > * table if IPv6 addresses are configured. > * priority: 90 > - * match - "inport == pb->port && eth.src == ps_addr.ea && icmp6 && > - * icmp6.code == 136 && icmp6.type == 0 && ip6.tll == 255 && > - * nd.tll == {00:00:00:00:00:00, ps_addr.ea} && > - * nd.target == {ps_addr.ipv6_addrs, lla}" > + * match - "inport == pb->port && eth.src == phys_mac && icmp6 && > + * icmp6.type == 136 && icmp6.code == 0 && ip6.tll == 255 && > + * nd.tll == {00:00:00:00:00:00, phys_mac, vrrp6_addrs} && > + * nd.target == {lla, ip6_addrs}" > * action - "port_sec_failed = 0;" > */ > struct in6_addr lla; > - in6_generate_lla(ps_addr->ea, &lla); > - match_set_arp_tha(m, eth_addr_zero); > - > - match_set_nd_target(m, &lla); > - ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > - pb->header_.uuid.parts[0], m, ofpacts, > - &pb->header_.uuid); > - match_set_arp_tha(m, ps_addr->ea); > + in6_generate_lla(phys_mac, &lla); > match_set_nd_target(m, &lla); > + > + match_set_arp_tha(m, eth_addr_zero); > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > pb->header_.uuid.parts[0], m, ofpacts, > &pb->header_.uuid); > > - for (size_t j = 0; j < ps_addr->n_ipv6_addrs; j++) { > - reset_match_for_port_sec_flows(pb, MFF_LOG_INPORT, m); > - match_set_dl_src(m, ps_addr->ea); > - match_set_dl_type(m, htons(ETH_TYPE_IPV6)); > - match_set_nw_proto(m, IPPROTO_ICMPV6); > - match_set_nw_ttl(m, 255); > - match_set_icmp_type(m, 136); > - match_set_icmp_code(m, 0); > - match_set_arp_tha(m, eth_addr_zero); > - > - if (ps_addr->ipv6_addrs[j].plen == 128 > - || !ipv6_addr_is_host_zero(&ps_addr->ipv6_addrs[j].addr, > - &ps_addr->ipv6_addrs[j].mask)) { > - match_set_nd_target(m, &ps_addr->ipv6_addrs[j].addr); > - } else { > - match_set_nd_target_masked(m, > &ps_addr->ipv6_addrs[j].network, > - &ps_addr->ipv6_addrs[j].mask); > - } > + if (!is_vrrp) { > + match_set_arp_tha(m, phys_mac); > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > + pb->header_.uuid.parts[0], m, ofpacts, > + &pb->header_.uuid); > + } > > + VECTOR_FOR_EACH_PTR (vrrp6_addrs, mmac) { > + match_set_arp_tha_masked(m, mmac->addr, mmac->mask); > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > pb->header_.uuid.parts[0], m, ofpacts, > &pb->header_.uuid); > + } > + > + const struct masked_ip6_addr *ip; > + VECTOR_FOR_EACH_PTR (ip6_addrs, ip) { > + match_set_nd_target_masked(m, &ip->addr, &ip->mask); > > - match_set_arp_tha(m, ps_addr->ea); > + match_set_arp_tha(m, eth_addr_zero); > ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > pb->header_.uuid.parts[0], m, ofpacts, > &pb->header_.uuid); > + > + if (!is_vrrp) { > + match_set_arp_tha(m, phys_mac); > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > + pb->header_.uuid.parts[0], m, ofpacts, > + &pb->header_.uuid); > + } > + > + VECTOR_FOR_EACH_PTR (vrrp6_addrs, mmac) { > + match_set_arp_tha_masked(m, mmac->addr, mmac->mask); > + ofctrl_add_flow(flow_table, OFTABLE_CHK_IN_PORT_SEC_ND, 90, > + pb->header_.uuid.parts[0], m, ofpacts, > + &pb->header_.uuid); > + } > } > } else { > /* Add the below logical flow equivalent OF rules in 'in_port_sec_nd' > * table if no IPv6 addresses are configured. > * priority: 90 > - * match - "inport == pb->port && eth.src == ps_addr.ea && icmp6 && > + * match - "inport == pb->port && eth.src == phys_mac && icmp6 && > * icmp6.code == 136 && icmp6.type == 0 && ip6.tll == 255 && nit: You fixed the misplaced code and type in other places. Maybe also fix here? > diff --git a/ovn-nb.xml b/ovn-nb.xml > index aab091883..85ac7b61b 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2057,6 +2057,23 @@ > addresses within an element may be space or comma separated. > </p> > > + <p> > + Each element in the set also supports the special prefix "VRRPv3" > + that allows specification of single physical MAC and multiple > + VRRPv3 MAC addresses. As for non VRRPv3 entries, multiple IP > + addresses can be associated with the specified MACs. "VRRPv3" with > + a single physical MAC translates to allowing traffic for the whole > + "VRRPv3" range of MACs. See more in the examples. > + > + When the port security configuration entry contains the VRRPv3 > label, > + the behavior for physical MAC (the first specified) is different. > + The installed flows will allow traffic from/to VRRP MACs + IPs. > + The physical MAC is there to properly allow ARP/ND with given > + VRRP MACs. To allow traffic that is not related to virtual router, > + e.g., IP traffic with a physical MAC as a source, a regular port nit: No need for the double space after a comma. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
