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

Message ID 20211202125455.2593763-1-arne@rfc2549.org
State Superseded
Headers show
Series
  • [Openvpn-devel] Make --nobind default for --pull
Related show

Commit Message

Arne Schwabe Dec. 2, 2021, 12:54 p.m.
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.
---
 Changes.rst           |  3 ++-
 src/openvpn/options.c | 11 ++++-------
 2 files changed, 6 insertions(+), 8 deletions(-)

Comments

Frank Lichtenheld Dec. 3, 2021, 4:18 p.m. | #1
> Arne Schwabe <arne@rfc2549.org> hat am 02.12.2021 13:54 geschrieben:
>  
> -    if (ce->proto == PROTO_TCP_CLIENT && !ce->local
> -        && !ce->local_port_defined && !ce->bind_defined)
> -    {
> -        ce->bind_local = false;
> -    }
>  
> -    if (ce->proto == PROTO_UDP && ce->socks_proxy_server && !ce->local
> -        && !ce->local_port_defined && !ce->bind_defined)
> +    bool need_bind = ce->local || ce->local_port_defined || ce->bind_defined;
> +    bool uses_socks = ce->proto == PROTO_UDP && ce->socks_proxy_server;
> +
> +    if (!need_bind && (ce->proto == PROTO_TCP_CLIENT || uses_socks || o->pull))
>      {
>          ce->bind_local = false;
>      }

The refactoring makes it very hard to see what the actual change is. Maybe it would be better to split this in two commits?

The only actual change in behavior is the "|| o->pull", right?

Regards,
  Frank

--
Frank Lichtenheld
Arne Schwabe Dec. 5, 2021, 2:57 a.m. | #2
Am 03.12.21 um 17:18 schrieb Frank Lichtenheld:
>> Arne Schwabe <arne@rfc2549.org> hat am 02.12.2021 13:54 geschrieben:
>>   
>> -    if (ce->proto == PROTO_TCP_CLIENT && !ce->local
>> -        && !ce->local_port_defined && !ce->bind_defined)
>> -    {
>> -        ce->bind_local = false;
>> -    }
>>   
>> -    if (ce->proto == PROTO_UDP && ce->socks_proxy_server && !ce->local
>> -        && !ce->local_port_defined && !ce->bind_defined)
>> +    bool need_bind = ce->local || ce->local_port_defined || ce->bind_defined;
>> +    bool uses_socks = ce->proto == PROTO_UDP && ce->socks_proxy_server;
>> +
>> +    if (!need_bind && (ce->proto == PROTO_TCP_CLIENT || uses_socks || o->pull))
>>       {
>>           ce->bind_local = false;
>>       }
> 
> The refactoring makes it very hard to see what the actual change is. Maybe it would be better to split this in two commits?
> 
> The only actual change in behavior is the "|| o->pull", right?
> 

Yes. But should I really split up this 5 line patch into two patches? 
That feels a bit excessive.

Arne
Gert Doering Dec. 5, 2021, 10:19 a.m. | #3
Hi,

On Sun, Dec 05, 2021 at 03:57:55AM +0100, Arne Schwabe wrote:
> > The refactoring makes it very hard to see what the actual change is. Maybe it would be better to split this in two commits?
> > 
> > The only actual change in behavior is the "|| o->pull", right?
> 
> Yes. But should I really split up this 5 line patch into two patches? 
> That feels a bit excessive.

I'm fine with a single patch here.

I need to think a bit more into "what conditions is this checking, exactly,
and why?" - but with refactoring, I'd have to do that thinking twice :-)

gert
Frank Lichtenheld Dec. 5, 2021, 11:16 a.m. | #4
> Gert Doering <gert@greenie.muc.de> hat am 05.12.2021 11:19 geschrieben:
> 
>  
> Hi,
> 
> On Sun, Dec 05, 2021 at 03:57:55AM +0100, Arne Schwabe wrote:
> > > The refactoring makes it very hard to see what the actual change is. Maybe it would be better to split this in two commits?
> > > 
> > > The only actual change in behavior is the "|| o->pull", right?
> > 
> > Yes. But should I really split up this 5 line patch into two patches? 
> > That feels a bit excessive.
> 
> I'm fine with a single patch here.
> 
> I need to think a bit more into "what conditions is this checking, exactly,
> and why?" - but with refactoring, I'd have to do that thinking twice :-)

I disagree. If you had one patch that does refactoring but does not intend any functional change,
and one patch that just adds "|| o->pull", the general understanding of each patch would require
much less time and thinking. Mostly it would be much easier to match the change with the intent
communicated by the commit message. Which for me was the hardest part, here.

Regards,
  Frank

--
Frank Lichtenheld
Gert Doering Dec. 5, 2021, 11:40 a.m. | #5
Hi,

On Sun, Dec 05, 2021 at 12:16:56PM +0100, Frank Lichtenheld wrote:
> > I need to think a bit more into "what conditions is this checking, exactly,
> > and why?" - but with refactoring, I'd have to do that thinking twice :-)
> 
> I disagree. If you had one patch that does refactoring but does not intend any functional change,
> and one patch that just adds "|| o->pull", the general understanding of each patch would require
> much less time and thinking. 

True, but the second patch would not be correct, then :-) - as it
actually adds more conditions than just "o->pull".  The original code
does not check for socks.

I am not exactly happy with the *new* code, as it is still too convoluted
for my coffee level ("if (!bind_needed && conditions) set bind_local=false",
what are these conditions?  why?) but I do not think an intermediate 
patch will make these particular conditional contortions easier...

gert
Arne Schwabe Dec. 5, 2021, 6:04 p.m. | #6
Am 05.12.2021 um 12:40 schrieb Gert Doering:
> Hi,
>
> On Sun, Dec 05, 2021 at 12:16:56PM +0100, Frank Lichtenheld wrote:
>>> I need to think a bit more into "what conditions is this checking, exactly,
>>> and why?" - but with refactoring, I'd have to do that thinking twice :-)
>> I disagree. If you had one patch that does refactoring but does not intend any functional change,
>> and one patch that just adds "|| o->pull", the general understanding of each patch would require
>> much less time and thinking.
> True, but the second patch would not be correct, then :-) - as it
> actually adds more conditions than just "o->pull".  The original code
> does not check for socks.


The original code checks for socks. That is not changed.  The orignal 
code basically has two ifs that checks for the code that is !bind_need 
and the particular condition (socks or tcp-client)

>
> I am not exactly happy with the *new* code, as it is still too convoluted
> for my coffee level ("if (!bind_needed && conditions) set bind_local=false",
> what are these conditions?  why?)

I can add more comments to the code. bind_needed is basically if the 
options have a condition that forces binding anyway.

>   but I do not think an intermediate
> patch will make these particular conditional contortions easier...
>
Gert Doering Dec. 5, 2021, 6:08 p.m. | #7
Hi,

On Sun, Dec 05, 2021 at 07:04:01PM +0100, Arne Schwabe wrote:
> The original code checks for socks. That is not changed.  The orignal 
> code basically has two ifs that checks for the code that is !bind_need 
> and the particular condition (socks or tcp-client)

You're right.  As I said, that mass if conditions, half of them
negated, exceeded my coffee level...

> > for my coffee level ("if (!bind_needed && conditions) set bind_local=false",
> > what are these conditions?  why?)
> 
> I can add more comments to the code. bind_needed is basically if the 
> options have a condition that forces binding anyway.

I think that would be good.

thanks :)

gert

Patch

diff --git a/Changes.rst b/Changes.rst
index c1a04deed..95bebc4ab 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..3aaad7bc8 100644
--- a/src/openvpn/options.c
+++ b/src/openvpn/options.c
@@ -2859,14 +2859,11 @@  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;
-    }
 
-    if (ce->proto == PROTO_UDP && ce->socks_proxy_server && !ce->local
-        && !ce->local_port_defined && !ce->bind_defined)
+    bool need_bind = ce->local || ce->local_port_defined || ce->bind_defined;
+    bool uses_socks = ce->proto == PROTO_UDP && ce->socks_proxy_server;
+
+    if (!need_bind && (ce->proto == PROTO_TCP_CLIENT || uses_socks || o->pull))
     {
         ce->bind_local = false;
     }