diff mbox

[ovs-dev] ovn-northd: Handle IPv4 addresses with prefixes in lport port security

Message ID E4A437CB-E6C8-4325-A228-0A072C4300E4@ovn.org
State Not Applicable
Headers show

Commit Message

Justin Pettit April 7, 2016, 4:18 p.m. UTC 
> 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.

--Justin


-=-=-=-=-=-=-

Comments

Numan Siddique April 7, 2016, 6:34 p.m. UTC | #1 
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 t​he 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.
>
>
>
>
>
Justin Pettit April 8, 2016, 12:23 a.m. UTC | #2 
> 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
Numan Siddique April 8, 2016, 5:39 a.m. UTC | #3 
>
> 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 mbox

Patch

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.