vswitch: Use "ipsec_gre" vport instead of "gre" with "other_config"

Previously, a GRE-over-IPsec tunnel was created as an interface with a
"type" of "gre" and the "other_config" column with "ipsec_cert" or
"ipsec_psk" set.  This could lead to a potential security problem if a user
intended to create a GRE-over-IPsec tunnel, but misconfigured the
"ipsec_*" config and created an unencrypted GRE tunnel.

This commit defines an "ipsec_gre" tunnel type, which should prevent
users from inadvertently establishing insecure tunnels.

debian/ovs-monitor-ipsec

@@ -255,8 +255,7 @@ def monitor_uuid_schema_cb(schema):
     new_tables["Interface"] = keep_table_columns(
         schema, "Interface", {"name": string_type,
                               "type": string_type,
-                              "options": string_map_type,
-                              "other_config": string_map_type})
+                              "options": string_map_type})
     schema.tables = new_tables
 
 def usage():
@@ -308,38 +307,42 @@ def main(argv):
         new_interfaces = {}
         for rec in idl.data["Interface"].itervalues():
             name = rec.name.as_scalar()
-            ipsec_cert = rec.other_config.get("ipsec_cert")
-            ipsec_psk = rec.other_config.get("ipsec_psk")
+            ipsec_cert = rec.options.get("ipsec_cert")
+            ipsec_psk = rec.options.get("ipsec_psk")
             is_ipsec = ipsec_cert or ipsec_psk
 
-            if rec.type.as_scalar() == "gre" and is_ipsec:
-                new_interfaces[name] = {
+            if rec.type.as_scalar() == "ipsec_gre":
+                if ipsec_cert or ipsec_psk:
+                    new_interfaces[name] = {
                         "remote_ip": rec.options.get("remote_ip"),
                         "local_ip": rec.options.get("local_ip", "0.0.0.0/0"),
                         "ipsec_cert": ipsec_cert,
                         "ipsec_psk": ipsec_psk }
+                else:
+                    s_log.warning(
+                        "no ipsec_cert or ipsec_psk defined for %s" % name)
 
         if interfaces != new_interfaces:
             for name, vals in interfaces.items():
                 if name not in new_interfaces.keys():
                     ipsec.ipsec_cert_del(vals["local_ip"], vals["remote_ip"])
             for name, vals in new_interfaces.items():
-                if vals == interfaces.get(name):
-                    s_log.warning(
-                        "configuration changed for %s, need to delete "
-                        "interface first" % name)
+                orig_vals = interfaces.get(name):
+                if orig_vals:
+                    # Configuration for this host already exists.  If
+                    # it has changed, this is an error.
+                    if vals != orig_vals:
+                        s_log.warning(
+                            "configuration changed for %s, need to delete "
+                            "interface first" % name)
                     continue
 
                 if vals["ipsec_cert"]:
                     ipsec.ipsec_cert_update(vals["local_ip"],
                             vals["remote_ip"], vals["ipsec_cert"])
-                elif vals["ipsec_psk"]:
+                else:
                     ipsec.ipsec_psk_update(vals["local_ip"], 
                             vals["remote_ip"], vals["ipsec_psk"])
-                else:
-                    s_log.warning(
-                        "no ipsec_cert or ipsec_psk defined for %s" % name)
-                    continue
 
             interfaces = new_interfaces
 

include/openvswitch/tunnel.h

@@ -50,6 +50,7 @@
 #define TNL_F_TTL_INHERIT	(1 << 5) /* Inherit the TTL from the inner packet. */
 #define TNL_F_PMTUD		(1 << 6) /* Enable path MTU discovery. */
 #define TNL_F_HDR_CACHE		(1 << 7) /* Enable tunnel header caching. */
+#define TNL_F_IPSEC		(1 << 8) /* Traffic is IPsec encrypted. */
 
 /* This goes in the "config" member of struct odp_port for tunnel vports. */
 struct tnl_port_config {

lib/dpif-linux.c

@@ -37,6 +37,7 @@
 #include "netdev.h"
 #include "netdev-vport.h"
 #include "ofpbuf.h"
+#include "openvswitch/tunnel.h"
 #include "poll-loop.h"
 #include "rtnetlink.h"
 #include "shash.h"
@@ -227,18 +228,31 @@ dpif_linux_set_drop_frags(struct dpif *dpif_, bool drop_frags)
 }
 
 static void
-translate_vport_type_to_netdev_type(char *type, size_t size)
+translate_vport_type_to_netdev_type(struct odp_port *port)
 {
+    char *type = port->type;
+
     if (!strcmp(type, "netdev")) {
-        ovs_strlcpy(type, "system", size);
+        ovs_strlcpy(type, "system", sizeof port->type);
+    } else if (!strcmp(type, "gre")) {
+        const struct tnl_port_config *config;
+
+        config = (struct tnl_port_config *)port->config;
+        if (config->flags & TNL_F_IPSEC) {
+            ovs_strlcpy(type, "ipsec_gre", sizeof port->type);
+        }
     }
 }
 
 static void
-translate_netdev_type_to_vport_type(char *type, size_t size)
+translate_netdev_type_to_vport_type(struct odp_port *port)
 {
+    char *type = port->type;
+
     if (!strcmp(type, "system")) {
-        ovs_strlcpy(type, "netdev", size);
+        ovs_strlcpy(type, "netdev", sizeof port->type);
+    } else if (!strcmp(type, "ipsec_gre")) {
+        ovs_strlcpy(type, "gre", sizeof port->type);
     }
 }
 
@@ -254,8 +268,8 @@ dpif_linux_port_add(struct dpif *dpif, struct netdev *netdev,
     memset(&port, 0, sizeof port);
     strncpy(port.devname, name, sizeof port.devname);
     strncpy(port.type, type, sizeof port.type);
-    translate_netdev_type_to_vport_type(port.type, sizeof port.type);
     netdev_vport_get_config(netdev, port.config);
+    translate_netdev_type_to_vport_type(&port);
 
     error = do_ioctl(dpif, ODP_VPORT_ATTACH, &port);
     if (!error) {
@@ -277,7 +291,7 @@ dpif_linux_port_query__(const struct dpif *dpif, struct odp_port *port)
 {
     int error = do_ioctl(dpif, ODP_VPORT_QUERY, port);
     if (!error) {
-        translate_vport_type_to_netdev_type(port->type, sizeof port->type);
+        translate_vport_type_to_netdev_type(port);
     }
     return error;
 }
@@ -323,7 +337,7 @@ dpif_linux_port_list(const struct dpif *dpif_, struct odp_port *ports, int n)
     for (i = 0; i < pv.n_ports; i++) {
         struct odp_port *port = &pv.ports[i];
 
-        translate_vport_type_to_netdev_type(port->type, sizeof port->type);
+        translate_vport_type_to_netdev_type(port);
     }
     return pv.n_ports;
 }

lib/netdev-vport.c

@@ -433,7 +433,8 @@ parse_tunnel_config(const struct netdev_dev *dev, const struct shash *args,
 {
     const char *name = netdev_dev_get_name(dev);
     const char *type = netdev_dev_get_type(dev);
-    bool is_gre = !strcmp(type, "gre");
+    bool is_gre = false;
+    bool is_ipsec = false;
     struct tnl_port_config config;
     struct shash_node *node;
     bool ipsec_mech_set = false;
@@ -442,6 +443,18 @@ parse_tunnel_config(const struct netdev_dev *dev, const struct shash *args,
     config.flags |= TNL_F_PMTUD;
     config.flags |= TNL_F_HDR_CACHE;
 
+    if (!strcmp(type, "gre")) {
+        is_gre = true;
+    } else if (!strcmp(type, "ipsec_gre")) {
+        is_gre = true;
+        is_ipsec = true;
+
+        config.flags |= TNL_F_IPSEC;
+
+        /* IPsec doesn't work when header caching is enabled. */
+        config.flags &= ~TNL_F_HDR_CACHE;
+    }
+
     SHASH_FOR_EACH (node, args) {
         if (!strcmp(node->name, "remote_ip")) {
             struct in_addr in_addr;
@@ -501,20 +514,19 @@ parse_tunnel_config(const struct netdev_dev *dev, const struct shash *args,
             if (!strcmp(node->data, "false")) {
                 config.flags &= ~TNL_F_HDR_CACHE;
             }
-        } else if (!strcmp(node->name, "ipsec_cert")
-                   || !strcmp(node->name, "ipsec_psk")) {
+        } else if ((!strcmp(node->name, "ipsec_cert")
+                   || !strcmp(node->name, "ipsec_psk")) && is_ipsec) {
             ipsec_mech_set = true;
         } else {
             VLOG_WARN("%s: unknown %s argument '%s'",
                       name, type, node->name);
         }
     }
 
-   /* IPsec doesn't work when header caching is enabled.  Disable it if the
-    * IPsec local IP address and authentication mechanism have been defined. */
-    if (ipsec_mech_set) {
-        VLOG_INFO("%s: header caching disabled due to use of IPsec", name);
-        config.flags &= ~TNL_F_HDR_CACHE;
+    if (is_ipsec && !ipsec_mech_set) {
+        VLOG_WARN("%s: IPsec requires an 'ipsec_cert' or ipsec_psk' argument",
+                  name);
+        return EINVAL;
     }
 
     if (!config.daddr) {
@@ -623,6 +635,7 @@ netdev_vport_register(void)
 {
     static const struct vport_class vport_classes[] = {
         { { "gre", VPORT_FUNCTIONS }, parse_tunnel_config },
+        { { "ipsec_gre", VPORT_FUNCTIONS }, parse_tunnel_config },
         { { "capwap", VPORT_FUNCTIONS }, parse_tunnel_config },
         { { "patch", VPORT_FUNCTIONS }, parse_patch_config }
     };

lib/odp-util.c

@@ -222,6 +222,7 @@ void
 format_odp_port_type(struct ds *ds, const struct odp_port *p)
 {
     if (!strcmp(p->type, "gre") 
+            || !strcmp(p->type, "ipsec_gre")
             || !strcmp(p->type, "capwap")) {
         const struct tnl_port_config *config;
 

vswitchd/vswitch.xml

@@ -755,9 +755,100 @@
                 bypass certain components of the IP stack (such as IP tables)
                 and it may be useful to disable it if these features are
                 required or as a debugging measure.  Default is enabled, set to
-                <code>false</code> to disable.  If IPsec is enabled through the
-                <ref column="other_config"/> parameters, header caching will be
-                automatically disabled.</dd>
+                <code>false</code> to disable.</dd>
+            </dl>
+          </dd>
+          <dt><code>ipsec_gre</code></dt>
+          <dd>An Ethernet over RFC 2890 Generic Routing Encapsulation over
+             IPv4 IPsec tunnel.  Each tunnel (including those of type
+             <code>gre</code>) must be uniquely identified by the
+             combination of <code>remote_ip</code> and
+             <code>local_ip</code>.  Note that if two ports are defined
+             that are the same except one has an optional identifier and
+             the other does not, the more specific one is matched first.
+             The following options may be specified in the 
+             <ref column="options"/> column:
+            <dl>
+              <dt><code>remote_ip</code></dt>
+              <dd>Required.  The tunnel endpoint.</dd>
+            </dl>
+            <dl>
+              <dt><code>local_ip</code></dt>
+              <dd>Optional.  The destination IP that received packets must
+                match.  Default is to match all addresses.</dd>
+            </dl>
+            <dl>
+              <dt><code>ipsec_psk</code></dt>
+              <dd>Required.  Specifies a pre-shared key for authentication 
+                that must be identical on both sides of the tunnel.</dd>
+            </dl>
+            <dl>
+              <dt><code>in_key</code></dt>
+              <dd>Optional.  The GRE key that received packets must contain.
+                It may either be a 32-bit number (no key and a key of 0 are
+                treated as equivalent) or the word <code>flow</code>.  If
+                <code>flow</code> is specified then any key will be accepted
+                and the key will be placed in the <code>tun_id</code> field
+                for matching in the flow table.  The ovs-ofctl manual page
+                contains additional information about matching fields in
+                OpenFlow flows.  Default is no key.</dd>
+            </dl>
+            <dl>
+              <dt><code>out_key</code></dt>
+              <dd>Optional.  The GRE key to be set on outgoing packets.  It may
+                either be a 32-bit number or the word <code>flow</code>.  If
+                <code>flow</code> is specified then the key may be set using
+                the <code>set_tunnel</code> Nicira OpenFlow vendor extension (0
+                is used in the absence of an action).  The ovs-ofctl manual
+                page contains additional information about the Nicira OpenFlow
+                vendor extensions.  Default is no key.</dd>
+            </dl>
+            <dl>
+              <dt><code>key</code></dt>
+              <dd>Optional.  Shorthand to set <code>in_key</code> and
+                <code>out_key</code> at the same time.</dd>
+            </dl>
+            <dl>
+              <dt><code>tos</code></dt>
+              <dd>Optional.  The value of the ToS bits to be set on the
+                encapsulating packet.  It may also be the word
+                <code>inherit</code>, in which case the ToS will be copied from
+                the inner packet if it is IPv4 or IPv6 (otherwise it will be
+                0).  Note that the ECN fields are always inherited.  Default is
+                0.</dd>
+            </dl>
+            <dl>
+              <dt><code>ttl</code></dt>
+              <dd>Optional.  The TTL to be set on the encapsulating packet.
+                It may also be the word <code>inherit</code>, in which case the
+                TTL will be copied from the inner packet if it is IPv4 or IPv6
+                (otherwise it will be the system default, typically 64).
+                Default is the system default TTL.</dd>
+            </dl>
+            <dl>
+              <dt><code>csum</code></dt>
+              <dd>Optional.  Compute GRE checksums on outgoing packets.
+                Checksums present on incoming packets will be validated
+                regardless of this setting.  Note that GRE checksums
+                impose a significant performance penalty as they cover the
+                entire packet.  As the contents of the packet is typically
+                covered by L3 and L4 checksums, this additional checksum only
+                adds value for the GRE and encapsulated Ethernet headers.
+                Default is disabled, set to <code>true</code> to enable.</dd>
+            </dl>
+            <dl>
+              <dt><code>pmtud</code></dt>
+              <dd>Optional.  Enable tunnel path MTU discovery.  If enabled
+                ``ICMP destination unreachable - fragmentation'' needed
+                messages will be generated for IPv4 packets with the DF bit set
+                and IPv6 packets above the minimum MTU if the packet size
+                exceeds the path MTU minus the size of the tunnel headers.  It
+                also forces the encapsulating packet DF bit to be set (it is
+                always set if the inner packet implies path MTU discovery).
+                Note that this option causes behavior that is typically
+                reserved for routers and therefore is not entirely in
+                compliance with the IEEE 802.1D specification for bridges.
+                Default is enabled, set to <code>false</code> to disable.</dd>
             </dl>
           </dd>
           <dt><code>capwap</code></dt>
@@ -991,16 +1082,7 @@
 
       <column name="other_config">
         Key-value pairs for rarely used interface features.  Currently,
-        the only key is for configuring GRE-over-IPsec, which is only
-        available through the <code>openvswitch-ipsec</code> package for
-        Debian.  The currently defined key-value pair is:
-        <dl>
-          <dt><code>ipsec_psk</code></dt>
-          <dd>Required key for GRE-over-IPsec interfaces.  Specifies a
-            pre-shared key for authentication that must be identical on
-            both sides of the tunnel.  Additionally, the
-            <ref column="type"/> must be <code>gre</code>.</dd>
-        </dl>
+        there are none defined.
       </column>
 
       <column name="statistics">