diff mbox series

[Openvpn-devel,v8,4/5] client-connect: Add deferred support to the client-connect plugin v1 handler

Message ID 20200719173436.16431-4-arne@rfc2549.org
State Accepted
Delegated to: Gert Doering
Headers
Series [Openvpn-devel,v8,1/5] Remove CAS_PARTIAL state |

Commit Message

Arne Schwabe July 19, 2020, 7:34 a.m. UTC 
  From: Fabian Knittel <fabian.knittel@lettink.de>

Uses the infrastructure provided and used in the previous patch to provide
deferral support to the v1 client-connect plugin handler as well.

Signed-off-by: Fabian Knittel <fabian.knittel@lettink.de>

PATCH V3: Modify the API to also (optionally) call the plugin on a deferred
call. This allows the plugin authors to be more flexible and make the V1 API
more similar to the V2 API.

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 include/openvpn-plugin.h.in | 29 +++++++-------
 src/openvpn/multi.c         | 78 ++++++++++++++++++++++++++-----------
 src/openvpn/plugin.c        |  3 ++
 3 files changed, 73 insertions(+), 37 deletions(-)

Comments

Gert Doering July 19, 2020, 11:30 p.m. UTC | #1 
Acked-by: Gert Doering <gert@greenie.muc.de>

Stared-at-code, server torture test, all succeeded.

Grammar change from Richard included (in the "v1 api" comment).

I have modified the commit message a bit to make it more clear that 
this is about CLIENT_CONNECT and CLIENT_CONNECT_V2, not plugin_func_v1()
vs. plugin_func_v2().


Testing this with my new "client connect tester" plugin, I discovered
two things:

 - one, if the plugin wants to use the "file based status signalling"
   API via $ENV{client_connect_deferred_file}, the file MUST be created
   (with content "2") before returning DEFER - otherwise the caller
   will treat the combination of "no CLIENT_CONNECT_DEFER handler and
   no file" as "success" and continue.  

   This can not be changed without deeper knowlege whether a DEFER 
   handler exists or not (possibly worth having a deeper look).


 - second, we read the "option file" ($ENV{client_connect_config_file}) 
   multiple times on every PUSH_REQUEST - it works, but this is a latent
   race condition, reading a "not yet fully written" file.  So we only
   should do this on "final SUCCESS".

   Here's a log with a plugin that sleeps 15 seconds - added some 
   msg() to see what ret/file_ret is at which point

2020-07-20 11:24:19 us=679280 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1 (enter): ret=3, deferred=0
2020-07-20 11:24:19 us=679456 PLUGIN sample-cc: OPENVPN_PLUGIN_CLIENT_CONNECT
2020-07-20 11:24:19 us=681322 PLUGIN sample-cc: in async/deferred handler, sleep(15)
2020-07-20 11:24:19 us=682060 cron2-freebsd-tc-amd64/194.97.140.21:63588 PLUGIN_CALL: POST /root/t_server/tun-udp-p2mp-112-mask/sample-client-connect.so/PLUGIN_CLIENT_CONNECT status=2
2020-07-20 11:24:19 us=682132 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2, file_ret=2
2020-07-20 11:24:19 us=682155 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2
2020-07-20 11:24:20 us=753014 cron2-freebsd-tc-amd64/194.97.140.21:63588 PUSH: Received control message: 'PUSH_REQUEST'
2020-07-20 11:24:20 us=753070 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1 (enter): ret=3, deferred=1
2020-07-20 11:24:20 us=753105 cron2-freebsd-tc-amd64/194.97.140.21:63588 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_7a30544bd345b76b6b2ea66939408119.tmp
2020-07-20 11:24:20 us=753180 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=1, file_ret=2
2020-07-20 11:24:20 us=753203 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2
2020-07-20 11:24:20 us=753278 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1 (enter): ret=3, deferred=1
2020-07-20 11:24:20 us=753312 cron2-freebsd-tc-amd64/194.97.140.21:63588 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_7a30544bd345b76b6b2ea66939408119.tmp
2020-07-20 11:24:20 us=753365 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=1, file_ret=2
2020-07-20 11:24:20 us=753386 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2
2020-07-20 11:24:21 us=879455 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1 (enter): ret=3, deferred=1
2020-07-20 11:24:21 us=879546 cron2-freebsd-tc-amd64/194.97.140.21:63588 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_7a30544bd345b76b6b2ea66939408119.tmp
2020-07-20 11:24:21 us=879618 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=1, file_ret=2
2020-07-20 11:24:21 us=879636 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2
2020-07-20 11:24:25 us=832648 cron2-freebsd-tc-amd64/194.97.140.21:63588 PUSH: Received control message: 'PUSH_REQUEST'
2020-07-20 11:24:25 us=832705 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1 (enter): ret=3, deferred=1
2020-07-20 11:24:25 us=832742 cron2-freebsd-tc-amd64/194.97.140.21:63588 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_7a30544bd345b76b6b2ea66939408119.tmp
2020-07-20 11:24:25 us=832835 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=1, file_ret=2
2020-07-20 11:24:25 us=832853 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2
2020-07-20 11:24:25 us=832920 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1 (enter): ret=3, deferred=1
2020-07-20 11:24:25 us=832955 cron2-freebsd-tc-amd64/194.97.140.21:63588 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_7a30544bd345b76b6b2ea66939408119.tmp
2020-07-20 11:24:25 us=833006 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=1, file_ret=2
2020-07-20 11:24:25 us=833027 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2
2020-07-20 11:24:30 us=865604 cron2-freebsd-tc-amd64/194.97.140.21:63588 PUSH: Received control message: 'PUSH_REQUEST'
2020-07-20 11:24:30 us=865667 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1 (enter): ret=3, deferred=1
2020-07-20 11:24:30 us=865710 cron2-freebsd-tc-amd64/194.97.140.21:63588 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_7a30544bd345b76b6b2ea66939408119.tmp
2020-07-20 11:24:30 us=865802 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=1, file_ret=2
2020-07-20 11:24:30 us=865821 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2
2020-07-20 11:24:30 us=865894 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1 (enter): ret=3, deferred=1
2020-07-20 11:24:30 us=865923 cron2-freebsd-tc-amd64/194.97.140.21:63588 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_7a30544bd345b76b6b2ea66939408119.tmp
2020-07-20 11:24:30 us=865974 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=1, file_ret=2
2020-07-20 11:24:30 us=865990 cron2-freebsd-tc-amd64/194.97.140.21:63588 mcccp1: ret=2
2020-07-20 11:24:34 us=681701 PLUGIN sample-cc: env has UV_WANT_CC_DISABLE, reject
2020-07-20 11:24:34 us=681955 PLUGIN sample-cc: cc_handle_deferred_v1: done, signalling success
2020-07-20 11:24:34 us=682094 mcccp1 (enter): ret=3, deferred=1
2020-07-20 11:24:34 us=682280 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_7a30544bd345b76b6b2ea66939408119.tmp
2020-07-20 11:24:34 us=682549 mcccp1: ret=1, file_ret=1
2020-07-20 11:24:34 us=682705 mcccp1: ret=1
2020-07-20 11:24:34 us=682972 PLUGIN sample-cc: OPENVPN_PLUGIN_CLIENT_CONNECT_V2
2020-07-20 11:24:34 us=683080 PLUGIN_CALL: POST /root/t_server/tun-udp-p2mp-112-mask/sample-client-connect.so/PLUGIN_CLIENT_CONNECT status=0
2020-07-20 11:24:34 us=690210 OPTIONS IMPORT: reading client specific options from: /tmp/openvpn_cc_2938f1331badc1d130fbe9454149b280.tmp
2020-07-20 11:24:34 us=690325 MULTI: client has been rejected due to 'disable' directive
2020-07-20 11:24:34 us=690364 PLUGIN sample-cc: OPENVPN_PLUGIN_CLIENT_DISCONNECT

(there is more weirdness here, because the "disable" is sent by CLIENT_CONNECT,
but only noticed after CLIENT_CONNECT_V2 is called...)


Your patch has been applied to the master branch.

commit 3d2af1568ccb5b64446fb77218193e6f2ad6e995
Author: Fabian Knittel
Date:   Sun Jul 19 19:34:35 2020 +0200

     client-connect: Add deferred support to the client-connect plugin v1 handler

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


--
kind regards,

Gert Doering
Gert Doering July 27, 2020, 10:21 p.m. UTC | #2 
Hi,

On Mon, Jul 20, 2020 at 11:30:55AM +0200, Gert Doering wrote:
> Testing this with my new "client connect tester" plugin, I discovered
> two things:
[..]
>  - second, we read the "option file" ($ENV{client_connect_config_file}) 
>    multiple times on every PUSH_REQUEST - it works, but this is a latent
>    race condition, reading a "not yet fully written" file.  So we only
>    should do this on "final SUCCESS".

This has been addressed and fixed in commit 08f3c1cab73.

JFTR :-)

gert
diff mbox series

Patch

diff --git a/include/openvpn-plugin.h.in b/include/openvpn-plugin.h.in
index 103844f7..99aa1678 100644
--- a/include/openvpn-plugin.h.in
+++ b/include/openvpn-plugin.h.in
@@ -116,20 +116,21 @@  extern "C" {
  * FUNC: openvpn_plugin_client_destructor_v1 (top-level "generic" client)
  * FUNC: openvpn_plugin_close_v1
  */
-#define OPENVPN_PLUGIN_UP                    0
-#define OPENVPN_PLUGIN_DOWN                  1
-#define OPENVPN_PLUGIN_ROUTE_UP              2
-#define OPENVPN_PLUGIN_IPCHANGE              3
-#define OPENVPN_PLUGIN_TLS_VERIFY            4
-#define OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY 5
-#define OPENVPN_PLUGIN_CLIENT_CONNECT        6
-#define OPENVPN_PLUGIN_CLIENT_DISCONNECT     7
-#define OPENVPN_PLUGIN_LEARN_ADDRESS         8
-#define OPENVPN_PLUGIN_CLIENT_CONNECT_V2     9
-#define OPENVPN_PLUGIN_TLS_FINAL             10
-#define OPENVPN_PLUGIN_ENABLE_PF             11
-#define OPENVPN_PLUGIN_ROUTE_PREDOWN         12
-#define OPENVPN_PLUGIN_N                     13
+#define OPENVPN_PLUGIN_UP                        0
+#define OPENVPN_PLUGIN_DOWN                      1
+#define OPENVPN_PLUGIN_ROUTE_UP                  2
+#define OPENVPN_PLUGIN_IPCHANGE                  3
+#define OPENVPN_PLUGIN_TLS_VERIFY                4
+#define OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY     5
+#define OPENVPN_PLUGIN_CLIENT_CONNECT            6
+#define OPENVPN_PLUGIN_CLIENT_DISCONNECT         7
+#define OPENVPN_PLUGIN_LEARN_ADDRESS             8
+#define OPENVPN_PLUGIN_CLIENT_CONNECT_V2         9
+#define OPENVPN_PLUGIN_TLS_FINAL                10
+#define OPENVPN_PLUGIN_ENABLE_PF                11
+#define OPENVPN_PLUGIN_ROUTE_PREDOWN            12
+#define OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER     13
+#define OPENVPN_PLUGIN_N                        14
 
 /*
  * Build a mask out of a set of plug-in types.
diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 3b73ffde..c66a4cea 100644
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -2022,53 +2022,85 @@  multi_client_connect_call_plugin_v1(struct multi_context *m,
                                     bool deferred,
                                     unsigned int *option_types_found)
 {
-    if (deferred)
-    {
-        return CC_RET_FAILED;
-    }
     enum client_connect_return ret = CC_RET_SKIPPED;
 #ifdef ENABLE_PLUGIN
     ASSERT(m);
     ASSERT(mi);
     ASSERT(option_types_found);
+    struct client_connect_defer_state *ccs = &(mi->client_connect_defer_state);
 
     /* deprecated callback, use a file for passing back return info */
     if (plugin_defined(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT))
     {
         struct argv argv = argv_new();
-        struct gc_arena gc = gc_new();
-        const char *dc_file =
-            platform_create_temp_file(mi->context.options.tmp_dir, "cc", &gc);
+        int call;
 
-        if (!dc_file)
+        if (!deferred)
         {
-            ret = CC_RET_FAILED;
-            goto cleanup;
+            call = OPENVPN_PLUGIN_CLIENT_CONNECT;
+            if (!ccs_gen_config_file(mi)
+                || !ccs_gen_deferred_ret_file(mi))
+            {
+                ret = CC_RET_FAILED;
+                goto cleanup;
+            }
+        }
+        else
+        {
+            call = OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER;
+            /* the initial call should have created these files */
+            ASSERT(ccs->config_file);
+            ASSERT(ccs->deferred_ret_file);
         }
 
-        argv_printf(&argv, "%s", dc_file);
-        if (plugin_call(mi->context.plugins, OPENVPN_PLUGIN_CLIENT_CONNECT,
-                        &argv, NULL, mi->context.c2.es)
-            != OPENVPN_PLUGIN_FUNC_SUCCESS)
+        argv_printf(&argv, "%s", ccs->config_file);
+        int plug_ret = plugin_call(mi->context.plugins, call,
+                                   &argv, NULL, mi->context.c2.es);
+        if (plug_ret == OPENVPN_PLUGIN_FUNC_SUCCESS)
         {
-            msg(M_WARN, "WARNING: client-connect plugin call failed");
-            ret = CC_RET_FAILED;
+            multi_client_connect_post(m, mi, ccs->config_file,
+                                      option_types_found);
+            ret = CC_RET_SUCCEEDED;
+        }
+        else if (plug_ret == OPENVPN_PLUGIN_FUNC_DEFERRED)
+        {
+            ret = CC_RET_DEFERRED;
+            /**
+             * Contrary to the plugin v2 API, we do not demand a working
+             * deferred plugin as all return can be handled by the files
+             * and plugin_call return success if a plugin is not defined
+             */
         }
         else
         {
-            multi_client_connect_post(m, mi, dc_file, option_types_found);
-            ret = CC_RET_SUCCEEDED;
+            msg(M_WARN, "WARNING: client-connect plugin call failed");
+            ret = CC_RET_FAILED;
         }
 
-        if (!platform_unlink(dc_file))
+
+        /**
+         * plugin api v1 client connect async feature has both plugin and
+         * file return status, so in case that the file has a code that
+         * demands override, we override our return code
+         */
+        int file_ret = ccs_test_deferred_ret_file(mi);
+
+        if (file_ret == CC_RET_FAILED)
         {
-            msg(D_MULTI_ERRORS, "MULTI: problem deleting temporary file: %s",
-                dc_file);
+            ret = CC_RET_FAILED;
+        }
+        else if (ret == CC_RET_SUCCEEDED && file_ret == CC_RET_DEFERRED)
+        {
+            ret = CC_RET_DEFERRED;
         }
-
 cleanup:
         argv_free(&argv);
-        gc_free(&gc);
+
+        if (ret != CC_RET_DEFERRED)
+        {
+            ccs_delete_config_file(mi);
+            ccs_delete_deferred_ret_file(mi);
+        }
     }
 #endif /* ifdef ENABLE_PLUGIN */
     return ret;
diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index 4de1d6b7..9a6fa3cb 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -104,6 +104,9 @@  plugin_type_name(const int type)
         case OPENVPN_PLUGIN_CLIENT_CONNECT_V2:
             return "PLUGIN_CLIENT_CONNECT";
 
+        case OPENVPN_PLUGIN_CLIENT_CONNECT_DEFER:
+            return "PLUGIN_CLIENT_CONNECT_DEFER";
+
         case OPENVPN_PLUGIN_CLIENT_DISCONNECT:
             return "PLUGIN_CLIENT_DISCONNECT";