[ovs-dev] [PATCH 3/3] ovn: Apply ACL changes to existing connections.
Han Zhou
zhouhan at gmail.com
Tue Mar 8 07:18:03 UTC 2016
On Wed, Mar 2, 2016 at 1:43 PM, Russell Bryant <russell at ovn.org> 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_mark" on
> the associated connection tracking entry.
>
> One difference in the implementation vs. the mailing list proposal is
> the use of "ct_mark" instead of "ct_label". The OpenFlow interface to
> both fields is identical, other than the size of the field (32-bit for
> ct_mark, 128-bit for ct_label). We see two uses for these fields in
> OVN: bit twiddling for state tracking (this patch) and possibly
> associating connection tracking entries with OVN ACLs. The easiest
> way to do that sort of association would be to use a UUID, which is
> 128-bits, so we reserve ct_label for that purpose and use the 32-bits
> of ct_mark for custom state tracking.
>
> The proposal on the mailing list also discussed the idea that
> ovn-controller could periodically sweep the connection tracker and
> delete entries with ct_mark 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_mark[0] == 0x0), action=(next;)
> table=2( ls_in_acl), priority=65535, match=(ct.est && !ct.rel &&
!ct.new && !ct.inv && ct.rpl && ct_mark[0] == 0x0), action=(next;)
> table=2( ls_in_acl), priority=65535, match=(ct.inv || ct_mark[0]
== 0x1), action=(drop;)
> table=2( ls_in_acl), priority= 2002, match=(!ct.new && ct.est &&
!ct.rpl && (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4)),
action=(next;)
> table=2( ls_in_acl), priority= 2002, match=(!ct.new && ct.est &&
!ct.rpl && (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip6)),
action=(next;)
> table=2( ls_in_acl), priority= 2002, match=(ct.new && !ct.est &&
(inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4)),
action=(ct_commit; next;)
> table=2( ls_in_acl), priority= 2002, match=(ct.new && !ct.est &&
(inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip6)),
action=(ct_commit; next;)
> table=2( ls_in_acl), priority= 2001, match=(!ct.est && (inport ==
"0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(drop;)
> table=2( ls_in_acl), priority= 2001, match=(ct.est && (inport ==
"0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(ct_commit(ct_mark =
1);)
> table=2( ls_in_acl), priority= 1, match=(ip && !ct.est),
action=(ct_commit; 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_mark[0] == 0x0), action=(next;)
> table=1( ls_out_acl), priority=65535, match=(ct.est && !ct.rel &&
!ct.new && !ct.inv && ct.rpl && ct_mark[0] == 0x0), action=(next;)
> table=1( ls_out_acl), priority=65535, match=(ct.inv || ct_mark[0]
== 0x1), action=(drop;)
> table=1( ls_out_acl), priority= 2002, match=(!ct.new && ct.est &&
!ct.rpl && (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 &&
icmp4)), action=(next;)
> table=1( ls_out_acl), priority= 2002, match=(!ct.new && ct.est &&
!ct.rpl && (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 &&
(outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 && icmp4)),
action=(ct_commit; next;)
> table=1( ls_out_acl), priority= 2002, match=(ct.new && !ct.est &&
(outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 && tcp && tcp.dst
== 22)), action=(ct_commit; next;)
> table=1( ls_out_acl), priority= 2001, match=(!ct.est && (outport
== "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(drop;)
> table=1( ls_out_acl), priority= 2001, match=(ct.est && (outport ==
"0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(ct_commit(ct_mark =
1);)
> table=1( ls_out_acl), priority= 1, match=(ip && !ct.est),
action=(ct_commit; next;)
> table=1( ls_out_acl), priority= 0, match=(1), action=(next;)
> ...
>
> I tested this by making sure an SSH connection was immediately
> interrupted if I deleted the rule allowing SSH. Once the rule is
> deleted, a packet will hit the following flow in the "ls_out_acl" stage:
>
> table=1( ls_out_acl), priority= 2001, match=(ct.est && (outport ==
"0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(ct_commit(ct_mark =
1);)
>
> Now that we have set "ct_mark" on this established flow in the
> connection tracker, all further packets associated with this connection
> will get immediately dropped.
>
> 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>
> ---
> ovn/northd/ovn-northd.8.xml | 33 ++++++++----
> ovn/northd/ovn-northd.c | 119
+++++++++++++++++++++++++++++++++-----------
> 2 files changed, 114 insertions(+), 38 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index 902b916..01b9415 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -146,7 +146,8 @@
> in table 2. It contains a priority-0 flow that simply moves
> traffic to table 2. If stateful ACLs are used in the logical
> datapath, a priority-100 flow is added that sends IP packets to
> - the connection tracker before advancing to table 2.
> + the connection tracker with the <code>ct_next;</code> action
> + before advancing to table 2.
> </p>
>
> <h3>Ingress table 2: <code>from-lport</code> ACLs</h3>
> @@ -158,10 +159,13 @@
> ACLs translate into logical flows with the <code>next;</code>
> action, <code>allow-related</code> ACLs translate into logical
> flows with the <code>ct_commit; next;</code> actions, other ACLs
> - translate to <code>drop;</code>. The <code>priority</code> values
> - from the <code>ACL</code> table have a limited range and have 1000
> - added to them to leave room for OVN default flows at both higher
> - and lower priorities.
> + translate to <code>drop;</code> for new or untracked connections
and
> + <code>ct_commit(ct_mark=1);</code> for known connections. Setting
> + <code>ct_mark</code> marks a connection as one that was previously
> + allowed, but should no longer be allowed due to a policy change.
> + The <code>priority</code> values from the <code>ACL</code> table
> + have a limited range and have 1000 added to them to leave room
> + for OVN default flows at both higher and lower priorities.
> </p>
>
> <p>
> @@ -181,19 +185,30 @@
> </li>
>
> <li>
> - A priority-65535 flow that allows any traffic that has been
> - committed to the connection tracker (i.e., established flows).
> + A priority-65535 flow that allows any traffic in the reply
> + direction for a connection that has been committed to the
> + connection tracker (i.e., established flows), as long as
> + the committed flow does not have <code>ct_mark=1</code> set.
> + We only handle traffic in the reply direction here because
> + we want all packets going in the request direction to still
> + go through the flows that implement the currently defined
> + policy based on ACLs. If a connection is no longer allowed by
> + policy, <code>ct_mark</code> will get set and packets in the
> + reply direction will no longer be allowed, either.
> </li>
>
> <li>
> A priority-65535 flow that allows any traffic that is considered
> related to a committed flow in the connection tracker (e.g., an
> - ICMP Port Unreachable from a non-listening UDP port).
> + ICMP Port Unreachable from a non-listening UDP port), as long
> + as the committed flow does not have <code>ct_mark=1</code> set.
> </li>
>
> <li>
> A priority-65535 flow that drops all traffic marked by the
> - connection tracker as invalid.
> + connection tracker as invalid or has <code>ct_mark=1</code>
> + meaning that the connection should no longer be allowed due
> + to a policy change.
> </li>
> </ul>
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 35ec267..8a50d85 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1134,48 +1134,66 @@ build_acls(struct ovn_datapath *od, struct hmap
*lflows, struct hmap *ports)
> * commit IP flows. This is because, while the initiater's
> * direction may not have any stateful rules, the server's may
> * and then its return traffic would not have an associated
> - * conntrack entry and would return "+invalid". */
> - ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, "ip",
> + * conntrack entry and would return "+invalid".
> + *
> + * We only use ct_commit; for a connection that is not already
known by
> + * the connection tracker. Once a connection is committed,
subsequent
> + * packets will hit the flow at priority 0 that just uses
"next;".
> + */
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, 1, "ip && !ct.est",
> "ct_commit; next;");
> - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, "ip",
> + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, "ip && !ct.est",
> "ct_commit; next;");
>
> /* Ingress and Egress ACL Table (Priority 65535).
> *
> - * Always drop traffic that's in an invalid state. This is
> - * enforced at a higher priority than ACLs can be defined. */
> + * Always drop traffic that's in an invalid state. Also drop
> + * connections that have been marked for deletion (bit 0 of
ct_mark is
> + * set).
> + *
> + * This is enforced at a higher priority than ACLs can be
defined. */
> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
> - "ct.inv", "drop;");
> + "ct.inv || ct_mark[0] == 0x1", "drop;");
> ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
> - "ct.inv", "drop;");
> + "ct.inv || ct_mark[0] == 0x1", "drop;");
>
> /* Ingress and Egress ACL Table (Priority 65535).
> *
> - * Always allow traffic that is established to a committed
> - * conntrack entry. This is enforced at a higher priority than
> + * Allow reply traffic that is part of an established
> + * conntrack entry that has not been marked for deletion
> + * (bit 0 of ct_mark). We only match traffic in the
> + * reply direction because we want traffic in the request
> + * direction to hit the currently defined policy from ACLs.
> + *
> + * This is enforced at a higher priority than
> * ACLs can be defined. */
> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
> - "ct.est && !ct.rel && !ct.new && !ct.inv",
> + "ct.est && !ct.rel && !ct.new && !ct.inv "
> + "&& ct.rpl && ct_mark[0] == 0x0",
> "next;");
> ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
> - "ct.est && !ct.rel && !ct.new && !ct.inv",
> + "ct.est && !ct.rel && !ct.new && !ct.inv "
> + "&& ct.rpl && ct_mark[0] == 0x0",
> "next;");
>
> /* Ingress and Egress ACL Table (Priority 65535).
> *
> - * Always allow traffic that is related to an existing conntrack
> - * entry. This is enforced at a higher priority than ACLs can
> - * be defined.
> + * Allow traffic that is related to an existing conntrack entry
that
> + * has not been marked for deletion (bit 0 of ct_mark).
> + *
> + * This is enforced at a higher priority than ACLs can be
defined.
> *
> * NOTE: This does not support related data sessions (eg,
> * a dynamically negotiated FTP data channel), but will allow
> * related traffic such as an ICMP Port Unreachable through
> * that's generated from a non-listening UDP port. */
> ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
> - "!ct.est && ct.rel && !ct.new && !ct.inv",
> + "!ct.est && ct.rel && !ct.new && !ct.inv "
> + "&& ct_mark[0] == 0x0",
> "next;");
> ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
> - "!ct.est && ct.rel && !ct.new && !ct.inv",
> + "!ct.est && ct.rel && !ct.new && !ct.inv "
> + "&& ct_mark[0] == 0x0",
> "next;");
> }
>
> @@ -1198,25 +1216,68 @@ build_acls(struct ovn_datapath *od, struct hmap
*lflows, struct hmap *ports)
> } else if (!strcmp(acl->action, "allow-related")) {
> struct ds match = DS_EMPTY_INITIALIZER;
>
> - /* Commit the connection tracking entry, which allows all
> - * other traffic related to this entry to flow due to the
> - * 65535 priority flow defined earlier. */
> - ds_put_format(&match, "ct.new && (%s)", acl->match);
> + /* Commit the connection tracking entry if its a new
connection that
> + * matches this ACL. After this commit, the reply traffic is
> + * allowed by a flow we create at priority 65535, defined
> + * earlier. */
> + ds_put_format(&match, "ct.new && !ct.est && (%s)",
acl->match);
> ovn_lflow_add(lflows, od, stage,
> acl->priority + OVN_ACL_PRI_OFFSET,
> ds_cstr(&match), "ct_commit; next;");
>
> - ds_destroy(&match);
> - } else if (!strcmp(acl->action, "drop")) {
> - ovn_lflow_add(lflows, od, stage,
> - acl->priority + OVN_ACL_PRI_OFFSET,
> - acl->match, "drop;");
> - } else if (!strcmp(acl->action, "reject")) {
> - /* xxx Need to support "reject". */
> - VLOG_INFO("reject is not a supported action");
> + /* Match on traffic in the request direction for an
established
> + * connection tracking entry. There is no need to commit
here, so
> + * we can just proceed to the next table. We use this to
ensure
> + * that this connection is still allowed by the currently
defined
> + * policy. */
> + ds_clear(&match);
> + ds_put_format(&match, "!ct.new && ct.est && !ct.rpl && (%s)",
> + acl->match);
> ovn_lflow_add(lflows, od, stage,
> acl->priority + OVN_ACL_PRI_OFFSET,
> - acl->match, "drop;");
> + ds_cstr(&match), "next;");
> +
> + ds_destroy(&match);
> + } else if (!strcmp(acl->action, "drop")
> + || !strcmp(acl->action, "reject")) {
> + struct ds match = DS_EMPTY_INITIALIZER;
> + /* XXX Need to support "reject", treat it as "drop;" for
now. */
> +
> + if (has_stateful) {
> + /* The implementation of "drop" differs if stateful ACLs
are in
> + * use for this datapath. In that case, the actions
differ
> + * depending on whether the connection was previous
committed
> + * to the connection tracker with ct_commit.
> + *
> + * If the packet is not part of an established
connection, then
> + * we can simply drop it. */
> + ds_put_format(&match, "!ct.est && (%s)", acl->match);
> + ovn_lflow_add(lflows, od, stage, acl->priority +
> + OVN_ACL_PRI_OFFSET, ds_cstr(&match), "drop;");
> +
> + /* For an existing connection, we've encountered a policy
> + * change. ACLs previously allowed this connection and we
> + * committed the connection tracking entry. Current
policy
> + * says that we should drop this connection. First, we
set bit
> + * 0 of ct_mark to indicate that this connection is set
for
> + * deletion. By not specifying "next;", we implicitly
drop the
> + * packet after updating conntrack state. */
> +
> + ds_clear(&match);
> + ds_put_format(&match, "ct.est && (%s)", acl->match);
> + ovn_lflow_add(lflows, od, stage,
> + acl->priority + OVN_ACL_PRI_OFFSET,
> + ds_cstr(&match), "ct_commit(ct_mark =
1);");
> +
> + ds_destroy(&match);
> + } else {
> + /* There are no stateful ACLs in use on this datapath,
> + * so a "drop" ACL is simply the "drop" logical flow
action
> + * in all cases. */
> + ovn_lflow_add(lflows, od, stage,
> + acl->priority + OVN_ACL_PRI_OFFSET,
> + acl->match, "drop;");
> + }
> }
> }
> }
> --
> 2.5.0
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
Hi Russell,
There is a small problem of this patch. For an established connection, if
the ACL rule allowing the connection is deleted, it will take effect by
setting the mark to 1 in CT table. However, if we add the ACL back before
the connection is dead, it will fail to connect because the mark = 1 is not
cleared. This can be verified by an ICMP ping test:
1. with ACL allowing the src IP, ping the port's IP, and keep the ping
session
2. remove ACL, the ping session blocked, but keep it
3. add the ACL back, ping session still blocked, until starting a new ping
session
I think we can set ct_commit(mark = 0) explicitly when applying the ACL.
--
Best regards,
Han
More information about the dev
mailing list