On 20 September 2016 at 20:52, Pravin B Shelar <pshe...@ovn.org> wrote:
> OVS IPsec tunnel support has issues:
> 1. It only works for GRE.
2. only works on Debian.
3. It does not allow user to match on packet-mark
> on packet received on tunnel ports.
> Therefore following patch provide alternative to completely
> disable ipsec-tunnel support by vswitchd command line option.
> This way user can use external daemon to manage IPsec tunnel
> traffic and stir it using skb-mark match action in OVS bridge.
> This patch deprecates support for IPsec tunnel port.
>
There are other alternative solutions worth to mention:
1) remove the special meaning of skb_mark bit #0 and update
ovs-monitor-ipsec not to depend on harcoded skb_mark value at all (I think
this can be done with some trickery);
2) allow users to chose OVS mode where OVS can be explicitly told to either
use skb_mark for its own needs (e.g. IPsec) OR to pass skb_mark to OpenFlow
pipeline as-is;
3) leave bit #0 assigned to IPsec and let OpenFlow to match only on bits
#1-32.
Your solutions is kinda like 2), except it discourages uses to configure
OVS in a way where it consumes skb_mark for itself.
I think solutions 1) could be implemented even after your patch. Except,
maybe then we should not mention that IPsec will be deprecated in the next
release. Also, I would need to think how to address corner cases if
ovs-monitor-ipsec can't use skb_mark anymore.
Solution 3) would be great from ovs-monitor-ipsec perspective because it
would not need to change. However, it possibly would make OpenFlow skb_mark
matching look weird compared to other fields that OVS can match on.
> Signed-off-by: Pravin B Shelar <pshe...@ovn.org>
> ---
> NEWS | 2 ++
> debian/changelog | 2 ++
> debian/control | 1 +
> lib/netdev-vport.c | 3 +++
> lib/netdev.c | 1 +
> lib/netdev.h | 1 +
> ofproto/tunnel.c | 30 ++++++++++++++++++++++--------
> ofproto/tunnel.h | 2 ++
> vswitchd/ovs-vswitchd.c | 7 +++++++
> vswitchd/vswitch.xml | 8 ++++++++
> 10 files changed, 49 insertions(+), 8 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 21ab538..057edfd 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -149,6 +149,8 @@ v2.6.0 - xx xxx xxxx
> * Flow based tunnel match and action can be used for IPv6 address
> using
> tun_ipv6_src, tun_ipv6_dst fields.
> * Added support for IPv6 tunnels, for details checkout FAQ.
> + * Allow external IPsec tunnel management. Deprecated support for
> IPsec
> + tunnels ports.
>
s/tunnels/tunnel
- A wrapper script, 'ovs-tcpdump', to easily port-mirror an OVS port and
> watch with tcpdump
> - Introduce --no-self-confinement flag that allows daemons to work with
> diff --git a/debian/changelog b/debian/changelog
> index d73e636..8add140 100644
> --- a/debian/changelog
> +++ b/debian/changelog
> @@ -108,6 +108,8 @@ openvswitch (2.6.0-1) unstable; urgency=low
> * Flow based tunnel match and action can be used for IPv6 address
> using
> tun_ipv6_src, tun_ipv6_dst fields.
> * Added support for IPv6 tunnels, for details checkout FAQ.
> + * Allow external IPsec tunnel management. Deprecated support for
> IPsec
> + tunnels ports.
>
same here
> - A wrapper script, 'ovs-tcpdump', to easily port-mirror an OVS port
> and
> watch with tcpdump
> - Introduce --no-self-confinement flag that allows daemons to work with
> diff --git a/debian/control b/debian/control
> index 6e704f1..da86fe9 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -200,6 +200,7 @@ Description: Open vSwitch GRE-over-IPsec support
> .
> The ovs-monitor-ipsec script provides support for encrypting GRE
> tunnels with IPsec.
> + IPsec tunnels support is deprecated.
>
s/tunnels/tunneling
>
> Package: openvswitch-pki
> Architecture: all
> diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
> index 8d22cf5..6bf4d2d 100755
> --- a/lib/netdev-vport.c
> +++ b/lib/netdev-vport.c
> @@ -543,6 +543,9 @@ set_tunnel_config(struct netdev *dev_, const struct
> smap *args)
> static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
> static pid_t pid = 0;
>
> + VLOG_ERR("%s: OVS IPsec tunnel support is deprecated. "
> + "See man page for details", name);
> +
>
I believe IPsec does not work anymore with the command line argument you
introduced. Should you give a special warning message in that case?
> #ifndef _WIN32
> ovs_mutex_lock(&mutex);
> if (pid <= 0) {
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 6c4c657..a626f18 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -98,6 +98,7 @@ static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(5, 20);
>
> static void restore_all_flags(void *aux OVS_UNUSED);
> void update_device_args(struct netdev *, const struct shash *args);
> +bool enable_ipsec_tnl = true;
>
Wouldn't it be preferred that enable_ipsec_tnl is set to false by default?
Otherwise, Open vSwitch users installing OVS from rpm and deb package may
not make use of your change (see init.d script and systemd configuration
file that actually starts ovs-vswitchd process).
>
> int
> netdev_n_txq(const struct netdev *netdev)
> diff --git a/lib/netdev.h b/lib/netdev.h
> index 634c665..870ce22 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -299,6 +299,7 @@ int netdev_dump_queue_stats(const struct netdev *,
> netdev_dump_queue_stats_cb *, void *aux);
>
> extern struct seq *tnl_conf_seq;
> +extern bool enable_ipsec_tnl;
>
> #ifndef _WIN32
> void netdev_get_addrs_list_flush(void);
> diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c
> index 9a69071..595a1bd 100644
> --- a/ofproto/tunnel.c
> +++ b/ofproto/tunnel.c
> @@ -164,7 +164,10 @@ tnl_port_add__(const struct ofport_dpif *ofport,
> const struct netdev *netdev,
> tnl_port->match.ipv6_dst = cfg->ipv6_dst;
> tnl_port->match.ip_src_flow = cfg->ip_src_flow;
> tnl_port->match.ip_dst_flow = cfg->ip_dst_flow;
> - tnl_port->match.pkt_mark = cfg->ipsec ? IPSEC_MARK : 0;
> +
> + if (enable_ipsec_tnl) {
> + tnl_port->match.pkt_mark = cfg->ipsec ? IPSEC_MARK : 0;
> + }
> tnl_port->match.in_key_flow = cfg->in_key_flow;
> tnl_port->match.odp_port = odp_port;
>
> @@ -357,7 +360,9 @@ tnl_process_ecn(struct flow *flow)
> flow->nw_tos |= IP_ECN_CE;
> }
>
> - flow->pkt_mark &= ~IPSEC_MARK;
> + if (enable_ipsec_tnl) {
> + flow->pkt_mark &= ~IPSEC_MARK;
> + }
> return true;
> }
>
> @@ -383,8 +388,11 @@ tnl_wc_init(struct flow *flow, struct flow_wildcards
> *wc)
> wc->masks.tunnel.tp_src = 0;
> wc->masks.tunnel.tp_dst = 0;
>
> - memset(&wc->masks.pkt_mark, 0xff, sizeof wc->masks.pkt_mark);
> -
> + if (enable_ipsec_tnl) {
> + memset(&wc->masks.pkt_mark, 0xff, sizeof wc->masks.pkt_mark);
> + } else {
> + memset(&wc->masks.pkt_mark, 0, sizeof wc->masks.pkt_mark);
> + }
> if (is_ip_any(flow)
> && IP_ECN_is_ce(flow->tunnel.ip_tos)) {
> wc->masks.nw_tos |= IP_ECN_MASK;
> @@ -435,8 +443,10 @@ tnl_port_send(const struct ofport_dpif *ofport,
> struct flow *flow,
> flow->tunnel.ipv6_dst = in6addr_any;
> }
> }
> - flow->pkt_mark |= tnl_port->match.pkt_mark;
> - wc->masks.pkt_mark |= tnl_port->match.pkt_mark;
> + if (enable_ipsec_tnl) {
> + flow->pkt_mark |= tnl_port->match.pkt_mark;
> + wc->masks.pkt_mark |= tnl_port->match.pkt_mark;
> + }
>
> if (!cfg->out_key_flow) {
> flow->tunnel.tun_id = cfg->out_key;
> @@ -561,7 +571,9 @@ tnl_find(const struct flow *flow)
> OVS_REQ_RDLOCK(rwlock)
> match.ipv6_dst = flow_tnl_src(&flow->tunnel);
> }
> match.odp_port = flow->in_port.odp_port;
> - match.pkt_mark = flow->pkt_mark;
> + if (enable_ipsec_tnl) {
> + match.pkt_mark = flow->pkt_mark;
> + }
> match.in_key_flow = in_key_flow;
> match.ip_dst_flow = ip_dst_flow;
> match.ip_src_flow = ip_src == IP_SRC_FLOW;
> @@ -616,7 +628,9 @@ tnl_match_fmt(const struct tnl_match *match, struct ds
> *ds)
> }
>
> ds_put_format(ds, ", dp port=%"PRIu32, match->odp_port);
> - ds_put_format(ds, ", pkt mark=%"PRIu32, match->pkt_mark);
> + if (enable_ipsec_tnl) {
> + ds_put_format(ds, ", pkt mark=%"PRIu32, match->pkt_mark);
> + }
> }
>
> static void
> diff --git a/ofproto/tunnel.h b/ofproto/tunnel.h
> index b0ec67c..b2f5590 100644
> --- a/ofproto/tunnel.h
> +++ b/ofproto/tunnel.h
> @@ -30,6 +30,8 @@ struct ofport_dpif;
> struct netdev;
> struct netdev_tnl_build_header_params;
>
> +extern bool enable_ipsec_tnl;
> +
> void ofproto_tunnel_init(void);
> bool tnl_port_reconfigure(const struct ofport_dpif *, const struct netdev
> *,
> odp_port_t, bool native_tnl, const char name[]);
> diff --git a/vswitchd/ovs-vswitchd.c b/vswitchd/ovs-vswitchd.c
> index 72448bb..84c9b96 100644
> --- a/vswitchd/ovs-vswitchd.c
> +++ b/vswitchd/ovs-vswitchd.c
> @@ -146,6 +146,7 @@ parse_options(int argc, char *argv[], char
> **unixctl_pathp)
> DAEMON_OPTION_ENUMS,
> OPT_DPDK,
> OPT_DUMMY_NUMA,
> + OPT_DISABLE_IPSEC_TNL,
};
> static const struct option long_options[] = {
> {"help", no_argument, NULL, 'h'},
> @@ -161,6 +162,7 @@ parse_options(int argc, char *argv[], char
> **unixctl_pathp)
> {"disable-system", no_argument, NULL, OPT_DISABLE_SYSTEM},
> {"dpdk", optional_argument, NULL, OPT_DPDK},
> {"dummy-numa", required_argument, NULL, OPT_DUMMY_NUMA},
> + {"external-ipsec-tunneling", no_argument, NULL,
> OPT_DISABLE_IPSEC_TNL},
>
How about having two Open vSwitch modes:
1. mode where skb mark passes through as-is and can be matched with
OpenFlow rules;
2. mode where Open vSwitch uses all bits in skb_mark and uses them for
features like IPsec.
Anyway, this may unnecessarily complicate OVS, especially, if there are no
other services besides IPsec that would want to make OVS-specific use of
skb_mark. Also, unless ovs-vswitchd process is started from command line,
it may not be easy to override these flags with "systemctl restart
openvswitch" command.
> {NULL, 0, NULL, 0},
> };
> char *short_options = ovs_cmdl_long_options_to_short
> _options(long_options);
> @@ -220,6 +222,10 @@ parse_options(int argc, char *argv[], char
> **unixctl_pathp)
> ovs_numa_set_dummy(optarg);
> break;
>
> + case OPT_DISABLE_IPSEC_TNL:
> + enable_ipsec_tnl = false;
>
Depending on what you want to be default behavior, this may have to be
inverted to true (see my other comment where you set the default value)
> + break;
> +
> default:
> abort();
> }
> @@ -259,6 +265,7 @@ usage(void)
> );
> printf("\nOther options:\n"
> " --unixctl=SOCKET override default control socket
> name\n"
> + " --external-ipsec-tunneling Disable native IPsec
> tunnel support to allow external IPsec tunnel management\n"
> " -h, --help display this help message\n"
> " -V, --version display version information\n");
> exit(EXIT_SUCCESS);
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index e73023d..d6fc508 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -2008,6 +2008,14 @@
> <dd>
> An Ethernet over RFC 2890 Generic Routing Encapsulation over
> IPv4/IPv6
> IPsec tunnel.
> + IPsec tunnel port are deprecated. The support will be
> completely
>
s/port/ports
s/The support/IPsec support
> + removed in next version.
>
s/next version/the next release (Maybe mention exact release here as well).
> + Better way to implement IPsec Tunnel is to use external
> daemon to manage
s/Better way to implement IPsec Tunnel/A better way to secure OVS tunnels
through IPsec
> + IPsec tunnel traffic using strongswan and stir it using
> skb-mark match
>
s/strongswan/strongSwan
I am not sure what you meant here with word "stir"? Did you mean "steer"?
If so, then how about some other word like "differentiate"?
> + action in OVS bridge. To match on skb-marked tunnel packet,
> OVS IPsec
+ tunnel port support needs to be disabled using command line
> ovs-vswitchd
> + daemon option (--external-ipsec-tunneling).
+
> </dd>
>
> <dt><code>vxlan</code></dt>
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
