Always release dhcp address in close_tun()

From: Selva Nair <[email protected]...>

Also make sure --dhcp-pre-release results in not just dhcp_release() in open_tun() but a subsequent dhcp_renew() as well. Else dhcp transaction gets aborted as this call to release() happens after the adapter status is changed to connected.

Alternatively, the undocumented --dhcp-pre-release may be removed.

Fixes Trac #807 (but can't say the same for Trac #665 without knowing how to reproduce it)

---
 src/openvpn/tun.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

index f812844..31fcd67 100644
@@ -6056,6 +6056,7 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
             if (tt->options.dhcp_pre_release)
             {
                 dhcp_release(tt);
+                dhcp_renew(tt);
             }
             if (tt->options.dhcp_renew)
             {

@@ -6224,10 +6225,7 @@ close_tun(struct tuntap *tt)
         }
 #endif
 
-        if (tt->options.dhcp_release)
-        {
-            dhcp_release(tt);
-        }
+        dhcp_release(tt);
 
         if (tt->hand != NULL)
         {

2.1.4

Should this be applied to 2.4.0 or 2.5_gitmaster ? thanks

Hi,

Generally speaking, everything goes to git master (unless specifically tagged as "this is for 2.3 / 2.4").

Hi,

I do not see any bad side-effects mentioned in the man page. Essentially what it says is --dhcp-renew is not necessary in most setups but may be needed in some cases (e.g., media status set to always connected -- more on that below).
And then it says the same "caveats" apply to --dhcp-release. But none of what is mentioned is a caveat -- its only saying these options are usually not needed.

However, --dhcp-release appears to be required and at least is a good practice. We do remove addresses set using netsh or ipapi on disconnect. So why not remove the one set by dhcp? What Trac #807 shows is that if release is not called the adapter remembers the previous address and assigning the same address to another adapter fails. In fact 807 is not just about dhcp, even a statically assigned ip if left behind on an disconnected adapter cannot be applied to another. Kind of obvious when stated that way.

On further testing, I've found a way to reproduce a similar behaviour as Trac #807 even with a single adapter and multiple remotes (assigning different VPN IPs). But only in a special case: the media status set to "always connected" (tried on Win10 only). In this case, if there is an address set on the adpater, Windows does not trigger a dhcp-renew when the new connection is made Logically, one would think the --dhcp-renew option should help here, but for some reason it doesn't. However, dhcp release on closing tun does fix it. When there no IP set on the adapter, subsequent connection does automatically trigger a dhcp renew and all goes well.

So if the bug reported in Trac 665 is related to media status = always connected, this patch may fix that one too.

That said, the patch should also remove the option --dhcp-release and associated documentation as it effectively makes release non-optional. I can do a v2 if the patch is otherwise acceptable. In the mean time I've added --dhcp-release to all my configs.

Selva

Hi,

I applied this to 2.5 and used the build system to build windows installer using windows-nsis/build-snapshot

I tested the windows client against Linux 2.4

The test was:

Connect to first server, receive DHCP 10.*8*.0.6 (net30) Idle 10 mins / Transfer some data / disconnect

Edit the config file to connect to a new server (This was an exact copy of first server on a new --port and with a new --server subnet / PKI identical)

Connect to second server, receive DHCP 10.*9*.0.6 (net30) Idle 10 mins / Transfer some data / disconnect

No problems encountered.

Note: I *was* able to replicate the original bug using windows 10 / openvpn 2.4.0 and the same procedure as above.

I am confident that this resolves the problem for Win10. ACK for W10

Regards

Hi,

I am curious about this as I could not reproduce "the bug" with only one adapter except in a special case (see below) -- the issue I described in Trac #807 was related to assigning a VPN ip to one adapter, disconnect and then try to assign the same to another adapter.

However, some reports in the user list and Trac #665 appears to have problems when changing the remote with one adapter. I can see dhcp failure in such a case only if the media status on the adapter is set to always connected. Is that the case in your tests?

Thanks,

Selva

You are correct.

What I have in my tests is as follows:

Server Linux - Openvpn 2.4.0
Client Win10 - Openvpn 2.4.0

Server & client configs are essentially the default config files.

(Both clients are on the same W10 machine)
client-1.conf
tap #1
client ip 10.8.0.6/30

client-2.conf
tap #2
client ip 10.9.0.6/30

Connect both clients and get IP assigned to tap, ping tests OK. Disconnect both clients, do *not* use --dhcp-release/renew

Change client-1.conf to connect to the port of server-2 Connect to server-2, receive+assign DHCP 10.9.0.6 to TAP #1 This is where something goes wrong ..

#665 is related to 2.3.10/12/13 and while it maybe the same issue I have not run any tests regarding #665

Regards

Hi,

After thinking about this for a while, I agree we should do this, so ACK (even if Debbie10t already has ACKed it).

I'd ask for a few changes, though :-) - if I'm reading this correctly, tt->options.dhcp_release is now unused, so I think we should do it all the way - get rid of that flag variable, print a message ("obsolete option detected, already on" or so) if --dhcp-release is configured, and update the man page ("this option is a no-op now, as the functionality is enabled by default starting in 2.4.1").

I'll take care of Changes.rst as these will differ 2.4 <-> master and make cherrypicking a patch more annoying than necessary.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Hi,

Agreed. Will send a v2 later today.

Thanks,

Selva

There is one corner case that I realized only now: what happens when a real dhcp server is in use (say in bridge mode). Current dhcp_release() function is a no-op unless the masquerading dhcp server in the tap driver is active (it checks ltt->options.ip_win32_type == IPW32_SET_DHCP_MASQ)

Simply removing that restriction may be risky as its unclear to me whether IpReleaseAddress() is a synchronous or async call (MSDN doesn't say). If it blocks until the address is released (like 'ipconfig /renew' appears to do) then it won't work with an external dhcp server as our process is also the router for packets through the tunnel. Internal one should be ok, as those packets are handled in the tap driver.

Anyway this is beyond the current patch as it just uses dhcp_release() as coded (which looks safe). So I suppose we just go ahead and handle other cases as and when they show up..

Selva

Hi,

I'd leave it at that, at least for the time being.

I'm sure bridge mode with a remote DHCP server has even more interesting caveats... (there is a trac ticket somewhere that claims OpenVPN eats *some* DHCP options in this scenario...)

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

From: Selva Nair <[email protected]...>

Also make sure --dhcp-pre-release results in not just dhcp_release() in open_tun() but a subsequent dhcp_renew() as well. Else dhcp transaction gets aborted as this call to release() happens after the adapter status is changed to connected.

Fixes Trac #807 (but can't say the same for Trac #665 without knowing how to reproduce it)

v2: Mark --dhcp-release as obsolete in mapage and option parser, and remove the unused dhcp_release varaible.
Enforce dhcp-renew with dhcp-pre-release while parsing the option instead of in open_tun().

---
 doc/openvpn.8         | 4 +---
 src/openvpn/options.c | 6 ++----
 src/openvpn/tun.c     | 5 +----
 src/openvpn/tun.h     | 1 -
 4 files changed, 4 insertions(+), 12 deletions(-)

index 7bd6d9d..3e4c2e7 100644
@@ -5819,9 +5819,7 @@ flag.
 .TP
 .B \-\-dhcp\-release
 Ask Windows to release the TAP adapter lease on shutdown.
-This option has the same caveats as
-.B \-\-dhcp\-renew
-above.
+This option has no effect now, as it is enabled by default starting with version 2.4.1.
 .\"*********************************************************
 .TP
 .B \-\-register\-dns

index bfedb6a..4560b52 100644
@@ -716,7 +716,6 @@ static const char usage_message[] =
     "--dhcp-renew       : Ask Windows to renew the TAP adapter lease on startup.\n"
     "--dhcp-pre-release : Ask Windows to release the previous TAP adapter lease on\n"
     "                       startup.\n"
-    "--dhcp-release     : Ask Windows to release the TAP adapter lease on shutdown.\n"
     "--register-dns  : Run ipconfig /flushdns and ipconfig /registerdns\n"
     "                  on connection initiation.\n"
     "--tap-sleep n   : Sleep for n seconds after TAP adapter open before\n"

@@ -1214,7 +1213,6 @@ show_tuntap_options(const struct tuntap_options *o)
     SHOW_BOOL(dhcp_options);
     SHOW_BOOL(dhcp_renew);
     SHOW_BOOL(dhcp_pre_release);
-    SHOW_BOOL(dhcp_release);
     SHOW_STR(domain);
     SHOW_STR(netbios_scope);
     SHOW_INT(netbios_node_type);

@@ -7200,11 +7198,11 @@ add_option(struct options *options,
     {
         VERIFY_PERMISSION(OPT_P_IPWIN32);
         options->tuntap_options.dhcp_pre_release = true;
+        options->tuntap_options.dhcp_renew = true;
     }
     else if (streq(p[0], "dhcp-release") && !p[1])
     {
-        VERIFY_PERMISSION(OPT_P_IPWIN32);
-        options->tuntap_options.dhcp_release = true;
+        msg(M_WARN, "Obsolete option --dhcp-release detected. This is now on by default");
     }
     else if (streq(p[0], "dhcp-internal") && p[1] && !p[2]) /* standalone method for internal use */
     {

index f812844..ec9997e 100644
@@ -6224,10 +6224,7 @@ close_tun(struct tuntap *tt)
         }
 #endif
 
-        if (tt->options.dhcp_release)
-        {
-            dhcp_release(tt);
-        }
+        dhcp_release(tt);
 
         if (tt->hand != NULL)
         {

index f4b600c..b39fe15 100644
@@ -104,7 +104,6 @@ struct tuntap_options {
 
     bool dhcp_renew;
     bool dhcp_pre_release;
-    bool dhcp_release;
 
     bool register_dns;
 

2.1.4

Hi,

Request typo fix at commit time:
mapage -> man page and varaible -> variable

Sorry for the extra work..

Thanks,

Selva

nudge++;

ACK. This change enforces good manners, as we do on all other platforms
- ensure the IP address is released/removed when we release the interface (some platforms do not have code for that because it's automatic, others have). Having an extra not-very-well-known option for this is a bit peculiar.

I have changed the commit message as requested, and also changed the tile line to add "... on Windows", to make clear this is platform specific.

Your patch has been applied to the master and release/2.4 branch.

commit db5b9b45508ea8f66ea80565279af3edd9300499 (master) commit 139cd1b14c57501bee36c1ec1dc5a037617ec29e (release/2.4)

Author: Selva Nair
Date:   Tue Jan 3 15:38:03 2017 -0500

     Always release dhcp address in close_tun() on Windows.

     Signed-off-by: Selva Nair <[email protected]...>
     Acked-by: Gert Doering <[email protected]...>
     Message-Id: <[email protected]...>
     URL: https://www.mail-archive.com/[email protected]/msg13814.html
     Signed-off-by: Gert Doering <[email protected]...>

--
kind regards,

Gert Doering