[Openvpn-devel,v5,04/14] client-connect: Move multi_client_connect_setenv into early_setup

Message ID 20200711093655.23686-4-arne@rfc2549.org
State Accepted
Headers show
Series
  • [Openvpn-devel,v5,01/14] Allow changing fallback cipher from ccd files/client-connect
Related show

Commit Message

Arne Schwabe July 11, 2020, 9:36 a.m.
From: Fabian Knittel <fabian.knittel@lettink.de>

This patch moves multi_client_connect_setenv into
multi_client_connect_early_setup and makes sure that every client-connect
handling function updates the virtual address selection.

Background: This unifies how the client-connect handling functions work.

Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
Signed-off-by: Arne Schwabe <arne@rfc2549.org>

Patch V5: Rebase on master

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/multi.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Gert Doering July 13, 2020, 12:06 p.m. | #1
On Sat, Jul 11, 2020 at 11:36:45AM +0200, Arne Schwabe wrote:
> From: Fabian Knittel <fabian.knittel@lettink.de>
> 
> This patch moves multi_client_connect_setenv into
> multi_client_connect_early_setup and makes sure that every client-connect
> handling function updates the virtual address selection.
> 
> Background: This unifies how the client-connect handling functions work.


Tested on the server side framework, all tests succeed
    
23...
Test sets succeeded: 1 1a 1b 1d 2 2a 2b 2c 2d 3 4 5 6 8 8a 9.
Test sets failed: none.
24...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 4a 5 6 8 8a 9.
Test sets failed: none.
master...
Test sets succeeded: 1 1a 1b 1c 1d 1e 2 2a 2b 2c 2d 2e 3 4 5 6 7 7a 8 8a 9 2f 4b
.
Test sets failed: none.
 

The patch itself looks a bit strange, now code seems to be duplicated...

... hopefully this gets better in the next patches.
 
gert
Antonio Quartulli July 14, 2020, 9:29 a.m. | #2
Hi,

On 11/07/2020 11:36, Arne Schwabe wrote:
> From: Fabian Knittel <fabian.knittel@lettink.de>
> 
> This patch moves multi_client_connect_setenv into
> multi_client_connect_early_setup and makes sure that every client-connect
> handling function updates the virtual address selection.
> 
> Background: This unifies how the client-connect handling functions work.
> 
> Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> 
> Patch V5: Rebase on master
> 
> Signed-off-by: Arne Schwabe <arne@rfc2549.org>
> ---
>  src/openvpn/multi.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
> index 35e0bd10..539ebfc0 100644
> --- a/src/openvpn/multi.c
> +++ b/src/openvpn/multi.c
> @@ -2150,6 +2150,12 @@ multi_client_connect_early_setup(struct multi_context *m,
>  
>      /* reset pool handle to null */
>      mi->vaddr_handle = -1;
> +
> +    /* do --client-connect setenvs */
> +    multi_select_virtual_addr(m, mi);
> +
> +    multi_client_connect_setenv(m, mi);
> +
>  }
>  
>  /**
> @@ -2195,6 +2201,13 @@ multi_client_connect_source_ccd(struct multi_context *m,
>                                    CLIENT_CONNECT_OPT_MASK,
>                                    option_types_found,
>                                    mi->context.c2.es);
> +            /*
> +             * Select a virtual address from either --ifconfig-push in
> +             * --client-config-dir file or --ifconfig-pool.
> +             */
> +            multi_select_virtual_addr(m, mi);
> +
> +            multi_client_connect_setenv(m, mi);
>          }
>          gc_free(&gc);
>      }


With this patch applied we are moving these 2 lines to:
- multi_client_connect_source_ccd()
- multi_client_connect_early_setup()

In multi_connection_established() we call the two aforementioned
functions one after the other. This means we are really executing the
select_virtual_addr() twice in a row. Am I wrong?

Maybe this gets fixed in a later patch, but are we sure we are not
breaking anything? If this double call gets rearranged in a later
patch...maybe it's better to merge these 2 patches? Bisect may become
quite ugly otherwise.

Cheers,


> @@ -2236,15 +2249,6 @@ multi_connection_established(struct multi_context *m, struct multi_instance *mi)
>  
>      multi_client_connect_source_ccd(m, mi, &option_types_found);
>  
> -    /*
> -     * Select a virtual address from either --ifconfig-push in
> -     * --client-config-dir file or --ifconfig-pool.
> -     */
> -    multi_select_virtual_addr(m, mi);
> -
> -    /* do --client-connect setenvs */
> -    multi_client_connect_setenv(m, mi);
> -
>      multi_client_connect_call_plugin_v1(m, mi, &option_types_found,
>                                          &cc_succeeded,
>                                          &cc_succeeded_count);
>
Gert Doering July 15, 2020, 11:25 a.m. | #3
Acked-by: Gert Doering <gert@greenie.muc.de>

Your patch has been applied to the master branch.

It has been stared-at, and tested on the server side test rig.

We've had quite a bit of discussion about this on IRC, and the conclusion is
"multiple calls to these functions might be needed in some situations"
(like, when a ccd/ file sets up a virtual address and a plugin or script
wants to act on the assigned IP address, by means of $ENV{ifconfig_pool_local}
etc.) - we might want to make it more clever ("if nothing changes, do not
free/reallocate stuff all the time"), or at least join them all in one place
with a clear comment

13:10 <@cron2> moving this to multi_client_connect_early_setup() seems wrong
13:10 <@cron2> we should move it to the very end, after all "bring in new 
               config" bits have been concluded
13:11 <@plaisthos> this might want to have a connect script act on the assigned ip
13:11 <@plaisthos> like dynamic routing etc
13:11 <@plaisthos> and if you move it to the end then you loose the environment 
                   variable with the assigned IP
13:12 <@cron2> in that case, you actually need to really call it after every 
               single "could alter config" call
13:12 <@cron2> and before the first
13:12 <@plaisthos> technically you don't need it before the first
13:12 <@plaisthos> because the first is ccd
13:12 <@plaisthos> that does not evaluate env
13:12 <@cron2> true
13:13 <@plaisthos> but for consistency and the later refactoring it is better 
                   to do it anyway I guess
13:14 <@cron2> okay.  I think 04/ can go in, then, but after the change to 
               "just walk the table of handlers" I'd move all calls to both 
               multi_select_virtual_addr() and multi_client_connect_setenv() 
               into this loop - so we know "it's evalued after each handler, 
               because further handles might need the environment" - and then 
               we can kick it from early_setup() again
13:14 <@cron2> the stuff is all reentrant, so 04/ is not the most elegant, but 
               it is not doing harm


commit 380a142a6b397e615ef809a94c5e6be7fcddad06
Author: Fabian Knittel
Date:   Sat Jul 11 11:36:45 2020 +0200

     client-connect: Move multi_client_connect_setenv into early_setup

     Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>
     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Gert Doering <gert@greenie.muc.de>
     Message-Id: <20200711093655.23686-4-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg20288.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering

Patch

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 35e0bd10..539ebfc0 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2150,6 +2150,12 @@  multi_client_connect_early_setup(struct multi_context *m,
 
     /* reset pool handle to null */
     mi->vaddr_handle = -1;
+
+    /* do --client-connect setenvs */
+    multi_select_virtual_addr(m, mi);
+
+    multi_client_connect_setenv(m, mi);
+
 }
 
 /**
@@ -2195,6 +2201,13 @@  multi_client_connect_source_ccd(struct multi_context *m,
                                   CLIENT_CONNECT_OPT_MASK,
                                   option_types_found,
                                   mi->context.c2.es);
+            /*
+             * Select a virtual address from either --ifconfig-push in
+             * --client-config-dir file or --ifconfig-pool.
+             */
+            multi_select_virtual_addr(m, mi);
+
+            multi_client_connect_setenv(m, mi);
         }
         gc_free(&gc);
     }
@@ -2236,15 +2249,6 @@  multi_connection_established(struct multi_context *m, struct multi_instance *mi)
 
     multi_client_connect_source_ccd(m, mi, &option_types_found);
 
-    /*
-     * Select a virtual address from either --ifconfig-push in
-     * --client-config-dir file or --ifconfig-pool.
-     */
-    multi_select_virtual_addr(m, mi);
-
-    /* do --client-connect setenvs */
-    multi_client_connect_setenv(m, mi);
-
     multi_client_connect_call_plugin_v1(m, mi, &option_types_found,
                                         &cc_succeeded,
                                         &cc_succeeded_count);