[Openvpn-devel,v2] Make --nobind default for --pull

Message ID 20211206010007.3072528-1-arne@rfc2549.org
State Accepted
Headers show
Series [Openvpn-devel,v2] Make --nobind default for --pull | expand

Commit Message

Arne Schwabe Dec. 5, 2021, 2 p.m. UTC
Currently we default to local binding with udp. But the majority of
configuration files actually uses --nobind in the configuration to
change the default for --client. And client protocols should normally
use a random source port. This changes the default. Local binding with
--client can still be done using --bind.

This commit refactors the current code to be more easy to add to understand
and adds the the o->pull condition as additional option to opt into setting
local binding to false.

Patch v2: add more commments

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 Changes.rst           |  3 ++-
 src/openvpn/options.c | 16 +++++++++-------
 2 files changed, 11 insertions(+), 8 deletions(-)

Comments

Frank Lichtenheld Dec. 6, 2021, 1:30 a.m. UTC | #1
Acked-By: Frank Lichtenheld <frank@lichtenheld.com>

> Arne Schwabe <arne@rfc2549.org> hat am 06.12.2021 02:00 geschrieben:
> 
> Patch v2: add more commments

I feel they help a lot :)

> +    /* If binding is not forced by an explicit options and we have (at least)

typo: "options" should be "option". Or "an" is wrong.

> +     * one of --tcp-client, --pull (or --client), or socks we do not bind
> +     * locally to have "normal" IP client behaviour of a random source port */
> +    if (!need_bind && (ce->proto == PROTO_TCP_CLIENT || uses_socks || o->pull))
>      {
>          ce->bind_local = false;
>      }

Regards,
  Frank

--
Frank Lichtenheld
Gert Doering Dec. 6, 2021, 6:27 a.m. UTC | #2
Your patch has been applied to the master branch.  Thanks for the
extra comments, with these, I can understand what's going on
without maximum coffee level :-)

Tested locally (my t_client tests all set --nobind, so this was
not spectacularily enlightening... ran a few cases manually to
see what would happen.  Does the job. --bind still binds, --lport still
binds, "default --client" does not bind).

Comment adjusted as instructed ("an explicit option", no "s").

commit de02c828f5aa3fbaca78918d4392b48b47547770 (master)
Author: Arne Schwabe
Date:   Mon Dec 6 02:00:07 2021 +0100

     Make --nobind default for --pull

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


--
kind regards,

Gert Doering

Patch

diff --git a/Changes.rst b/Changes.rst
index e83dda023..b7d7f2054 100644
--- a/Changes.rst
+++ b/Changes.rst
@@ -120,7 +120,8 @@  PF (Packet Filtering) support has been removed
 User-visible Changes
 --------------------
 - CHACHA20-POLY1305 is included in the default of ``--data-ciphers`` when available.
-- Option ``--prng`` is ignored as we rely on the SSL library radnom generator.
+- Option ``--prng`` is ignored as we rely on the SSL library random number generator.
+- Option ``--nobind`` is default when ``--client`` or ``--pull`` is used in the configuration
 
 Overview of changes in 2.5
 ==========================
diff --git a/src/openvpn/options.c b/src/openvpn/options.c
index 312efb36c..39cf3e99a 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2859,14 +2859,16 @@  options_postprocess_mutate_ce(struct options *o, struct connection_entry *ce)
         }
     }
 
-    if (ce->proto == PROTO_TCP_CLIENT && !ce->local
-        && !ce->local_port_defined && !ce->bind_defined)
-    {
-        ce->bind_local = false;
-    }
+    /* an option is present that requires local bind to enabled */
+    bool need_bind = ce->local || ce->local_port_defined || ce->bind_defined;
+
+    /* socks proxy is enabled */
+    bool uses_socks = ce->proto == PROTO_UDP && ce->socks_proxy_server;
 
-    if (ce->proto == PROTO_UDP && ce->socks_proxy_server && !ce->local
-        && !ce->local_port_defined && !ce->bind_defined)
+    /* If binding is not forced by an explicit options and we have (at least)
+     * one of --tcp-client, --pull (or --client), or socks we do not bind
+     * locally to have "normal" IP client behaviour of a random source port */
+    if (!need_bind && (ce->proto == PROTO_TCP_CLIENT || uses_socks || o->pull))
     {
         ce->bind_local = false;
     }