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.

diff --git a/debian/ovs-monitor-ipsec b/debian/ovs-monitor-ipsec
index 1cea800..27c15e8 100755 (executable)
--- a/debian/ovs-monitor-ipsec
+++ b/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,
     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():
     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()
         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
 
             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 }
                         "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 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"])
                     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"])
                     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
  
 
             interfaces = new_interfaces
  

diff --git a/include/openvswitch/tunnel.h b/include/openvswitch/tunnel.h
index d545e40..44facfa 100644 (file)
--- a/include/openvswitch/tunnel.h
+++ b/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_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 {
 
 /* This goes in the "config" member of struct odp_port for tunnel vports. */
 struct tnl_port_config {

diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
index 870e03e..66610cf 100644 (file)
--- a/lib/dpif-linux.c
+++ b/lib/dpif-linux.c
@@ -37,6 +37,7 @@
 #include "netdev.h"
 #include "netdev-vport.h"
 #include "ofpbuf.h"
 #include "netdev.h"
 #include "netdev-vport.h"
 #include "ofpbuf.h"

+#include "openvswitch/tunnel.h"

 #include "poll-loop.h"
 #include "rtnetlink.h"
 #include "shash.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
 }
 
 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")) {
     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
     }
 }
 
 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")) {
     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);
     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);
     netdev_vport_get_config(netdev, port.config);

+    translate_netdev_type_to_vport_type(&port);

 
     error = do_ioctl(dpif, ODP_VPORT_ATTACH, &port);
     if (!error) {
 
     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) {
 {
     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;
 }
     }
     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];
 
     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;
 }
     }
     return pv.n_ports;
 }

diff --git a/lib/netdev-vport.c b/lib/netdev-vport.c
index 13b1d93..681bc69 100644 (file)
--- a/lib/netdev-vport.c
+++ b/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);
 {
     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;
     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;
 
     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;
     SHASH_FOR_EACH (node, args) {
         if (!strcmp(node->name, "remote_ip")) {
             struct in_addr in_addr;


@@ -501,8 +514,8 @@ parse_tunnel_config(const struct netdev_dev *dev, const struct shash *args,
             if (!strcmp(node->data, "false")) {
                 config.flags &= ~TNL_F_HDR_CACHE;
             }
             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'",
             ipsec_mech_set = true;
         } else {
             VLOG_WARN("%s: unknown %s argument '%s'",


@@ -510,11 +523,10 @@ parse_tunnel_config(const struct netdev_dev *dev, const struct shash *args,
         }
     }
 
         }
     }
 

-   /* 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) {
     }
 
     if (!config.daddr) {


@@ -623,6 +635,7 @@ netdev_vport_register(void)
 {
     static const struct vport_class vport_classes[] = {
         { { "gre", VPORT_FUNCTIONS }, parse_tunnel_config },
 {
     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 }
     };
         { { "capwap", VPORT_FUNCTIONS }, parse_tunnel_config },
         { { "patch", VPORT_FUNCTIONS }, parse_patch_config }
     };

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 1f6d93e..29f536d 100644 (file)
--- a/lib/odp-util.c
+++ b/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") 
 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;
 
             || !strcmp(p->type, "capwap")) {
         const struct tnl_port_config *config;
 

diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index f78a579..4cc29da 100644 (file)
--- a/vswitchd/vswitch.xml
+++ b/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
                 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>
             </dl>
           </dd>
           <dt><code>capwap</code></dt>


@@ -991,16 +1082,7 @@
 
       <column name="other_config">
         Key-value pairs for rarely used interface features.  Currently,
 
       <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">
       </column>
 
       <column name="statistics">