ifconfig-ipv6(-push): allow using hostnames

Similarly to ifconfig(-push), its IPv6 counterpart is now able to accept hostnames as well instead of IP addresses in numeric form.

Basically this means that the user is now allowed to specify something like this:

ifconfig-ipv6-push my.hostname.cx/64

This is exactly the same behaviour that we already have with ifconfig(-push).

The generic code introduced in this patch will be later used to implement the /bits parsing support for IPv4 addresses.

Trac: #808

Signed-off-by Antonio Quartulli <[email protected]...>

---

v2:
- rebased on top of master
- style adapted to new CodingStyle

 src/openvpn/options.c |  61 ------------------------
 src/openvpn/options.h |   4 --
 src/openvpn/socket.c  | 126 ++++++++++++++++++++++++++++++
+++++++++++++++-----
 src/openvpn/socket.h  |  12 +++++
 4 files changed, 126 insertions(+), 77 deletions(-)

src/openvpn/options.c +0 -30

index 8e5cdf7f..767cdaeb 100644
@@ -1033,67 +1033,6 @@ get_ip_addr(const char *ip_string, int msglevel,
 bool *error)
     return ret;
 }
 
-/* helper: parse a text string containing an IPv6 address + netbits
- * in "standard format" (2001:dba::/32)
- * "/nn" is optional, default to /64 if missing
- *
- * return true if parsing succeeded, modify *network and *netbits
- */
-bool
-get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
-               unsigned int *netbits, int msglevel)
-{
-    char *sep, *endp;
-    int bits;
-    struct in6_addr t_network;
-
-    sep = strchr( prefix_str, '/' );
-    if (sep == NULL)
-    {
-        bits = 64;
-    }
-    else
-    {
-        bits = strtol( sep+1, &endp, 10 );
-        if (*endp != '\0' || bits < 0 || bits > 128)
-        {
-            msg(msglevel, "IPv6 prefix '%s': invalid '/bits' spec",
 prefix_str);
-            return false;
-        }
-    }
-
-    /* temporary replace '/' in caller-provided string with '\0',

otherwise

src/openvpn/options.c +0 -12

-     * inet_pton() will refuse prefix string
-     * (alternative would be to strncpy() the prefix to temporary buffer)
-     */
-
-    if (sep != NULL)
-    {
-        *sep = '\0';
-    }
-
-    if (inet_pton( AF_INET6, prefix_str, &t_network ) != 1)
-    {
-        msg(msglevel, "IPv6 prefix '%s': invalid IPv6 address",

prefix_str);

src/openvpn/options.c +0 -19

-        return false;
-    }
-
-    if (sep != NULL)
-    {
-        *sep = '/';
-    }
-
-    if (netbits != NULL)
-    {
-        *netbits = bits;
-    }
-    if (network != NULL)
-    {
-        *network = t_network;
-    }
-    return true;                /* parsing OK, values set */
-}
-
 /**
  * Returns newly allocated string containing address part without "/nn".
  *

src/openvpn/options.h +0 -4

index 035c6d15..d67c2785 100644
@@ -817,8 +817,4 @@ void options_string_import(struct options *options,
                            unsigned int *option_types_found,
                            struct env_set *es);
 
-bool get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
-                    unsigned int *netbits, int msglevel );
-
-
 #endif /* ifndef OPTIONS_H */

src/openvpn/socket.c +29 -0

index 0fc91f21..4cadae23 100644
@@ -74,12 +74,102 @@ sf2gaf(const unsigned int getaddr_flags,
 /*
  * Functions related to the translation of DNS names to IP addresses.
  */
+static int
+get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname,
+                 void *network, unsigned int *netbits,
+                 int resolve_retry_seconds, volatile int *signal_received,
+                 int msglevel)
+{
+    char *endp, *sep, *var_host;
+    uint8_t bits, max_bits;
+    struct addrinfo *ai;
+    int ret = -1;
+
+    ASSERT(hostname);
+
+    /* assign family specific default values */
+    switch (af)
+    {
+        case AF_INET:
+            bits = 0;
+            max_bits = sizeof(in_addr_t) * 8;
+            break;
+        case AF_INET6:
+            bits = 64;
+            max_bits = sizeof(struct in6_addr) * 8;
+            break;
+        default:
+            ASSERT(0);
+    }
+
+    /* we need to modify the hostname received as input, but we don't

want to

src/openvpn/socket.c +5 -0

+        *sep = '\0';
+    }
+
+    ret = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, var_host, NULL,
+                              resolve_retry_seconds, signal_received, af,

&ai);

src/openvpn/socket.c +10 -0

+    if ((ret == 0) && network)
+    {
+        struct in6_addr *ip6;
+        in_addr_t *ip4;
+
+        switch (af)
+        {
+            case AF_INET:
+                ip4 = network;
+                *ip4 = ((struct sockaddr_in *)ai->ai_addr)->sin_addr.s_

addr;

src/openvpn/socket.c +16 -0

+
+                if (flags & GETADDR_HOST_ORDER)
+                {
+                    *ip4 = ntohl(*ip4);
+                }
+                break;
+            case AF_INET6:
+                ip6 = network;
+                *ip6 = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
+                break;
+            default:
+                ASSERT(0);
+        }
+        freeaddrinfo(ai);
+    }
+

+ * Translate an IPv4 addr or hostname from string form to in_addr_t
+ *
+ * In case of resolve error, it will try again for
+ * resolve_retry_seconds seconds.
+ */

const char *hostname, int resolve_retry_seconds, bool *succeeded, volatile int *signal_received);

Hi,

Quoted 3 lines · Antonio Quartulli

Similarly to ifconfig(-push), its IPv6 counterpart is now able to accept hostnames as well instead of IP addresses in numeric form.

If dns names currently work for ifconfig-push (I didn't know), makes sense to
support it for ipv6 as well.

But getaddrinfo can take a long time to timeout, so we are adding another potentially blocking call on the server. Should not be an issue on a well administered server, but just saying...

Otherwise looks good except for a couple of issues as below:

Quoted 154 lines

Basically this means that the user is now allowed to specify something like this:

ifconfig-ipv6-push my.hostname.cx/64

This is exactly the same behaviour that we already have with ifconfig(-push).

The generic code introduced in this patch will be later used to implement the /bits parsing support for IPv4 addresses.

Trac: #808

Signed-off-by Antonio Quartulli <[email protected]...>

---

v2:
- rebased on top of master
- style adapted to new CodingStyle

 src/openvpn/options.c |  61 ------------------------
 src/openvpn/options.h |   4 --
 src/openvpn/socket.c  | 126 ++++++++++++++++++++++++++++++
+++++++++++++++-----
 src/openvpn/socket.h  |  12 +++++
 4 files changed, 126 insertions(+), 77 deletions(-)

src/openvpn/options.c +0 -30

index 8e5cdf7f..767cdaeb 100644
@@ -1033,67 +1033,6 @@ get_ip_addr(const char *ip_string, int msglevel,
 bool *error)
     return ret;
 }
 
-/* helper: parse a text string containing an IPv6 address + netbits
- * in "standard format" (2001:dba::/32)
- * "/nn" is optional, default to /64 if missing
- *
- * return true if parsing succeeded, modify *network and *netbits
- */
-bool
-get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
-               unsigned int *netbits, int msglevel)
-{
-    char *sep, *endp;
-    int bits;
-    struct in6_addr t_network;
-
-    sep = strchr( prefix_str, '/' );
-    if (sep == NULL)
-    {
-        bits = 64;
-    }
-    else
-    {
-        bits = strtol( sep+1, &endp, 10 );
-        if (*endp != '\0' || bits < 0 || bits > 128)
-        {
-            msg(msglevel, "IPv6 prefix '%s': invalid '/bits' spec",
 prefix_str);
-            return false;
-        }
-    }
-
-    /* temporary replace '/' in caller-provided string with '\0',

otherwise

src/openvpn/options.c +0 -12

-     * inet_pton() will refuse prefix string
-     * (alternative would be to strncpy() the prefix to temporary buffer)
-     */
-
-    if (sep != NULL)
-    {
-        *sep = '\0';
-    }
-
-    if (inet_pton( AF_INET6, prefix_str, &t_network ) != 1)
-    {
-        msg(msglevel, "IPv6 prefix '%s': invalid IPv6 address",

prefix_str);

src/openvpn/options.c +0 -19

-        return false;
-    }
-
-    if (sep != NULL)
-    {
-        *sep = '/';
-    }
-
-    if (netbits != NULL)
-    {
-        *netbits = bits;
-    }
-    if (network != NULL)
-    {
-        *network = t_network;
-    }
-    return true;                /* parsing OK, values set */
-}
-
 /**
  * Returns newly allocated string containing address part without "/nn".
  *

src/openvpn/options.h +0 -4

index 035c6d15..d67c2785 100644
@@ -817,8 +817,4 @@ void options_string_import(struct options *options,
                            unsigned int *option_types_found,
                            struct env_set *es);
 
-bool get_ipv6_addr( const char *prefix_str, struct in6_addr *network,
-                    unsigned int *netbits, int msglevel );
-
-
 #endif /* ifndef OPTIONS_H */

src/openvpn/socket.c +29 -0

index 0fc91f21..4cadae23 100644
@@ -74,12 +74,102 @@ sf2gaf(const unsigned int getaddr_flags,
 /*
  * Functions related to the translation of DNS names to IP addresses.
  */
+static int
+get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname,
+                 void *network, unsigned int *netbits,
+                 int resolve_retry_seconds, volatile int *signal_received,
+                 int msglevel)
+{
+    char *endp, *sep, *var_host;
+    uint8_t bits, max_bits;
+    struct addrinfo *ai;
+    int ret = -1;
+
+    ASSERT(hostname);
+
+    /* assign family specific default values */
+    switch (af)
+    {
+        case AF_INET:
+            bits = 0;
+            max_bits = sizeof(in_addr_t) * 8;
+            break;
+        case AF_INET6:
+            bits = 64;
+            max_bits = sizeof(struct in6_addr) * 8;
+            break;
+        default:
+            ASSERT(0);
+    }
+
+    /* we need to modify the hostname received as input, but we don't

want to

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 111

Quoted 6 lines

src/openvpn/socket.c +6 -0

+     * touch it directly as it might be a constant string.
+     *
+     * Therefore, we clone the string here and free it at the end of the
+     * function */
+    var_host = strdup(hostname);
+    ASSERT(var_host);

I think ASSERT should be used only to catch errors in coding logic, not for plausible runtime errors like this. Especially since this happens on the server, no reason to terminate the process here.

Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1 or goto out. The option parser will log a generic warning, but still useful to log here using M_ERRNO for more specific info.

Alternatively one could use a buffer on the stack -- easy to do as good old dns names are limited in size (255 octets max?) and the current option parser also limits the argument passed here to < 255 bytes. But if we support
internationalized domain names (currently we do not, do we?) and if the line length in option parser is ever increased, a much larger buffer would be needed.

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 117

Quoted 8 lines

src/openvpn/socket.c +6 -0

+
+    /* check if this hostname has a /bits suffix */
+    sep = strchr(var_host , '/');
+    if (sep)
+    {
+        bits = strtoul(sep + 1, &endp, 10);

There are a number of such type coercions in the patch
(ulong to uint8_t, size_t to unit8_t etc.) that some compilers (aka MSVC:) may warn about, but I do not personally care. All are safe and deliberate except for the nitpicking below.

+ if ((*endp != '\0') || (bits > max_bits))

That (bits > max_bits) check will not catch many input errors, as the input is already truncated to uint8_t. For example, /255 will be flagged as an error, but /256 will pass as 0.

Though its not possible to catch all input errors, keeping bits as unsigned int (instead of uint8_t) may be better here. That'll also match netbits in the function signature, return value of strtoul etc..

+ {

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 125

Quoted 8 lines

+ msg(msglevel, "IP prefix '%s': invalid '/bits' spec",
hostname);

src/openvpn/socket.c +5 -0

+            goto out;
+        }
+        /* temporary truncate string at '/'. This allows the IP
+         * parsing routines to properly work. Will be restored later.
+         */

This comment is no longer required as there is no need to restore '/' (also see below)

Quoted 39 lines

src/openvpn/socket.c +5 -0

+        *sep = '\0';
+    }
+
+    ret = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, var_host, NULL,
+                              resolve_retry_seconds, signal_received, af,

&ai);

src/openvpn/socket.c +10 -0

+    if ((ret == 0) && network)
+    {
+        struct in6_addr *ip6;
+        in_addr_t *ip4;
+
+        switch (af)
+        {
+            case AF_INET:
+                ip4 = network;
+                *ip4 = ((struct sockaddr_in *)ai->ai_addr)->sin_addr.s_

addr;

src/openvpn/socket.c +22 -0

+
+                if (flags & GETADDR_HOST_ORDER)
+                {
+                    *ip4 = ntohl(*ip4);
+                }
+                break;
+            case AF_INET6:
+                ip6 = network;
+                *ip6 = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;
+                break;
+            default:
+                ASSERT(0);
+        }
+        freeaddrinfo(ai);
+    }
+
+    if (netbits)
+    {
+        *netbits = bits;
+    }
+
+    /* restore '/' separator, if any */

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 166

Quoted 5 lines

src/openvpn/socket.c +4 -0

+    if (sep)
+    {
+        *sep = '/';
+    }

This restore is redundant as we are now working on a copy which is going to be freed two lines below.

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 171

Quoted 8 lines

src/openvpn/socket.c +5 -0

+out:
+    free(var_host);
+
+    return ret;
+}

...

+/**

Quoted 6 lines

+ * Translate an IPv4 addr or hostname from string form to in_addr_t
+ *
+ * In case of resolve error, it will try again for
+ * resolve_retry_seconds seconds.
+ */

More docs is nice, but this is confusing. It will retry for (resolve_retry_seconds / 5)
times will be more correct. Its as if someone thought if the caller says try for 15
seconds its ok to try three times and wait 5 seconds in between. In reality that
could take 90 seconds or even more in some cases as getaddrinfo may not return
for 30 seconds or more. But that's a separate issue.

And, it will try only once if flags contain GETADDR_TRY_ONCE

in_addr_t getaddr(unsigned int flags,

Quoted 4 lines

const char *hostname, int resolve_retry_seconds, bool *succeeded, volatile int *signal_received);

The rest looks good though I've not tested it.

Hi,

Quoted 5 lines · Selva Nair

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags,

Quoted 5 lines

+ /* we need to modify the hostname received as input, but we don't
want to

src/openvpn/socket.c +3 -0

+     * touch it directly as it might be a constant string.
+     *
+     * Therefore, we clone the string here and free it at the end of

the

Quoted 11 lines

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 111

Quoted 4 lines

src/openvpn/socket.c +3 -0

+     * function */
+    var_host = strdup(hostname);
+    ASSERT(var_host);

I think ASSERT should be used only to catch errors in coding logic, not for plausible runtime errors like this. Especially since this happens on the server, no reason to terminate the process here.

Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1 or goto out. The option parser will log a generic warning, but still

useful

Quoted 2 lines

to log here using M_ERRNO for more specific info.

I agree. I am also not a big fan of ASSERT(). I think I had copy/pasted this chunk of code from somewhere else (and consider that this patch is quite old even though I have re-sent it now).

I will get rid of the ASSERTs.

Quoted 2 lines

Alternatively one could use a buffer on the stack -- easy to do as good old dns names are limited in size (255 octets max?) and the current

option

Quoted 3 lines

parser also limits the argument passed here to < 255 bytes. But if we support
internationalized domain names (currently we do not, do we?) and if the

line

Quoted 2 lines

length in option parser is ever increased, a much larger buffer would be needed.

Are you sure we have a limit of 255 octects? I am not so sure. Anyway, this is not extremely important for now.

Quoted 10 lines

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 117

Quoted 8 lines

src/openvpn/socket.c +6 -0

+
+    /* check if this hostname has a /bits suffix */
+    sep = strchr(var_host , '/');
+    if (sep)
+    {
+        bits = strtoul(sep + 1, &endp, 10);

Quoted 2 lines

There are a number of such type coercions in the patch (ulong to uint8_t, size_t to unit8_t etc.) that some compilers (aka

MSVC:)

Quoted 10 lines

may warn about, but I do not personally care. All are safe and deliberate except for the nitpicking below.

+ if ((*endp != '\0') || (bits > max_bits))

That (bits > max_bits) check will not catch many input errors, as the input is already truncated to uint8_t. For example, /255 will be flagged as an error, but /256 will pass as 0.

These are probably all copy/paste from other parts of the code. So the logic here is also used in other parts of the code. If we believe this is not the proper way to handle these cases, I'd suggest to take a note and fix these behaviors with a dedicated patch.

Though its not possible to catch all input errors, keeping bits as unsigned

Quoted 2 lines

int (instead of uint8_t) may be better here. That'll also match netbits in the function signature, return value of strtoul etc..

oops forgot to cc the list..

---------- Forwarded message ----------
From: Selva Nair <[email protected]...>
Date: Sat, Dec 2, 2017 at 10:16 PM
Subject: Re: [Openvpn-devel] [PATCH v2] ifconfig-ipv6(-push): allow using hostnames
To: Antonio Quartulli <[email protected]...>

Hi,

Quoted 37 lines · Antonio Quartulli

Hi,

Quoted 5 lines · Selva Nair

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags,

Quoted 5 lines

+ /* we need to modify the hostname received as input, but we don't
want to

src/openvpn/socket.c +3 -0

+     * touch it directly as it might be a constant string.
+     *
+     * Therefore, we clone the string here and free it at the end of

the

Quoted 11 lines

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 111

Quoted 4 lines

src/openvpn/socket.c +3 -0

+     * function */
+    var_host = strdup(hostname);
+    ASSERT(var_host);

I think ASSERT should be used only to catch errors in coding logic, not for plausible runtime errors like this. Especially since this happens on the server, no reason to terminate the process here.

Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1 or goto out. The option parser will log a generic warning, but still

useful

Quoted 2 lines

to log here using M_ERRNO for more specific info.

I agree. I am also not a big fan of ASSERT(). I think I had copy/pasted this chunk of code from somewhere else (and consider that this patch is quite old even though I have re-sent it now).

I will get rid of the ASSERTs.

Quoted 2 lines

Alternatively one could use a buffer on the stack -- easy to do as good old dns names are limited in size (255 octets max?) and the current

option

Quoted 3 lines

parser also limits the argument passed here to < 255 bytes. But if we support
internationalized domain names (currently we do not, do we?) and if the

Quoted 7 lines

line

Quoted 2 lines

length in option parser is ever increased, a much larger buffer would be needed.

Are you sure we have a limit of 255 octects? I am not so sure. Anyway, this is not extremely important for now.

Yes, not important for this patch and stdup is anyway fine as long as we do not
ASSERT.

The 255 byte limit comes from the config file or option parser which reads the
input from ccd or client-connect script output. The length of a line is limited
to #define OPTION_LINE_SIZE 256 in options.h

Quoted 26 lines

Quoted 12 lines

src/openvpn/socket.c sf2gaf(const unsigned int getaddr_flags, · line 117

Quoted 10 lines

src/openvpn/socket.c +6 -0

+
+    /* check if this hostname has a /bits suffix */
+    sep = strchr(var_host , '/');
+    if (sep)
+    {
+        bits = strtoul(sep + 1, &endp, 10);

Quoted 2 lines

There are a number of such type coercions in the patch (ulong to uint8_t, size_t to unit8_t etc.) that some compilers (aka

MSVC:)

Quoted 10 lines

may warn about, but I do not personally care. All are safe and deliberate except for the nitpicking below.

+ if ((*endp != '\0') || (bits > max_bits))

That (bits > max_bits) check will not catch many input errors, as the input is already truncated to uint8_t. For example, /255 will be flagged as an error, but /256 will pass as 0.

Quoted 5 lines

These are probably all copy/paste from other parts of the code. So the logic here is also used in other parts of the code. If we believe this is not the proper way to handle these cases, I'd suggest to take a note and fix these behaviors with a dedicated patch.

Please bear with me: Quoting from the patch:

src/openvpn/options.c +0 -3

-    char *sep, *endp;
-    int bits;
-    struct in6_addr t_network;

So, the original code had bits defined as int, so why change to unit8_t? Changing the original int to unsigned int would've made sense
as we are parsing using strtoul and returning the result in an unsigned int. But uint8_t brings in hardly any gain and causes all those regressions and type conversions I pointed out..

Hi,

Responding to this old version just to be on record.

I realized patch this was assigned to Gert on patchwork too late after started responding on my own. Sorry for jumping the gun. Have to make keeping an eye on patchwork a habit..

I'll leave the latest v4 alone.

Responding to this old version just to be on record.

I realized patch this was assigned to Gert on patchwork too late after started responding on my own. Sorry for jumping the gun. Have to make keeping an eye on patchwork a habit..

I'll leave the latest v4 alone.

Hi,,

Quoted 7 lines · Selva Nair

Responding to this old version just to be on record.

I realized patch this was assigned to Gert on patchwork too late after started responding on my own. Sorry for jumping the gun. Have to make keeping an eye on patchwork a habit..

I'll leave the latest v4 alone.

we should stop doing so (Antonio, are you listening? :-) ).

Obviously
the patch was in dire need of a good review, and if you have time while I am busy with family things, this is more than welcome.

Voluntarily *taking* a patch for review is useful as a flag "I'm going to handle this", but third-party-tagging "this is for Gert!" seems to have potential for backfiring, so maybe we shouldn't do that, then.