[Openvpn-devel] Set DNS Domain using iservice

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

Commit Message

Selva Nair Sept. 24, 2020, 2:49 p.m. UTC
From: Selva Nair <selva.nair@gmail.com>

Use wmic instead of directly editing the registry
as the former does not take full effect unless the dns
client service is restarted.

Editing the registry appears to work erratically depending
on whether its followed with a dchp renew or ipconfig /registerdns
etc.

DOMAIN-SEARCH is not handled here as wmic only supports
setting the global search list which will over-ride all
interface specific values.  Editing the registry directly
combined with a wmic command to reset the global SearchList
is an option that could be considered in a separate patch.

Trac # 1209, 1331

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
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.

 src/openvpn/tun.c             |  21 +++++--
 src/openvpnserv/interactive.c | 133 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 147 insertions(+), 7 deletions(-)

Comments

Lev Stipakov Sept. 24, 2020, 11:23 p.m. UTC | #1
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?
Selva Nair Sept. 25, 2020, 4:29 a.m. UTC | #2
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 &lt;<a href="mailto:lstipakov@gmail.com">lstipakov@gmail.com</a>&gt; 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>
&gt; Note: this will set the domain twice if both v4 and v6 DNS<br>
&gt; servers are defined. It cant hurt, but could be avoided by<br>
&gt; making the domain setting a separate call from the DNS<br>
&gt; 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&#39;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)-&gt;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>
&gt; +        strncpy(dns.domains, tt-&gt;options.domain, _countof(dns.domains));<br>
&gt; +       /* truncation of domain name is not checked as it cant happen<br>
&gt; +        * with 512 bytes room in dns.domains.<br>
&gt; +        */<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>
&gt; +    /* comma separated list must be enclosed in parenthesis */<br>
<br>
We don&#39;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>
&gt; +    if (msg-&gt;domains[0]) {<br>
&gt; +        err = SetDNSDomain(wide_name, msg-&gt;domains, lists);<br>
&gt; +    }<br>
<br>
msg-&gt;domains comes from (unprivileged) client and might<br>
not be NULL terminated. Shall we do something like<br>
<br>
    msg-&gt;domains[sizeof(msg-&gt;domains) - 1] = &#39;\0&#39;;<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>

Patch

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);