| Message ID | 1600994969-7623-1-git-send-email-selva.nair@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Set DNS Domain using iservice | expand |
Hi, > Note: this will set the domain twice if both v4 and v6 DNS > servers are defined. It cant hurt, but could be avoided by > making the domain setting a separate call from the DNS > server setting. I think we should do that - there is no reason to make expensive call twice: $ time wmic nicconfig where \(InterfaceIndex=38\) call SetDNSDomain vpntest.lev Executing (\\LAPTOP-4L3N7KFS\ROOT\CIMV2:Win32_NetworkAdapterConfiguration.Index=17)->SetDNSDomain() Method execution successful. Out Parameters: instance of __PARAMETERS { ReturnValue = 0; }; real 0m0,236s user 0m0,000s sys 0m0,015s Besides, DOMAIN could also be set by DHCP - there is no point to do it via service in this case. > + strncpy(dns.domains, tt->options.domain, _countof(dns.domains)); > + /* truncation of domain name is not checked as it cant happen > + * with 512 bytes room in dns.domains. > + */ Mix of tabs and whitespaces. > + /* comma separated list must be enclosed in parenthesis */ We don't pass lists yet, is this for future SearchDomains support? > + if (msg->domains[0]) { > + err = SetDNSDomain(wide_name, msg->domains, lists); > + } msg->domains comes from (unprivileged) client and might not be NULL terminated. Shall we do something like msg->domains[sizeof(msg->domains) - 1] = '\0'; Same for interface_t::name. Or am I missing something?
Hi Thanks for the review. On Fri, Sep 25, 2020 at 5:24 AM Lev Stipakov <lstipakov@gmail.com> wrote: > Hi, > > > Note: this will set the domain twice if both v4 and v6 DNS > > servers are defined. It cant hurt, but could be avoided by > > making the domain setting a separate call from the DNS > > server setting. > > I think we should do that - there is no reason to make expensive call > twice: > Separating the two is easy, but not sure how hard it would be to avoid calling it twice. With the ipv6-only feature, ipv4 setting is no longer guaranteed and elsewhere in the code we do some tasks twice: once for v4 and once for v6 instead of keeping a tab. I'll take a closer look. > > $ time wmic nicconfig where \(InterfaceIndex=38\) call SetDNSDomain > vpntest.lev > Executing > (\\LAPTOP-4L3N7KFS\ROOT\CIMV2:Win32_NetworkAdapterConfiguration.Index=17)->SetDNSDomain() > Method execution successful. > Out Parameters: > instance of __PARAMETERS > { > ReturnValue = 0; > }; > > real 0m0,236s > user 0m0,000s > sys 0m0,015s Besides, DOMAIN could also be set by DHCP - there is no point > to do it via service in this case. > If DHCP is in use, do_dns_service() will not get called and the DOMAIN setting will not be delegated to the service even with this patch. > > + strncpy(dns.domains, tt->options.domain, _countof(dns.domains)); > > + /* truncation of domain name is not checked as it cant happen > > + * with 512 bytes room in dns.domains. > > + */ > > Mix of tabs and whitespaces. > Will fix. > > > + /* comma separated list must be enclosed in parenthesis */ > > We don't pass lists yet, is this for future SearchDomains support? > Yes, kind of... I have been playing with this for a while and was motivated because wmic does support SetDNSSuffixSearchOrder. But works only for the global setting, not per interface -- it errors if a where clause is added. I left the code in there as it may get supported in future. Also it took awhile for me to figure out what format is needed for lists vs single objects from the command line. > > > + if (msg->domains[0]) { > > + err = SetDNSDomain(wide_name, msg->domains, lists); > > + } > > msg->domains comes from (unprivileged) client and might > not be NULL terminated. Shall we do something like > > msg->domains[sizeof(msg->domains) - 1] = '\0'; > > Same for interface_t::name. Or am I missing something? > My mistake. Will fix. Also our own safer strncpynt instead of strncpy in do_dns_service(). Thanks, Selva <div dir="ltr"><div>Hi</div><div><br></div>Thanks for the review.<div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Sep 25, 2020 at 5:24 AM Lev Stipakov <<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi,<br> <br> > Note: this will set the domain twice if both v4 and v6 DNS<br> > servers are defined. It cant hurt, but could be avoided by<br> > making the domain setting a separate call from the DNS<br> > server setting.<br> <br> I think we should do that - there is no reason to make expensive call twice:<br></blockquote><div><br></div><div>Separating the two is easy, but not sure how hard it would be to avoid calling it twice. With the ipv6-only feature, ipv4 setting is no longer guaranteed and elsewhere in the code we do some tasks twice: once for v4 and once for v6 instead of keeping a tab.</div><div><br></div><div>I'll take a closer look.</div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> $ time wmic nicconfig where \(InterfaceIndex=38\) call SetDNSDomain vpntest.lev<br> Executing (\\LAPTOP-4L3N7KFS\ROOT\CIMV2:Win32_NetworkAdapterConfiguration.Index=17)->SetDNSDomain()<br> Method execution successful.<br> Out Parameters:<br> instance of __PARAMETERS<br> {<br> ReturnValue = 0;<br> };<br> <br> real 0m0,236s<br> user 0m0,000s<br> sys 0m0,015s </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> Besides, DOMAIN could also be set by DHCP - there is no point<br> to do it via service in this case.<br></blockquote><div><br></div><div>If DHCP is in use, do_dns_service() will not get called and the DOMAIN setting will not be delegated to the service even with this patch. <br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> > + strncpy(dns.domains, tt->options.domain, _countof(dns.domains));<br> > + /* truncation of domain name is not checked as it cant happen<br> > + * with 512 bytes room in dns.domains.<br> > + */<br> <br> Mix of tabs and whitespaces.<br></blockquote><div><br></div><div>Will fix.</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"> <br> > + /* comma separated list must be enclosed in parenthesis */<br> <br> We don't pass lists yet, is this for future SearchDomains support?<br></blockquote><div><br></div><div>Yes, kind of...</div><div><br></div><div>I have been playing with this for a while and was motivated because wmic does support SetDNSSuffixSearchOrder. But works only for the global setting, not per interface -- it errors if a where clause is added.</div><div><br></div><div>I left the code in there as it may get supported in future. Also it took awhile for me to figure out what format is needed for lists vs single objects from the command line.</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"> <br> > + if (msg->domains[0]) {<br> > + err = SetDNSDomain(wide_name, msg->domains, lists);<br> > + }<br> <br> msg->domains comes from (unprivileged) client and might<br> not be NULL terminated. Shall we do something like<br> <br> msg->domains[sizeof(msg->domains) - 1] = '\0';<br> <br> Same for interface_t::name. Or am I missing something?<br></blockquote><div><br></div><div>My mistake. Will fix. Also our own safer strncpynt instead of strncpy in do_dns_service().</div><div><br></div><div>Thanks,</div><div><br></div><div>Selva</div></div></div></div>
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c index 80ae695..b681905 100644 --- a/src/openvpn/tun.c +++ b/src/openvpn/tun.c @@ -159,7 +159,7 @@ do_dns_service(bool add, const short family, const struct tuntap *tt) int addr_len = add ? len : 0; const char *ip_proto_name = family == AF_INET6 ? "IPv6" : "IPv4"; - if (addr_len == 0 && add) /* no addresses to add */ + if (addr_len == 0 && !tt->options.domain && add) /* no addresses or domain to add */ { return true; } @@ -199,9 +199,21 @@ do_dns_service(bool add, const short family, const struct tuntap *tt) dns.addr[i].ipv4.s_addr = htonl(tt->options.dns[i]); } } + if (tt->options.domain) + { + strncpy(dns.domains, tt->options.domain, _countof(dns.domains)); + /* truncation of domain name is not checked as it cant happen + * with 512 bytes room in dns.domains. + */ + msg(D_LOW, "%s dns domain on '%s' (if_index = %d) using service", + (add ? "Setting" : "Deleting"), dns.iface.name, dns.iface.index); + } - msg(D_LOW, "%s %s dns servers on '%s' (if_index = %d) using service", - (add ? "Setting" : "Deleting"), ip_proto_name, dns.iface.name, dns.iface.index); + if (addr_len > 0 || !add) + { + msg(D_LOW, "%s %s dns servers on '%s' (if_index = %d) using service", + (add ? "Setting" : "Deleting"), ip_proto_name, dns.iface.name, dns.iface.index); + } if (!send_msg_iservice(pipe, &dns, sizeof(dns), &ack, "TUN")) { @@ -216,7 +228,8 @@ do_dns_service(bool add, const short family, const struct tuntap *tt) goto out; } - msg(M_INFO, "%s dns servers %s using service", ip_proto_name, (add ? "set" : "deleted")); + msg(M_INFO, "%s dns servers %s%s using service", ip_proto_name, + (tt->options.domain? "and domain " : ""), (add ? "set" : "deleted")); ret = true; out: diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c index 207cc4a..b7f144d 100644 --- a/src/openvpnserv/interactive.c +++ b/src/openvpnserv/interactive.c @@ -91,6 +91,7 @@ typedef enum { block_dns, undo_dns4, undo_dns6, + undo_domain, _undo_type_max } undo_type_t; typedef list_item_t *undo_lists_t[_undo_type_max]; @@ -564,6 +565,24 @@ InterfaceLuid(const char *iface_name, PNET_LUID luid) return status; } +static DWORD +ConvertInterfaceNameToIndex(const wchar_t *ifname, NET_IFINDEX *index) +{ + NET_LUID luid; + DWORD err; + + err = ConvertInterfaceAliasToLuid(ifname, &luid); + if (err == ERROR_SUCCESS) + { + err = ConvertInterfaceLuidToIndex(&luid, index); + } + if (err != ERROR_SUCCESS) + { + MsgToEventLog(M_ERR, L"Failed to find interface index for <%s>", ifname); + } + return err; +} + static BOOL CmpAddress(LPVOID item, LPVOID address) { @@ -1057,6 +1076,53 @@ out: return err; } +/** + * Run command: wmic nicconfig (InterfaceIndex=$if_index) call $action ($data) + * @param if_index "index of interface" + * @param action e.g., "SetDNSDomain" + * @param data data if required for action + * - a single word for SetDNSDomain, empty or NULL to delete + * - comma separated values for a list + */ +static DWORD +wmic_nicconfig_cmd(const wchar_t *action, const NET_IFINDEX if_index, + const wchar_t *data) +{ + DWORD err = 0; + wchar_t argv0[MAX_PATH]; + wchar_t *cmdline = NULL; + int timeout = 10000; /* in msec */ + + swprintf(argv0, _countof(argv0), L"%s\\%s", get_win_sys_path(), L"wbem\\wmic.exe"); + argv0[_countof(argv0) - 1] = L'\0'; + + const wchar_t *fmt; + /* comma separated list must be enclosed in parenthesis */ + if (data && wcschr(data, L',')) + { + fmt = L"wmic nicconfig where (InterfaceIndex=%ld) call %s (%s)"; + } + else + { + fmt = L"wmic nicconfig where (InterfaceIndex=%ld) call %s %s"; + } + + size_t ncmdline = wcslen(fmt) + 20 + wcslen(action) /* max 20 for ifindex */ + + (data ? wcslen(data) + 1 : 1); + cmdline = malloc(ncmdline*sizeof(wchar_t)); + if (!cmdline) + { + return ERROR_OUTOFMEMORY; + } + + openvpn_sntprintf(cmdline, ncmdline, fmt, if_index, action, + data? data : L""); + err = ExecCommand(argv0, cmdline, timeout); + + free(cmdline); + return err; +} + /* Delete all IPv4 or IPv6 dns servers for an interface */ static DWORD DeleteDNS(short family, wchar_t *if_name) @@ -1079,6 +1145,54 @@ CmpWString(LPVOID item, LPVOID str) return (wcscmp(item, str) == 0) ? TRUE : FALSE; } +/** + * Set interface specific DNS domain suffix + * @param if_name name of the the interface + * @param domain a single domain name + * @param lists pointer to the undo lists. If NULL + * undo lists are not altered. + * Will delete the currently set value if domain is empty. + */ +static DWORD +SetDNSDomain(const wchar_t *if_name, const char *domain, undo_lists_t *lists) +{ + NET_IFINDEX if_index; + + DWORD err = ConvertInterfaceNameToIndex(if_name, &if_index); + if (err != ERROR_SUCCESS) + { + return err; + } + + wchar_t *wdomain = utf8to16(domain); /* utf8 to wide-char */ + if (!wdomain) + { + return ERROR_OUTOFMEMORY; + } + + /* free undo list if previously set */ + if (lists) + { + free(RemoveListItem(&(*lists)[undo_domain], CmpWString, (void *)if_name)); + } + + err = wmic_nicconfig_cmd(L"SetDNSDomain", if_index, wdomain); + + /* Add to undo list if domain is non-empty */ + if (err == 0 && wdomain[0] && lists) + { + wchar_t *tmp_name = wcsdup(if_name); + if (!tmp_name || AddListItem(&(*lists)[undo_domain], tmp_name)) + { + free(tmp_name); + err = ERROR_OUTOFMEMORY; + } + } + + free(wdomain); + return err; +} + static DWORD HandleDNSConfigMessage(const dns_cfg_message_t *msg, undo_lists_t *lists) { @@ -1117,9 +1231,14 @@ HandleDNSConfigMessage(const dns_cfg_message_t *msg, undo_lists_t *lists) free(RemoveListItem(&(*lists)[undo_type], CmpWString, wide_name)); } - if (msg->header.type == msg_del_dns_cfg) /* job done */ + if (msg->header.type == msg_del_dns_cfg) { - goto out; + if (msg->domains[0]) + { + /* setting an empty domain removes any previous value */ + err = SetDNSDomain(wide_name, "", lists); + } + goto out; /* job done */ } for (int i = 0; i < addr_len; ++i) @@ -1142,6 +1261,8 @@ HandleDNSConfigMessage(const dns_cfg_message_t *msg, undo_lists_t *lists) */ } + err = 0; + if (msg->addr_len > 0) { wchar_t *tmp_name = wcsdup(wide_name); @@ -1154,7 +1275,9 @@ HandleDNSConfigMessage(const dns_cfg_message_t *msg, undo_lists_t *lists) } } - err = 0; + if (msg->domains[0]) { + err = SetDNSDomain(wide_name, msg->domains, lists); + } out: free(wide_name); @@ -1445,6 +1568,10 @@ Undo(undo_lists_t *lists) DeleteDNS(AF_INET6, item->data); break; + case undo_domain: + SetDNSDomain(item->data, "", NULL); + break; + case block_dns: interface_data = (block_dns_data_t *)(item->data); delete_block_dns_filters(interface_data->engine);