On Wed, Mar 9, 2016 at 1:51 PM, Russell Bryant <russ...@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. > > There are also some differences in the flows needed to account for > re-allowing a known connection after ct_mark 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_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] == 0), action=(next;) > table=2( ls_in_acl), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_mark[0] == 0), action=(next;) > table=2( ls_in_acl), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_mark[0] == 1)), action=(drop;) > table=2( ls_in_acl), priority= 2002, match=(!ct.new && ct.est && !ct.rpl && ct_mark[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_mark[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_mark[0] == 1)) && (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4)), action=(ct_commit(ct_mark=0); next;) > table=2( ls_in_acl), priority= 2002, match=(((ct.new && !ct.est) || (!ct.new && ct.est && !ct.rpl && ct_mark[0] == 1)) && (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip6)), action=(ct_commit(ct_mark=0); next;) > table=2( ls_in_acl), priority= 2001, match=((!ct.est || (ct.est && ct_mark[0] == 1)) && (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(drop;) > table=2( ls_in_acl), priority= 2001, match=(ct.est && ct_mark[0] == 0 && (inport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(ct_commit(ct_mark=1);) > table=2( ls_in_acl), priority= 1, match=(ip && (!ct.est || (ct.est && ct_mark[0] == 1))), action=(ct_commit(ct_mark=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_mark[0] == 0), action=(next;) > table=1( ls_out_acl), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_mark[0] == 0), action=(next;) > table=1( ls_out_acl), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_mark[0] == 1)), action=(drop;) > table=1( ls_out_acl), priority= 2002, match=(!ct.new && ct.est && !ct.rpl && ct_mark[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_mark[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_mark[0] == 1)) && (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 && icmp4)), action=(ct_commit(ct_mark=0); next;) > table=1( ls_out_acl), priority= 2002, match=(((ct.new && !ct.est) || (!ct.new && ct.est && !ct.rpl && ct_mark[0] == 1)) && (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip4 && tcp && tcp.dst == 22)), action=(ct_commit(ct_mark=0); next;) > table=1( ls_out_acl), priority= 2001, match=((!ct.est || (ct.est && ct_mark[0] == 1)) && (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(drop;) > table=1( ls_out_acl), priority= 2001, match=(ct.est && ct_mark[0] == 0 && (outport == "0a7409c8-d179-4915-9eb2-f53426ae16dd" && ip)), action=(ct_commit(ct_mark=1);) > table=1( ls_out_acl), priority= 1, match=(ip && (!ct.est || (ct.est && ct_mark[0] == 1))), action=(ct_commit(ct_mark=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_mark 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> > --- > ovn/northd/ovn-northd.8.xml | 56 ++++++++++--- > ovn/northd/ovn-northd.c | 189 +++++++++++++++++++++++++++++++++----------- > 2 files changed, 184 insertions(+), 61 deletions(-) > > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml > index 902b916..c8ddb1b 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> > @@ -154,15 +155,31 @@ > <p> > Logical flows in this table closely reproduce those in the > <code>ACL</code> table in the <code>OVN_Northbound</code> database > - for the <code>from-lport</code> direction. <code>allow</code> > - 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. > + for the <code>from-lport</code> direction. 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> > + <ul> > + <li> > + <code>allow</code> ACLs translate into logical flows with > + the <code>next;</code> action. If there are any stateful ACLs > + on this datapath, then <code>alloc</code> ACLs translate to > + <code>ct_commit; next;</code>. > + </li> > + <li> > + <code>allow-related</code> ACLs translate into logical > + flows with the <code>ct_commit(ct_mark=0); next;</code> actions > + for new connections and "next;" for existing connections. > + </li> > + <li> > + Other ACLs 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. > + </li> > + </ul> > > <p> > Ingress table 2 also contains a priority 0 flow with action > @@ -181,19 +198,32 @@ > </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 reply packets with > + <code>ct_mark=1</code> set meaning that the connection should > + no longer be allowed due to a policy change. Packets in the > + request direction are skipped here to let a newly created > + ACL re-allow this connection. > </li> > </ul> > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 35ec267..04364a6 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -1134,48 +1134,77 @@ 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", > - "ct_commit; next;"); > - ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, "ip", > - "ct_commit; next;"); > + * conntrack entry and would return "+invalid". > + * > + * We 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;" > + * > + * We also check for established connections that have ct_mark set > + * on them. That's a connection that was disallowed, but is now > + * allowed by policy again since it hit this default-allow flow. > + * We need to clear the mark to let the connection continue. > + * 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.est && ct_mark[0] == 1))", > + "ct_commit(ct_mark=0); next;"); > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, 1, > + "ip && (!ct.est || (ct.est && ct_mark[0] == 1))", > + "ct_commit(ct_mark=0); 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 > + * reply direction packets for 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.est && ct.rpl && ct_mark[0] == 1)", > + "drop;"); > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX, > - "ct.inv", "drop;"); > + "ct.inv || (ct.est && ct.rpl && ct_mark[0] == 1)", > + "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] == 0", > "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] == 0", > "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] == 0", > "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] == 0", > "next;"); > } > > @@ -1185,38 +1214,102 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows, struct hmap *ports) > bool ingress = !strcmp(acl->direction, "from-lport") ? true :false; > enum ovn_stage stage = ingress ? S_SWITCH_IN_ACL : S_SWITCH_OUT_ACL; > > - if (!strcmp(acl->action, "allow")) { > - /* If there are any stateful flows, we must even commit "allow" > - * actions. 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". */ > - const char *actions = has_stateful ? "ct_commit; next;" : "next;"; > - ovn_lflow_add(lflows, od, stage, > - acl->priority + OVN_ACL_PRI_OFFSET, > - acl->match, actions); > - } else if (!strcmp(acl->action, "allow-related")) { > + if (!strcmp(acl->action, "allow") > + || !strcmp(acl->action, "allow-related")) { > + if (!has_stateful) { > + /* If there are any stateful flows, we must even commit "allow" > + * actions. 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, stage, > + acl->priority + OVN_ACL_PRI_OFFSET, > + acl->match, "next;"); > + } else { > + struct ds match = DS_EMPTY_INITIALIZER; > + > + /* 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. > + * > + * It's also possible that a known connection was marked for > + * deletion after a policy was deleted, but the policy was > + * re-added while that connection is still known. We catch that > + * case here and un-set ct_mark to indicate that the connection > + * should be allowed to resume. > + */ > + ds_put_format(&match, "((ct.new && !ct.est)" > + " || (!ct.new && ct.est && !ct.rpl " > + "&& ct_mark[0] == 1)) " > + "&& (%s)", acl->match); > + ovn_lflow_add(lflows, od, stage, > + acl->priority + OVN_ACL_PRI_OFFSET, > + ds_cstr(&match), "ct_commit(ct_mark=0); next;"); > + > + /* Match on traffic in the request direction for an established > + * connection tracking entry that has not been marked for > + * deletion. 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" > + " && ct_mark[0] == 0 && (%s)", > + acl->match); > + ovn_lflow_add(lflows, od, stage, > + acl->priority + OVN_ACL_PRI_OFFSET, > + 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 || (ct.est && ct_mark[0] == 1)) " > + "&& (%s)", > + acl->match); > + ovn_lflow_add(lflows, od, stage, acl->priority + > + OVN_ACL_PRI_OFFSET, ds_cstr(&match), "drop;"); > + > + /* For an existing connection without ct_mark set, 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 && ct_mark[0] == 0 && (%s)", > + acl->match); > + ovn_lflow_add(lflows, od, stage, > + acl->priority + OVN_ACL_PRI_OFFSET, > + ds_cstr(&match), "ct_commit(ct_mark=1);"); > > - /* 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); > - 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"); > - ovn_lflow_add(lflows, od, stage, > - acl->priority + OVN_ACL_PRI_OFFSET, > - acl->match, "drop;"); > + 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 >
Acked-by: Han Zhou <zhou...@gmail.com> -- Best regards, Han _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
