Hi,
On Sat, Dec 2, 2017 at 3:54 AM, Antonio Quartulli <a...@unstable.cc> wrote:
> 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:
> 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 <a...@unstable.cc>
> ---
>
> 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(-)
>
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 8e5cdf7f..767cdaeb 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -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
> - * 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);
> - 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".
> *
> diff --git a/src/openvpn/options.h b/src/openvpn/options.h
> index 035c6d15..d67c2785 100644
> --- a/src/openvpn/options.h
> +++ b/src/openvpn/options.h
> @@ -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 */
> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
> index 0fc91f21..4cadae23 100644
> --- a/src/openvpn/socket.c
> +++ b/src/openvpn/socket.c
> @@ -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
> + * 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.
> +
> + /* 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..
+ {
> + msg(msglevel, "IP prefix '%s': invalid '/bits' spec",
> hostname);
> + 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)
> + *sep = '\0';
> + }
> +
> + ret = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, var_host, NULL,
> + resolve_retry_seconds, signal_received, af,
> &ai);
> + 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;
> +
> + 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 */
> + if (sep)
> + {
> + *sep = '/';
> + }
>
This restore is redundant as we are now working on a copy which
is going to be freed two lines below.
> +out:
> + free(var_host);
> +
> + return ret;
> +}
>
...
+/**
> + * 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,
> const char *hostname,
> int resolve_retry_seconds,
> bool *succeeded,
> volatile int *signal_received);
The rest looks good though I've not tested it.
Selva
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
