[Openvpn-devel,v9] override ai_family if 'local' numeric address was specified

Message ID 20241227161755.4010-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v9] override ai_family if 'local' numeric address was specified | expand

Commit Message

Gert Doering Dec. 27, 2024, 4:17 p.m. UTC
From: Antonio Quartulli <a@unstable.cc>

This change ensures that when a numeric IP address is specified
as argument to a 'local' directive, its ai_family overrides
the one extracted from the 'proto' config option.

Change-Id: Ie2471e6b2d6974e70423b09918ad1c2136253754
Signed-off-by: Antonio Quartulli <a@unstable.cc>
Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
Acked-by: Gert Doering <gert@greenie.muc.de>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/762
This mail reflects revision 9 of this Change.

Acked-by according to Gerrit (reflected above):
Gert Doering <gert@greenie.muc.de>

Comments

Gert Doering Dec. 27, 2024, 5:37 p.m. UTC | #1
This does change semantics of "local <address>" or "remote <address>",
but in a useful way.  The old code would just fail on

  proto udp4
  remote 2001:db8::1

because it tried to force-resolve the v6 address as "proto v4" (and vice
versa):

2024-12-24 13:24:18 RESOLVE: Cannot resolve host address: 2001:db8::1:51194 (Name does not resolve)
2024-12-24 13:25:01 RESOLVE: Cannot resolve host address: 199.102.77.82:51194 (Name does not resolve)

The new code will basically change "proto udp4" to "udp6" if a numeric v6
address is seen (and vice versa).

This has a small risk for unforeseen breakage (configs that have v4+v6
connection entries in them, and purposely selecting one or the other 
using "proto udp4/udp6"), but I'm willing to accept that - if we really
need that use case (instead of just letting the transport layer decide if
v4 or v6 succeeds) we might enhance `--proto-force` to also serve as a
v4/v6 switch.  Or introduce -4/-6.

Tested client-side with the "mismatched" combinations, and subjected to
the full client/server side tests - which uncovered a unforeseen surprise
in v7 (instead of a dual-stack socket for "ANY", I only got v4 sockets)
- this has been addressed in v9, and my zoo behaves "as before".


Your patch has been applied to the master branch.

commit 8b209d9e5d80799eec931e2fec9b15f7e2c1a7b0
Author: Antonio Quartulli
Date:   Fri Dec 27 17:17:55 2024 +0100

     override ai_family if 'local' numeric address was specified

     Signed-off-by: Antonio Quartulli <a@unstable.cc>
     Signed-off-by: Gianmarco De Gregori <gianmarco@mandelbit.com>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20241227161755.4010-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg30257.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 7921433..b84521a 100644
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -488,9 +488,8 @@ 
         sig_info = &sigrec;
     }
 
-    /* try numeric ipv6 addr first */
+    /* try numeric ip addr first */
     CLEAR(hints);
-    hints.ai_family = ai_family;
     hints.ai_flags = AI_NUMERICHOST;
 
     if (flags & GETADDR_PASSIVE)
@@ -507,6 +506,13 @@ 
         hints.ai_socktype = SOCK_STREAM;
     }
 
+    /* if hostname is not set, we want to bind to 'ANY', with
+     * the correct address family - v4-only or v6/v6-dual-stack */
+    if (!hostname)
+    {
+        hints.ai_family = ai_family;
+    }
+
     status = getaddrinfo(hostname, servname, &hints, res);
 
     if (status != 0) /* parse as numeric address failed? */
@@ -518,6 +524,10 @@ 
         const char *fmt;
         int level = 0;
 
+        /* this is not a numeric IP, therefore force resolution using the
+         * provided ai_family */
+        hints.ai_family = ai_family;
+
         if (hostname && (flags & GETADDR_RANDOMIZE))
         {
             hostname = hostname_randomize(hostname, &gc);
@@ -1716,6 +1726,10 @@ 
                 sock->local_host, sock->local_port,
                 gai_strerror(status));
         }
+
+        /* the resolved 'local entry' might have a different family than what
+         * was globally configured */
+        sock->info.af = sock->info.lsa->bind_local->ai_family;
     }
 
     gc_free(&gc);