| Message ID | 20200719173436.16431-4-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Delegated to: | Gert Doering |
| Headers | show |
| Series | [Openvpn-devel,v8,1/5] Remove CAS_PARTIAL state | expand |
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
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 --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";