[Openvpn-devel,v2] wintun: set adapter properties via interactive service

Message ID 20191218061818.1504-1-simon@rozman.si
State Accepted
Headers show
Series [Openvpn-devel,v2] wintun: set adapter properties via interactive service | expand

Commit Message

Simon Rozman Dec. 17, 2019, 7:18 p.m. UTC
From: Lev Stipakov <lev@openvpn.net>

Since Wintun doesn't do DHCP, use interactive service
calls to set up adapter properties.

This also fixes bug in previously unused IPv4 code of
do_address_service():

 - ipv4 address must be in network byte order
 - prefix length cannot be hardcoded /32 but
 must be calculated from netmask

Signed-off-by: Lev Stipakov <lev@openvpn.net>
Acked-by: Simon Rozman <simon@rozman.si>
---
 src/openvpn/route.c |  2 +-
 src/openvpn/route.h |  3 +-
 src/openvpn/tun.c   | 75 +++++++++++++++++++++++++++++++++++----------
 3 files changed, 61 insertions(+), 19 deletions(-)

Comments

Gert Doering Dec. 18, 2019, 8:50 a.m. UTC | #1
Your patch has been applied to the master branch.

(This is a bit of a weird one, as the "v3" of 6/7 was sent by Simon,
fixing the strcpy() part, but having "From: Lev" in it... I cannot
technically accept the acked-by: in such a patch because it would be
from the same person that send the new version.  *But* I verified that
it's really the same patch minus the strcpy() part, and "Lev as author,
Simon as ACKer" is fine under these circumstances)


Code-wise I'm not exactly happy with "do_route_ipv4_service_tun()" 
- we do have a function called add_route_connected_v6_net(), so if 
we need the same functionality for IPv4, it should be called 
"add_route_connected_v4_net()" or whatever, not "something completely
different"...  also I don't think it should hook right into 
do_route_ipv4_service(), which is (was...) an *internal* function in 
route.c for a reason - the well-defined API between tun.c and route.c 
is "add_route()".

Finally the patch breaks indentation rules, so might have been 
coded in a nicer way *or* fix the indent right away - now we have 
another uncrustify patch coming which has to changes code without 
it being what the underlying change was.


Test built on Ubuntu 1604/mingw, not tested further.


commit 1c4a47f796639da45ec7c9bf5f38a9f0bb6aa9b2
Author: Lev Stipakov
Date:   Wed Dec 18 07:18:18 2019 +0100

     wintun: set adapter properties via interactive service

     Signed-off-by: Lev Stipakov <lev@openvpn.net>
     Acked-by: Simon Rozman <simon@rozman.si>
     Message-Id: <20191218061818.1504-1-simon@rozman.si>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg19253.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>

--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/route.c b/src/openvpn/route.c
index 97e90e56..cc6d5519 100644
--- a/src/openvpn/route.c
+++ b/src/openvpn/route.c
@@ -3019,7 +3019,7 @@  out:
     return ret;
 }
 
-static bool
+bool
 do_route_ipv4_service(const bool add, const struct route_ipv4 *r, const struct tuntap *tt)
 {
     DWORD if_index = windows_route_find_if_index(r, tt);
diff --git a/src/openvpn/route.h b/src/openvpn/route.h
index 2e68091c..27b652cd 100644
--- a/src/openvpn/route.h
+++ b/src/openvpn/route.h
@@ -321,7 +321,8 @@  void setenv_routes(struct env_set *es, const struct route_list *rl);
 
 void setenv_routes_ipv6(struct env_set *es, const struct route_ipv6_list *rl6);
 
-
+bool do_route_ipv4_service(const bool add, const struct route_ipv4 *r,
+                           const struct tuntap *tt);
 
 bool is_special_addr(const char *addr_str);
 
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 77d84fb2..32f4f483 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -109,8 +109,8 @@  do_address_service(const bool add, const short family, const struct tuntap *tt)
 
     if (addr.family == AF_INET)
     {
-        addr.address.ipv4.s_addr = tt->local;
-        addr.prefix_len = 32;
+        addr.address.ipv4.s_addr = htonl(tt->local);
+        addr.prefix_len = netmask_to_netbits2(tt->adapter_netmask);
     }
     else
     {
@@ -139,13 +139,15 @@  out:
 }
 
 static bool
-do_dns6_service(bool add, const struct tuntap *tt)
+do_dns_service(bool add, const short family, const struct tuntap *tt)
 {
     bool ret = false;
     ack_message_t ack;
     struct gc_arena gc = gc_new();
     HANDLE pipe = tt->options.msg_channel;
-    int addr_len = add ? tt->options.dns6_len : 0;
+    int len = family == AF_INET6 ? tt->options.dns6_len : tt->options.dns_len;
+    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 */
     {
@@ -160,7 +162,7 @@  do_dns6_service(bool add, const struct tuntap *tt)
         },
         .iface = { .index = tt->adapter_index, .name = "" },
         .domains = "",
-        .family = AF_INET6,
+        .family = family,
         .addr_len = addr_len
     };
 
@@ -172,17 +174,24 @@  do_dns6_service(bool add, const struct tuntap *tt)
     {
         addr_len = _countof(dns.addr);
         dns.addr_len = addr_len;
-        msg(M_WARN, "Number of IPv6 DNS addresses sent to service truncated to %d",
-            addr_len);
+        msg(M_WARN, "Number of %s DNS addresses sent to service truncated to %d",
+            ip_proto_name, addr_len);
     }
 
     for (int i = 0; i < addr_len; ++i)
     {
-        dns.addr[i].ipv6 = tt->options.dns6[i];
+        if (family == AF_INET6)
+        {
+            dns.addr[i].ipv6 = tt->options.dns6[i];
+        }
+        else
+        {
+            dns.addr[i].ipv4.s_addr = htonl(tt->options.dns[i]);
+        }
     }
 
-    msg(D_LOW, "%s IPv6 dns servers 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 (!send_msg_iservice(pipe, &dns, sizeof(dns), &ack, "TUN"))
     {
@@ -191,13 +200,13 @@  do_dns6_service(bool add, const struct tuntap *tt)
 
     if (ack.error_number != NO_ERROR)
     {
-        msg(M_WARN, "TUN: %s IPv6 dns failed using service: %s [status=%u if_name=%s]",
-            (add ? "adding" : "deleting"), strerror_win32(ack.error_number, &gc),
+        msg(M_WARN, "TUN: %s %s dns failed using service: %s [status=%u if_name=%s]",
+            (add ? "adding" : "deleting"), ip_proto_name, strerror_win32(ack.error_number, &gc),
             ack.error_number, dns.iface.name);
         goto out;
     }
 
-    msg(M_INFO, "IPv6 dns servers %s using service", (add ? "set" : "deleted"));
+    msg(M_INFO, "%s dns servers %s using service", ip_proto_name, (add ? "set" : "deleted"));
     ret = true;
 
 out:
@@ -819,7 +828,7 @@  init_tun_post(struct tuntap *tt,
  * an extra call to "route add..."
  * -> helper function to simplify code below
  */
-void
+static void
 add_route_connected_v6_net(struct tuntap *tt,
                            const struct env_set *es)
 {
@@ -851,6 +860,21 @@  delete_route_connected_v6_net(struct tuntap *tt,
 }
 #endif /* if defined(_WIN32) || defined(TARGET_DARWIN) || defined(TARGET_NETBSD) || defined(TARGET_OPENBSD) */
 
+#if defined(_WIN32)
+void
+do_route_ipv4_service_tun(bool add, const struct tuntap *tt)
+{
+    struct route_ipv4 r4;
+    CLEAR(r4);
+    r4.network = tt->local & tt->remote_netmask;
+    r4.netmask = tt->remote_netmask;
+    r4.gateway = tt->local;
+    r4.metric = 0;                     /* connected route */
+    r4.flags = RT_DEFINED | RT_METRIC_DEFINED;
+    do_route_ipv4_service(add, &r4, tt);
+}
+#endif
+
 #if defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)  \
     || defined(TARGET_NETBSD) || defined(TARGET_OPENBSD)
 /* we can't use true subnet mode on tun on all platforms, as that
@@ -1007,7 +1031,7 @@  do_ifconfig_ipv6(struct tuntap *tt, const char *ifname, int tun_mtu,
     else if (tt->options.msg_channel)
     {
         do_address_service(true, AF_INET6, tt);
-        do_dns6_service(true, tt);
+        do_dns_service(true, AF_INET6, tt);
     }
     else
     {
@@ -1379,8 +1403,16 @@  do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,
     {
         ASSERT(ifname != NULL);
 
-        switch (tt->options.ip_win32_type)
+        if (tt->options.msg_channel && tt->wintun)
+        {
+            do_address_service(true, AF_INET, tt);
+            do_route_ipv4_service_tun(true, tt);
+            do_dns_service(true, AF_INET, tt);
+        }
+        else
         {
+            switch (tt->options.ip_win32_type)
+            {
             case IPW32_SET_MANUAL:
                 msg(M_INFO,
                     "******** NOTE:  Please manually set the IP/netmask of '%s' to %s/%s (if it is not already set)",
@@ -1393,6 +1425,7 @@  do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,
                                tt->adapter_netmask, NI_IP_NETMASK|NI_OPTIONS);
 
                 break;
+            }
         }
     }
 
@@ -6119,6 +6152,7 @@  open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
     }
 
     /* possibly use IP Helper API to set IP address on adapter */
+    if (!tt->wintun)
     {
         const DWORD index = tt->adapter_index;
 
@@ -6341,7 +6375,7 @@  close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
             do_address_service(false, AF_INET6, tt);
             if (tt->options.dns6_len > 0)
             {
-                do_dns6_service(false, tt);
+                do_dns_service(false, AF_INET6, tt);
             }
         }
         else
@@ -6378,6 +6412,13 @@  close_tun(struct tuntap *tt, openvpn_net_ctx_t *ctx)
         }
     }
 #if 1
+    if (tt->wintun && tt->options.msg_channel)
+    {
+        do_route_ipv4_service_tun(false, tt);
+        do_address_service(false, AF_INET, tt);
+        do_dns_service(false, AF_INET, tt);
+    }
+    else
     if (tt->ipapi_context_defined)
     {
         DWORD status;