[ovs-dev] [PATCH v3 3/3] ovn: Apply ACL changes to existing connections.

[Ben Pfaff]

On Mon, Mar 21, 2016 at 10:54:59AM -0400, 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 <xiaolixu at cn.ibm.com>
> Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1536080
> Suggested-by: Justin Pettit <jpettit at ovn.org>
> Signed-off-by: Russell Bryant <russell at ovn.org>
> Acked-by: Han Zhou <zhouhan at gmail.com>

Thanks for doing all this work!

Acked-by: Ben Pfaff <blp at ovn.org>