Message ID | E4A437CB-E6C8-4325-A228-0A072C4300E4@ovn.org |
---|---|
State | Not Applicable |
Headers | show |
On Thu, Apr 7, 2016 at 9:48 PM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Apr 6, 2016, at 11:26 PM, Numan Siddique <nusiddiq@redhat.com> wrote: > > > > > > Thanks for the comments Justin. I tried a similar approach. It will not > work in the cases where the port security address also has a prefix defined. > > For example with port security - "00:00:00:00:00:02 10.0.0.4/24", the > ovn lexer parser is throwing the below error, > > > > ------- > > lflow|WARN|error parsing match "outport == "sw0-port2" && eth.dst == > 00:00:00:00:00:02 && ip4.dst == {255.255.255.255, 224.0.0.0/4, 10.0.0.4/24}": > Value contains unmasked 1-bits. > > ------ > > Ah, it should probably be added to the unit tests to make sure we don't > reintroduce a problem. (Thanks for writing unit tests, by the way.) > What if you apply the mask first like the patch at the end of this > message? I also expanded your unit tests to include a check for the issue > you mentioned. > > Hi Justin, there is still a problem with the below approach. In the case where port security has "10.0.0.4/24" it means that the logical port is restricted in sending and receiving IP traffic with ip address 10.0.0.4. IP traffic with any other ip address should be dropped. But with the below approach we would be allowing all the ip addresses in the 10.0.0.0/24. Below is the port security description <p> Each element in the set may additionally contain one or more IPv4 or IPv6 addresses (or both), with optional masks. If a mask is given, it must be a CIDR mask. In addition to the restrictions described for Ethernet addresses above, such an element restricts the IPv4 or IPv6 addresses from which the host may send and to which it may receive packets to the specified addresses. A masked address, if the host part is zero, indicates that the host is allowed to use any address in the subnet; if the host part is nonzero, the mask simply indicates the size of the subnet. In addition: </p> In the initial implementation I had missed to implement the case where the host part is zero. :) Thanks Numan --Justin > > > -=-=-=-=-=-=- > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 302cc1d..e60f72e 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -1179,8 +1179,11 @@ build_port_security_nd(struct ovn_port *op, struct > hmap * > if (ps.n_ipv4_addrs) { > ds_put_cstr(&match, " && ("); > for (size_t i = 0; i < ps.n_ipv4_addrs; i++) { > - ds_put_format(&match, "arp.spa == "IP_FMT" || ", > - IP_ARGS(ps.ipv4_addrs[i].addr)); > + ovs_be32 mask = > be32_prefix_mask(ps.ipv4_addrs[i].plen); > + ds_put_cstr(&match, "arp.spa == "); > + ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask, > + &match); > + ds_put_cstr(&match, " || "); > } > ds_chomp(&match, ' '); > ds_chomp(&match, '|'); > @@ -1264,7 +1267,9 @@ build_port_security_ip(enum ovn_pipeline pipeline, > struct > } > > for (int i = 0; i < ps.n_ipv4_addrs; i++) { > - ds_put_format(&match, IP_FMT", ", > IP_ARGS(ps.ipv4_addrs[i].addr > + ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen); > + ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask, > &match); > + ds_put_cstr(&match, ", "); > } > > /* Replace ", " by "}". */ > diff --git a/tests/ovn.at b/tests/ovn.at > index 22121e1..d8bc395 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1930,6 +1930,27 @@ for i in 1 2 3; do > test_ipv6 ${i}3 f00000000${i}${i}3 f00000000021 $sip $tip > done > > +# configure lport13 to send and received IPv4 packets with an address > range > +ovn-nbctl lport-set-port-security lp13 "f0:00:00:00:00:13 192.168.0.13 > 10.0.0.4 > + > +sip=`ip_to_hex 10 0 0 14` > +tip=`ip_to_hex 192 168 0 23` > +# IPv4 packet from lport13 with src ip 10.0.0.14 destined to lport23 > +# with dst ip 192.168.0.23 should be allowed > +test_ip 13 f00000000013 f00000000023 $sip $tip 23 > + > +sip=`ip_to_hex 192 168 0 33` > +tip=`ip_to_hex 10 0 0 15` > +# IPv4 packet from lport33 with src ip 192.168.0.33 destined to lport13 > +# with dst ip 10.0.0.15 should be received by lport13 > +test_ip 33 f00000000033 f00000000013 $sip $tip 13 > + > +sip=`ip_to_hex 10 0 0 13` > +tip=`ip_to_hex 192 168 0 22` > +# arp packet with inner ip 10.0.0.13 should be allowed for lport13 > +test_arp 13 f00000000013 f00000000013 $sip $tip 0 f00000000022 > + > + > > # Allow some time for packet forwarding. > > > > >
> On Apr 7, 2016, at 11:34 AM, Numan Siddique <nusiddiq@redhat.com> wrote: > > Hi Justin, there is still a problem with the below approach. > > In the case where port security has "10.0.0.4/24" it means that the logical port is restricted in sending and receiving IP traffic with ip address 10.0.0.4. IP traffic with any other ip address should be dropped. But with the below approach we would be allowing all the ip addresses in the 10.0.0.0/24. Huh, there's a fair amount of subtlety there. What about logic similar to the following (untested) code? -=-=-=-=-=-=-=-=- ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen); /* When the netmask is applied, if the host portion is * non-zero, the host can only use the specified * address. If zero, the host is allowed to use any * address in the subnet. */ if (ps.ipv4_addrs[i].addr & ~mask) { ds_put_format(&match, IP_FMT, IP_ARGS(ps.ipv4_addrs[i].addr)); } else { ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask, &match); } -=-=-=-=-=-=-=-=- > Below is the port security description > > <p> > Each element in the set may additionally contain one or more IPv4 or > IPv6 addresses (or both), with optional masks. If a mask is given, it > must be a CIDR mask. In addition to the restrictions described for > Ethernet addresses above, such an element restricts the IPv4 or IPv6 > addresses from which the host may send and to which it may receive > packets to the specified addresses. A masked address, if the host part > is zero, indicates that the host is allowed to use any address in the > subnet; if the host part is nonzero, the mask simply indicates the size > of the subnet. In addition: > </p> The next paragraph is interesting because it describes what should happen with the subnet: · If any IPv4 address is given, the host is also allowed to receive packets to the IPv4 local broadcast address 255.255.255.255 and to IPv4 multicast addresses (224.0.0.0/4). If an IPv4 address with a mask is given, the host is also allowed to receive packets to the broad‐ cast address in that specified subnet. Would you mind expanding the tests to make sure that we're testing these different combinations both on the send and receive enforcement? We clearly had some gaps before. Thanks for noticing these issues. --Justin
> > Huh, there's a fair amount of subtlety there. What about logic similar to > the following (untested) code? > > -=-=-=-=-=-=-=-=- > ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen); > /* When the netmask is applied, if the host portion is > * non-zero, the host can only use the specified > * address. If zero, the host is allowed to use any > * address in the subnet. */ > if (ps.ipv4_addrs[i].addr & ~mask) { > ds_put_format(&match, IP_FMT, > IP_ARGS(ps.ipv4_addrs[i].addr)); > } else { > ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask, > &match); > } > -=-=-=-=-=-=-=-=- > > > Below is the port security description > > > > <p> > > Each element in the set may additionally contain one or more > IPv4 or > > IPv6 addresses (or both), with optional masks. If a mask is > given, it > > must be a CIDR mask. In addition to the restrictions > described for > > Ethernet addresses above, such an element restricts the IPv4 > or IPv6 > > addresses from which the host may send and to which it may > receive > > packets to the specified addresses. A masked address, if the > host part > > is zero, indicates that the host is allowed to use any address > in the > > subnet; if the host part is nonzero, the mask simply indicates > the size > > of the subnet. In addition: > > </p> > > The next paragraph is interesting because it describes what should happen > with the subnet: > > · If any IPv4 address is given, the host is also > allowed to > receive packets to the IPv4 local broadcast > address > 255.255.255.255 and to IPv4 multicast > addresses > (224.0.0.0/4). If an IPv4 address with a mask is > given, > the host is also allowed to receive packets to the > broad‐ > cast address in that specified subnet. > > Would you mind expanding the tests to make sure that we're testing these > different combinations both on the send and receive enforcement? We > clearly had some gaps before. > > Sure. I will do that. Thanks for the suggestions and comments. > Thanks for noticing these issues. > > --Justin > > >
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 302cc1d..e60f72e 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -1179,8 +1179,11 @@ build_port_security_nd(struct ovn_port *op, struct hmap * if (ps.n_ipv4_addrs) { ds_put_cstr(&match, " && ("); for (size_t i = 0; i < ps.n_ipv4_addrs; i++) { - ds_put_format(&match, "arp.spa == "IP_FMT" || ", - IP_ARGS(ps.ipv4_addrs[i].addr)); + ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen); + ds_put_cstr(&match, "arp.spa == "); + ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask, + &match); + ds_put_cstr(&match, " || "); } ds_chomp(&match, ' '); ds_chomp(&match, '|'); @@ -1264,7 +1267,9 @@ build_port_security_ip(enum ovn_pipeline pipeline, struct } for (int i = 0; i < ps.n_ipv4_addrs; i++) { - ds_put_format(&match, IP_FMT", ", IP_ARGS(ps.ipv4_addrs[i].addr + ovs_be32 mask = be32_prefix_mask(ps.ipv4_addrs[i].plen); + ip_format_masked(ps.ipv4_addrs[i].addr & mask, mask, &match); + ds_put_cstr(&match, ", "); } /* Replace ", " by "}". */ diff --git a/tests/ovn.at b/tests/ovn.at index 22121e1..d8bc395 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1930,6 +1930,27 @@ for i in 1 2 3; do test_ipv6 ${i}3 f00000000${i}${i}3 f00000000021 $sip $tip done +# configure lport13 to send and received IPv4 packets with an address range +ovn-nbctl lport-set-port-security lp13 "f0:00:00:00:00:13 192.168.0.13 10.0.0.4 + +sip=`ip_to_hex 10 0 0 14` +tip=`ip_to_hex 192 168 0 23` +# IPv4 packet from lport13 with src ip 10.0.0.14 destined to lport23 +# with dst ip 192.168.0.23 should be allowed +test_ip 13 f00000000013 f00000000023 $sip $tip 23 + +sip=`ip_to_hex 192 168 0 33` +tip=`ip_to_hex 10 0 0 15` +# IPv4 packet from lport33 with src ip 192.168.0.33 destined to lport13 +# with dst ip 10.0.0.15 should be received by lport13 +test_ip 33 f00000000033 f00000000013 $sip $tip 13 + +sip=`ip_to_hex 10 0 0 13` +tip=`ip_to_hex 192 168 0 22` +# arp packet with inner ip 10.0.0.13 should be allowed for lport13 +test_arp 13 f00000000013 f00000000013 $sip $tip 0 f00000000022 + + # Allow some time for packet forwarding.