On Fri, Nov 8, 2019 at 10:16 PM venugopal iyer via dev <[email protected]> wrote: > > Sorry about that, hopefully this is better (else, I'll just attach it the > next time) > Assuming :Logical Switch (ls1) with ls1_vm1 : 02:ac:10:ff:00:11/172.16.255.11 > ls1_vm2 : > 02:ac:10:ff:00:22/172.16.255.22 ls1_vm3 > : 02:ac:10:ff:00:33/172.16.255.33 > Looking to : To have MAC ACLs between vm1 and vm2 so they can't talk to each > other. > Using: > # ovn-nbctl create Address_Set name=macs > addresses=\"02:ac:10:ff:00:11\",\"02:ac:10:ff:00:22\" # ovn-nbctl create > Port_Group name=l2pg # ovn-nbctl add Port_Group l2pg ports <ls1_vm1> # > ovn-nbctl add Port_Group l2pg ports <ls2_vm1> # ovn-nbctl acl-add ls1 > to-lport 32767 "outport == @l2pg && eth.src == \$macs" drop > What we are seeing: The above, by itself, will limit L3 and L2 between vm1 > and vm2, but not vm3 (as expected) > lflow: table=4 (ls_out_acl ), priority=33767, match=(outport == > @l2pg && eth.src == $macs), action=(/* drop */) > dpflow (sending a arbit packet of ether type 0x7a05): > > (0),in_port(vm1),eth(src=02:ac:10:ff:00:11,dst=02:ac:10:ff:00:22),eth_type(0x7a05), > packets:0, bytes:0, used:never, actions:drop However, if there is a > stateful rule, e.g.: > # ovn-nbctl acl-add ls1 from-lport 2000 "inport == \"ls1-vm3\" && ip" > allow-related > Then, the behavior differs lflow: table=6 (ls_in_acl ), > priority=3000 , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0 > && (inport == "ls1-vm3" && ip)), action=(next;) table=6 (ls_in_acl > ), priority=3000 , match=(((ct.new && !ct.est) || (!ct.new && ct.est && > !ct.rpl && ct_label.blocked == 1)) && (inport == "ls1-vm3" && ip)), > action=(reg0[1] = 1; next;) table=4 (ls_out_acl ), priority=33767, > match=((!ct.est || (ct.est && ct_label.blocked == 1)) && (outport == @l2pg && > eth.src == $macs)), action=(/* drop */) table=4 (ls_out_acl ), > priority=33767, match=(ct.est && ct_label.blocked == 0 && (outport == @l2pg > && eth.src == $macs)), action=(ct_commit(ct_label=1/1); /* drop */) > *****Note: the condition !ct.new && ct.est && !ct.rpl && ct_label.blocked == > 0****** > > This does block L3 traffic: > > dpctl flow for ICMP > recirc_id(0),in_port(vm1),eth(src=02:ac:10:ff:00:11),eth_type(0x0800),ipv4(proto=1,frag=no),icmp(type=8/0xf8), > packets:6, bytes:588, used:0.864s, actions:ct(zone=1),recirc(0x1) > recirc_id(0x1),in_port(vm1),ct_state(+new-est-rel-rpl-inv+trk),ct_label(0/0x1),eth(dst=02:ac:10:ff:00:22),eth_type(0x0800),ipv4(proto=1,frag=no),icmp(type=8/0xf8), > packets:6, bytes:588, used:0.864s, > actions:ct(commit,zone=1,label=0/0x1),ct(zone=4),recirc(0x2) > recirc_id(0x2),in_port(vm1),ct_state(+new-est-rel-rpl-inv+trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11),eth_type(0x0800),ipv4(frag=no), > packets:6, bytes:588, used:0.864s, actions:drop > recirc_id(0),in_port(vm1),ct_state(-new-est-rel-rpl-inv-trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11,dst=02:ac:10:ff:00:22),eth_type(0x0806),arp(sha=02:ac:10:ff:00:11), > packets:0, bytes:0, used:never, actions:vm2 > recirc_id(0),in_port(vm2),ct_state(-new-est-rel-rpl-inv-trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:22,dst=02:ac: 10:ff:00:11),eth_type(0x0806),arp(sha=02:ac:10:ff:00:22), packets:0, bytes:0, used:never, actions:vm1 > But, it doesn't block L2, see the ARP going through, without the stateful > rule the ARP would have been dropped too. > From what i understand it is because of the condition "the condition > !ct.new && ct.est && !ct.rpl && ct_label.blocked == 0", which specifically > looks fo "+trk" which makes sense for IP packets, but probably not for non-IP > packets, i.e. the ARP rule above has "-trk". Hence, I believe it gets > through. > Proposed changes: If my understanding is right (and objective to remove > the inconsistency between when there are stateful rules and not, w.r.t. L2 > packets), > we should have the condition as: # git diffdiff --git > a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.cindex 6c6de2afd..b0f524531 > 100644--- a/ovn/northd/ovn-northd.c+++ b/ovn/northd/ovn-northd.c@@ -4191,10 > +4191,13 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, > * depending on whether the connection was previously committed * > to the connection tracker with ct_commit. */ if (has_stateful) {- > /* If the packet is not part of an established connection, then- > * we can simply reject/drop it. */++ /* If the packet is not > tracked or part of an established connection, then+ * we can > simply reject/drop it.+ */ ds_put_cstr(&match,- > "(!ct.est || (ct.est && ct_label.blocked == 1))");+ > "(!ct.trk || !ct.est || (ct.est && ct_label.blocked == 1))"); > if (!strcmp(acl->action, "reject")) { > build_reject_acl_r ules(od, lflows, stage, acl, &match, &actions); > with this the rules above look like: > lflow: table=6 (ls_in_acl ), priority=3000 , match=(!ct.new && > ct.est && !ct.rpl && ct_label.blocked == 0 && (inport == "ls1-vm3" && ip)), > action=(next;) table=6 (ls_in_acl ), priority=3000 , > match=(((ct.new && !ct.est) || (!ct.new && ct.est && !ct.rpl && > ct_label.blocked == 1)) && (inport == "ls1-vm3" && ip)), action=(reg0[1] = 1; > next;) table=4 (ls_out_acl ), priority=33767, match=((!ct.trk || > !ct.est || (ct.est && ct_label.blocked == 1)) && (outport == @l2pg && eth.src > == $macs)), action=(/* drop */) table=4 (ls_out_acl ), > priority=33767, match=(ct.est && ct_label.blocked == 0 && (outport == @l2pg > && eth.src == $macs)), action=(ct_commit(ct_label=1/1); /* drop */) > dpctl rules: > > recirc_id(0),in_port(vm1),ct_state(-new-est-rel-rpl-inv-trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11,dst=01:00:00:00:00:00/01:00:00:00:00:00),eth_type(0x0806),arp(sha=02:ac:10:ff:00:11), > packets:1, bytes:42, used:0.548s, actions:vm3 > ARP doesn't resolve. Using the arbit packet with of ether type 0x7a05 > > recirc_id(0),in_port(vm1),ct_state(-new-est-rel-rpl-inv-trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11,dst=02:ac:10:ff:00:22),eth_type(0x7a05), > packets:0, bytes:0, used:never, actions:drop > > Let me know if I am missing anything, else I can look at proceeding with the > above change.
Hi Venu, I haven't come across the use case of MAC ACLs. And I think this use case was not considered in the initial design. Would you mind submitting a patch (an RFC patch is also fine) so that we can discuss further ? Thanks Numan > > > thanks, > -venu > > > > On Friday, November 8, 2019, 03:09:37 AM PST, Numan Siddique > <[email protected]> wrote: > > Your email is really cluttered. > > Could you please send again with plain text ? > > Thanks > Numan > > > On Fri, Nov 8, 2019 at 6:22 AM venugopal iyer via dev > <[email protected]> wrote: > > > > [Pardon the length of the mail; have left out the ofctl flows, but if it > > helps i can send them too] > > Assuming : logical Switch (ls1) with > > ls1_vm1 : 02:ac:10:ff:00:11/172.16.255.11 > > ls1_vm2 : 02:ac:10:ff:00:22/172.16.255.22 > > ls1_vm3 : 02:ac:10:ff:00:33/172.16.255.33 > > Looking to : have MAC ACLs between vm1 and vm2 so they can't talk to > > each other. > > Using: # ovn-nbctl create Address_Set name=macs > > addresses=\"02:ac:10:ff:00:11\",\"02:ac:10:ff:00:22\" # ovn-nbctl > > create Port_Group name=l2pg # ovn-nbctl add Port_Group l2pg ports > > <ls1_vm1> # ovn-nbctl add Port_Group l2pg ports <ls2_vm2> # > > ovn-nbctl acl-add ls1 to-lport 32767 "outport == @l2pg && eth.src == > > \$macs" drop > > What we are seeing: The above, by itself, will limit L3 and L2 between > > vm1 and vm2, but not vm3 (as expected) > > lflow:-------------------- table=4 (ls_out_acl ), > > priority=33767, match=(outport == @l2pg && eth.src == $macs), action=(/* > > drop */) > > dpflow (sending a arbit packet of ether type 0x7a05):---------------------- > > > > (0),in_port(vm1),eth(src=02:ac:10:ff:00:11,dst=02:ac:10:ff:00:22),eth_type(0x7a05), > > packets:0, bytes:0, used:never, actions:drop > > However, if there is a stateful rule, e.g.: # ovn-nbctl acl-add ls1 > > from-lport 2000 "inport == \"ls1-vm3\" && ip" allow-related > > Then, the behavior differs > > lflow:------------------- table=6 (ls_in_acl ), > > priority=3000 , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == > > 0 && (inport == "ls1-vm3" && ip)), action=(next;) table=6 > > (ls_in_acl ), priority=3000 , match=(((ct.new && !ct.est) || > > (!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1)) && (inport == > > "ls1-vm3" && ip)), action=(reg0[1] = 1; next;) table=4 (ls_out_acl > > ), priority=33767, match=((!ct.est || (ct.est && ct_label.blocked == > > 1)) && (outport == @l2pg && eth.src == $macs)), action=(/* drop */) > > table=4 (ls_out_acl ), priority=33767, match=(ct.est && > > ct_label.blocked == 0 && (outport == @l2pg && eth.src == $macs)), > > action=(ct_commit(ct_label=1/1); /* drop */) > > *****Note: the condition !ct.new && ct.est && !ct.rpl && ct_label.blocked > > == 0****** > > This does block L3 traffic: > > dpctl flow for ICMP------------------------- > > recirc_id(0),in_port(vm1),eth(src=02:ac:10:ff:00:11),eth_type(0x0800),ipv4(proto=1,frag=no),icmp(type=8/0xf8), > > packets:6, bytes:588, used:0.864s, actions:ct(zone=1),recirc(0x1) > > recirc_id(0x1),in_port(vm1),ct_state(+new-est-rel-rpl-inv+trk),ct_label(0/0x1),eth(dst=02:ac:10:ff:00:22),eth_type(0x0800),ipv4(proto=1,frag=no),icmp(type=8/0xf8), > > packets:6, bytes:588, used:0.864s, > > actions:ct(commit,zone=1,label=0/0x1),ct(zone=4),recirc(0x2) > > recirc_id(0x2),in_port(vm1),ct_state(+new-est-rel-rpl-inv+trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11),eth_type(0x0800),ipv4(frag=no), > > packets:6, bytes:588, used:0.864s, actions:drop > > recirc_id(0),in_port(vm1),ct_state(-new-est-rel-rpl-inv-trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11,dst=02:ac:10:ff:00:22),eth_type(0x0806),arp(sha=02:ac:10:ff:00:11), > > packets:0, bytes:0, used:never, actions:vm2 > > recirc_id(0),in_port(vm2),ct_state(-new-est-rel-rpl-inv-tr k),ct_label(0/0x1),eth(src=02:ac:10:ff:00:22,dst=02:ac:10:ff:00:11),eth_type(0x0806),arp(sha=02:ac:10:ff:00:22), packets:0, bytes:0, used:never, actions:vm1 > > > > But, it doesn't block L2, see the ARP going through, without the stateful > > rule the ARP would have been dropped too. > > Using the arbit packet: > > > > recirc_id(0),in_port(vm1),ct_state(-new-est-rel-rpl-inv-trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11,dst=02:ac:10:ff:00:22),eth_type(0x7a05), > > packets:0, bytes:0, used:never, actions:vm2 > > > > > > From what i understand it is because of the condition "!ct.new && ct.est && > > !ct.rpl && ct_label.blocked == 0", which implies trk, I suppose, which > > makes sense for IP packets, but probably not for non-IP packets, i.e. the > > rules above have "-trk". Hence, I believe it gets through > > > > Proposed changes:================ > > If my understanding is right (and objective to remove the inconsistency > > between when there are stateful rules and not, w.r.t. L2 packets), we > > couldhave the condition as: > > # git diffdiff --git a/ovn/northd/ovn-northd.c > > b/ovn/northd/ovn-northd.cindex 6c6de2afd..b0f524531 100644--- > > a/ovn/northd/ovn-northd.c+++ b/ovn/northd/ovn-northd.c@@ -4191,10 +4191,13 > > @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od, * > > depending on whether the connection was previously committed * to > > the connection tracker with ct_commit. */ if (has_stateful) {- > > /* If the packet is not part of an established connection, then- > > * we can simply reject/drop it. */++ /* If the packet is > > not tracked or part of an established connection, then+ * we can > > simply reject/drop it.+ */ ds_put_cstr(&match,- > > "(!ct.est || (ct.est && ct_label.blocked == 1))");+ > > "(!ct.trk || !ct.est || (ct.est && ct_label.blocked == > > 1))"); if (!strcmp(acl->action, "reject")) { > > build_reject_acl_rules(od, lflows, stage, acl, &match, &actions); > > > > with this the rules above look like: > > lflow---------- table=6 (ls_in_acl ), priority=3000 , > > match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0 && (inport == > > "ls1-vm3" && ip)), action=(next;) table=6 (ls_in_acl ), > > priority=3000 , match=(((ct.new && !ct.est) || (!ct.new && ct.est && > > !ct.rpl && ct_label.blocked == 1)) && (inport == "ls1-vm3" && ip)), > > action=(reg0[1] = 1; next;) table=4 (ls_out_acl ), > > priority=33767, match=((!ct.trk || !ct.est || (ct.est && ct_label.blocked > > == 1)) && (outport == @l2pg && eth.src == $macs)), action=(/* drop */) > > table=4 (ls_out_acl ), priority=33767, match=(ct.est && > > ct_label.blocked == 0 && (outport == @l2pg && eth.src == $macs)), > > action=(ct_commit(ct_label=1/1); /* drop */) > > dpctl rules------------ > > recirc_id(0),in_port(vm1),ct_state(-new-est-rel-rpl-inv-trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11,dst=01:00:00:00:00:00/01:00:00:00:00:00),eth_type(0x0806),arp(sha=02:ac:10:ff:00:11), > > packets:1, bytes:42, used:0.548s, actions:vm3 > > ARP doesn't resolve. Using the arbit packet with of ether type 0x7a05 > > > > recirc_id(0),in_port(vm1),ct_state(-new-est-rel-rpl-inv-trk),ct_label(0/0x1),eth(src=02:ac:10:ff:00:11,dst=02:ac:10:ff:00:22),eth_type(0x7a05), > > packets:0, bytes:0, used:never, actions:drop > > We have a case for using such ACL, let me know if I am missing anything, > > else I can look at proceeding with the above change. > > > > thanks, > > -venu > > > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
