| Message ID | 1534742851-36287-1-git-send-email-hzhou8@ebay.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,1/2] ovn-northd: LR respond ARP from valid subnet only. | expand |
Bleep bloop. Greetings Han Zhou, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Han Zhou <zhouhan@gmail.com> needs to sign off. WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Han Zhou <hzhou8@ebay.com> Lines checked: 107, Warnings: 1, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@bytheb.org Thanks, 0-day Robot
On Mon, Aug 20, 2018 at 01:56:38AM -0400, 0-day Robot wrote: > ERROR: Author Han Zhou <zhouhan@gmail.com> needs to sign off. > WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Han Zhou <hzhou8@ebay.com> It's best to use the same email address for committer and sign-off.
On Sun, Aug 19, 2018 at 10:27:30PM -0700, Han Zhou wrote: > Currently ovn LR datapath responds ARP requests even if the ARP > requestor's src IP doesn't belong to the LR port's subnets. This > may generate unnecessary ARP responses and there could also be > security concerns. This patch restricts the ARP response only if > the requestor's IP matches the LR port's subnets. > > Signed-off-by: Han Zhou <hzhou8@ebay.com> Thanks, this series seems fine and the tests pass, so I applied it to master.
On Tue, Aug 21, 2018 at 11:28 AM Ben Pfaff <blp@ovn.org> wrote: > > On Mon, Aug 20, 2018 at 01:56:38AM -0400, 0-day Robot wrote: > > ERROR: Author Han Zhou <zhouhan@gmail.com> needs to sign off. > > WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Han Zhou <hzhou8@ebay.com> > > It's best to use the same email address for committer and sign-off. ok. It is just easier to setup git send-email for gmail :) I will figure out how to setup my ebay account.
On Tue, Aug 21, 2018 at 11:36 AM Ben Pfaff <blp@ovn.org> wrote: > > On Sun, Aug 19, 2018 at 10:27:30PM -0700, Han Zhou wrote: > > Currently ovn LR datapath responds ARP requests even if the ARP > > requestor's src IP doesn't belong to the LR port's subnets. This > > may generate unnecessary ARP responses and there could also be > > security concerns. This patch restricts the ARP response only if > > the requestor's IP matches the LR port's subnets. > > > > Signed-off-by: Han Zhou <hzhou8@ebay.com> > > Thanks, this series seems fine and the tests pass, so I applied it to > master. Thanks Ben. Shall we backport to at least 2.9 and 2.10? Without this, GARP request won't work for mac-binding update.
On Tue, Aug 21, 2018 at 03:03:16PM -0700, Han Zhou wrote: > On Tue, Aug 21, 2018 at 11:36 AM Ben Pfaff <blp@ovn.org> wrote: > > > > On Sun, Aug 19, 2018 at 10:27:30PM -0700, Han Zhou wrote: > > > Currently ovn LR datapath responds ARP requests even if the ARP > > > requestor's src IP doesn't belong to the LR port's subnets. This > > > may generate unnecessary ARP responses and there could also be > > > security concerns. This patch restricts the ARP response only if > > > the requestor's IP matches the LR port's subnets. > > > > > > Signed-off-by: Han Zhou <hzhou8@ebay.com> > > > > Thanks, this series seems fine and the tests pass, so I applied it to > > master. > > Thanks Ben. Shall we backport to at least 2.9 and 2.10? Without this, GARP > request won't work for mac-binding update. How much of a problem is it in practice? The patch series was the first I'd heard of the problem.
On Tue, Aug 21, 2018 at 3:38 PM Ben Pfaff <blp@ovn.org> wrote: > > On Tue, Aug 21, 2018 at 03:03:16PM -0700, Han Zhou wrote: > > On Tue, Aug 21, 2018 at 11:36 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > On Sun, Aug 19, 2018 at 10:27:30PM -0700, Han Zhou wrote: > > > > Currently ovn LR datapath responds ARP requests even if the ARP > > > > requestor's src IP doesn't belong to the LR port's subnets. This > > > > may generate unnecessary ARP responses and there could also be > > > > security concerns. This patch restricts the ARP response only if > > > > the requestor's IP matches the LR port's subnets. > > > > > > > > Signed-off-by: Han Zhou <hzhou8@ebay.com> > > > > > > Thanks, this series seems fine and the tests pass, so I applied it to > > > master. > > > > Thanks Ben. Shall we backport to at least 2.9 and 2.10? Without this, GARP > > request won't work for mac-binding update. > > How much of a problem is it in practice? The patch series was the first > I'd heard of the problem. The first patch in this series may not be critical, but the second one regarding GARP is quite important IMHO, since GARP request is very commonly used for announcing IP-MAC bindings. In practice, there are 2 common cases that will have problem: 1) When IP-MAC bindings update in the external network behind OVN GW. The IPs are usually default next hop GW of OVN logical routers, or next hop of some static routes in OVN. There is a good chance that these bindings change after device replacement/upgrade etc. These are not frequent operations, but once it happens it will be a big impact. Someone has to debug this and finally delete the stale mac-binding entries manually from SB DB, so that traffic can go through the new device. 2) When nested workloads are running behind OVN logical ports, such as, running containers inside VMs. If we don't use child port feature, we'd rely on mac-binding for container's IP to be accessible. If the implementation of the container orchestration system uses GARP request to update the neighbors, then it won't work without the GARP patch. There is a workaround in this case - change the container orchestration system implementation to use ARP reply to update the neighbors, but it would be better if OVN supports the general scenario.
On Tue, Aug 21, 2018 at 04:24:27PM -0700, Han Zhou wrote: > On Tue, Aug 21, 2018 at 3:38 PM Ben Pfaff <blp@ovn.org> wrote: > > > > On Tue, Aug 21, 2018 at 03:03:16PM -0700, Han Zhou wrote: > > > On Tue, Aug 21, 2018 at 11:36 AM Ben Pfaff <blp@ovn.org> wrote: > > > > > > > > On Sun, Aug 19, 2018 at 10:27:30PM -0700, Han Zhou wrote: > > > > > Currently ovn LR datapath responds ARP requests even if the ARP > > > > > requestor's src IP doesn't belong to the LR port's subnets. This > > > > > may generate unnecessary ARP responses and there could also be > > > > > security concerns. This patch restricts the ARP response only if > > > > > the requestor's IP matches the LR port's subnets. > > > > > > > > > > Signed-off-by: Han Zhou <hzhou8@ebay.com> > > > > > > > > Thanks, this series seems fine and the tests pass, so I applied it to > > > > master. > > > > > > Thanks Ben. Shall we backport to at least 2.9 and 2.10? Without this, > GARP > > > request won't work for mac-binding update. > > > > How much of a problem is it in practice? The patch series was the first > > I'd heard of the problem. > > The first patch in this series may not be critical, but the second one > regarding GARP is quite important IMHO, since GARP request is very commonly > used for announcing IP-MAC bindings. > In practice, there are 2 common cases that will have problem: > > 1) When IP-MAC bindings update in the external network behind OVN GW. The > IPs are usually default next hop GW of OVN logical routers, or next hop of > some static routes in OVN. There is a good chance that these bindings > change after device replacement/upgrade etc. These are not frequent > operations, but once it happens it will be a big impact. Someone has to > debug this and finally delete the stale mac-binding entries manually from > SB DB, so that traffic can go through the new device. > > 2) When nested workloads are running behind OVN logical ports, such as, > running containers inside VMs. If we don't use child port feature, we'd > rely on mac-binding for container's IP to be accessible. If the > implementation of the container orchestration system uses GARP request to > update the neighbors, then it won't work without the GARP patch. There is a > workaround in this case - change the container orchestration system > implementation to use ARP reply to update the neighbors, but it would be > better if OVN supports the general scenario. Thanks a lot for the explanation. This applied cleanly and built as far back as branch-2.8, so I backported as far as that.
diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml index f1771c6..ffe2727 100644 --- a/ovn/northd/ovn-northd.8.xml +++ b/ovn/northd/ovn-northd.8.xml @@ -1123,11 +1123,15 @@ next; <p> These flows reply to ARP requests for the router's own IP address. - For each router port <var>P</var> that owns IP address <var>A</var> + The ARP requests are responded only if the requestor's IP belongs + to the same subnets of the logical router port. + For each router port <var>P</var> that owns IP address <var>A</var>, + which belongs to subnet <var>S</var> with prefix length <var>L</var>, and Ethernet address <var>E</var>, a priority-90 flow matches - <code>inport == <var>P</var> && arp.op == 1 && - arp.tpa == <var>A</var></code> (ARP request) with the following - actions: + <code>inport == <var>P</var> && + arp.spa == <var>S</var>/<var>L</var> && arp.op == 1 + && arp.tpa == <var>A</var></code> (ARP request) with the + following actions: </p> <pre> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index ba86bf5..4478cd3 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -5183,8 +5183,12 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { ds_clear(&match); ds_put_format(&match, - "inport == %s && arp.tpa == %s && arp.op == 1", - op->json_key, op->lrp_networks.ipv4_addrs[i].addr_s); + "inport == %s && arp.spa == %s/%u && arp.tpa == %s" + " && arp.op == 1", + op->json_key, + op->lrp_networks.ipv4_addrs[i].network_s, + op->lrp_networks.ipv4_addrs[i].plen, + op->lrp_networks.ipv4_addrs[i].addr_s); if (op->od->l3dgw_port && op == op->od->l3dgw_port && op->od->l3redirect_port) { /* Traffic with eth.src = l3dgw_port->lrp_networks.ea_s diff --git a/tests/ovn.at b/tests/ovn.at index 70c6c50..663db22 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -2671,9 +2671,9 @@ test_arp() { # 5. Router replies to query for its MAC address from any random IP address # in its subnet. # -# 6. Router replies to query for its MAC address from another subnet. +# 6. No reply to query for IP address other than router IP. # -# 7. No reply to query for IP address other than router IP. +# 7. No reply to query from another subnet. for i in 1 2 3; do for j in 1 2 3; do for k in 1 2 3; do @@ -2682,10 +2682,21 @@ for i in 1 2 3; do rip=`ip_to_hex 192 168 $i$j 254` # Router IP rmac=00000000ff$i$j # Router MAC otherip=`ip_to_hex 192 168 $i$j 55` # Some other IP in subnet - test_arp $i$j$k $smac $sip $rip $rmac #4 - test_arp $i$j$k $smac $otherip $rip $rmac #5 - test_arp $i$j$k $smac 0a123456 $rip $rmac #6 - test_arp $i$j$k $smac $sip $otherip #7 + externalip=`ip_to_hex 1 2 3 4` # Some other IP not in subnet + + test_arp $i$j$k $smac $sip $rip $rmac #4 + test_arp $i$j$k $smac $otherip $rip $rmac #5 + test_arp $i$j$k $smac $sip $otherip #6 + + # When rip is 192.168.33.254, ARP request from externalip won't be + # filtered, because 192.168.33.254 is configured to switch peer port + # for lrp33. + lrp33_rsp= + if test $i = 3 && test $j = 3; then + lrp33_rsp=$rmac + fi + test_arp $i$j$k $smac $externalip $rip $lrp33_rsp #7 + done done done
Currently ovn LR datapath responds ARP requests even if the ARP requestor's src IP doesn't belong to the LR port's subnets. This may generate unnecessary ARP responses and there could also be security concerns. This patch restricts the ARP response only if the requestor's IP matches the LR port's subnets. Signed-off-by: Han Zhou <hzhou8@ebay.com> --- ovn/northd/ovn-northd.8.xml | 12 ++++++++---- ovn/northd/ovn-northd.c | 8 ++++++-- tests/ovn.at | 23 +++++++++++++++++------ 3 files changed, 31 insertions(+), 12 deletions(-)