| Message ID | cdbe5ce00e6bbd8766b15b1d17ec2f28ffb096dc.1515606160.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | add acl reject rule support introducing icmp4 action | expand |
On Wed, Jan 10, 2018 at 06:59:01PM +0100, Lorenzo Bianconi wrote: > Whenever the acl reject rule is hit send back an ICMPv4 destination > unreachable packet and do not handle reject rule as drop one > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> It's nice to finally get this right! Thank you. I wonder about the treatment for TCP connections. A connection attempt to a TCP port that is not listening ordinarily yields a TCP RST response. I do not know whether an ICMP reply is acceptable. Do you have any thoughts on that? I think that this should add an item to NEWS that describes the new feature. Thanks, Ben.
On Jan 23, Ben Pfaff wrote: > On Wed, Jan 10, 2018 at 06:59:01PM +0100, Lorenzo Bianconi wrote: > > Whenever the acl reject rule is hit send back an ICMPv4 destination > > unreachable packet and do not handle reject rule as drop one > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > It's nice to finally get this right! Thank you. > > I wonder about the treatment for TCP connections. A connection attempt > to a TCP port that is not listening ordinarily yields a TCP RST > response. I do not know whether an ICMP reply is acceptable. Do you > have any thoughts on that? > I agree, we need to add tcp feature, I was thinking to send a different patchset adding tcp stuff. Do you prefer to squash tcp action to this patchset or repin it with your comments? > I think that this should add an item to NEWS that describes the new > feature. > > Thanks, > > Ben. Regards, Lorenzo
On Fri, Feb 09, 2018 at 11:13:09AM +0100, Lorenzo Bianconi wrote: > On Jan 23, Ben Pfaff wrote: > > On Wed, Jan 10, 2018 at 06:59:01PM +0100, Lorenzo Bianconi wrote: > > > Whenever the acl reject rule is hit send back an ICMPv4 destination > > > unreachable packet and do not handle reject rule as drop one > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > > > It's nice to finally get this right! Thank you. > > > > I wonder about the treatment for TCP connections. A connection attempt > > to a TCP port that is not listening ordinarily yields a TCP RST > > response. I do not know whether an ICMP reply is acceptable. Do you > > have any thoughts on that? > > > > I agree, we need to add tcp feature, I was thinking to send a different patchset adding tcp stuff. > Do you prefer to squash tcp action to this patchset or repin it with your comments? It's OK with me to do TCP in a different patch set. It takes extra work to write code to generate TCP RSTs. I don't want to delay these patches by requiring that extra work now. I would like to see the TCP work done, however. For this patch set, do you think it is better to send ICMP for TCP or to continue treating reject as drop for TCP? Thanks, Ben.
> On Fri, Feb 09, 2018 at 11:13:09AM +0100, Lorenzo Bianconi wrote: >> On Jan 23, Ben Pfaff wrote: >> > On Wed, Jan 10, 2018 at 06:59:01PM +0100, Lorenzo Bianconi wrote: >> > > Whenever the acl reject rule is hit send back an ICMPv4 destination >> > > unreachable packet and do not handle reject rule as drop one >> > > >> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >> > >> > It's nice to finally get this right! Thank you. >> > >> > I wonder about the treatment for TCP connections. A connection attempt >> > to a TCP port that is not listening ordinarily yields a TCP RST >> > response. I do not know whether an ICMP reply is acceptable. Do you >> > have any thoughts on that? >> > >> >> I agree, we need to add tcp feature, I was thinking to send a different patchset adding tcp stuff. >> Do you prefer to squash tcp action to this patchset or repin it with your comments? > > It's OK with me to do TCP in a different patch set. It takes extra work > to write code to generate TCP RSTs. I don't want to delay these patches > by requiring that extra work now. I would like to see the TCP work > done, however. ack, I will send a new patchset soon > > For this patch set, do you think it is better to send ICMP for TCP or to > continue treating reject as drop for TCP? > I guess we can maintain the standard 'drop' action for TCP connections for the moment > Thanks, > > Ben. Thanks. Regards, Lorenzo
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 63ed97efb..9b7063f45 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -2890,13 +2890,18 @@ build_pre_acls(struct ovn_datapath *od, struct hmap *lflows) /* Ingress and Egress Pre-ACL Table (Priority 110). * - * Not to do conntrack on ND packets. */ + * Not to do conntrack on ND and ICMP destination + * unreachable packets. */ ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, "nd", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, "(nd_rs || nd_ra)", "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110, + "ip4 && icmp4.type == 3", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, "nd", "next;"); ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, "(nd_rs || nd_ra)", "next;"); + ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 110, + "ip4 && icmp4.type == 3", "next;"); /* Ingress and Egress Pre-ACL Table (Priority 100). * @@ -3256,28 +3261,30 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows) struct ds match = DS_EMPTY_INITIALIZER; struct ds actions = DS_EMPTY_INITIALIZER; - /* XXX Need to support "reject", treat it as "drop;" for now. */ - if (!strcmp(acl->action, "reject")) { - VLOG_INFO("reject is not a supported action"); - } - /* 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 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 drop it. */ + * we can simply reject/drop it. */ ds_put_format(&match, "(!ct.est || (ct.est && ct_label.blocked == 1)) " "&& (%s)", acl->match); build_acl_log(&actions, acl); - ds_put_cstr(&actions, "/* drop */"); - ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, - ds_cstr(&match), ds_cstr(&actions), - stage_hint); + if (!strcmp(acl->action, "reject")) { + ds_put_format(&actions, "reg0 = 0; " + "eth.dst <-> eth.src; ip4.dst <-> ip4.src; " + "icmp4 { outport <-> inport; %s };", + ingress ? "output;" + : "next(pipeline=ingress,table=0);"); + } else { + ds_put_cstr(&actions, "/* drop */"); + } + ovn_lflow_add(lflows, od, stage, + acl->priority + OVN_ACL_PRI_OFFSET, + ds_cstr(&match), ds_cstr(&actions)); /* For an existing connection without ct_label set, we've * encountered a policy change. ACLs previously allowed @@ -3288,7 +3295,8 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows) * specifying "next;", we implicitly drop the packet after * updating conntrack state. We would normally defer * ct_commit() to the "stateful" stage, but since we're - * dropping the packet, we go ahead and do it here. */ + * rejecting/dropping the packet, we go ahead and do it here. + */ ds_clear(&match); ds_clear(&actions); ds_put_format(&match, @@ -3296,22 +3304,38 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows) acl->match); ds_put_cstr(&actions, "ct_commit(ct_label=1/1); "); build_acl_log(&actions, acl); - ds_put_cstr(&actions, "/* drop */"); - ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, - ds_cstr(&match), ds_cstr(&actions), - stage_hint); - + if (!strcmp(acl->action, "reject")) { + ds_put_format(&match, " && ip4"); + ds_put_format(&actions, "reg0 = 0; " + "eth.dst <-> eth.src; ip4.dst <-> ip4.src; " + "icmp4 { outport <-> inport; %s };", + ingress ? "output;" + : "next(pipeline=ingress,table=0);"); + } else { + ds_put_cstr(&actions, "/* drop */"); + } + ovn_lflow_add(lflows, od, stage, + acl->priority + OVN_ACL_PRI_OFFSET, + ds_cstr(&match), ds_cstr(&actions)); } 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. */ + * so a "reject/drop" ACL is simply the "reject/drop" + * logical flow action in all cases. */ build_acl_log(&actions, acl); - ds_put_cstr(&actions, "/* drop */"); - ovn_lflow_add_with_hint(lflows, od, stage, - acl->priority + OVN_ACL_PRI_OFFSET, - acl->match, ds_cstr(&actions), - stage_hint); + if (!strcmp(acl->action, "reject")) { + ds_put_format(&match, "(%s) && ip4", acl->match); + ds_put_format(&actions, + "eth.dst <-> eth.src; ip4.dst <-> ip4.src; " + "icmp4 { outport <-> inport; %s };", + ingress ? "output;" + : "next(pipeline=ingress,table=0);"); + } else { + ds_put_cstr(&match, acl->match); + ds_put_cstr(&actions, "/* drop */"); + } + ovn_lflow_add(lflows, od, stage, + acl->priority + OVN_ACL_PRI_OFFSET, + ds_cstr(&match), ds_cstr(&actions)); } ds_destroy(&match); ds_destroy(&actions);
Whenever the acl reject rule is hit send back an ICMPv4 destination unreachable packet and do not handle reject rule as drop one Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- ovn/northd/ovn-northd.c | 76 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 50 insertions(+), 26 deletions(-)