[Openvpn-devel] Use DCO on Windows by default

Message ID 20220909144546.672-1-lstipakov@gmail.com
State Superseded
Headers show
Series [Openvpn-devel] Use DCO on Windows by default | expand

Commit Message

Lev Stipakov Sept. 9, 2022, 4:45 a.m. UTC
From: Lev Stipakov <lev@openvpn.net>

On startup, check following conditions:

 - ovpn-dco-win driver is installed. Perform this check
by trying to open adapter by symbolic name.

 - options are compatible with dco. Same checks as on
Linux and FreeBSD. In addition, check that --mode server
is not used and --windows-driver is not set to tap-windows6/wintun.

If both checks are passed, use DCO.

Move options_postprocess_mutate_invariant() call
below since it depends on selected windows driver.

dco_check_option() has side effect on Windows -
if dco is not used, it might complain "cipher chachapoly
not supported by dco, disabling dco" if chachapoly
support is missing system-wide. To not to see this,
check dco options only if dco is enabled. This means
moving dco_enabled() from dco_check_startup_option()
to one level above. We do similar thing in
multi_connection_established() before checking ccd options.

Signed-off-by: Lev Stipakov <lev@openvpn.net>
---
 src/openvpn/dco.c     | 19 +++++++++++------
 src/openvpn/dco_win.c | 23 ++++++++++++++++++++-
 src/openvpn/options.c | 48 +++++++++++++++++++++++++++++--------------
 src/openvpn/options.h |  4 +---
 src/openvpn/tun.c     | 19 -----------------
 src/openvpn/tun.h     | 19 +++++++++++++++++
 6 files changed, 88 insertions(+), 44 deletions(-)

Comments

Gert Doering Sept. 11, 2022, 10:47 a.m. UTC | #1
Hi,

On Fri, Sep 09, 2022 at 05:45:46PM +0300, Lev Stipakov wrote:
> +#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32)
>      if (dco_enabled(o))
>      {
> -        dco_check_option(M_USAGE, o);
> -        dco_check_startup_option(M_USAGE, o);
> +        /* check if any option should force disabling DCO */
> +        o->tuntap_options.disable_dco = !dco_check_option(D_DCO, o)
> +                                        || !dco_check_startup_option(D_DCO, o);
> +    }
> +    else
> +    {
> +        o->tuntap_options.disable_dco = true;
> +    }

This is a bit weird.  With the latest change, dco_enabled(o) will
only check tuntap_options.disable_dco - so, if that is false, disable_dco
is already true, so why set it in the "else" branch again?

> +#endif
> +
> +#ifdef _WIN32
> +    if (!o->tuntap_options.disable_dco)

... and why reference the variable directly here, instead of querying
dco_enabled(o)?

gert

Patch

diff --git a/src/openvpn/dco.c b/src/openvpn/dco.c
index d9e49781..a76cdd0c 100644
--- a/src/openvpn/dco.c
+++ b/src/openvpn/dco.c
@@ -257,11 +257,11 @@  dco_check_option_ce(const struct connection_entry *ce, int msglevel)
 bool
 dco_check_startup_option(int msglevel, const struct options *o)
 {
-    /* check if DCO was already disabled by the user or if no dev name was
-     * specified at all. In the latter case, later logic will most likely stop
-     * OpenVPN, so no need to print any message here.
+    /* check if no dev name was specified at all. In the case,
+     * later logic will most likely stop OpenVPN, so no need to
+     * print any message here.
      */
-    if (!dco_enabled(o) || !o->dev)
+    if (!o->dev)
     {
         return false;
     }
@@ -294,8 +294,15 @@  dco_check_startup_option(int msglevel, const struct options *o)
 #if defined(_WIN32)
     if (o->mode == MODE_SERVER)
     {
-        msg(msglevel, "Only client and p2p data channel offload is supported "
-            "with ovpn-dco.");
+        msg(msglevel, "--mode server is set. Disabling Data Channel Offload");
+        return false;
+    }
+
+    if ((o->windows_driver == WINDOWS_DRIVER_WINTUN)
+        || (o->windows_driver == WINDOWS_DRIVER_TAP_WINDOWS6))
+    {
+        msg(msglevel, "--windows-driver is set to '%s'. Disabling Data Channel Offload",
+            print_windows_driver(o->windows_driver));
         return false;
     }
 
diff --git a/src/openvpn/dco_win.c b/src/openvpn/dco_win.c
index 22f30280..48a1755a 100644
--- a/src/openvpn/dco_win.c
+++ b/src/openvpn/dco_win.c
@@ -359,7 +359,28 @@  dco_swap_keys(dco_context_t *dco, unsigned int peer_id)
 bool
 dco_available(int msglevel)
 {
-    return true;
+    /* try to open device by symbolic name */
+    HANDLE h = CreateFile("\\\\.\\ovpn-dco", GENERIC_READ | GENERIC_WRITE,
+                          0, NULL, OPEN_EXISTING, FILE_ATTRIBUTE_SYSTEM | FILE_FLAG_OVERLAPPED, NULL);
+
+    if (h != INVALID_HANDLE_VALUE)
+    {
+        CloseHandle(h);
+        return true;
+    }
+
+    DWORD err = GetLastError();
+    if (err == ERROR_ACCESS_DENIED)
+    {
+        /* this likely means that device exists but is already in use,
+         * don't bail out since later we try to open all existing dco
+         * devices and then bail out if all devices are in use
+         */
+        return true;
+    }
+
+    msg(msglevel, "Note: ovpn-dco-win driver is missing, disabling data channel offload.");
+    return false;
 }
 
 int
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 2e567571..e27a957f 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -183,7 +183,7 @@  static const char usage_message[] =
     "                  does not begin with \"tun\" or \"tap\".\n"
     "--dev-node node : Explicitly set the device node rather than using\n"
     "                  /dev/net/tun, /dev/tun, /dev/tap, etc.\n"
-#if defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD))
+#if defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32))
     "--disable-dco   : Do not attempt using Data Channel Offload.\n"
 #endif
     "--lladdr hw     : Set the link layer address of the tap device.\n"
@@ -851,7 +851,7 @@  init_options(struct options *o, const bool init_gc)
     o->tuntap_options.dhcp_masq_offset = 0;     /* use network address as internal DHCP server address */
     o->route_method = ROUTE_METHOD_ADAPTIVE;
     o->block_outside_dns = false;
-    o->windows_driver = WINDOWS_DRIVER_TAP_WINDOWS6;
+    o->windows_driver = WINDOWS_DRIVER_UNSPECIFIED;
 #endif
     o->vlan_accept = VLAN_ALL;
     o->vlan_pvid = 1;
@@ -3606,8 +3606,6 @@  options_postprocess_mutate(struct options *o, struct env_set *es)
     options_set_backwards_compatible_options(o);
 
     options_postprocess_cipher(o);
-    options_postprocess_mutate_invariant(o);
-
     o->ncp_ciphers = mutate_ncp_cipher_list(o->ncp_ciphers, &o->gc);
     if (o->ncp_ciphers == NULL)
     {
@@ -3683,18 +3681,35 @@  options_postprocess_mutate(struct options *o, struct env_set *es)
             "incompatible with each other.");
     }
 
-#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD)
-    /* check if any option should force disabling DCO */
-    o->tuntap_options.disable_dco = !dco_check_option(D_DCO, o)
-                                    || !dco_check_startup_option(D_DCO, o);
-#elif defined(_WIN32)
-    /* in Windows we have no 'fallback to non-DCO' strategy, so if a conflicting
-     * option is found, we simply bail out by means of M_USAGE
-     */
+#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32)
     if (dco_enabled(o))
     {
-        dco_check_option(M_USAGE, o);
-        dco_check_startup_option(M_USAGE, o);
+        /* check if any option should force disabling DCO */
+        o->tuntap_options.disable_dco = !dco_check_option(D_DCO, o)
+                                        || !dco_check_startup_option(D_DCO, o);
+    }
+    else
+    {
+        o->tuntap_options.disable_dco = true;
+    }
+#endif
+
+#ifdef _WIN32
+    if (!o->tuntap_options.disable_dco)
+    {
+        o->windows_driver = WINDOWS_DRIVER_DCO;
+    }
+    else
+    {
+        if (o->windows_driver == WINDOWS_DRIVER_DCO)
+        {
+            msg(M_WARN, "Option --windows-driver ovpn-dco is ignored because Data Channel Offload is disabled");
+            o->windows_driver = WINDOWS_DRIVER_TAP_WINDOWS6;
+        }
+        else if (o->windows_driver == WINDOWS_DRIVER_UNSPECIFIED)
+        {
+            o->windows_driver = WINDOWS_DRIVER_TAP_WINDOWS6;
+        }
     }
 #endif
 
@@ -3705,6 +3720,9 @@  options_postprocess_mutate(struct options *o, struct env_set *es)
         o->dev_node = NULL;
     }
 
+    /* this depends on o->windows_driver, which is set above */
+    options_postprocess_mutate_invariant(o);
+
     /*
      * Save certain parms before modifying options during connect, especially
      * when using --pull
@@ -5903,7 +5921,7 @@  add_option(struct options *options,
 #endif
     else if (streq(p[0], "disable-dco"))
     {
-#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD)
+#if defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32)
         options->tuntap_options.disable_dco = true;
 #endif
     }
diff --git a/src/openvpn/options.h b/src/openvpn/options.h
index 6d9174a4..557054ba 100644
--- a/src/openvpn/options.h
+++ b/src/openvpn/options.h
@@ -882,9 +882,7 @@  bool key_is_external(const struct options *options);
 static inline bool
 dco_enabled(const struct options *o)
 {
-#if defined(_WIN32)
-    return o->windows_driver == WINDOWS_DRIVER_DCO;
-#elif defined(ENABLE_DCO)
+#if defined(ENABLE_DCO)
     return !o->tuntap_options.disable_dco;
 #else
     return false;
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 94803acd..cb2a8fb6 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -3539,25 +3539,6 @@  read_tun(struct tuntap *tt, uint8_t *buf, int len)
 
 #elif defined(_WIN32)
 
-static const char *
-print_windows_driver(enum windows_driver_type windows_driver)
-{
-    switch (windows_driver)
-    {
-        case WINDOWS_DRIVER_TAP_WINDOWS6:
-            return "tap-windows6";
-
-        case WINDOWS_DRIVER_WINTUN:
-            return "wintun";
-
-        case WINDOWS_DRIVER_DCO:
-            return "ovpn-dco";
-
-        default:
-            return "unspecified";
-    }
-}
-
 int
 tun_read_queue(struct tuntap *tt, int maxsize)
 {
diff --git a/src/openvpn/tun.h b/src/openvpn/tun.h
index 1cca1cfb..b7d54f2f 100644
--- a/src/openvpn/tun.h
+++ b/src/openvpn/tun.h
@@ -660,6 +660,25 @@  tuntap_is_dco_win_timeout(struct tuntap *tt, int status)
     return tuntap_is_dco_win(tt) && (status < 0) && (openvpn_errno() == ERROR_NETNAME_DELETED);
 }
 
+static const char *
+print_windows_driver(enum windows_driver_type windows_driver)
+{
+    switch (windows_driver)
+    {
+        case WINDOWS_DRIVER_TAP_WINDOWS6:
+            return "tap-windows6";
+
+        case WINDOWS_DRIVER_WINTUN:
+            return "wintun";
+
+        case WINDOWS_DRIVER_DCO:
+            return "ovpn-dco";
+
+        default:
+            return "unspecified";
+    }
+}
+
 #else  /* ifdef _WIN32 */
 
 static inline bool