| Message ID | 20171202085438.2393-1-a@unstable.cc |
|---|---|
| State | Superseded |
| Delegated to: | Gert Doering |
| Headers | show |
| Series | [Openvpn-devel,v2] ifconfig-ipv6(-push): allow using hostnames | expand |
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 <div dir="ltr">Hi,<div><div class="gmail_extra"><br><div class="gmail_quote">On Sat, Dec 2, 2017 at 3:54 AM, Antonio Quartulli <span dir="ltr"><<a href="mailto:a@unstable.cc" target="_blank">a@unstable.cc</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Similarly to ifconfig(-push), its IPv6 counterpart is now able to<br> accept hostnames as well instead of IP addresses in numeric form.<br></blockquote><div><br></div><div>If dns names currently work for ifconfig-push (I didn't know), makes sense to</div><div>support it for ipv6 as well.</div><div><br></div><div>But getaddrinfo can take a long time to timeout, so we are adding another</div><div>potentially blocking call on the server. Should not be an issue on a well</div><div>administered server, but just saying...</div><div><br></div><div>Otherwise looks good except for a couple of issues as below:</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> <br> Basically this means that the user is now allowed to specify<br> something like this:<br> <br> ifconfig-ipv6-push <a href="http://my.hostname.cx/64" rel="noreferrer" target="_blank">my.hostname.cx/64</a><br> <br> This is exactly the same behaviour that we already have with<br> ifconfig(-push).<br> <br> The generic code introduced in this patch will be later used to<br> implement the /bits parsing support for IPv4 addresses.<br> <br> Trac: #808<br> Signed-off-by: Antonio Quartulli <a@unstable.cc><br> ---<br> <br> v2:<br> - rebased on top of master<br> - style adapted to new CodingStyle<br> <br> src/openvpn/options.c | 61 ------------------------<br> src/openvpn/options.h | 4 --<br> src/openvpn/socket.c | 126 ++++++++++++++++++++++++++++++<wbr>+++++++++++++++-----<br> src/openvpn/socket.h | 12 +++++<br> 4 files changed, 126 insertions(+), 77 deletions(-)<br> <br> diff --git a/src/openvpn/options.c b/src/openvpn/options.c<br> index 8e5cdf7f..767cdaeb 100644<br> --- a/src/openvpn/options.c<br> +++ b/src/openvpn/options.c<br> @@ -1033,67 +1033,6 @@ get_ip_addr(const char *ip_string, int msglevel, bool *error)<br> return ret;<br> }<br> <br> -/* helper: parse a text string containing an IPv6 address + netbits<br> - * in "standard format" (2001:dba::/32)<br> - * "/nn" is optional, default to /64 if missing<br> - *<br> - * return true if parsing succeeded, modify *network and *netbits<br> - */<br> -bool<br> -get_ipv6_addr( const char *prefix_str, struct in6_addr *network,<br> - unsigned int *netbits, int msglevel)<br> -{<br> - char *sep, *endp;<br> - int bits;<br> - struct in6_addr t_network;<br> -<br> - sep = strchr( prefix_str, '/' );<br> - if (sep == NULL)<br> - {<br> - bits = 64;<br> - }<br> - else<br> - {<br> - bits = strtol( sep+1, &endp, 10 );<br> - if (*endp != '\0' || bits < 0 || bits > 128)<br> - {<br> - msg(msglevel, "IPv6 prefix '%s': invalid '/bits' spec", prefix_str);<br> - return false;<br> - }<br> - }<br> -<br> - /* temporary replace '/' in caller-provided string with '\0', otherwise<br> - * inet_pton() will refuse prefix string<br> - * (alternative would be to strncpy() the prefix to temporary buffer)<br> - */<br> -<br> - if (sep != NULL)<br> - {<br> - *sep = '\0';<br> - }<br> -<br> - if (inet_pton( AF_INET6, prefix_str, &t_network ) != 1)<br> - {<br> - msg(msglevel, "IPv6 prefix '%s': invalid IPv6 address", prefix_str);<br> - return false;<br> - }<br> -<br> - if (sep != NULL)<br> - {<br> - *sep = '/';<br> - }<br> -<br> - if (netbits != NULL)<br> - {<br> - *netbits = bits;<br> - }<br> - if (network != NULL)<br> - {<br> - *network = t_network;<br> - }<br> - return true; /* parsing OK, values set */<br> -}<br> -<br> /**<br> * Returns newly allocated string containing address part without "/nn".<br> *<br> diff --git a/src/openvpn/options.h b/src/openvpn/options.h<br> index 035c6d15..d67c2785 100644<br> --- a/src/openvpn/options.h<br> +++ b/src/openvpn/options.h<br> @@ -817,8 +817,4 @@ void options_string_import(struct options *options,<br> unsigned int *option_types_found,<br> struct env_set *es);<br> <br> -bool get_ipv6_addr( const char *prefix_str, struct in6_addr *network,<br> - unsigned int *netbits, int msglevel );<br> -<br> -<br> #endif /* ifndef OPTIONS_H */<br> diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c<br> index 0fc91f21..4cadae23 100644<br> --- a/src/openvpn/socket.c<br> +++ b/src/openvpn/socket.c<br> @@ -74,12 +74,102 @@ sf2gaf(const unsigned int getaddr_flags,<br> /*<br> * Functions related to the translation of DNS names to IP addresses.<br> */<br> +static int<br> +get_addr_generic(sa_family_t af, unsigned int flags, const char *hostname,<br> + void *network, unsigned int *netbits,<br> + int resolve_retry_seconds, volatile int *signal_received,<br> + int msglevel)<br> +{<br> + char *endp, *sep, *var_host;<br> + uint8_t bits, max_bits;<br> + struct addrinfo *ai;<br> + int ret = -1;<br> +<br> + ASSERT(hostname);<br> +<br> + /* assign family specific default values */<br> + switch (af)<br> + {<br> + case AF_INET:<br> + bits = 0;<br> + max_bits = sizeof(in_addr_t) * 8;<br> + break;<br> + case AF_INET6:<br> + bits = 64;<br> + max_bits = sizeof(struct in6_addr) * 8;<br> + break;<br> + default:<br> + ASSERT(0);<br> + }<br> +<br> + /* we need to modify the hostname received as input, but we don't want to<br> + * touch it directly as it might be a constant string.<br> + *<br> + * Therefore, we clone the string here and free it at the end of the<br> + * function */<br> + var_host = strdup(hostname);<br> + ASSERT(var_host);<br></blockquote><div><br></div><div>I think ASSERT should be used only to catch errors in coding logic,</div><div>not for plausible runtime errors like this. Especially since this happens</div><div>on the server, no reason to terminate the process here.</div><div><br></div><div>Instead, log an error (M_NONFTAL|M_ERRNO will do nicely) and return -1</div><div>or goto out. The option parser will log a generic warning, but still useful </div><div>to log here using M_ERRNO for more specific info.</div><div><br></div><div>Alternatively one could use a buffer on the stack -- easy to do as good </div><div>old dns names are limited in size (255 octets max?) and the current option</div><div>parser also limits the argument passed here to < 255 bytes. But if we support</div><div>internationalized domain names (currently we do not, do we?) and if the line</div><div>length in option parser is ever increased, a much larger buffer would be needed.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> +<br> + /* check if this hostname has a /bits suffix */<br> + sep = strchr(var_host , '/');<br> + if (sep)<br> + {<br> + bits = strtoul(sep + 1, &endp, 10);</blockquote><div><br></div><div>There are a number of such type coercions in the patch </div><div>(ulong to uint8_t, size_t to unit8_t etc.) that some compilers (aka MSVC:)</div><div>may warn about, but I do not personally care. All are safe and deliberate</div><div>except for the nitpicking below.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + if ((*endp != '\0') || (bits > max_bits))</blockquote><div><br></div><div>That (bits > max_bits) check will not catch many input errors, as the</div><div>input is already truncated to uint8_t. For example, /255 will be flagged</div><div>as an error, but /256 will pass as 0.</div><div><br></div><div>Though its not possible to catch all input errors, keeping bits as unsigned</div><div>int (instead of uint8_t) may be better here. That'll also match netbits</div><div>in the function signature, return value of strtoul etc..</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + {<br> + msg(msglevel, "IP prefix '%s': invalid '/bits' spec", hostname);<br> + goto out;<br> + }<br> + /* temporary truncate string at '/'. This allows the IP<br> + * parsing routines to properly work. Will be restored later.<br> + */<br></blockquote><div><br></div><div>This comment is no longer required as there is no need to restore '/'</div><div>(also see below)</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> + *sep = '\0';<br> + }<br> +<br> + ret = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, var_host, NULL,<br> + resolve_retry_seconds, signal_received, af, &ai);<br> + if ((ret == 0) && network)<br> + {<br> + struct in6_addr *ip6;<br> + in_addr_t *ip4;<br> +<br> + switch (af)<br> + {<br> + case AF_INET:<br> + ip4 = network;<br> + *ip4 = ((struct sockaddr_in *)ai->ai_addr)->sin_addr.s_<wbr>addr;<br> +<br> + if (flags & GETADDR_HOST_ORDER)<br> + {<br> + *ip4 = ntohl(*ip4);<br> + }<br> + break;<br> + case AF_INET6:<br> + ip6 = network;<br> + *ip6 = ((struct sockaddr_in6 *)ai->ai_addr)->sin6_addr;<br> + break;<br> + default:<br> + ASSERT(0);<br> + }<br> + freeaddrinfo(ai);<br> + }<br> +<br> + if (netbits)<br> + {<br> + *netbits = bits;<br> + }<br> +<br> + /* restore '/' separator, if any */<br> + if (sep)<br> + {<br> + *sep = '/';<br> + }<br> </blockquote><div><br></div><div>This restore is redundant as we are now working on a copy which</div><div>is going to be freed two lines below.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+out:<br> + free(var_host);<br> +<br> + return ret;<br> +}<br></blockquote><div><br></div><div>...</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px">+/**<br></span><span style="font-size:12.8px">+ * Translate an IPv4 addr or hostname from string form to in_addr_t<br></span><span style="font-size:12.8px">+ *<br></span><span style="font-size:12.8px">+ * In case of resolve error, it will try again for<br></span><span style="font-size:12.8px">+ * resolve_retry_seconds seconds.<br></span><span style="font-size:12.8px">+ */<br></span></blockquote><div><br></div><div>More docs is nice, but this is confusing. It will retry for (resolve_retry_seconds / 5)</div><div>times will be more correct. Its as if someone thought if the caller says try for 15</div><div>seconds its ok to try three times and wait 5 seconds in between. In reality that</div><div>could take 90 seconds or even more in some cases as getaddrinfo may not return</div><div>for 30 seconds or more. But that's a separate issue.</div><div><br></div><div>And, it will try only once if flags contain GETADDR_TRY_ONCE<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span style="font-size:12.8px"></span><span style="font-size:12.8px"> in_addr_t getaddr(unsigned int flags,<br></span><span style="font-size:12.8px"> const char *hostname,<br></span><span style="font-size:12.8px"> int resolve_retry_seconds,<br></span><span style="font-size:12.8px"> bool *succeeded,<br></span><span style="font-size:12.8px"> volatile int *signal_received);</span></blockquote><div> </div><div>The rest looks good though I've not tested it.</div><div><br></div><div>Selva</div></div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 03/12/17 04:27, Selva Nair wrote: > 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... Yeah, I think this the 'drawback' that the admin has to consider if you want hostnames in the config. > > 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. > 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. > 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. Are you sure we have a limit of 255 octects? I am not so sure. Anyway, this is not extremely important for now. > > >> + >> + /* 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. > 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 > 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. Absolutely right. I thought I had removed this though....thanks for catching it! > > >> +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. Thanks a lot for your review! I'll address the issues you raised and send v3. Cheers,
oops forgot to cc the list.. ---------- Forwarded message ---------- From: Selva Nair <selva.nair@gmail.com> Date: Sat, Dec 2, 2017 at 10:16 PM Subject: Re: [Openvpn-devel] [PATCH v2] ifconfig-ipv6(-push): allow using hostnames To: Antonio Quartulli <a@unstable.cc> Hi, On Sat, Dec 2, 2017 at 9:25 PM, Antonio Quartulli <a@unstable.cc> wrote: > Hi, > > On 03/12/17 04:27, Selva Nair wrote: > > >> + /* 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. > > > > 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. > > > 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. > > 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 > > > > > >> + > >> + /* 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. > > > > 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: - 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.. > 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.. Regards, Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 03/12/17 11:39, Selva Nair wrote: > oops forgot to cc the list.. > > ---------- Forwarded message ---------- > From: Selva Nair <selva.nair@gmail.com> > Date: Sat, Dec 2, 2017 at 10:16 PM > Subject: Re: [Openvpn-devel] [PATCH v2] ifconfig-ipv6(-push): allow using > hostnames > To: Antonio Quartulli <a@unstable.cc> > > > Hi, > > On Sat, Dec 2, 2017 at 9:25 PM, Antonio Quartulli <a@unstable.cc> wrote: > >> Hi, >> >> On 03/12/17 04:27, Selva Nair wrote: >> >>>> + /* 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. >>> >> >> 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. >> >>> 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. >> >> 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 oh ok. thanks for showing this. > > >>> >>> >>>> + >>>> + /* 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. >>> >> >> 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: > > - 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.. right. I am converting bits to unsigned long (which is what is returned by strtoul()) and left max_bits as uint8_t. Cheers,
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. cheers, Selva <div dir="ltr">Hi,<div><br></div><div>Responding to this old version just to be on record.</div><div><br></div><div>I realized patch this was assigned to Gert on patchwork too late after</div><div>started responding on my own. Sorry for jumping the gun. Have to make</div><div>keeping an eye on patchwork a habit..</div><div><br></div><div>I'll leave the latest v4 alone.</div><div><br></div><div>cheers,</div><div><br></div><div>Selva</div><div class="gmail_extra"><br></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
Hi, On 03/12/17 12:38, Selva Nair wrote: > 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.. No problem at all! Actually it was not "assigned" to Gert, but *I* simply delegated it to him, so that he could remember in the future (whenever he will have time) that *I* though this patch would fall into his area of interest. > > I'll leave the latest v4 alone. As I said above there is no "assignment", just treat that as "reminders" for other people. I am glad you jumped in and provided some early feedback (and I am sure Gert is happy too). So, please, feel free to comment further. Cheers,
Hi,, On Sat, Dec 02, 2017 at 11:38:28PM -0500, Selva Nair wrote: > 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. If that is the consequence of assigning patches to people on patchwork, 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. (There currently is one patch in patchwork which I took myself, and that's the "lowest metric interface" one) gert
On Sun, Dec 3, 2017 at 1:54 PM, Gert Doering <gert@greenie.muc.de> wrote: > Hi,, > > On Sat, Dec 02, 2017 at 11:38:28PM -0500, Selva Nair wrote: > > 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. If that is the consequence of assigning patches to people on patchwork, > we should stop doing so (Antonio, are you listening? :-) ). I didn't want to step on anyone's toes... But based on Antonio's explanation, I take that remark back. Will look into v4 as soon as I get some time to test it. > 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. Agreed. I just didn't realize Antonio delegated it to you and assumed all "delegation" are indications that the person has volunteered to look into it. As for me, I totally welcome others reviewing something I delegated to myself, or commenting/criticizing/extoling (or tipping :) my reviews (multiple reviews are indeed great). But I wouldn't particularly want others delegating work to me :) So let's continue the practice of delegating to self, not to others. We can always use the list (or IRC for those who frequent the channel) for requesting reviews etc. Best, Selva <div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Dec 3, 2017 at 1:54 PM, Gert Doering <span dir="ltr"><<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,,<br> <span class=""><br> On Sat, Dec 02, 2017 at 11:38:28PM -0500, Selva Nair wrote:<br> > Responding to this old version just to be on record.<br> ><br> > I realized patch this was assigned to Gert on patchwork too late after<br> > started responding on my own. Sorry for jumping the gun. Have to make<br> > keeping an eye on patchwork a habit..<br> ><br> > I'll leave the latest v4 alone.</span> </blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">If that is the consequence of assigning patches to people on patchwork,<br> we should stop doing so (Antonio, are you listening? :-) ). </blockquote><div><br></div><div>I didn't want to step on anyone's toes... But based on Antonio's explanation,</div><div>I take that remark back. Will look into v4 as soon as I get some time to test it.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Obviously<br> the patch was in dire need of a good review, and if you have time while<br> I am busy with family things, this is more than welcome.<br> <br> Voluntarily *taking* a patch for review is useful as a flag "I'm going to<br> handle this", but third-party-tagging "this is for Gert!" seems to have<br> potential for backfiring, so maybe we shouldn't do that, then.</blockquote><div><br></div><div>Agreed. I just didn't realize Antonio delegated it to you and assumed</div><div>all "delegation" are indications that the person has volunteered to look</div><div>into it. As for me, I totally welcome others reviewing something I delegated</div><div>to myself, or commenting/criticizing/extoling (or tipping :) my reviews </div><div>(multiple reviews are indeed great).</div><div><br></div><div>But I wouldn't particularly want others delegating work to me :) </div><div>So let's continue the practice of delegating to self, not to others.</div><div>We can always use the list (or IRC for those who frequent the channel)</div><div>for requesting reviews etc.</div><div><br></div><div>Best,</div><div><br></div><div>Selva</div></div></div></div> ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
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); + + /* check if this hostname has a /bits suffix */ + sep = strchr(var_host , '/'); + if (sep) + { + bits = strtoul(sep + 1, &endp, 10); + if ((*endp != '\0') || (bits > max_bits)) + { + 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. + */ + *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 = '/'; + } +out: + free(var_host); + + return ret; +} -/* - * Translate IP addr or hostname to in_addr_t. - * If resolve error, try again for - * resolve_retry_seconds seconds. - */ in_addr_t getaddr(unsigned int flags, const char *hostname, @@ -87,20 +177,19 @@ getaddr(unsigned int flags, bool *succeeded, volatile int *signal_received) { - struct addrinfo *ai; + in_addr_t addr; int status; - status = openvpn_getaddrinfo(flags & ~GETADDR_HOST_ORDER, hostname, NULL, - resolve_retry_seconds, signal_received, AF_INET, &ai); + + status = get_addr_generic(AF_INET, flags, hostname, &addr, NULL, + resolve_retry_seconds, signal_received, + M_WARN); if (status==0) { - struct in_addr ia; if (succeeded) { *succeeded = true; } - ia = ((struct sockaddr_in *)ai->ai_addr)->sin_addr; - freeaddrinfo(ai); - return (flags & GETADDR_HOST_ORDER) ? ntohl(ia.s_addr) : ia.s_addr; + return addr; } else { @@ -112,6 +201,19 @@ getaddr(unsigned int flags, } } +bool +get_ipv6_addr(const char *hostname, struct in6_addr *network, + unsigned int *netbits, int msglevel) +{ + if (get_addr_generic(AF_INET6, GETADDR_RESOLVE, hostname, network, netbits, + 0, NULL, msglevel) < 0) + { + return false; + } + + return true; /* parsing OK, values set */ +} + static inline bool streqnull(const char *a, const char *b) { diff --git a/src/openvpn/socket.h b/src/openvpn/socket.h index 2d7f2187..81e9e9ae 100644 --- a/src/openvpn/socket.h +++ b/src/openvpn/socket.h @@ -532,12 +532,24 @@ bool unix_socket_get_peer_uid_gid(const socket_descriptor_t sd, int *uid, int *g #define GETADDR_CACHE_MASK (GETADDR_DATAGRAM|GETADDR_PASSIVE) +/** + * 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. + */ in_addr_t getaddr(unsigned int flags, const char *hostname, int resolve_retry_seconds, bool *succeeded, volatile int *signal_received); +/** + * Translate an IPv6 addr or hostname from string form to in6_addr + */ +bool get_ipv6_addr(const char *hostname, struct in6_addr *network, + unsigned int *netbits, int msglevel); + int openvpn_getaddrinfo(unsigned int flags, const char *hostname, const char *servname,
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 <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(-)