On 1/23/26 1:23 PM, Ilya Maximets wrote: > On 1/23/26 1:08 PM, Dumitru Ceara wrote: >> On 1/23/26 12:53 PM, Ilya Maximets wrote: >>> On 1/23/26 11:38 AM, Dumitru Ceara wrote: >>>> Hi all, >>>> >>>> Thanks for the patch and discussion! I'll add my own opinion below. >>>> >>>> On 1/23/26 7:30 AM, Ales Musil wrote: >>>>> On Thu, Jan 22, 2026 at 7:40 PM Mark Michelson <[email protected]> >>>>> wrote: >>>>> >>>>>> In the commit title and description, "compliant" is misspelled as >>>>>> "complaint". It is spelled correctly in the code, though. >>>>>> >>>>>> I have comments below, both regarding the code submission and to >>>>>> respond to Ilya's remarks. >>>>>> >>>>>> On Thu, Jan 22, 2026 at 12:18 PM Ilya Maximets <[email protected]> >>>>>> wrote: >>>>>>> >>>>>>> On 1/22/26 3:27 PM, 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 >>>>>>>> complaint add an option which when enabled will add extra flows >>>>>>>> that match on the MAC address range specified by the RFC. >>>>>>>> >>>>>>>> [0] https://datatracker.ietf.org/doc/html/rfc5798 >>>>>>>> Reported-at: https://issues.redhat.com/browse/FDP-2979 >>>>>>>> Signed-off-by: Ales Musil <[email protected]> >>>>>>>> --- >>>>>>>> NEWS | 2 + >>>>>>>> controller/lflow.c | 67 ++++++++++++++++++++ >>>>>>>> ovn-nb.xml | 9 +++ >>>>>>>> tests/ovn.at | 149 >>>>>> +++++++++++++++++++++++++++++++++++++++++++++ >>>>>>>> 4 files changed, 227 insertions(+) >>>>>>>> >>>>>>>> diff --git a/NEWS b/NEWS >>>>>>>> index dc9d28f2c..e58f8ee1e 100644 >>>>>>>> --- a/NEWS >>>>>>>> +++ b/NEWS >>>>>>>> @@ -87,6 +87,8 @@ Post v25.09.0 >>>>>>>> other_config column. >>>>>>>> - Introduce the capability to specify multiple ips for >>>>>> ovn-evpn-local-ip >>>>>>>> option. >>>>>>>> + - Add an LSP option called "port-security-rfc5798-compliant", this >>>>>>>> + allows VRRP v3 (RFC 5798) to work with port security enabled. >>>>>>> >>>>>>> Hi, Ales. >>>>>>> >>>>>>> This option name seems a bit misleading, as someone seeing it may assume >>>>>>> that just enbaling it will allow the port to participate in VRRPv3. But >>>>>>> it's not the case as port security will drop all the actual VRRP >>>>>>> traffic, >>>>>>> unless the virtual router mac address is also added to the port >>>>>>> security, >>>>>>> as all the actual VRRP packets with have this address as a source. As >>>>>> per >>>>>>> RFC itself: >>>>>>> >>>>>>> Note: VRRP packets are transmitted with the virtual router MAC >>>>>>> address as the source MAC address to ensure that learning bridges >>>>>>> correctly determine the LAN segment the virtual router is >>>>>>> attached to. >>>>>>> >>>>>>> We should either add explicit indication that this option only affects >>>>>>> ARP/ND packets, which would still be a bit confusing and will make the >>>>>>> name even longer, or just have one knob like port-security-allow-vrrpv3 >>>>>>> that also allows all the actual VRRP traffic, so users don't have to >>>>>>> add all the possible router IDs into port security. This option may >>>>>>> potentially accept a list of allowed router IDs or "yes" for all, but >>>>>>> that can be added later if necessary. >>>>>> >>>>>> If this is the case, then does this mean that non-ARP and non-ND VRRP >>>>>> traffic will still be dropped by port security flows when this option >>>>>> is enabled? If so, then it seems like allowing the ARPs and NDs is >>>>>> only getting rid of the surface issue. The ARPs/NDs will be allowed >>>>>> through, but then other VRRP traffic will still be dropped. Is that >>>>>> OK? It seems like we're going to immediately get a complaint about >>>>>> this patch not actually fixing the problem if that's the case. I think >>>>>> Ilya is on the right track by suggesting that we allow all VRRP >>>>>> traffic with this option, not just the ARP/ND packets. >>>>>> >>>> >>>> The bug report was about ARP replies not being allowed. VRRP ARP >>>> replies for the virtual IP are special: >>>> >>>> https://datatracker.ietf.org/doc/html/rfc5798#section-8.1.2 >>>> >>>> When a host sends an ARP request for one of the virtual router IPv4 >>>> addresses, the Virtual Router Master MUST respond to the ARP request >>>> with an ARP response that indicates the virtual MAC address for the >>>> virtual router. Note that the source address of the Ethernet frame >>>> of this ARP response is the physical MAC address of the physical >>>> router. >>>> >>>> The traffic the virtual router actually forwards, IP traffic, is sent >>>> using the _virtual_ MAC as source, i.e., from 00-00-5E-00-01-{VRID} (for >>>> IPv4). >>>> >>>> But as the RFC states, the ARP replies are sent from with eth.src == >>>> _physical_ MAC of the VRRP member and arp.sha == 00-00-5E-00-01-{VRID}. >>>> >>>> Actual VRRP protocol packets are multicast and use the _physical_ MAC >>>> address as source. >>> >>> I suppose, you meant "virtual" here. See the note at the end of >>> https://datatracker.ietf.org/doc/html/rfc5798#section-7.2 that I >>> cited before. >>> >> >> You're right, it should be "virtual". But that doesn't my argument >> above much. >> >>>> >>>> So, in order for ALL this to work, the CMS must _anyway_ configure two >>>> MACs (physical and 00-00-5E-00-01-{VRID}) in the port_security field of >>>> the LSP. >>> >>> That is true. But why not save users from extra configuration and >>> just allow the new knob to allow both the ARP/ND and the VRRP and >>> the routed traffic? AFAIU, anyone setting the option will always >>> have to set the MACs in the port_security column as well. There is >>> no point in setting the option and not adding virtual MAC to the >>> port_security list as well. So I'm not sure why we're forcing users >>> to do that when we already adding a special vrrp knob. >>> >> >> Your suggestion was: >> >>>>>>> We should either add explicit indication that this option >>>>>>> only affects ARP/ND packets, which would still be a bit >>>>>>> confusing and will make the name even longer, or just have >>>>>>> one knob like port-security-allow-vrrpv3 that also allows >>>>>>> all the actual VRRP traffic, so users don't have to add all >>>>>>> the possible router IDs into port security. This option may >>>>>>> potentially accept a list of allowed router IDs or "yes" for >>>>>>> all, but that can be added later if necessary. >> I read it as: >> >> 1. for now support "yes/no" for this option. >> 2. if "yes" then the user needs to configure the physical MAC in the >> port_security field. >> >> In that case OVN will allow: >> - all traffic from/to the physical MAC, including VRRP ARPs with arp.sha >> == VRRP-MAC (all VRIDs) >> >> but also >> >> - all traffic from/to any VRRP VID mac, i.e, any 00-00-5E-00-01-{VRID} >> >> This is less "secure" than the additional knob added by Ales in this >> patch which, in combination with explicitly adding the user VRID MAC to >> port_security along with the physical MAC, would allow routed IP traffic >> only if it's routed by that explicitly configured VRRP VRID. > > If we're allowing to spoof any of the virtual MAC addresses, what's > the point of not allowing to send packets from subset of them? > Not sure what the use case would be for that. >
That's why I wrote "secure" between quotes above. I'm not sure. It's bad enough to allow spoofing of any VRID mac. >> >> Now, if we assume in the future we allow a list of VRRP MACs instead of >> "yes" as value to the option, the user still needs to configure: >> - port_security=[<physical-MAC>] >> - port-security-allow-vrrpv3=[<VRRP-MAC1>, <VRRP-MAC2>] > > This may be a list of IDs or ID ranges, don't really need to be full > MAC addresses. > I'd prefer MAC addresses, it would save people looking at the config a trip to the RFC to retrieve the 00-00-5E-00-01- or 00-00-5E-00-02- prefixes. >> >> While that's one config less, it does mean that we're not as restrictive >> as we could be in 26.03 (or until we backport the extended config support). > > If you don't like the "yes" option, the list of IDs can be implemented > from the beginning. I don't see why that would be complicated to do > in v2 of this patch. > Maybe it's not, I'll let Ales comment. Regards, Dumitru > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
