Daniele and I discussed
1) Seems ok in that there is security at the VM LP so weakening the
Check at the router port for ICMP seems ok.
2) The same applies to V6 ?
Thanks
On 3/8/17, 1:32 PM, "ovs-dev-boun...@openvswitch.org on behalf of Ben Pfaff"
<ovs-dev-boun...@openvswitch.org on behalf of b...@ovn.org> wrote:
Hi Darrell and Daniele, do one of you have an opinion on whether this is
the right approach?
Thanks,
Ben.
On Mon, Feb 27, 2017 at 11:29:14AM +0530, nusid...@redhat.com wrote:
> From: Numan Siddique <nusid...@redhat.com>
>
> Presently, the icmp4 requests to the router gateway ip are sent to the
> connectiont tracker, but the icmp4 reply packets responded by
> 'lr_in_ip_input' stage are not sent to the connection tracker.
> Also no zone ids are assigned for the router ports. Because of which
> the icmp4 request packets in the connection tracker will be in the
> UNREPLIED state. If the CMS has added ACLs to drop packets which
> are not in ESTABLISHED state, the icmp4 replies will be dropped.
>
> To fix this issue, this patch adds a priority-110 flow in 'ls_in_pre_acl'
> stage which doesn't set reg0[0] = 1 for icmp4 request packets destined
> to the router gateway ips.
>
> Alternate solution would be to assign zone ids for the router ports
> and send the packets from the router ports to the connection tracker.
>
> The approach used in this patch seems to be simpler.
>
> Signed-off-by: Numan Siddique <nusid...@redhat.com>
> ---
> ovn/northd/ovn-northd.8.xml | 29 +++++++++++---
> ovn/northd/ovn-northd.c | 92
+++++++++++++++++++++++++++------------------
> 2 files changed, 79 insertions(+), 42 deletions(-)
>
> diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml
> index ab8fd88..06465ff 100644
> --- a/ovn/northd/ovn-northd.8.xml
> +++ b/ovn/northd/ovn-northd.8.xml
> @@ -245,11 +245,30 @@
> <p>
> This table prepares flows for possible stateful ACL processing in
> ingress table <code>ACLs</code>. It contains a priority-0 flow
that
> - simply moves traffic to the next table. If stateful ACLs are used
in the
> - logical datapath, a priority-100 flow is added that sets a hint
> - (with <code>reg0[0] = 1; next;</code>) for table
> - <code>Pre-stateful</code> to send IP packets to the connection
tracker
> - before eventually advancing to ingress table <code>ACLs</code>.
> + simply moves traffic to the next table. It adds the following
flows if
> + stateful ACLs are used in the logical datapath.
> +
> + <ul>
> + <li>
> + A priority-100 flow is added that sets a hint
> + (with <code>reg0[0] = 1; next;</code>) for table
> + <code>Pre-stateful</code> to send IP packets to the connection
tracker
> + before eventually advancing to ingress table <code>ACLs</code>.
> + </li>
> +
> + <li>
> + A priority-110 flow which doesn't set the connection tracking
hint for
> + the packets from the router ports.
This above comment is not needed as there is only the below flow added.
> + </li>
> +
> + <li>
> + A priority-110 flow which doesn't set the connection tracking
hint
> + for the packets with the match
> + <code>ip4 && icmp4 && ip4.dst =
{<var>R</var>}</code>
> + where <var>R</var> is the IPv4 address(es) of the router ports
> + connected to the logical datapath.
> + </li>
> + </ul>
> </p>
>
> <h3>Ingress Table 4: Pre-LB</h3>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 03dc850..4fc86f4 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -2339,6 +2339,43 @@ has_stateful_acl(struct ovn_datapath *od)
> }
>
> static void
> +op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool
add_bcast)
> +{
> + if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) {
> + ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s);
> + return;
> + }
> +
> + ds_put_cstr(ds, "{");
> + for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> + ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s);
> + if (add_bcast) {
> + ds_put_format(ds, "%s, ",
op->lrp_networks.ipv4_addrs[i].bcast_s);
> + }
> + }
> + ds_chomp(ds, ' ');
> + ds_chomp(ds, ',');
> + ds_put_cstr(ds, "}");
> +}
> +
> +static void
> +op_put_v6_networks(struct ds *ds, const struct ovn_port *op)
> +{
> + if (op->lrp_networks.n_ipv6_addrs == 1) {
> + ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s);
> + return;
> + }
> +
> + ds_put_cstr(ds, "{");
> + for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> + ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s);
> + }
> + ds_chomp(ds, ' ');
> + ds_chomp(ds, ',');
> + ds_put_cstr(ds, "}");
> +}
> +
> +static void
> build_pre_acls(struct ovn_datapath *od, struct hmap *lflows)
> {
> bool has_stateful = has_stateful_acl(od);
> @@ -2378,6 +2415,24 @@ build_pre_acls(struct ovn_datapath *od, struct
hmap *lflows)
>
> ds_destroy(&match_in);
> ds_destroy(&match_out);
> +
> + /* The icmp4 request to the router ip (eg. ip4.dst =
10.0.0.1) will
> + * be sent to the CT, but the reply from the router ip is
not sent
> + * to the CT. Also the zone id's are not assigned for the
router
> + * ports. Because of which the icmp request packet in the CT
will
> + * be in the UNREPLIED state.
> + * The icmp4 reply packets could be dropped if the CMS has
added
> + * ACLs to drop packets which are not in ct.est state.
> + * So, don't send the icpmp4 request packet for the router
ips
> + * to the CT.
> + */
> + if (op->peer && op->peer->lrp_networks.n_ipv4_addrs) {
> + struct ds match = DS_EMPTY_INITIALIZER;
> + ds_put_format(&match, "ip4 && icmp4 && ip4.dst == ");
> + op_put_v4_networks(&match, op->peer, false);
> + ovn_lflow_add(lflows, od, S_SWITCH_IN_PRE_ACL, 110,
> + ds_cstr(&match), "next;");
> + }
> }
> /* Ingress and Egress Pre-ACL Table (Priority 110).
> *
> @@ -3644,43 +3699,6 @@ free_prefix_s:
> free(prefix_s);
> }
>
> -static void
> -op_put_v4_networks(struct ds *ds, const struct ovn_port *op, bool
add_bcast)
> -{
> - if (!add_bcast && op->lrp_networks.n_ipv4_addrs == 1) {
> - ds_put_format(ds, "%s", op->lrp_networks.ipv4_addrs[0].addr_s);
> - return;
> - }
> -
> - ds_put_cstr(ds, "{");
> - for (int i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) {
> - ds_put_format(ds, "%s, ", op->lrp_networks.ipv4_addrs[i].addr_s);
> - if (add_bcast) {
> - ds_put_format(ds, "%s, ",
op->lrp_networks.ipv4_addrs[i].bcast_s);
> - }
> - }
> - ds_chomp(ds, ' ');
> - ds_chomp(ds, ',');
> - ds_put_cstr(ds, "}");
> -}
> -
> -static void
> -op_put_v6_networks(struct ds *ds, const struct ovn_port *op)
> -{
> - if (op->lrp_networks.n_ipv6_addrs == 1) {
> - ds_put_format(ds, "%s", op->lrp_networks.ipv6_addrs[0].addr_s);
> - return;
> - }
> -
> - ds_put_cstr(ds, "{");
> - for (int i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) {
> - ds_put_format(ds, "%s, ", op->lrp_networks.ipv6_addrs[i].addr_s);
> - }
> - ds_chomp(ds, ' ');
> - ds_chomp(ds, ',');
> - ds_put_cstr(ds, "}");
> -}
> -
> static const char *
> get_force_snat_ip(struct ovn_datapath *od, const char *key_type,
ovs_be32 *ip)
> {
> --
> 2.9.3
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
>
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=1bF0sSIIafODfWGrMWfrLDZp6Gan2-gIhuE5AqyVSS8&s=Gt9_RJ5SYYMdi7iiM_NPyk3NXkB4wLYbxyx_pQjlRrA&e=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=1bF0sSIIafODfWGrMWfrLDZp6Gan2-gIhuE5AqyVSS8&s=Gt9_RJ5SYYMdi7iiM_NPyk3NXkB4wLYbxyx_pQjlRrA&e=
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
