[Openvpn-devel,1/2] FreeBSD: for topology subnet, put tun interface into IFF_BROADCAST mode

Message ID 20221012145915.25810-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,1/2] FreeBSD: for topology subnet, put tun interface into IFF_BROADCAST mode | expand

Commit Message

Gert Doering Oct. 12, 2022, 2:59 p.m. UTC
For reasons unknown, OpenVPN has always put FreeBSD tun(4) interfaces
into point-to-point mode (IFF_POINTOPOINT), which means "local and
remote address, no on-link subnet".

"--topology subnet" was emulated by adding a subnet-route to the "remote"
(which was just picking a free address from the subnet).

This works well enough for classic tun(4) interfaces that have no
next-hop resolution, and routes pointing to "that fake remote" only
(because all routing is done inside OpenVPN and it does not matter how
packets get there) - but for ovpn(4) interfaces, it breaks iroute setup,
where the route next-hop must be an on-link address.

Thus, change interface to IFF_BROADCAST mode, and get rid of all the
special code needed to "fake" subnet mode.

Tested with tun(4) and ovpn(4) on FreeBSD 14, client and server, and
with tun(4) on FreeBSD 12 and 7.4

To actually work with ovpn(4) / FreeBSD DCO, a followup patch for
kernel ovpn(4) and OpenVPN dco_freebsd.c is needed.

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 Changes.rst       |  5 +++++
 src/openvpn/tun.c | 37 ++++++++++++-------------------------
 2 files changed, 17 insertions(+), 25 deletions(-)

Comments

Kristof Provost Oct. 17, 2022, 12:06 p.m. UTC | #1
Signed-off-by:	Kristof Provost <kprovost@netgate.com>

On 12 Oct 2022, at 16:59, Gert Doering wrote:
> For reasons unknown, OpenVPN has always put FreeBSD tun(4) interfaces
> into point-to-point mode (IFF_POINTOPOINT), which means "local and
> remote address, no on-link subnet".
>
> "--topology subnet" was emulated by adding a subnet-route to the "remote"
> (which was just picking a free address from the subnet).
>
> This works well enough for classic tun(4) interfaces that have no
> next-hop resolution, and routes pointing to "that fake remote" only
> (because all routing is done inside OpenVPN and it does not matter how
> packets get there) - but for ovpn(4) interfaces, it breaks iroute setup,
> where the route next-hop must be an on-link address.
>
> Thus, change interface to IFF_BROADCAST mode, and get rid of all the
> special code needed to "fake" subnet mode.
>
> Tested with tun(4) and ovpn(4) on FreeBSD 14, client and server, and
> with tun(4) on FreeBSD 12 and 7.4
>
> To actually work with ovpn(4) / FreeBSD DCO, a followup patch for
> kernel ovpn(4) and OpenVPN dco_freebsd.c is needed.
>
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  Changes.rst       |  5 +++++
>  src/openvpn/tun.c | 37 ++++++++++++-------------------------
>  2 files changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/Changes.rst b/Changes.rst
> index df56f76a..0397cb94 100644
> --- a/Changes.rst
> +++ b/Changes.rst
> @@ -163,6 +163,11 @@ User-visible Changes
>  - :code:`link_mtu` parameter is removed from environment or replaced with 0 when scripts are
>    called with parameters. This parameter is unreliable and no longer internally calculated.
>
> +- FreeBSD tun interfaces with ``--topology subnet`` are now put into real
> +  subnet mode (IFF_BROADCAST instead of IFF_POINTOPOINT) - this might upset
> +  software that enumerates interfaces, looking for "broadcast capable?" and
> +  expecting certain results.  Normal uses should not see any difference.
> +
>  Overview of changes in 2.5
>  ==========================
>
> diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
> index ddee49f9..a83ec9e6 100644
> --- a/src/openvpn/tun.c
> +++ b/src/openvpn/tun.c
> @@ -1479,43 +1479,23 @@ do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,
>
>  #elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)
>
> -    in_addr_t remote_end;           /* for "virtual" subnet topology */
> -
>      /* example: ifconfig tun2 10.2.0.2 10.2.0.1 mtu 1450 netmask 255.255.255.255 up */
> -    if (tun)
> +    if (tun)       /* point-to-point tun */
>      {
>          argv_printf(&argv, "%s %s %s %s mtu %d netmask 255.255.255.255 up",
>                      IFCONFIG_PATH, ifname, ifconfig_local,
>                      ifconfig_remote_netmask, tun_mtu);
>      }
> -    else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
> -    {
> -        remote_end = create_arbitrary_remote( tt );
> -        argv_printf(&argv, "%s %s %s %s mtu %d netmask %s up", IFCONFIG_PATH,
> -                    ifname, ifconfig_local, print_in_addr_t(remote_end, 0, &gc),
> -                    tun_mtu, ifconfig_remote_netmask);
> -    }
> -    else
> +    else            /* tun with topology subnet and tap mode (always subnet) */
>      {
> -        argv_printf(&argv, "%s %s %s netmask %s mtu %d up", IFCONFIG_PATH,
> -                    ifname, ifconfig_local, ifconfig_remote_netmask, tun_mtu);
> +        int netbits = netmask_to_netbits2(tt->remote_netmask);
> +        argv_printf(&argv, "%s %s %s/%d mtu %d up", IFCONFIG_PATH,
> +                    ifname, ifconfig_local, netbits, tun_mtu );
>      }
>
>      argv_msg(M_INFO, &argv);
>      openvpn_execve_check(&argv, es, S_FATAL, "FreeBSD ifconfig failed");
>
> -    /* Add a network route for the local tun interface */
> -    if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
> -    {
> -        struct route_ipv4 r;
> -        CLEAR(r);
> -        r.flags = RT_DEFINED;
> -        r.network = tt->local & tt->remote_netmask;
> -        r.netmask = tt->remote_netmask;
> -        r.gateway = remote_end;
> -        add_route(&r, tt, 0, NULL, es, NULL);
> -    }
> -
>  #elif defined(TARGET_AIX)
>      {
>          /* AIX ifconfig will complain if it can't find ODM path in env */
> @@ -2949,12 +2929,19 @@ open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
>
>          if (tt->fd >= 0 && tt->type == DEV_TYPE_TUN)
>          {
> +            /* see "Interface Flags" in ifnet(9) */
>              int i = IFF_POINTOPOINT | IFF_MULTICAST;
> +            if (tt->topology == TOP_SUBNET)
> +            {
> +                i = IFF_BROADCAST | IFF_MULTICAST;
> +            }
>
>              if (ioctl(tt->fd, TUNSIFMODE, &i) < 0)
>              {
>                  msg(M_WARN | M_ERRNO, "ioctl(TUNSIFMODE)");
>              }
> +
> +            /* multi_af mode for v4+v6, see "tun(4)" */
>              i = 1;
>              if (ioctl(tt->fd, TUNSIFHEAD, &i) < 0)
>              {
> -- 
> 2.37.3
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Gert Doering Oct. 19, 2022, 7:20 a.m. UTC | #2
Thanks for the review, Kristof.

Just to be sure, I've run this through extensive client/server tests
on FreeBSD 14 + 12.3 again.  Still works :-)

(Except that this patch, as documented, breaks "topology subnet with DCO"
- to be fixed in the next commit)

Patch has been applied to the master branch.

commit 94db32616597497e57eb2fa6fab05297da314a53
Author: Gert Doering
Date:   Wed Oct 12 16:59:14 2022 +0200

     FreeBSD: for topology subnet, put tun interface into IFF_BROADCAST mode

     Signed-off-by: Gert Doering <gert@greenie.muc.de>
     Acked-by: Kristof Provost <kprovost@netgate.com>
     Message-Id: <20221012145915.25810-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25396.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/Changes.rst b/Changes.rst
index df56f76a..0397cb94 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -163,6 +163,11 @@  User-visible Changes
 - :code:`link_mtu` parameter is removed from environment or replaced with 0 when scripts are
   called with parameters. This parameter is unreliable and no longer internally calculated.
 
+- FreeBSD tun interfaces with ``--topology subnet`` are now put into real
+  subnet mode (IFF_BROADCAST instead of IFF_POINTOPOINT) - this might upset
+  software that enumerates interfaces, looking for "broadcast capable?" and
+  expecting certain results.  Normal uses should not see any difference.
+
 Overview of changes in 2.5
 ==========================
 
diff --git a/src/openvpn/tun.c b/src/openvpn/tun.c
index ddee49f9..a83ec9e6 100644
--- a/src/openvpn/tun.c
+++ b/src/openvpn/tun.c
@@ -1479,43 +1479,23 @@  do_ifconfig_ipv4(struct tuntap *tt, const char *ifname, int tun_mtu,
 
 #elif defined(TARGET_FREEBSD) || defined(TARGET_DRAGONFLY)
 
-    in_addr_t remote_end;           /* for "virtual" subnet topology */
-
     /* example: ifconfig tun2 10.2.0.2 10.2.0.1 mtu 1450 netmask 255.255.255.255 up */
-    if (tun)
+    if (tun)       /* point-to-point tun */
     {
         argv_printf(&argv, "%s %s %s %s mtu %d netmask 255.255.255.255 up",
                     IFCONFIG_PATH, ifname, ifconfig_local,
                     ifconfig_remote_netmask, tun_mtu);
     }
-    else if (tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
-    {
-        remote_end = create_arbitrary_remote( tt );
-        argv_printf(&argv, "%s %s %s %s mtu %d netmask %s up", IFCONFIG_PATH,
-                    ifname, ifconfig_local, print_in_addr_t(remote_end, 0, &gc),
-                    tun_mtu, ifconfig_remote_netmask);
-    }
-    else
+    else            /* tun with topology subnet and tap mode (always subnet) */
     {
-        argv_printf(&argv, "%s %s %s netmask %s mtu %d up", IFCONFIG_PATH,
-                    ifname, ifconfig_local, ifconfig_remote_netmask, tun_mtu);
+        int netbits = netmask_to_netbits2(tt->remote_netmask);
+        argv_printf(&argv, "%s %s %s/%d mtu %d up", IFCONFIG_PATH,
+                    ifname, ifconfig_local, netbits, tun_mtu );
     }
 
     argv_msg(M_INFO, &argv);
     openvpn_execve_check(&argv, es, S_FATAL, "FreeBSD ifconfig failed");
 
-    /* Add a network route for the local tun interface */
-    if (!tun && tt->type == DEV_TYPE_TUN && tt->topology == TOP_SUBNET)
-    {
-        struct route_ipv4 r;
-        CLEAR(r);
-        r.flags = RT_DEFINED;
-        r.network = tt->local & tt->remote_netmask;
-        r.netmask = tt->remote_netmask;
-        r.gateway = remote_end;
-        add_route(&r, tt, 0, NULL, es, NULL);
-    }
-
 #elif defined(TARGET_AIX)
     {
         /* AIX ifconfig will complain if it can't find ODM path in env */
@@ -2949,12 +2929,19 @@  open_tun(const char *dev, const char *dev_type, const char *dev_node, struct tun
 
         if (tt->fd >= 0 && tt->type == DEV_TYPE_TUN)
         {
+            /* see "Interface Flags" in ifnet(9) */
             int i = IFF_POINTOPOINT | IFF_MULTICAST;
+            if (tt->topology == TOP_SUBNET)
+            {
+                i = IFF_BROADCAST | IFF_MULTICAST;
+            }
 
             if (ioctl(tt->fd, TUNSIFMODE, &i) < 0)
             {
                 msg(M_WARN | M_ERRNO, "ioctl(TUNSIFMODE)");
             }
+
+            /* multi_af mode for v4+v6, see "tun(4)" */
             i = 1;
             if (ioctl(tt->fd, TUNSIFHEAD, &i) < 0)
             {