diff mbox series

[Openvpn-devel] Skip DHCP renew with Wintun adapter

Message ID 20201215173004.26170-1-domagoj@pensa.hr
State Accepted
Headers show
Series [Openvpn-devel] Skip DHCP renew with Wintun adapter | expand

Commit Message

Domagoj Pensa Dec. 15, 2020, 6:30 a.m. UTC 
Wintun does not support DHCP.
Running  DHCP renew with Wintun adapter fails with a logged warning.

Fixed so that DHCP renewing is called only for TAP-Windows6 adapters.
---
 src/openvpn/tun.c | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

Comments

Lev Stipakov Dec. 17, 2020, 11:45 p.m. UTC | #1 
Hi,

The patch looks fine, but I wonder how did you manage to trigger this case?

dhcp_release() or dhcp_renew() are called if dhcp_masq_post is set to true,
which only happens for tap-windows6.

fork_dhcp_action() is no-op unless dhcp-renew/dhcp-pre-release
are explicitly defined in the ovpn profile.

If latter is the case, I wonder if options postprocessing would be a
better place
to fix it.

-Lev
Domagoj Pensa Dec. 19, 2020, 2:22 a.m. UTC | #2 
Hi!

I've done some more investigation and I've found out that problem was 
introduced in commit:

09ae6280 tun.c: revise the IPv4 ifconfig flow on Windows

in the following change:

@@ -6420,10 +6419,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
     struct gc_arena gc = gc_new(); /* used also for device_guid allocation */
     tun_open_device(tt, dev_node, &device_guid, &gc);
 
-    if (tt->windows_driver == WINDOWS_DRIVER_TAP_WINDOWS6)
-    {
     tuntap_post_open(tt, device_guid);
-    }
 
     gc_free(&gc);

Due to that, DHCP setup section in tuntap_set_ip_addr() is executed when 
Wintun is used, but it shouldn't.

As dhcp_masq_post is false for Wintun, fork_dhcp_action() is executed and 
that outputs warning and does unnecessary fork.

With my patch applied, DHCP setup is done only when TAP-Windows6 is used.

In my case server pushes both dhcp-renew and dhcp-release.

When I pull-filter ignore them no forking occurs and thus no warning.

I hope this helps.

Regards,
Domagoj

On Fri, Dec 18, 2020 at 12:45:43PM +0200, Lev Stipakov wrote:
> Hi,
> 
> The patch looks fine, but I wonder how did you manage to trigger this case?
> 
> dhcp_release() or dhcp_renew() are called if dhcp_masq_post is set to true,
> which only happens for tap-windows6.
> 
> fork_dhcp_action() is no-op unless dhcp-renew/dhcp-pre-release
> are explicitly defined in the ovpn profile.
> 
> If latter is the case, I wonder if options postprocessing would be a
> better place
> to fix it.
> 
> -Lev
Lev Stipakov Dec. 20, 2020, 10:30 p.m. UTC | #3 
Hi,

> I've done some more investigation and I've found out that problem was
> introduced in commit:
>
> 09ae6280 tun.c: revise the IPv4 ifconfig flow on Windows

Right, we refactored code, added support for IPAPI and as a side-effect
DHCP renew/release code is always executed no matter adapter type.

With your explanation patch makes sense to me. Stared at the code
and tested with wintun.

Acked-by: Lev Stipakov <lstipakov@gmail.com>
Gert Doering Jan. 18, 2021, 8:16 a.m. UTC | #4 
Your patch has been applied to the master and release/2.5 branch.

I've not tested it in any way, but looked at "git show -w", and this
this makes it quite clear that the patch does not actually change
as much code as it looks like, just moves the whole "dhcp_masq_post"
code block into the "if (WINDOWS_DRIVER_TAP_WINDOWS6)" block - so,
for TAP6, nothing changes, and for wintun, the code is not reached.

commit e0e7625c6b15f85b81d4f11d02f3daf4f32f1200 (master)
commit b8f4ca323969c13b039f35e5792a35f3e903b28c (release/2.5)
Author: Domagoj Pensa
Date:   Tue Dec 15 18:30:04 2020 +0100

     Skip DHCP renew with Wintun adapter

     Acked-by: Lev Stipakov <lstipakov@gmail.com>
     Message-Id: <20201215173004.26170-1-domagoj@pensa.hr>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg21364.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 400a50ca..6892fdb7 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -6155,35 +6155,35 @@  tuntap_set_ip_addr(struct tuntap *tt,
                 status,
                 strerror_win32(status, &gc));
         }
-    }
 
-    /*
-     * If the TAP-Windows driver is masquerading as a DHCP server
-     * make sure the TCP/IP properties for the adapter are
-     * set correctly.
-     */
-    if (dhcp_masq_post)
-    {
-        /* check dhcp enable status */
-        if (dhcp_status(index) == DHCP_STATUS_DISABLED)
+        /*
+         * If the TAP-Windows driver is masquerading as a DHCP server
+         * make sure the TCP/IP properties for the adapter are
+         * set correctly.
+         */
+        if (dhcp_masq_post)
         {
-            msg(M_WARN, "WARNING: You have selected '--ip-win32 dynamic', which will not work unless the TAP-Windows TCP/IP properties are set to 'Obtain an IP address automatically'");
-        }
+            /* check dhcp enable status */
+            if (dhcp_status(index) == DHCP_STATUS_DISABLED)
+            {
+                msg(M_WARN, "WARNING: You have selected '--ip-win32 dynamic', which will not work unless the TAP-Windows TCP/IP properties are set to 'Obtain an IP address automatically'");
+            }
 
-        /* force an explicit DHCP lease renewal on TAP adapter? */
-        if (tt->options.dhcp_pre_release)
-        {
-            dhcp_release(tt);
+            /* force an explicit DHCP lease renewal on TAP adapter? */
+            if (tt->options.dhcp_pre_release)
+            {
+                dhcp_release(tt);
+            }
+            if (tt->options.dhcp_renew)
+            {
+                dhcp_renew(tt);
+            }
         }
-        if (tt->options.dhcp_renew)
+        else
         {
-            dhcp_renew(tt);
+            fork_dhcp_action(tt);
         }
     }
-    else
-    {
-        fork_dhcp_action(tt);
-    }
 
     if (tt->did_ifconfig_setup && tt->options.ip_win32_type == IPW32_SET_IPAPI)
     {