diff mbox series

[Openvpn-devel,v1] Use topology default of "subnet" only for server mode

Message ID 20240501124254.29114-1-gert@greenie.muc.de
State Accepted
Headers show
Series [Openvpn-devel,v1] Use topology default of "subnet" only for server mode | expand

Commit Message

Gert Doering May 1, 2024, 12:42 p.m. UTC 
From: Frank Lichtenheld <frank@lichtenheld.com>

The setting of --topology changes the syntax of --ifconfig.
So changing the default of --topology breaks all existing
configs that use --ifconfig but not --topology.

For P2P setups that is probably a signification percentage.
For server setups the percentage is hopefully lower since
--ifconfig is implicitly set by --server. Also more people
might have set their topology explicitly since it makes a
much bigger difference. Clients will usually get the
topology and the IP config pushed by the server.

So we decided to not switch the default for everyone to
not affect P2P setups. What we care about is to change
the default for --mode server, so we only do that now. For
people using --server this should be transparent except
for a pool reset.

Change-Id: Iefd209c0856ef395ab74055496130de00b86ead0
Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
---

This change was reviewed on Gerrit and approved by at least one
developer. I request to merge it to master.

Gerrit URL: https://gerrit.openvpn.net/c/openvpn/+/554
This mail reflects revision 1 of this Change.

Acked-by according to Gerrit (reflected above):
Arne Schwabe <arne-openvpn@rfc2549.org>

Comments

Gert Doering May 1, 2024, 8:12 p.m. UTC | #1 
This looks good, and fixes my "reference" test case (p2p, no --server
but --tls-server / --secret, --ifconfig 10.2.3.4 10.2.3.5)

The twisty maze of passages surrounding o->mode is... interesting, but
helper_setdefault_topology() solves this in a reasonable way, I think
(the call inside helper_client_server() could have just set it, if undef,
as "we're server, and dev tun" is already known - but this way it's
"one common code").

I *do* wonder why options.c -> "server" doesn't just set o->mode = MODE_SERVER,
but that's propably lost in the mists of time...

Your patch has been applied to the master branch.

commit 066fcdba9741319fa38cbe40c1761c49727d3f9a (master)
Author: Frank Lichtenheld
Date:   Wed May 1 14:42:54 2024 +0200

     Use topology default of subnet only for server mode

     Signed-off-by: Frank Lichtenheld <frank@lichtenheld.com>
     Acked-by: Arne Schwabe <arne-openvpn@rfc2549.org>
     Message-Id: <20240501124254.29114-1-gert@greenie.muc.de>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg28592.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/Changes.rst b/Changes.rst
index b2278ab..fa0fb22 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -23,11 +23,12 @@ 
 ``persist-key`` option has been enabled by default.
     All the keys will be kept in memory across restart.
 
-Default for ``--topology`` changed to ``subnet``
-    Previous releases used ``net30`` as default. This only affects
-    configs with ``--dev tun`` and only IPv4. Note that this
-    changes the semantics of ``--ifconfig``, so if you have manual
-    settings for that in your config but not set ``--topology``
+Default for ``--topology`` changed to ``subnet`` for ``--mode server``
+    Previous releases always used ``net30`` as default. This only affects
+    configs with ``--mode server`` or ``--server`` (the latter implies the
+    former), and ``--dev tun``, and only if IPv4 is enabled.
+    Note that this changes the semantics of ``--ifconfig``, so if you have
+    manual settings for that in your config but not set ``--topology``
     your config might fail to parse with the new version. Just adding
     ``--topology net30`` to the config should fix the problem.
     By default ``--topology`` is pushed from server to client.
diff --git a/src/openvpn/helper.c b/src/openvpn/helper.c
index 1bab84c..5681718 100644
--- a/src/openvpn/helper.c
+++ b/src/openvpn/helper.c
@@ -137,6 +137,32 @@ 
 }
 
 
+/**
+ * Set --topology default depending on --mode
+ */
+void
+helper_setdefault_topology(struct options *o)
+{
+    if (o->topology != TOP_UNDEF)
+    {
+        return;
+    }
+    int dev = dev_type_enum(o->dev, o->dev_type);
+    if (dev != DEV_TYPE_TUN)
+    {
+        return;
+    }
+    if (o->mode == MODE_SERVER)
+    {
+        o->topology = TOP_SUBNET;
+    }
+    else
+    {
+        o->topology = TOP_NET30;
+    }
+}
+
+
 /*
  * Process server, server-bridge, and client helper
  * directives after the parameters themselves have been
@@ -151,7 +177,6 @@ 
      * Get tun/tap/null device type
      */
     const int dev = dev_type_enum(o->dev, o->dev_type);
-    const int topology = o->topology;
 
     /*
      *
@@ -177,11 +202,11 @@ 
 
         if (o->server_flags & SF_NOPOOL)
         {
-            msg( M_USAGE, "--server-ipv6 is incompatible with 'nopool' option" );
+            msg(M_USAGE, "--server-ipv6 is incompatible with 'nopool' option");
         }
         if (o->ifconfig_ipv6_pool_defined)
         {
-            msg( M_USAGE, "--server-ipv6 already defines an ifconfig-ipv6-pool, so you can't also specify --ifconfig-pool explicitly");
+            msg(M_USAGE, "--server-ipv6 already defines an ifconfig-ipv6-pool, so you can't also specify --ifconfig-pool explicitly");
         }
 
         o->mode = MODE_SERVER;
@@ -207,7 +232,7 @@ 
                                                   o->server_netbits_ipv6 < 112 ? 0x1000 : 2);
         o->ifconfig_ipv6_pool_netbits = o->server_netbits_ipv6;
 
-        push_option( o, "tun-ipv6", M_USAGE );
+        push_option(o, "tun-ipv6", M_USAGE);
     }
 
     /*
@@ -305,8 +330,10 @@ 
 
             o->mode = MODE_SERVER;
             o->tls_server = true;
+            /* Need to know topology now */
+            helper_setdefault_topology(o);
 
-            if (topology == TOP_NET30 || topology == TOP_P2P)
+            if (o->topology == TOP_NET30 || o->topology == TOP_P2P)
             {
                 o->ifconfig_local = print_in_addr_t(o->server_network + 1, 0, &o->gc);
                 o->ifconfig_remote_netmask = print_in_addr_t(o->server_network + 2, 0, &o->gc);
@@ -324,12 +351,12 @@ 
                 {
                     push_option(o, print_opt_route(o->server_network, o->server_netmask, &o->gc), M_USAGE);
                 }
-                else if (topology == TOP_NET30)
+                else if (o->topology == TOP_NET30)
                 {
                     push_option(o, print_opt_route(o->server_network + 1, 0, &o->gc), M_USAGE);
                 }
             }
-            else if (topology == TOP_SUBNET)
+            else if (o->topology == TOP_SUBNET)
             {
                 o->ifconfig_local = print_in_addr_t(o->server_network + 1, 0, &o->gc);
                 o->ifconfig_remote_netmask = print_in_addr_t(o->server_netmask, 0, &o->gc);
@@ -354,9 +381,9 @@ 
                 ASSERT(0);
             }
 
-            push_option(o, print_opt_topology(topology, &o->gc), M_USAGE);
+            push_option(o, print_opt_topology(o->topology, &o->gc), M_USAGE);
 
-            if (topology == TOP_NET30 && !(o->server_flags & SF_NOPOOL))
+            if (o->topology == TOP_NET30 && !(o->server_flags & SF_NOPOOL))
             {
                 msg(M_WARN, "WARNING: --topology net30 support for server "
                     "configs with IPv4 pools will be removed in a future "
@@ -394,7 +421,7 @@ 
         }
 
         /* set push-ifconfig-constraint directive */
-        if ((dev == DEV_TYPE_TAP || topology == TOP_SUBNET))
+        if ((dev == DEV_TYPE_TAP || o->topology == TOP_SUBNET))
         {
             o->push_ifconfig_constraint_defined = true;
             o->push_ifconfig_constraint_network = o->server_network;
diff --git a/src/openvpn/helper.h b/src/openvpn/helper.h
index d0fd17d..6b42e13 100644
--- a/src/openvpn/helper.h
+++ b/src/openvpn/helper.h
@@ -30,6 +30,8 @@ 
 
 #include "options.h"
 
+void helper_setdefault_topology(struct options *o);
+
 void helper_keepalive(struct options *o);
 
 void helper_client_server(struct options *o);
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index e2bfe0e..07387cd 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -796,7 +796,7 @@ 
         o->gc_owned = true;
     }
     o->mode = MODE_POINT_TO_POINT;
-    o->topology = TOP_SUBNET;
+    o->topology = TOP_UNDEF;
     o->ce.proto = PROTO_UDP;
     o->ce.af = AF_UNSPEC;
     o->ce.bind_ipv6_only = false;
@@ -3478,6 +3478,7 @@ 
     }
 }
 
+
 /**
  * Checks for availibility of Chacha20-Poly1305 and sets
  * the ncp_cipher to either AES-256-GCM:AES-128-GCM or
@@ -3680,6 +3681,8 @@ 
      * sequences of options.
      */
     helper_client_server(o);
+    /* must be called after helpers that might set --mode */
+    helper_setdefault_topology(o);
     helper_keepalive(o);
     helper_tcp_nodelay(o);