Enable dhcp on tap adapter using interactive service

Hi,

Thanks, I tested on Windows 10 with Visual Studio build and works as expected.

A few nitpicks:

+ if (!WriteFile(pipe, &dhcp, sizeof(dhcp), &len, NULL)

src/openvpn/tun.c netsh_enable_dhcp(const struct tuntap_options *to, · line 5231

Quoted 7 lines

src/openvpn/tun.c +6 -0

+        || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))
+    {
+        msg(M_WARN, "TUN: could not talk to service: %s [%lu]",
+            strerror_win32(GetLastError(), &gc), GetLastError());
+        goto out;
+    }

A very similar code (communicate with service) is used in do_address_service and do_dns6_service, so
how about factoring that code into separate method?

src/openvpnserv/interactive.c out: · line 1195

Quoted 13 lines

src/openvpnserv/interactive.c +10 -0

+    /* Path of netsh */
+    int n = GetSystemDirectory(argv0, MAX_PATH);
+    if (n > 0 && n < MAX_PATH) /* got system directory */
+    {
+        wcsncat(argv0, L"\\netsh.exe", MAX_PATH - n - 1);
+    }
+    else
+    {
+        wcsncpy(argv0, L"C:\\Windows\\system32\\netsh.exe", MAX_PATH);
+    }

src/openvpnserv/interactive.c out: · line 1203

Quoted 6 lines

src/openvpnserv/interactive.c +2 -0

+    /* max cmdline length in wchars -- include room for if index */
+    size_t ncmdline = wcslen(fmt) + 10 + 1;

Maybe comment that 10 is a max string length of int type and 1 is a nul terminator (do we need it here?).

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

Currently, if dhcp on the TAP interface is disabled, OpenVPN on Windows tries to enable it using netsh but that succeeds only when run with admin privileges.

When interactive service is available, delegate this task to the service.

Trac #1111
Tested on Windows 7

Signed-off-by Selva Nair <[email protected]...>

---
v2 changes: As suggested by Lev
- In comment, clarify the 10 chars room is for printing 32 bit int
- Use get_win_sys_path() added in the accompanying patch (1 of 3)

Refactoring of writing to the message channel is in the following patch.

 include/openvpn-msg.h         |  8 ++++++-
 src/openvpn/tun.c             | 53
++++++++++++++++++++++++++++++++++++++++++-
 src/openvpnserv/interactive.c | 47 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 2 deletions(-)

include/openvpn-msg.h +7 -1

index 82ecfe8..66177a2 100644
@@ -37,7 +37,8 @@ typedef enum {
     msg_flush_neighbors,
     msg_add_block_dns,
     msg_del_block_dns,
-    msg_register_dns
+    msg_register_dns,
+    msg_enable_dhcp,
 } message_type_t;
 
 typedef struct {

@@ -111,4 +112,9 @@ typedef struct {
     interface_t iface;
 } block_dns_message_t;
 
+typedef struct {
+    message_header_t header;
+    interface_t iface;
+} enable_dhcp_message_t;
+
 #endif /* ifndef OPENVPN_MSG_H_ */

src/openvpn/tun.c +43 -0

index 50f158c..a2d5315 100644
@@ -5203,6 +5203,49 @@ netsh_enable_dhcp(const struct tuntap_options *to,
     argv_reset(&argv);
 }
 
+/* Enable dhcp on tap adapter using iservice */
+static bool
+service_enable_dhcp(const struct tuntap *tt)
+{
+    DWORD len;
+    bool ret = false;
+    ack_message_t ack;
+    struct gc_arena gc = gc_new();
+    HANDLE pipe = tt->options.msg_channel;
+
+    enable_dhcp_message_t dhcp = {
+        .header = {
+            msg_enable_dhcp,
+            sizeof(enable_dhcp_message_t),
+            0
+        },
+        .iface = { .index = tt->adapter_index, .name = "" }
+    };
+
+    if (!WriteFile(pipe, &dhcp, sizeof(dhcp), &len, NULL)
+        || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))
+    {
+        msg(M_WARN, "Enable_dhcp: could not talk to service: %s [%lu]",
+            strerror_win32(GetLastError(), &gc), GetLastError());
+        goto out;
+    }
+
+    if (ack.error_number != NO_ERROR)
+    {
+        msg(M_NONFATAL, "TUN: enabling dhcp using service failed: %s
 [status=%u if_index=%d]",
+            strerror_win32(ack.error_number, &gc), ack.error_number,
 dhcp.iface.index);
+    }
+    else
+    {
+        msg(M_INFO, "DHCP enabled on interface %d using service",
 dhcp.iface.index);
+        ret = true;
+    }
+
+out:
+    gc_free(&gc);
+    return ret;
+}
+

/*
* Return a TAP name for netsh commands.
*/

src/openvpn/tun.c +0 -0

@@ -5683,7 +5726,15 @@ open_tun(const char *dev, const char *dev_type,

const char *dev_node, struct tun

              */
             if (dhcp_status(tt->adapter_index) == DHCP_STATUS_DISABLED)
             {

src/openvpn/tun.c +1 -1

-                netsh_enable_dhcp(&tt->options, tt->actual_name);
+                /* try using the service if available, else directly

execute netsh */

src/openvpn/tun.c +8 -0

+                if (tt->options.msg_channel)
+                {
+                    service_enable_dhcp(tt);
+                }
+                else
+                {
+                    netsh_enable_dhcp(&tt->options, tt->actual_name);
+                }
             }
             dhcp_masq = true;
             dhcp_masq_post = true;

src/openvpnserv/interactive.c +39 -0

index 0489684..c0fdc19 100644
@@ -1165,6 +1165,45 @@ out:
     return err;
 }
 
+static DWORD
+HandleEnableDHCPMessage(const enable_dhcp_message_t *dhcp)
+{
+    DWORD err = 0;
+    DWORD timeout = 5000; /* in milli seconds */
+    wchar_t argv0[MAX_PATH];
+
+    /* Path of netsh */
+    swprintf(argv0, _countof(argv0), L"%s\\%s", get_win_sys_path(),
 L"netsh.exe");
+    argv0[_countof(argv0) - 1] = L'\0';
+
+    /* cmd template:
+     * netsh interface ipv4 set address name=$if_index source=dhcp
+     */
+    const wchar_t *fmt = L"netsh interface ipv4 set address name=\"%d\"
 source=dhcp";
+
+    /* max cmdline length in wchars -- include room for if index:
+     * 10 chars for 32 bit int in decimal and +1 for NUL
+     */
+    size_t ncmdline = wcslen(fmt) + 10 + 1;
+    wchar_t *cmdline = malloc(ncmdline*sizeof(wchar_t));
+    if (!cmdline)
+    {
+        err = ERROR_OUTOFMEMORY;
+        return err;
+    }
+
+    openvpn_sntprintf(cmdline, ncmdline, fmt, dhcp->iface.index);
+
+    err = ExecCommand(argv0, cmdline, timeout);
+
+    /* Note: This could fail if dhcp is already enabled, so the caller
+     * may not want to treat errors as FATAL.
+     */
+
+    free(cmdline);
+    return err;
+}
+
 static VOID

HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events,
undo_lists_t *lists)
{

src/openvpnserv/interactive.c +0 -0

@@ -1176,6 +1215,7 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count,

LPHANDLE events, undo_lists

         flush_neighbors_message_t flush_neighbors;
         block_dns_message_t block_dns;
         dns_cfg_message_t dns;

src/openvpnserv/interactive.c +1 -0

+        enable_dhcp_message_t dhcp;
     } msg;
     ack_message_t ack = {
         .header = {

@@ -1236,6 +1276,13 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD

count, LPHANDLE events, undo_lists

             ack.error_number = HandleDNSConfigMessage(&msg.dns, lists);
             break;

src/openvpnserv/interactive.c +7 -0

+        case msg_enable_dhcp:
+            if (msg.header.size == sizeof(msg.dhcp))
+            {
+                ack.error_number = HandleEnableDHCPMessage(&msg.dhcp);
+            }
+            break;
+
         default:
             ack.error_number = ERROR_MESSAGE_TYPE;
             MsgToEventLog(MSG_FLAGS_ERROR, TEXT("Unknown message type

%d"), msg.header.type);
--
2.1.4

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

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

Move writing the message buffer to the interactive service pipe and reading acknowledgement to a function.

A minor bug in open_tun where the ack data could be read even after a communication error is fixed.

Signed-off-by Selva Nair <[email protected]...>

---
 src/openvpn/route.c |  6 +-----
 src/openvpn/tun.c   | 34 +++++++++-------------------------
 src/openvpn/win32.c | 27 ++++++++++++++++++++++-----
 src/openvpn/win32.h |  9 +++++++++
 4 files changed, 41 insertions(+), 35 deletions(-)

src/openvpn/route.c +1 -5

index ff39230..8a3e8b4 100644
@@ -2991,16 +2991,12 @@ del_route_ipapi(const struct route_ipv4 *r, const
 struct tuntap *tt)
 static bool
 do_route_service(const bool add, const route_message_t *rt, const size_t
 size, HANDLE pipe)
 {
-    DWORD len;
     bool ret = false;
     ack_message_t ack;
     struct gc_arena gc = gc_new();
 
-    if (!WriteFile(pipe, rt, size, &len, NULL)
-        || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))
+    if (!send_msg_iservice(pipe, rt, size, &ack, "ROUTE"))
     {
-        msg(M_WARN, "ROUTE: could not talk to service: %s [%lu]",
-            strerror_win32(GetLastError(), &gc), GetLastError());
         goto out;

     }

src/openvpn/tun.c +0 -1

index a2d5315..948fd17 100644
@@ -82,7 +82,6 @@ static DWORD get_adapter_index_flexible(const char
 *name);
 static bool
 do_address_service(const bool add, const short family, const struct
 tuntap *tt)
 {
-    DWORD len;
     bool ret = false;

     ack_message_t ack;
     struct gc_arena gc = gc_new();

src/openvpn/tun.c +6 -18

@@ -115,11 +114,8 @@ do_address_service(const bool add, const short
 family, const struct tuntap *tt)
         addr.prefix_len = tt->netbits_ipv6;
     }
 
-    if (!WriteFile(pipe, &addr, sizeof(addr), &len, NULL)
-        || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))
+    if (!send_msg_iservice(pipe, &addr, sizeof(addr), &ack, "TUN"))
     {
-        msg(M_WARN, "TUN: could not talk to service: %s [%lu]",
-            strerror_win32(GetLastError(), &gc), GetLastError());
         goto out;
     }

@@ -141,7 +137,6 @@ out:
 static bool
 do_dns6_service(bool add, const struct tuntap *tt)
 {
-    DWORD len;
     bool ret = false;
     ack_message_t ack;
     struct gc_arena gc = gc_new();

@@ -185,11 +180,8 @@ do_dns6_service(bool add, const struct tuntap *tt)
     msg(D_LOW, "%s IPv6 dns servers on '%s' (if_index = %d) using
 service",
         (add ? "Setting" : "Deleting"), dns.iface.name, dns.iface.index);
 
-    if (!WriteFile(pipe, &dns, sizeof(dns), &len, NULL)
-        || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))
+    if (!send_msg_iservice(pipe, &dns, sizeof(dns), &ack, "TUN"))
     {
-        msg(M_WARN, "TUN: could not talk to service: %s [%lu]",
-            strerror_win32(GetLastError(), &gc), GetLastError());
         goto out;
     }

@@ -5222,11 +5214,8 @@ service_enable_dhcp(const struct tuntap *tt)
         .iface = { .index = tt->adapter_index, .name = "" }
     };
 
-    if (!WriteFile(pipe, &dhcp, sizeof(dhcp), &len, NULL)
-        || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))
+    if (!send_msg_iservice(pipe, &dhcp, sizeof(dhcp), &ack,
 "Enable_dhcp"))
     {
-        msg(M_WARN, "Enable_dhcp: could not talk to service: %s [%lu]",
-            strerror_win32(GetLastError(), &gc), GetLastError());
         goto out;
     }

@@ -5461,18 +5450,16 @@ fork_dhcp_action(struct tuntap *tt)
 static void
 register_dns_service(const struct tuntap *tt)
 {
-    DWORD len;
     HANDLE msg_channel = tt->options.msg_channel;
     ack_message_t ack;
     struct gc_arena gc = gc_new();
 
     message_header_t rdns = { msg_register_dns, sizeof(message_header_t),
 0 };
 
-    if (!WriteFile(msg_channel, &rdns, sizeof(rdns), &len, NULL)
-        || !ReadFile(msg_channel, &ack, sizeof(ack), &len, NULL))
+    if (!send_msg_iservice(msg_channel, &rdns, sizeof(rdns), &ack,
 "Register_dns"))
     {
-        msg(M_WARN, "Register_dns: could not talk to service: %s
 [status=0x%lx]",
-            strerror_win32(GetLastError(), &gc), GetLastError());
+        gc_free(&gc);
+        return;

     }

     else if (ack.error_number != NO_ERROR)

src/openvpn/tun.c +0 -0

@@ -5936,14 +5923,11 @@ open_tun(const char *dev, const char *dev_type,

const char *dev_node, struct tun

                     .iface = { .index = index, .name = "" }
                 };

- if (!WriteFile(tt->options.msg_channel, &msg,
sizeof(msg), &len, NULL)
- || !ReadFile(tt->options.msg_channel, &ack,
sizeof(ack), &len, NULL))
+ if (send_msg_iservice(tt->options.msg_channel, &msg,
sizeof(msg),

src/openvpn/tun.c +1 -1

+                    &ack, "TUN"))
                 {
-                    msg(M_WARN, "TUN: could not talk to service: %s

[%lu]",
- strerror_win32(GetLastError(), &gc),
GetLastError());

src/openvpn/tun.c +1 -2

+                    status = ack.error_number;
                 }
-
-                status = ack.error_number;
             }
             else
             {

src/openvpn/win32.c +22 -5

index 3905524..e43296e 100644
@@ -1264,7 +1264,6 @@ win_get_tempdir(void)
 static bool
 win_block_dns_service(bool add, int index, const HANDLE pipe)
 {
-    DWORD len;
     bool ret = false;
     ack_message_t ack;
     struct gc_arena gc = gc_new();

@@ -1278,11 +1277,8 @@ win_block_dns_service(bool add, int index, const
 HANDLE pipe)
         .iface = { .index = index, .name = "" }
     };
 
-    if (!WriteFile(pipe, &data, sizeof(data), &len, NULL)
-        || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))
+    if (!send_msg_iservice(pipe, &data, sizeof(data), &ack, "Block_DNS"))
     {
-        msg(M_WARN, "Block_DNS: could not talk to service: %s [%lu]",
-            strerror_win32(GetLastError(), &gc), GetLastError());
         goto out;
     }

@@ -1473,4 +1469,25 @@ win32_version_string(struct gc_arena *gc, bool
 add_name)
     return (const char *)out.data;
 }
 
+bool
+send_msg_iservice(HANDLE pipe, const void *data, size_t size,
+                  ack_message_t *ack, const char *context)
+{
+    struct gc_arena gc = gc_new();
+    DWORD len;
+    bool ret = true;
+
+    if (!WriteFile(pipe, data, size, &len, NULL)
+        || !ReadFile(pipe, ack, sizeof(*ack), &len, NULL))
+    {
+        msg(M_WARN, "%s: could not talk to service: %s [%lu]",
+            context? context : "Unknown",
+            strerror_win32(GetLastError(), &gc), GetLastError());
+        ret = false;
+    }
+
+    gc_free(&gc);
+    return ret;
+}
+

#endif /* ifdef _WIN32 */

src/openvpn/win32.h +1 -0

index 4b99a5e..b5cbe25 100644
@@ -26,6 +26,7 @@
 #define OPENVPN_WIN32_H
 
 #include "mtu.h"
+#include "openvpn-msg.h"
 
 /* location of executables */
 #define SYS_PATH_ENV_VAR_NAME "SystemRoot"  /* environmental variable

name that normally contains the system path */

src/openvpn/win32.h +2 -0

@@ -307,5 +308,13 @@ int win32_version_info(void);
  */
 const char *win32_version_string(struct gc_arena *gc, bool add_name);
 
+/*
+ * Send the |size| bytes in buffer |data| to the interactive service

|pipe|
+ * and read the result in |ack|. Returns false on communication error.
+ * The string in |context| is used to prefix error messages.

+ */

src/openvpn/win32.h +3 -1

+bool send_msg_iservice(HANDLE pipe, const void *data, size_t size,
+                       ack_message_t *ack, const char *context);
+
 #endif /* ifndef OPENVPN_WIN32_H */
 #endif /* ifdef _WIN32 */
--

2.1.4

_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel

Hi,

src/openvpnserv/common.c utf8to16(const char *utf8) · line 298

Quoted 6 lines

src/openvpnserv/common.c +5 -0

+    if (!GetSystemDirectoryW(win_sys_path, _countof(win_sys_path)))
+    {
+        wcsncpy(win_sys_path, default_sys_path, _countof(win_sys_path));
+        win_sys_path[_countof(win_sys_path) - 1] = L'\0';
+    }

Is there need in adding null terminator to win_sys_path?

Since it is static, it will be initialized with zeroes at startup. Besides, wcsncpy also adds null terminator.

--
-Lev