[Openvpn-devel] Make 2nd parameter to --ifconfig-ipv6 no longer mandatory.

Message ID 20210806110114.21722-1-gert@greenie.muc.de
State Changes Requested
Delegated to: Antonio Quartulli
Headers show
Series
  • [Openvpn-devel] Make 2nd parameter to --ifconfig-ipv6 no longer mandatory.
Related show

Commit Message

Gert Doering Aug. 6, 2021, 11:01 a.m.
--ifconfig-ipv6 takes two parameters, "local ipv6 address / netbits"
and "remote ipv6 address".

We only *need* a remote ipv6 address if we are in TAP mode, want to
install --route-ipv6 routes, and neither --route-ipv6-gateway is set
nor a gateway is included in the --route-ipv6 statement.

The documentation always implied "this is optional", but the option
checking code and init_tun() always mandated it, with no good reason.

Remove requirement, adjust help message.

NOTE: on SOLARIS, this is actually required for "ifconfig", so this
add a new #ifdef check to init_tun().  Sorry.

Reported-By: Fran├žois Kooman <fkooman@tuxed.net>
Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/options.c | 27 +++++++++++++++++++--------
 src/openvpn/tun.c     | 15 ++++++++++-----
 2 files changed, 29 insertions(+), 13 deletions(-)

Comments

Antonio Quartulli Aug. 9, 2021, 9:24 a.m. | #1
Hi,

On 06/08/2021 13:01, Gert Doering wrote:
> --ifconfig-ipv6 takes two parameters, "local ipv6 address / netbits"
> and "remote ipv6 address".
> 
> We only *need* a remote ipv6 address if we are in TAP mode, want to
> install --route-ipv6 routes, and neither --route-ipv6-gateway is set
> nor a gateway is included in the --route-ipv6 statement.
> 
> The documentation always implied "this is optional", but the option
> checking code and init_tun() always mandated it, with no good reason.
> 
> Remove requirement, adjust help message.
> 
> NOTE: on SOLARIS, this is actually required for "ifconfig", so this
> add a new #ifdef check to init_tun().  Sorry.
> 
> Reported-By: Fran├žois Kooman <fkooman@tuxed.net>
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  src/openvpn/options.c | 27 +++++++++++++++++++--------
>  src/openvpn/tun.c     | 15 ++++++++++-----
>  2 files changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/src/openvpn/options.c b/src/openvpn/options.c
> index 7e146db9..149e17de 100644
> --- a/src/openvpn/options.c
> +++ b/src/openvpn/options.c
> @@ -185,8 +185,8 @@ static const char usage_message[] =
>      "                  addresses outside of the subnets used by either peer.\n"
>      "                  TAP: configure device to use IP address l as a local\n"
>      "                  endpoint and rn as a subnet mask.\n"
> -    "--ifconfig-ipv6 l r : configure device to use IPv6 address l as local\n"
> -    "                      endpoint (as a /64) and r as remote endpoint\n"
> +    "--ifconfig-ipv6 l/nn [r] : configure device to use IPv6 address l as local\n"
> +    "                      endpoint (as a /nn) and r as remote endpoint\n"
>      "--ifconfig-noexec : Don't actually execute ifconfig/netsh command, instead\n"
>      "                    pass --ifconfig parms by environment to scripts.\n"
>      "--ifconfig-nowarn : Don't warn if the --ifconfig option on this side of the\n"
> @@ -201,7 +201,7 @@ static const char usage_message[] =
>      "                  Add IPv6 route to routing table after connection\n"
>      "                  is established.  Multiple routes can be specified.\n"
>      "                  gateway default: taken from --route-ipv6-gateway or 'remote'\n"
> -    "                  in --ifconfig-ipv6\n"
> +    "                  in --ifconfig-ipv6 (only required in TAP mode)\n"
>      "--route-gateway gw|'dhcp' : Specify a default gateway for use with --route.\n"
>      "--route-ipv6-gateway gw : Specify a default gateway for use with --route-ipv6.\n"
>      "--route-metric m : Specify a default metric for use with --route.\n"
> @@ -5600,13 +5600,12 @@ add_option(struct options *options,
>              goto err;
>          }
>      }
> -    else if (streq(p[0], "ifconfig-ipv6") && p[1] && p[2] && !p[3])
> +    else if (streq(p[0], "ifconfig-ipv6") && p[1] && !p[3])
>      {
>          unsigned int netbits;
>  
>          VERIFY_PERMISSION(OPT_P_UP);
> -        if (get_ipv6_addr( p[1], NULL, &netbits, msglevel )
> -            && ipv6_addr_safe( p[2] ) )
> +        if (get_ipv6_addr( p[1], NULL, &netbits, msglevel ))
>          {
>              if (netbits < 64 || netbits > 124)
>              {
> @@ -5616,13 +5615,25 @@ add_option(struct options *options,
>  
>              options->ifconfig_ipv6_local = get_ipv6_addr_no_netbits(p[1], &options->gc);
>              options->ifconfig_ipv6_netbits = netbits;
> -            options->ifconfig_ipv6_remote = p[2];
>          }
>          else
>          {
> -            msg(msglevel, "ifconfig-ipv6 parms '%s' and '%s' must be valid addresses", p[1], p[2]);
> +            msg(msglevel, "ifconfig-ipv6 '%s' must be valid IPv6 address", p[1]);
>              goto err;
>          }
> +
> +        if (p[2])               /* "ipv6_remote" is optional */

why 2km between the if-condition and the comment? :-D

> +        {
> +            if (ipv6_addr_safe(p[2]))
> +            {
> +                options->ifconfig_ipv6_remote = p[2];
> +            }
> +            else
> +            {
> +                msg(msglevel, "ifconfig-ipv6 '%s' must be valid IPv6 address", p[2]);
> +                goto err;
> +            }
> +        }
>      }
>      else if (streq(p[0], "ifconfig-noexec") && !p[1])
>      {
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index 28f803ec..1002054b 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -852,17 +852,22 @@ init_tun(const char *dev,        /* --dev option */
>          tt->did_ifconfig_setup = true;
>      }
>  
> -    if (ifconfig_ipv6_local_parm && ifconfig_ipv6_remote_parm)
> +    if (ifconfig_ipv6_local_parm)
>      {
> -
> +#ifdef TARGET_SOLARIS
> +        if (!ifconfig_ipv6_remote_parm ||
> +            || inet_pton( AF_INET6, ifconfig_ipv6_remote_parm, &tt->remote_ipv6 ) != 1)

there is a double ||

> +        {
> +            msg(M_FATAL, "init_tun: Solaris needs valid IPv6 address as second parameter to '--ifconfig-ipv6'");
> +        }
> +#endif

At this point tt->remote_ipv6 makes sense exclusively in the
TARGET_SOLARIS context.

First of all I think we should put a nice comment here explaining why we
do this for solaris only.

Secondly, do we want to make this more explicit by adding more ifdefs?
(probably not, even though it would make it very explicit)

By not setting tt->remote_ipv6 for all platforms, we will end up with
the env variable ifconfig_ipv6_remote set to null (or addr-zero or
something like that) in do_ifconfig_setenv().

This does not sound right, because if an argument was provided, it
should then be exposed via environment.

Thoughts?

>          /*
>           * Convert arguments to binary IPv6 addresses.
>           */
>  
> -        if (inet_pton( AF_INET6, ifconfig_ipv6_local_parm, &tt->local_ipv6 ) != 1
> -            || inet_pton( AF_INET6, ifconfig_ipv6_remote_parm, &tt->remote_ipv6 ) != 1)
> +        if (inet_pton( AF_INET6, ifconfig_ipv6_local_parm, &tt->local_ipv6 ) != 1)
>          {
> -            msg( M_FATAL, "init_tun: problem converting IPv6 ifconfig addresses %s and %s to binary", ifconfig_ipv6_local_parm, ifconfig_ipv6_remote_parm );
> +            msg(M_FATAL, "init_tun: problem converting IPv6 ifconfig address %s to binary", ifconfig_ipv6_local_parm);
>          }
>          tt->netbits_ipv6 = ifconfig_ipv6_netbits_parm;
>  
>
Gert Doering Aug. 10, 2021, 9 a.m. | #2
Hi,

On Mon, Aug 09, 2021 at 11:24:44AM +0200, Antonio Quartulli wrote:
> On 06/08/2021 13:01, Gert Doering wrote:
> > --ifconfig-ipv6 takes two parameters, "local ipv6 address / netbits"
> > and "remote ipv6 address".
> > 
> > We only *need* a remote ipv6 address if we are in TAP mode, want to
> > install --route-ipv6 routes, and neither --route-ipv6-gateway is set
> > nor a gateway is included in the --route-ipv6 statement.

So that it's not lost:  more review of route.c uncovered that the
FreeBSD, OpenBSD, NetBSD and AIX code paths always need a gateway,
but this is not checked properly.

#elif defined(TARGET_NETBSD)
     
    argv_printf(&argv, "%s add -inet6 %s/%d %s",
                ROUTE_PATH,
                network, r6->netbits, gateway );

(etc)

v2 coming...

gert

Patch

diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 7e146db9..149e17de 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -185,8 +185,8 @@  static const char usage_message[] =
     "                  addresses outside of the subnets used by either peer.\n"
     "                  TAP: configure device to use IP address l as a local\n"
     "                  endpoint and rn as a subnet mask.\n"
-    "--ifconfig-ipv6 l r : configure device to use IPv6 address l as local\n"
-    "                      endpoint (as a /64) and r as remote endpoint\n"
+    "--ifconfig-ipv6 l/nn [r] : configure device to use IPv6 address l as local\n"
+    "                      endpoint (as a /nn) and r as remote endpoint\n"
     "--ifconfig-noexec : Don't actually execute ifconfig/netsh command, instead\n"
     "                    pass --ifconfig parms by environment to scripts.\n"
     "--ifconfig-nowarn : Don't warn if the --ifconfig option on this side of the\n"
@@ -201,7 +201,7 @@  static const char usage_message[] =
     "                  Add IPv6 route to routing table after connection\n"
     "                  is established.  Multiple routes can be specified.\n"
     "                  gateway default: taken from --route-ipv6-gateway or 'remote'\n"
-    "                  in --ifconfig-ipv6\n"
+    "                  in --ifconfig-ipv6 (only required in TAP mode)\n"
     "--route-gateway gw|'dhcp' : Specify a default gateway for use with --route.\n"
     "--route-ipv6-gateway gw : Specify a default gateway for use with --route-ipv6.\n"
     "--route-metric m : Specify a default metric for use with --route.\n"
@@ -5600,13 +5600,12 @@  add_option(struct options *options,
             goto err;
         }
     }
-    else if (streq(p[0], "ifconfig-ipv6") && p[1] && p[2] && !p[3])
+    else if (streq(p[0], "ifconfig-ipv6") && p[1] && !p[3])
     {
         unsigned int netbits;
 
         VERIFY_PERMISSION(OPT_P_UP);
-        if (get_ipv6_addr( p[1], NULL, &netbits, msglevel )
-            && ipv6_addr_safe( p[2] ) )
+        if (get_ipv6_addr( p[1], NULL, &netbits, msglevel ))
         {
             if (netbits < 64 || netbits > 124)
             {
@@ -5616,13 +5615,25 @@  add_option(struct options *options,
 
             options->ifconfig_ipv6_local = get_ipv6_addr_no_netbits(p[1], &options->gc);
             options->ifconfig_ipv6_netbits = netbits;
-            options->ifconfig_ipv6_remote = p[2];
         }
         else
         {
-            msg(msglevel, "ifconfig-ipv6 parms '%s' and '%s' must be valid addresses", p[1], p[2]);
+            msg(msglevel, "ifconfig-ipv6 '%s' must be valid IPv6 address", p[1]);
             goto err;
         }
+
+        if (p[2])               /* "ipv6_remote" is optional */
+        {
+            if (ipv6_addr_safe(p[2]))
+            {
+                options->ifconfig_ipv6_remote = p[2];
+            }
+            else
+            {
+                msg(msglevel, "ifconfig-ipv6 '%s' must be valid IPv6 address", p[2]);
+                goto err;
+            }
+        }
     }
     else if (streq(p[0], "ifconfig-noexec") && !p[1])
     {
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index 28f803ec..1002054b 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -852,17 +852,22 @@  init_tun(const char *dev,        /* --dev option */
         tt->did_ifconfig_setup = true;
     }
 
-    if (ifconfig_ipv6_local_parm && ifconfig_ipv6_remote_parm)
+    if (ifconfig_ipv6_local_parm)
     {
-
+#ifdef TARGET_SOLARIS
+        if (!ifconfig_ipv6_remote_parm ||
+            || inet_pton( AF_INET6, ifconfig_ipv6_remote_parm, &tt->remote_ipv6 ) != 1)
+        {
+            msg(M_FATAL, "init_tun: Solaris needs valid IPv6 address as second parameter to '--ifconfig-ipv6'");
+        }
+#endif
         /*
          * Convert arguments to binary IPv6 addresses.
          */
 
-        if (inet_pton( AF_INET6, ifconfig_ipv6_local_parm, &tt->local_ipv6 ) != 1
-            || inet_pton( AF_INET6, ifconfig_ipv6_remote_parm, &tt->remote_ipv6 ) != 1)
+        if (inet_pton( AF_INET6, ifconfig_ipv6_local_parm, &tt->local_ipv6 ) != 1)
         {
-            msg( M_FATAL, "init_tun: problem converting IPv6 ifconfig addresses %s and %s to binary", ifconfig_ipv6_local_parm, ifconfig_ipv6_remote_parm );
+            msg(M_FATAL, "init_tun: problem converting IPv6 ifconfig address %s to binary", ifconfig_ipv6_local_parm);
         }
         tt->netbits_ipv6 = ifconfig_ipv6_netbits_parm;