diff mbox series

[Openvpn-devel] Allow --dhcp-option in config file when windows-driver is wintun

Message ID 1600126181-16364-1-git-send-email-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel] Allow --dhcp-option in config file when windows-driver is wintun | expand

Commit Message

Selva Nair Sept. 14, 2020, 1:29 p.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

When wintun is in use we mutate ip_win32_type to NETSH
and then complain that ip-win32 option should be dynamic or adaptive
if any --dhcp-option directive is present in the config file. This
causes a fatal error.

How to reproduce: specify a --dhcp-option in the config and change the
--windows-driver to wintun.

Fix this behaviour. A typo in the message is also corrected.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 src/openvpn/options.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Lev Stipakov Sept. 14, 2020, 8:48 p.m. UTC | #1 
Hi,

> -        msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or adaptive");
> +        msg(M_USAGE, "--dhcp-option requires --ip-win32 dynamic or adaptive");

Nice, this typo has been there since at least 2005.

It looks like that warning is not quite correct - for example,
DNS can be set via iservice or netsh, which means that

--ip-win32 netsh
--dhcp-option DNS 8.8.8.8

is a valid combination regardless of driver type. On the other hand,
NBDD/NTP are set via DHCP.

It would be good to make the warning condition more fine-grained, but
this patch is good enough as it is, since it fixes important case - specify
DNS in the ovpn profile when using wintun.

Built and tested with MSVC.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gert Doering Sept. 14, 2020, 9:02 p.m. UTC | #2 
Your patch has been applied to the master and release/2.5 branch.

("Looks good to me" as well, Lev just beat me with testing and ACKing)

commit b8625abbd5be21a810b648058e6e411a7ff19702 (master)
commit 9fe13491de31fff127bdceb6026fc05732f7dfb3 (release/2.5)
Author: Selva Nair
Date:   Mon Sep 14 19:29:41 2020 -0400

     Allow --dhcp-option in config file when windows-driver is wintun

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <1600126181-16364-1-git-send-email-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21005.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Selva Nair Sept. 15, 2020, 4:13 a.m. UTC | #3 
Hi

On Tue, Sep 15, 2020 at 2:48 AM Lev Stipakov <lstipakov@gmail.com> wrote:

> Hi,
>
> > -        msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or
> adaptive");
> > +        msg(M_USAGE, "--dhcp-option requires --ip-win32 dynamic or
> adaptive");
>
> Nice, this typo has been there since at least 2005.
>
> It looks like that warning is not quite correct - for example,
> DNS can be set via iservice or netsh, which means that
>
> --ip-win32 netsh
> --dhcp-option DNS 8.8.8.8
>
> is a valid combination regardless of driver type. On the other hand,
> NBDD/NTP are set via DHCP.
>
> It would be good to make the warning condition more fine-grained, but
> this patch is good enough as it is, since it fixes important case - specify
> DNS in the ovpn profile when using wintun.
>

We could deprecate --ip-win32 as a user option. Default to dynamic or
adaptive, automatically fail-over to alternate methods or change it
internally as required for wintun etc.

And work towards supporting more dhcp-options when dhcp is not possible --
using iservice, API, netsh etc.

Selva
<div dir="ltr"><div>Hi</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 15, 2020 at 2:48 AM Lev Stipakov &lt;<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br>
<br>
&gt; -        msg(M_USAGE, &quot;--dhcp-options requires --ip-win32 dynamic or adaptive&quot;);<br>
&gt; +        msg(M_USAGE, &quot;--dhcp-option requires --ip-win32 dynamic or adaptive&quot;);<br>
<br>
Nice, this typo has been there since at least 2005.<br>
<br>
It looks like that warning is not quite correct - for example,<br>
DNS can be set via iservice or netsh, which means that<br>
<br>
--ip-win32 netsh<br>
--dhcp-option DNS 8.8.8.8<br>
<br>
is a valid combination regardless of driver type. On the other hand,<br>
NBDD/NTP are set via DHCP.<br>
<br>
It would be good to make the warning condition more fine-grained, but<br>
this patch is good enough as it is, since it fixes important case - specify<br>
DNS in the ovpn profile when using wintun.<br></blockquote><div><br></div><div>We could deprecate --ip-win32 as a user option. Default to dynamic or adaptive, automatically fail-over to alternate methods or change it internally as required for wintun etc.</div><div><br></div><div>And work towards supporting more dhcp-options when dhcp is not possible -- using iservice, API, netsh etc.</div><div><br></div><div>Selva</div></div></div>
diff mbox series

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 8bf82c5..4b22d3d 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2181,10 +2181,11 @@  options_postprocess_verify_ce(const struct options *options, const struct connec
     }
 
     if (options->tuntap_options.dhcp_options
+        && options->windows_driver != WINDOWS_DRIVER_WINTUN
         && options->tuntap_options.ip_win32_type != IPW32_SET_DHCP_MASQ
         && options->tuntap_options.ip_win32_type != IPW32_SET_ADAPTIVE)
     {
-        msg(M_USAGE, "--dhcp-options requires --ip-win32 dynamic or adaptive");
+        msg(M_USAGE, "--dhcp-option requires --ip-win32 dynamic or adaptive");
     }
 
     if (options->windows_driver == WINDOWS_DRIVER_WINTUN && dev != DEV_TYPE_TUN)