[Openvpn-devel,v2] Use DCO on Windows by default

  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>
---
v2:

 - initialize disable_dco to true if ENABLE_DCO is not set,
this allows to get rid of "else" branch in options_postprocess_mutate()

 - use dco_enabled() wrapper instead of accessing o->tt.disable_dco

 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(-)
  

On Mon, Sep 12, 2022 at 12:10:57PM +0300, Lev Stipakov wrote:
> 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.
> 

One general question about the patch: Whenever we have
defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32)
in the DCO code, couldn't we just remove it completely?
Since this is bascially "every time" anyway, isn't it?

Regards,
  

Hi,

> One general question about the patch: Whenever we have
> defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32)
> in the DCO code, couldn't we just remove it completely?
> Since this is bascially "every time" anyway, isn't it?

Well, almost. There is also Mac and some other platforms which do not have DCO.

We could likely get rid of those defines if we add disable_dco into
platform-specific tuntap_options
structures. Would it be better to have disable_dco member, assuming
platform doesn't support dco?
  

Hi,

On 14/09/2022 16:41, Lev Stipakov wrote:
> Hi,
> 
>> One general question about the patch: Whenever we have
>> defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32)
>> in the DCO code, couldn't we just remove it completely?
>> Since this is bascially "every time" anyway, isn't it?
> 
> Well, almost. There is also Mac and some other platforms which do not have DCO.
> 

but we could still use "ifdef ENABLE_DCO" instead of the OR of those 3, no?

Cheers,

> We could likely get rid of those defines if we add disable_dco into
> platform-specific tuntap_options
> structures. Would it be better to have disable_dco member, assuming
> platform doesn't support dco?
>
  

So, to be more specific about the suggested changes

On Mon, Sep 12, 2022 at 12:10:57PM +0300, Lev Stipakov wrote:
[...]
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 2e567571..2a379f94 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))

As Antonio said, here the part after the && is redundant with ENABLE_DCO now.

>      "--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;
> @@ -903,6 +903,10 @@ init_options(struct options *o, const bool init_gc)
>      }
>  #endif /* _WIN32 */
>      o->allow_recursive_routing = false;
> +
> +#if !defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32))

Here I would definitely say it would be better to just always have a disable_dco in tuntap_options
instead of limiting this to platforms that have DCO.

> +    o->tuntap_options.disable_dco = true;
> +#endif /* !defined(ENABLE_DCO) && (defined(TARGET_LINUX) || defined(TARGET_FREEBSD) || defined(_WIN32)) */
>  }
>  
>  void
> @@ -3606,8 +3610,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 +3685,31 @@ 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)

Here the #if seems redundant with dco_enabled, which will already return false
if ENABLE_DCO is not set.

> +    if (dco_enabled(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);
> +    }
> +#endif
> +
> +#ifdef _WIN32
>      if (dco_enabled(o))
>      {
> -        dco_check_option(M_USAGE, o);
> -        dco_check_startup_option(M_USAGE, o);
> +        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)

Again, no need to limit this by platform.

>          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;

BTW, you can't see it in the diff, but there is a wrong
#endif /* defined(_WIN32) */
now immediately afterwards. I wonder why uncrustify didn't
complain?

Regards,