Russell, shall this be merged? On Mon, Apr 11, 2016 at 5:44 PM, Ben Pfaff <b...@ovn.org> wrote:
> On Fri, Apr 01, 2016 at 11:20:21AM -0700, Russell Bryant wrote: > > Prior to this commit, once a connection had been committed to the > > connection tracker, the connection would continue to be allowed, even > > if the policy defined in the ACL table changed. This patch changes > > the implementation so that existing connections are affected by policy > > changes. > > > > The implementation is based on the suggested approach in this mailing > > list thread: > > > > http://openvswitch.org/pipermail/dev/2016-February/065716.html > > > > Instead of always allowing packets associated with an established > > connection, we now put all packets in the request direction through > > the flows generated based on OVN ACLs. If a packet associated with an > > established connection hits a "drop" ACL, that means we have > > encountered a policy change and should drop packets associated with > > this connection from now on. We handle this by setting "ct_label" on > > the associated connection tracking entry. > > > > These changes also account for re-allowing a known connection after > > ct_label had been set on it. This can happen if you delete an ACL and > > then re-create it while connection state is still known. > > > > The proposal on the mailing list also discussed the idea that > > ovn-controller could periodically sweep the connection tracker and > > delete entries with ct_label set. That is not implemented in this > > patch. Instead, we rely on connections dying since we're dropping > > its packets and then allowing the connection tracking entry to > > eventually time out. More proactively clearing them out could be a > > future enhancement. > > > > As a realistic example of how this works, consider this security policy > > from an OpenStack+OVN development environment. > > > > +---------+-----------------------+ > > | name | security_group_rules | > > +---------+-----------------------+ > > | default | egress, IPv4 | > > | | egress, IPv6 | > > | | ingress, IPv4, 22/tcp | > > | | ingress, IPv4, icmp | > > +---------+-----------------------+ > > > > The OpenStack Neutron plugin creates ACLs that drop traffic by default > > and higher priority ACLs for each type of traffic that is allowed. In > > this case, the ACLs for a port using the "default" security group are: > > > > from-lport 1002 (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && > ip4) allow-related > > from-lport 1002 (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && > ip6) allow-related > > from-lport 1001 (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && > ip) drop > > to-lport 1002 (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && > ip4 && icmp4) allow-related > > to-lport 1002 (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && > ip4 && tcp && tcp.dst == 22) allow-related > > to-lport 1001 (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && > ip) drop > > > > which results in the following logical flows: > > > > table=1( ls_in_pre_acl), priority= 100, match=(ip), > action=(ct_next;) > > table=1( ls_in_pre_acl), priority= 0, match=(1), action=(next;) > > table=2( ls_in_acl), priority=65535, match=(!ct.est && ct.rel && > !ct.new && !ct.inv && ct_label[0] == 0), action=(next;) > > table=2( ls_in_acl), priority=65535, match=(ct.est && !ct.rel && > !ct.new && !ct.inv && ct.rpl && ct_label[0] == 0), action=(next;) > > table=2( ls_in_acl), priority=65535, match=(ct.inv || (ct.est && > ct.rpl && ct_label[0] == 1)), action=(drop;) > > table=2( ls_in_acl), priority= 2002, match=(!ct.new && ct.est && > !ct.rpl && ct_label[0] == 0 && (inport == > "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4)), action=(next;) > > table=2( ls_in_acl), priority= 2002, match=(!ct.new && ct.est && > !ct.rpl && ct_label[0] == 0 && (inport == > "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip6)), action=(next;) > > table=2( ls_in_acl), priority= 2002, match=(((ct.new && !ct.est) > || (!ct.new && ct.est && !ct.rpl && ct_label[0] == 1)) && (inport == > "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4)), > action=(ct_commit(ct_label=0); next;) > > table=2( ls_in_acl), priority= 2002, match=(((ct.new && !ct.est) > || (!ct.new && ct.est && !ct.rpl && ct_label[0] == 1)) && (inport == > "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip6)), > action=(ct_commit(ct_label=0); next;) > > table=2( ls_in_acl), priority= 2001, match=((!ct.est || (ct.est > && ct_label[0] == 1)) && (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" > && ip)), action=(drop;) > > table=2( ls_in_acl), priority= 2001, match=(ct.est && > ct_label[0] == 0 && (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && > ip)), action=(ct_commit(ct_label=1);) > > table=2( ls_in_acl), priority= 1, match=(ip && (!ct.est || > (ct.est && ct_label[0] == 1))), action=(ct_commit(ct_label=0); next;) > > table=2( ls_in_acl), priority= 0, match=(1), action=(next;) > > > > table=0( ls_out_pre_acl), priority= 100, match=(ip), > action=(ct_next;) > > table=0( ls_out_pre_acl), priority= 0, match=(1), action=(next;) > > table=1( ls_out_acl), priority=65535, match=(!ct.est && ct.rel && > !ct.new && !ct.inv && ct_label[0] == 0), action=(next;) > > table=1( ls_out_acl), priority=65535, match=(ct.est && !ct.rel && > !ct.new && !ct.inv && ct.rpl && ct_label[0] == 0), action=(next;) > > table=1( ls_out_acl), priority=65535, match=(ct.inv || (ct.est && > ct.rpl && ct_label[0] == 1)), action=(drop;) > > table=1( ls_out_acl), priority= 2002, match=(!ct.new && ct.est && > !ct.rpl && ct_label[0] == 0 && (outport == > "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 && icmp4)), action=(next;) > > table=1( ls_out_acl), priority= 2002, match=(!ct.new && ct.est && > !ct.rpl && ct_label[0] == 0 && (outport == > "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 && tcp && tcp.dst == 22)), > action=(next;) > > table=1( ls_out_acl), priority= 2002, match=(((ct.new && !ct.est) > || (!ct.new && ct.est && !ct.rpl && ct_label[0] == 1)) && (outport == > "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 && icmp4)), > action=(ct_commit(ct_label=0); next;) > > table=1( ls_out_acl), priority= 2002, match=(((ct.new && !ct.est) > || (!ct.new && ct.est && !ct.rpl && ct_label[0] == 1)) && (outport == > "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 && tcp && tcp.dst == 22)), > action=(ct_commit(ct_label=0); next;) > > table=1( ls_out_acl), priority= 2001, match=((!ct.est || (ct.est > && ct_label[0] == 1)) && (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" > && ip)), action=(drop;) > > table=1( ls_out_acl), priority= 2001, match=(ct.est && > ct_label[0] == 0 && (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && > ip)), action=(ct_commit(ct_label=1);) > > table=1( ls_out_acl), priority= 1, match=(ip && (!ct.est || > (ct.est && ct_label[0] == 1))), action=(ct_commit(ct_label=0); next;) > > table=1( ls_out_acl), priority= 0, match=(1), action=(next;) > > > > One way I tested this by leaving ping running, ensuring that it was > > blocked when the rule for ICMP was deleted, and then re-allowed when > > the rule allowing ICMP was restored. In this case, the ICMP > > connection is still known by the connection tracker, but the flows > > ensure that ct_label gets reset back to 0. > > > > Reported-by: Xiao Li Xu <xiaol...@cn.ibm.com> > > Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1536080 > > Suggested-by: Justin Pettit <jpet...@ovn.org> > > Signed-off-by: Russell Bryant <russ...@ovn.org> > > Acked-by: Han Zhou <zhou...@gmail.com> > > Acked-by: Ben Pfaff <b...@ovn.org> > > I skimmed this again and don't have further comments. Thank you! > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
