[Openvpn-devel,2/2] tun.c: revise the IPv4 ifconfig flow on Windows

Message ID 20200310094822.588-2-simon@rozman.si
State Superseded
Headers show
Series [Openvpn-devel,1/2] tun.c: reorder IPv6 ifconfig on Windows | expand

Commit Message

Simon Rozman March 9, 2020, 10:48 p.m. UTC
When provisioning IP configuration, we shall not ask what kind of
adapter this is. Rather, we should ask what method of provisioning we
are configured to use.

It is options.c's job to rule out invalid combinations.

- do_ifconfig_ipv4(): unify the workflow with its IPv6 counterpart
  No need to distinguish Wintun and TAP-Windows6 here. This also fixes
  an issue with --windows-driver wintun overriding --ip-win32 manual,
  the later being perfectly fine choice for Wintun too.

- open_tun() & tuntap_post_open(): unify Wintun and TAP-Windows6
  workflow. This allows allows --ip-win32 ipapi now.

- close_tun() the cleanup has been revised to match the ifconfig
  workflow in reverse.

Signed-off-by: Simon Rozman <simon@rozman.si>
---
 src/openvpn/tun.c | 127 ++++++++++++++++++++++++----------------------
 1 file changed, 65 insertions(+), 62 deletions(-)

Comments

Lev Stipakov March 12, 2020, 4 a.m. UTC | #1
Hi,

Looks good, tested with interactive service and without (netsh, ipapi).

A few questions:

>   This also fixes an issue with --windows-driver wintun overriding
>  --ip-win32 manual, the later being perfectly fine choice for Wintun too.

We do still have code which forces netsh for wintun:

    if (options->windows_driver == WINDOWS_DRIVER_WINTUN)
    {
        options->tuntap_options.ip_win32_type = IPW32_SET_NETSH;
    }

Shouldn't we update options.c and ensure that only allowed ip_win32
for wintun are manual, netsh and ipapi?

> - open_tun() & tuntap_post_open(): unify Wintun and TAP-Windows6 workflow.

With that change, we flush ARP cache also for wintun. Is this needed?

> This allows allows --ip-win32 ipapi now.

See above - we need to get rid of forcing netsh with wintun for that to work.
To test ipapi case, I had to comment out netsh forcing.

Also, while testing this I found (and fixed) a bug
(https://patchwork.openvpn.net/patch/1039/),
I think that fix should go first - your patch exposes bug (prints
device_guid value) for wintun case.
Simon Rozman March 12, 2020, 3:47 p.m. UTC | #2
Hi,

> A few questions:
> 
> >   This also fixes an issue with --windows-driver wintun overriding
> >  --ip-win32 manual, the later being perfectly fine choice for Wintun
> too.
> 
> We do still have code which forces netsh for wintun:
> 
>     if (options->windows_driver == WINDOWS_DRIVER_WINTUN)
>     {
>         options->tuntap_options.ip_win32_type = IPW32_SET_NETSH;
>     }
> 
> Shouldn't we update options.c and ensure that only allowed ip_win32 for
> wintun are manual, netsh and ipapi?

True. I'm sure I got this in the original commit back then. Found it... When rebasing onto the current master, it conflicted and I dropped that change rather than fixing it. Wrong choice, sorry.

> > - open_tun() & tuntap_post_open(): unify Wintun and TAP-Windows6
> workflow.
> 
> With that change, we flush ARP cache also for wintun. Is this needed?ΒΈ

Nope, FlushIpNetTable() is definitely not needed on wintun adapters. I shall modify the ARP-flushing condition to include "&& tt->windows_driver == WINDOWS_DRIVER_TAP_WINDOWS6" and update the commit message.

> Also, while testing this I found (and fixed) a bug
> (https://patchwork.openvpn.net/patch/1039/),
> I think that fix should go first - your patch exposes bug (prints
> device_guid value) for wintun case.

I agree. Your patch should be applied first.

Regards,
Simon
Gert Doering March 14, 2020, 1:44 a.m. UTC | #3
Hi,

On Fri, Mar 13, 2020 at 02:47:39AM +0000, Simon Rozman wrote:
> True. I'm sure I got this in the original commit back then. Found it... When rebasing onto the current master, it conflicted and I dropped that change rather than fixing it. Wrong choice, sorry.

Am I reading this correctly, a v2 version of this patch is coming?

gert

Patch

diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 42193d97..f7224093 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1381,34 +1381,29 @@  do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,
         env_set_destroy(aix_es);
     }
 #elif defined (_WIN32)
-    {
-        ASSERT(ifname != NULL);
-
-        if (tt->options.msg_channel && tt->windows_driver == WINDOWS_DRIVER_WINTUN)
-        {
-            do_address_service(true, AF_INET, tt);
-            do_dns_service(true, AF_INET, tt);
-        }
-        else
-        {
-            switch (tt->options.ip_win32_type)
-            {
-                case IPW32_SET_MANUAL:
-                    msg(M_INFO,
-                        "******** NOTE:  Please manually set the IP/netmask of '%s' to %s/%s (if it is not already set)",
-                        ifname, ifconfig_local,
-                        print_in_addr_t(tt->adapter_netmask, 0, &gc));
-                    break;
+    ASSERT(ifname != NULL);
 
-                case IPW32_SET_NETSH:
-                    netsh_ifconfig(&tt->options, ifname, tt->local,
-                                   tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS);
-
-                    break;
-            }
-        }
+    if (tt->options.ip_win32_type == IPW32_SET_MANUAL)
+    {
+        msg(M_INFO,
+            "******** NOTE:  Please manually set the IP/netmask of '%s' to %s/%s (if it is not already set)",
+            ifname, ifconfig_local,
+            print_in_addr_t(tt->adapter_netmask, 0, &gc));
+    }
+    else if (tt->options.ip_win32_type == IPW32_SET_DHCP_MASQ || tt->options.ip_win32_type == IPW32_SET_ADAPTIVE)
+    {
+        /* Let the DHCP configure the interface. */
+    }
+    else if (tt->options.msg_channel)
+    {
+        do_address_service(true, AF_INET, tt);
+        do_dns_service(true, AF_INET, tt);
+    }
+    else if (tt->options.ip_win32_type == IPW32_SET_NETSH)
+    {
+        netsh_ifconfig(&tt->options, ifname, tt->local,
+                       tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS);
     }
-
 #else  /* if defined(TARGET_LINUX) */
     msg(M_FATAL, "Sorry, but I don't know how to do 'ifconfig' commands on this operating system.  You should ifconfig your TUN/TAP device manually or use an --up script.");
 #endif /* if defined(TARGET_LINUX) */
@@ -6357,36 +6352,39 @@  tuntap_post_open(struct tuntap *tt, const char *device_guid)
     bool dhcp_masq = false;
     bool dhcp_masq_post = false;
 
-    /* get driver version info */
-    tuntap_get_version_info(tt);
+    if (tt->windows_driver == WINDOWS_DRIVER_TAP_WINDOWS6)
+    {
+        /* get driver version info */
+        tuntap_get_version_info(tt);
 
-    /* get driver MTU */
-    tuntap_get_mtu(tt);
+        /* get driver MTU */
+        tuntap_get_mtu(tt);
 
-    /*
-     * Preliminaries for setting TAP-Windows adapter TCP/IP
-     * properties via --ip-win32 dynamic or --ip-win32 adaptive.
-     */
-    if (tt->did_ifconfig_setup)
-    {
-        tuntap_set_ip_props(tt, &dhcp_masq, &dhcp_masq_post);
-    }
+        /*
+         * Preliminaries for setting TAP-Windows adapter TCP/IP
+         * properties via --ip-win32 dynamic or --ip-win32 adaptive.
+         */
+        if (tt->did_ifconfig_setup)
+        {
+            tuntap_set_ip_props(tt, &dhcp_masq, &dhcp_masq_post);
+        }
 
-    /* set point-to-point mode if TUN device */
-    if (tt->type == DEV_TYPE_TUN)
-    {
-        tuntap_set_ptp(tt);
-    }
+        /* set point-to-point mode if TUN device */
+        if (tt->type == DEV_TYPE_TUN)
+        {
+            tuntap_set_ptp(tt);
+        }
 
-    /* should we tell the TAP-Windows driver to masquerade as a DHCP server as a means
-     * of setting the adapter address? */
-    if (dhcp_masq)
-    {
-        tuntap_dhcp_mask(tt, device_guid);
-    }
+        /* should we tell the TAP-Windows driver to masquerade as a DHCP server as a means
+         * of setting the adapter address? */
+        if (dhcp_masq)
+        {
+            tuntap_dhcp_mask(tt, device_guid);
+        }
 
-    /* set driver media status to 'connected' */
-    tuntap_set_connected(tt);
+        /* set driver media status to 'connected' */
+        tuntap_set_connected(tt);
+    }
 
     /* possibly use IP Helper API to set IP address on adapter */
     tuntap_set_ip_addr(tt, device_guid, dhcp_masq_post);
@@ -6413,10 +6411,7 @@  open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
 
     tun_open_device(tt, dev_node, &device_guid);
 
-    if (tt->windows_driver == WINDOWS_DRIVER_TAP_WINDOWS6)
-    {
-        tuntap_post_open(tt, device_guid);
-    }
+    tuntap_post_open(tt, device_guid);
 
     /*netcmd_semaphore_release ();*/
 }
@@ -6533,20 +6528,29 @@  close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
             netsh_delete_address_dns(tt, true, &gc);
         }
     }
-#if 1
-    if (tt->windows_driver == WINDOWS_DRIVER_WINTUN)
+
+    if (tt->did_ifconfig_setup)
     {
-        if (tt->options.msg_channel)
+        if (tt->options.ip_win32_type == IPW32_SET_MANUAL)
+        {
+            /* We didn't do ifconfig. */
+        }
+        else if (tt->options.ip_win32_type == IPW32_SET_DHCP_MASQ || tt->options.ip_win32_type == IPW32_SET_ADAPTIVE)
+        {
+            /* We don't have to clean the configuration with DHCP. */
+        }
+        else if (tt->options.msg_channel)
         {
-            do_address_service(false, AF_INET, tt);
             do_dns_service(false, AF_INET, tt);
+            do_address_service(false, AF_INET, tt);
         }
-        else
+        else if (tt->options.ip_win32_type == IPW32_SET_NETSH)
         {
             netsh_delete_address_dns(tt, false, &gc);
         }
     }
-    else if (tt->ipapi_context_defined)
+
+    if (tt->ipapi_context_defined)
     {
         DWORD status;
         if ((status = DeleteIPAddress(tt->ipapi_context)) != NO_ERROR)
@@ -6557,7 +6561,6 @@  close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
                 strerror_win32(status, &gc));
         }
     }
-#endif /* if 1 */
 
     dhcp_release(tt);