| Message ID | 1538229228-10620-1-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Enable dhcp on tap adapter using interactive service | expand |
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) > + || !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? > + /* 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); > + } > Same as above (code in netsh_dns_cmd). > + /* 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?). -Lev <div dir="ltr">Hi,<div><br></div><div>Thanks, I tested on Windows 10 with Visual Studio build and works as expected.</div><div><br></div><div>A few nitpicks:</div><div><br></div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ if (!WriteFile(pipe, &dhcp, sizeof(dhcp), &len, NULL)<br>+ || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))<br>+ {<br>+ msg(M_WARN, "TUN: could not talk to service: %s [%lu]",<br>+ strerror_win32(GetLastError(), &gc), GetLastError());<br>+ goto out;<br>+ }<br></blockquote><div><br></div><div>A very similar code (communicate with service) is used in do_address_service and do_dns6_service, so </div><div>how about factoring that code into separate method?</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ /* Path of netsh */<br>+ int n = GetSystemDirectory(argv0, MAX_PATH);<br>+ if (n > 0 && n < MAX_PATH) /* got system directory */<br>+ {<br>+ wcsncat(argv0, L"\\netsh.exe", MAX_PATH - n - 1);<br>+ }<br>+ else<br>+ {<br>+ wcsncpy(argv0, L"C:\\Windows\\system32\\netsh.exe", MAX_PATH);<br>+ }<br></blockquote><div><br></div><div>Same as above (code in netsh_dns_cmd).</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ /* max cmdline length in wchars -- include room for if index */<br>+ size_t ncmdline = wcslen(fmt) + 10 + 1;<br></blockquote><div><br></div><div>Maybe comment that 10 is a max string length of int type and 1 is a nul terminator (do we need it here?).</div></div></div><div class="gmail_quote"><div dir="ltr"><br></div></div><div dir="ltr" class="gmail_signature" data-smartmail="gmail_signature">-Lev</div></div>
Hi, Thanks for the review and comments. A quick reply below, will send a v2 later. On Mon, Oct 1, 2018 at 7:11 AM Lev Stipakov <lstipakov@gmail.com> wrote: > 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) >> + || !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? > Sounds like a good idea. As this affects a number of unrelated contexts, will follow up with a refactoring patch. > > >> + /* 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); >> + } >> > > Same as above (code in netsh_dns_cmd). > Will do in v2. > > > >> + /* 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?). > Extra room for NUL is strictly not necessary as there is space in the format specifier "%d" etc. Still, I think its better to have an explicit +1. Selva <div dir="ltr"><div>Hi,<br><br></div>Thanks for the review and comments. A quick reply below, will send a v2 later.<br><br><div class="gmail_quote"><div dir="ltr">On Mon, Oct 1, 2018 at 7:11 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hi,<div><br></div><div>Thanks, I tested on Windows 10 with Visual Studio build and works as expected.</div><div><br></div><div>A few nitpicks:</div><div><br></div><div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ if (!WriteFile(pipe, &dhcp, sizeof(dhcp), &len, NULL)<br>+ || !ReadFile(pipe, &ack, sizeof(ack), &len, NULL))<br>+ {<br>+ msg(M_WARN, "TUN: could not talk to service: %s [%lu]",<br>+ strerror_win32(GetLastError(), &gc), GetLastError());<br>+ goto out;<br>+ }<br></blockquote><div><br></div><div>A very similar code (communicate with service) is used in do_address_service and do_dns6_service, so </div><div>how about factoring that code into separate method?</div></div></div></div></blockquote><div><br></div><div>Sounds like a good idea. As this affects a number of unrelated contexts, will follow up with a refactoring patch.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ /* Path of netsh */<br>+ int n = GetSystemDirectory(argv0, MAX_PATH);<br>+ if (n > 0 && n < MAX_PATH) /* got system directory */<br>+ {<br>+ wcsncat(argv0, L"\\netsh.exe", MAX_PATH - n - 1);<br>+ }<br>+ else<br>+ {<br>+ wcsncpy(argv0, L"C:\\Windows\\system32\\netsh.exe", MAX_PATH);<br>+ }<br></blockquote><div> <br></div></div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><div></div><div>Same as above (code in netsh_dns_cmd).</div></div></div></div></blockquote><div><br></div><div>Will do in v2.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div class="gmail_quote"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ /* max cmdline length in wchars -- include room for if index */<br>+ size_t ncmdline = wcslen(fmt) + 10 + 1;<br></blockquote><div><br></div><div>Maybe comment that 10 is a max string length of int type and 1 is a nul terminator (do we need it here?).</div></div></div></div></blockquote><div><br></div><div>Extra room for NUL is strictly not necessary as there is space in the format specifier "%d" etc. Still, I think its better to have an explicit +1.<br><br></div><div>Selva<br></div></div></div>
diff --git a/include/openvpn-msg.h b/include/openvpn-msg.h index 82ecfe8..66177a2 100644 --- a/include/openvpn-msg.h +++ b/include/openvpn-msg.h @@ -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_ */ diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 50f158c..9bf7b27 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -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, "TUN: 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. */ @@ -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) { - netsh_enable_dhcp(&tt->options, tt->actual_name); + /* try using the service if available, else directly execute netsh */ + 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; diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 861f5e7..d0bb120 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -1176,6 +1176,50 @@ 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+1]; + + /* 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); + } + + /* 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 */ + 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) { @@ -1187,6 +1231,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; + enable_dhcp_message_t dhcp; } msg; ack_message_t ack = { .header = { @@ -1247,6 +1292,13 @@ HandleMessage(HANDLE pipe, DWORD bytes, DWORD count, LPHANDLE events, undo_lists ack.error_number = HandleDNSConfigMessage(&msg.dns, lists); break; + 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);