[Openvpn-devel,M] Change in openvpn[master]: dns: do not use netsh to set name server addresses

Message ID 51a3e4271eccec02ef4ecdd0031cd84457832677-HTML@gerrit.openvpn.net
State New
Headers show
Series [Openvpn-devel,M] Change in openvpn[master]: dns: do not use netsh to set name server addresses | expand

Commit Message

ralf_lici (Code Review) Dec. 4, 2024, 4 p.m. UTC
Attention is currently required from: flichtenheld, plaisthos.

Hello plaisthos, flichtenheld,

I'd like you to do a code review.
Please visit

    http://gerrit.openvpn.net/c/openvpn/+/825?usp=email

to review the following change.


Change subject: dns: do not use netsh to set name server addresses
......................................................................

dns: do not use netsh to set name server addresses

Instead of spawning a netsh process, set the name server addresses
directly in the registry hive of the VPN interface.

This is a first step to get rid of the use of command line tools in the
service and move to a more API driven style of modifying the VPN adapter
configuration.

Change-Id: Id2bed0908e84c19b8fb6fe806376316793e550b4
Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net>
---
M src/openvpnserv/interactive.c
1 file changed, 41 insertions(+), 115 deletions(-)



  git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/25/825/1

Patch

diff --git a/src/openvpnserv/interactive.c b/src/openvpnserv/interactive.c
index cad8b02..8d000f1 100644
--- a/src/openvpnserv/interactive.c
+++ b/src/openvpnserv/interactive.c
@@ -1024,65 +1024,6 @@ 
 }
 
 /**
- * Run the command: netsh interface $proto $action dns $if_name $addr [validate=no]
- * @param  action      "delete" or "add"
- * @param  proto       "ipv6" or "ip"
- * @param  if_name     "name_of_interface"
- * @param  addr         IPv4 (for proto = ip) or IPv6 address as a string
- *
- * If addr is null and action = "delete" all addresses are deleted.
- */
-static DWORD
-netsh_dns_cmd(const wchar_t *action, const wchar_t *proto, const wchar_t *if_name, const wchar_t *addr)
-{
-    DWORD err = 0;
-    int timeout = 30000; /* in msec */
-    wchar_t argv0[MAX_PATH];
-    wchar_t *cmdline = NULL;
-
-    if (!addr)
-    {
-        if (wcscmp(action, L"delete") == 0)
-        {
-            addr = L"all";
-        }
-        else /* nothing to do -- return success*/
-        {
-            goto out;
-        }
-    }
-
-    /* Path of netsh */
-    swprintf(argv0, _countof(argv0), L"%ls\\%ls", get_win_sys_path(), L"netsh.exe");
-
-    /* cmd template:
-     * netsh interface $proto $action dns $if_name $addr [validate=no]
-     */
-    const wchar_t *fmt = L"netsh interface %ls %ls dns \"%ls\" %ls";
-
-    /* max cmdline length in wchars -- include room for worst case and some */
-    size_t ncmdline = wcslen(fmt) + wcslen(if_name) + wcslen(addr) + 32 + 1;
-    cmdline = malloc(ncmdline*sizeof(wchar_t));
-    if (!cmdline)
-    {
-        err = ERROR_OUTOFMEMORY;
-        goto out;
-    }
-
-    swprintf(cmdline, ncmdline, fmt, proto, action, if_name, addr);
-
-    if (IsWindows7OrGreater())
-    {
-        wcscat_s(cmdline, ncmdline, L" validate=no");
-    }
-    err = ExecCommand(argv0, cmdline, timeout);
-
-out:
-    free(cmdline);
-    return err;
-}
-
-/**
  * Run the command: netsh interface ip $action wins $if_name [static] $addr
  * @param  action      "delete", "add" or "set"
  * @param  if_name     "name_of_interface"
@@ -1139,22 +1080,6 @@ 
     return err;
 }
 
-/* Delete all IPv4 or IPv6 dns servers for an interface */
-static DWORD
-DeleteDNS(short family, wchar_t *if_name)
-{
-    wchar_t *proto = (family == AF_INET6) ? L"ipv6" : L"ip";
-    return netsh_dns_cmd(L"delete", proto, if_name, NULL);
-}
-
-/* Add an IPv4 or IPv6 dns server to an interface */
-static DWORD
-AddDNS(short family, wchar_t *if_name, wchar_t *addr)
-{
-    wchar_t *proto = (family == AF_INET6) ? L"ipv6" : L"ip";
-    return netsh_dns_cmd(L"add", proto, if_name, addr);
-}
-
 static BOOL
 CmpWString(LPVOID item, LPVOID str)
 {
@@ -1810,7 +1735,6 @@ 
 HandleDNSConfigMessage(const dns_cfg_message_t *msg, undo_lists_t *lists)
 {
     DWORD err = 0;
-    wchar_t addr[46]; /* large enough to hold string representation of an ipv4 / ipv6 address */
     undo_type_t undo_type = (msg->family == AF_INET6) ? undo_dns4 : undo_dns6;
     int addr_len = msg->addr_len;
 
@@ -1832,10 +1756,11 @@ 
         msgptr->domains[_countof(msg->domains)-1] = '\0';
     }
 
-    wchar_t *wide_name = utf8to16(msg->iface.name); /* utf8 to wide-char */
-    if (!wide_name)
+    WCHAR iid[64];
+    err = InterfaceIdString(msg->iface.name, iid, _countof(iid));
+    if (err)
     {
-        return ERROR_OUTOFMEMORY;
+        return err;
     }
 
     /* We delete all current addresses before adding any
@@ -1843,12 +1768,12 @@ 
      */
     if (addr_len > 0 || msg->header.type == msg_del_dns_cfg)
     {
-        err = DeleteDNS(msg->family, wide_name);
+        err = ResetNameServers(iid, msg->family);
         if (err)
         {
-            goto out;
+            return err;
         }
-        free(RemoveListItem(&(*lists)[undo_type], CmpWString, wide_name));
+        free(RemoveListItem(&(*lists)[undo_type], CmpAny, iid));
     }
 
     if (msg->header.type == msg_del_dns_cfg)
@@ -1860,40 +1785,43 @@ 
             err = SetDnsSearchDomains(msg->iface.name, NULL, &gpol, lists);
         }
         ApplyDnsSettings(gpol);
-        goto out;  /* job done */
+        return err;  /* job done */
     }
 
-    for (int i = 0; i < addr_len; ++i)
-    {
-        if (msg->family == AF_INET6)
-        {
-            RtlIpv6AddressToStringW(&msg->addr[i].ipv6, addr);
-        }
-        else
-        {
-            RtlIpv4AddressToStringW(&msg->addr[i].ipv4, addr);
-        }
-        err = AddDNS(msg->family, wide_name, addr);
-        if (i == 0 && err)
-        {
-            goto out;
-        }
-        /* We do not check for duplicate addresses, so any error in adding
-         * additional addresses is ignored.
-         */
-    }
-
-    err = 0;
-
     if (msg->addr_len > 0)
     {
-        wchar_t *tmp_name = _wcsdup(wide_name);
-        if (!tmp_name || AddListItem(&(*lists)[undo_type], tmp_name))
+        /* prepare the comma separated address list */
+        CHAR addrs[256]; /* large enough to hold four IPv4 / IPv6 address strings */
+        size_t offset = 0;
+        for (int i = 0; i < addr_len; ++i)
         {
-            free(tmp_name);
-            DeleteDNS(msg->family, wide_name);
-            err = ERROR_OUTOFMEMORY;
-            goto out;
+            if (i != 0)
+            {
+                addrs[offset++] = ',';
+            }
+            if (msg->family == AF_INET6)
+            {
+                RtlIpv6AddressToStringA(&msg->addr[i].ipv6, addrs + offset);
+            }
+            else
+            {
+                RtlIpv4AddressToStringA(&msg->addr[i].ipv4, addrs + offset);
+            }
+            offset += strlen(addrs);
+        }
+
+        err = SetNameServers(iid, msg->family, addrs);
+        if (err)
+        {
+            return err;
+        }
+
+        wchar_t *tmp_iid = _wcsdup(iid);
+        if (!tmp_iid || AddListItem(&(*lists)[undo_type], tmp_iid))
+        {
+            free(tmp_iid);
+            ResetNameServers(iid, msg->family);
+            return ERROR_OUTOFMEMORY;
         }
     }
 
@@ -1904,8 +1832,6 @@ 
     }
     ApplyDnsSettings(gpol);
 
-out:
-    free(wide_name);
     return err;
 }
 
@@ -2286,11 +2212,11 @@ 
                     break;
 
                 case undo_dns4:
-                    DeleteDNS(AF_INET, item->data);
+                    ResetNameServers(item->data, AF_INET);
                     break;
 
                 case undo_dns6:
-                    DeleteDNS(AF_INET6, item->data);
+                    ResetNameServers(item->data, AF_INET6);
                     break;
 
                     break;