| Message ID | 20210407154951.13330-1-arne@rfc2549.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [Openvpn-devel,v6] Implement deferred auth for scripts | expand |
Hi, On 07/04/2021 17:49, Arne Schwabe wrote: > This patch also refactors the if condition that checks the result of > the authentication since that has become quite unreadable. It renames > s1/s2 and extracts some parts of the condition into individual variables > to make the condition better understandle > > Patch v2: add refactoring of the if condition > Patch v4: fix documentation not mentioning method as 2nd line > Patch v5: fix deferred auth used by both plugin and script not working > Patch v6: Add missing async inotify for script deferred auth > > Signed-off-by: Arne Schwabe <arne@rfc2549.org> Very nice feature! deferred auth made easy! Thanks Arne :) > --- > Changes.rst | 10 +++ > doc/man-sections/script-options.rst | 14 +++- > src/openvpn/multi.c | 6 ++ > src/openvpn/ssl.c | 1 + > src/openvpn/ssl_common.h | 1 + > src/openvpn/ssl_verify.c | 105 ++++++++++++++++++++-------- > 6 files changed, 106 insertions(+), 31 deletions(-) > > diff --git a/Changes.rst b/Changes.rst > index 457dfc07e..9185b55f7 100644 > --- a/Changes.rst > +++ b/Changes.rst > @@ -30,6 +30,16 @@ TLS mode with self-signed certificates > become optional. This allows for small OpenVPN setups without setting up > a PKI with Easy-RSA or similar software. > > +Deferred auth support for scripts > + The ``--auth-user-pass-verify`` script supports now deferred authentication. > + > +Pending auth support for plugins and scripts > + Both auth plugin and script can now signal pending authentication to > + the client when using deferred authentication. The new ``client-crresponse`` > + script option and ``OPENVPN_PLUGIN_CLIENT_CRRESPONSE`` plugin function can > + be used to parse a client response to a ``CR_TEXT`` two factor challenge. > + > + See ``sample/sample-scripts/totpauth.py`` for an example. > > Deprecated features > ------------------- > diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst > index 03b3dd77b..22990f4f4 100644 > --- a/doc/man-sections/script-options.rst > +++ b/doc/man-sections/script-options.rst > @@ -90,7 +90,19 @@ SCRIPT HOOKS > > The script should examine the username and password, returning a success > exit code (:code:`0`) if the client's authentication request is to be > - accepted, or a failure code (:code:`1`) to reject the client. > + accepted, a failure code (:code:`1`) to reject the client, or a that typ0: "or a that" -> "or that" ? > + the authentication is deferred (:code:`2`). If the authentication is > + deferred, the script must fork/start a background or another non-blocking > + operation to continue the authentication in the background. When finshing > + the authentication, a :code:`1` or :code:`0` must be written to the > + file specified by the :code:`auth_control_file`. > + > + When deferred authentication is in use, the script can also request > + pending authentication by writing to the file specified by the > + :code:`auth_pending_file`. The first line must be the timeout in > + seconds, the required method on the second line (e.g. crtext) and > + third line must be the EXTRA as documented in the > + ``client-pending-auth`` section of `doc/management.txt`. > > This directive is designed to enable a plugin-style interface for > extending OpenVPN's authentication capabilities. > diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c > index 7c9500f3e..e5e466631 100644 > --- a/src/openvpn/multi.c > +++ b/src/openvpn/multi.c > @@ -2990,6 +2990,12 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns > add_inotify_file_watch(m, mi, m->top.c2.inotify_fd, > ks->plugin_auth.auth_control_file); > } > + if (ks && ks->script_auth.auth_control_file && was_unauthenticated > + && (ks->authenticated == KS_AUTH_DEFERRED)) > + { > + add_inotify_file_watch(m, mi, m->top.c2.inotify_fd, > + ks->script_auth.auth_control_file); > + } > #endif > > if (!IS_SIG(&mi->context)) > diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c > index d8662d000..b27a78b3d 100644 > --- a/src/openvpn/ssl.c > +++ b/src/openvpn/ssl.c > @@ -994,6 +994,7 @@ key_state_free(struct key_state *ks, bool clear) > packet_id_free(&ks->crypto_options.packet_id); > > key_state_rm_auth_control_files(&ks->plugin_auth); > + key_state_rm_auth_control_files(&ks->script_auth); > > if (clear) > { > diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h > index b2fa44e56..acb815955 100644 > --- a/src/openvpn/ssl_common.h > +++ b/src/openvpn/ssl_common.h > @@ -222,6 +222,7 @@ struct key_state > time_t acf_last_mod; > > struct auth_deferred_status plugin_auth; > + struct auth_deferred_status script_auth; > }; > > /** Control channel wrapping (--tls-auth/--tls-crypt) context */ > diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c > index 7608155cd..7c02d46ce 100644 > --- a/src/openvpn/ssl_verify.c > +++ b/src/openvpn/ssl_verify.c > @@ -1102,20 +1102,25 @@ tls_authentication_status(struct tls_multi *multi, const int latency) > } > else > { > - unsigned int s1 = ACF_DISABLED; > - unsigned int s2 = ACF_DISABLED; > - s1 = key_state_test_auth_control_file(&ks->plugin_auth); > + unsigned int auth_plugin = ACF_DISABLED; > + unsigned int auth_script = ACF_DISABLED; > + unsigned int auth_man = ACF_DISABLED; > + auth_plugin = key_state_test_auth_control_file(&ks->plugin_auth); > + auth_script = key_state_test_auth_control_file(&ks->script_auth); > #ifdef ENABLE_MANAGEMENT > - s2 = man_def_auth_test(ks); > + auth_man = man_def_auth_test(ks); > #endif > - ASSERT(s1 < 4 && s2 < 4); > + ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4); > > - if (s1 == ACF_FAILED || s2 == ACF_FAILED) > + if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED > + || auth_man == ACF_FAILED) indentation is off by one space. > { > ks->authenticated = KS_AUTH_FALSE; > failed_auth = true; > } > - else if (s1 == ACF_UNDEFINED || s2 == ACF_UNDEFINED) > + else if (auth_plugin == ACF_UNDEFINED > + || auth_script == ACF_UNDEFINED > + || auth_man == ACF_UNDEFINED) > { > if (now < ks->auth_deferred_expire) > { > @@ -1124,7 +1129,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency) > } > else > { > - /* s1 and s2 are either ACF_DISABLED or ACF_SUCCEDED */ > + /* auth_plugin and auth_man are either ACF_DISABLED or ACF_SUCCEDED */ > success = true; > ks->authenticated = KS_AUTH_TRUE; > } > @@ -1204,14 +1209,15 @@ tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, con > /* > * Verify the user name and password using a script > */ > -static bool > +static int > verify_user_pass_script(struct tls_session *session, struct tls_multi *multi, > const struct user_pass *up) > { > struct gc_arena gc = gc_new(); > struct argv argv = argv_new(); > const char *tmp_file = ""; > - bool ret = false; > + int retval = OPENVPN_PLUGIN_FUNC_ERROR; > + struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ > > /* Set environmental variables prior to calling script */ > setenv_str(session->opt->es, "script_type", "user-pass-verify"); > @@ -1239,25 +1245,58 @@ verify_user_pass_script(struct tls_session *session, struct tls_multi *multi, > /* pass temp file name to script */ > argv_printf_cat(&argv, "%s", tmp_file); > } > - else > - { > - msg(D_TLS_ERRORS, "TLS Auth Error: could not create write " > - "username/password to temp file"); > - } > } > else > { > + setenv_str(session->opt->es, "username", up->username); > setenv_str(session->opt->es, "password", up->password); > } > > + /* generate filename for deferred auth control file */ > + if (!key_state_gen_auth_control_files(&ks->script_auth, session->opt)) > + { > + msg(D_TLS_ERRORS, "TLS Auth Error (%s): " > + "could not create deferred auth control file", __func__); > + return OPENVPN_PLUGIN_FUNC_ERROR; > + } > + > /* call command */ > - ret = openvpn_run_script(&argv, session->opt->es, 0, > - "--auth-user-pass-verify"); > + int script_ret = openvpn_run_script(&argv, session->opt->es, S_EXITCODE, > + "--auth-user-pass-verify"); > + switch (script_ret) > + { > + case 0: > + retval = OPENVPN_PLUGIN_FUNC_SUCCESS; > + break; > + case 2: > + retval = OPENVPN_PLUGIN_FUNC_DEFERRED; > + break; > + default: > + retval = OPENVPN_PLUGIN_FUNC_ERROR; > + break; > + } > + if (retval == OPENVPN_PLUGIN_FUNC_DEFERRED) > + { > + /* Check if we the plugin has written the pending auth control > + * file and send the pending auth to the client */ > + if(!key_state_check_auth_pending_file(&ks->script_auth, > + multi)) > + { > + retval = OPENVPN_PLUGIN_FUNC_ERROR; > + key_state_rm_auth_control_files(&ks->script_auth); > + } > > + } > + else > + { > + /* purge auth control filename (and file itself) for non-deferred returns */ > + key_state_rm_auth_control_files(&ks->script_auth); > + } > if (!session->opt->auth_user_pass_verify_script_via_file) > { > setenv_del(session->opt->es, "password"); > } > + > done: > if (tmp_file && strlen(tmp_file) > 0) > { > @@ -1266,7 +1305,7 @@ done: > > argv_free(&argv); > gc_free(&gc); > - return ret; > + return retval; > } > > /* > @@ -1387,8 +1426,6 @@ void > verify_user_pass(struct user_pass *up, struct tls_multi *multi, > struct tls_session *session) > { > - int s1 = OPENVPN_PLUGIN_FUNC_SUCCESS; > - bool s2 = true; > struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ > > #ifdef ENABLE_MANAGEMENT > @@ -1448,30 +1485,32 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, > } > } > > + int plugin_status = OPENVPN_PLUGIN_FUNC_SUCCESS; > + int script_status = OPENVPN_PLUGIN_FUNC_SUCCESS; > /* Set the environment variables used by all auth variants */ > if (!set_verify_user_pass_env(up, multi, session)) > { > skip_auth = true; > - s1 = OPENVPN_PLUGIN_FUNC_ERROR; > + plugin_status = OPENVPN_PLUGIN_FUNC_ERROR; > } > > /* call plugin(s) and/or script */ > if (!skip_auth) > { > #ifdef ENABLE_MANAGEMENT > - if (man_def_auth==KMDA_DEF) > + if (man_def_auth == KMDA_DEF) > { > man_def_auth = verify_user_pass_management(session, multi, up); > } > #endif > if (plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY)) > { > - s1 = verify_user_pass_plugin(session, multi, up); > + plugin_status = verify_user_pass_plugin(session, multi, up); > } > > if (session->opt->auth_user_pass_verify_script) > { > - s2 = verify_user_pass_script(session, multi, up); > + script_status = verify_user_pass_script(session, multi, up); > } > } > > @@ -1482,19 +1521,25 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, > msg(D_TLS_ERRORS, > "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters", > TLS_USERNAME_LEN); > - s1 = OPENVPN_PLUGIN_FUNC_ERROR; > + plugin_status = OPENVPN_PLUGIN_FUNC_ERROR; > + script_status = OPENVPN_PLUGIN_FUNC_ERROR; > } > /* auth succeeded? */ > - if ((s1 == OPENVPN_PLUGIN_FUNC_SUCCESS > - || s1 == OPENVPN_PLUGIN_FUNC_DEFERRED > - ) && s2 > + bool plugin_ok = plugin_status == OPENVPN_PLUGIN_FUNC_SUCCESS > + || plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED; > + > + bool script_ok = script_status == OPENVPN_PLUGIN_FUNC_SUCCESS > + || script_status == OPENVPN_PLUGIN_FUNC_DEFERRED; both assignment have indentation off. > + > + if (script_ok && plugin_ok && tls_lock_username(multi, up->username) > #ifdef ENABLE_MANAGEMENT > && man_def_auth != KMDA_ERROR > #endif > - && tls_lock_username(multi, up->username)) > + ) > { > ks->authenticated = KS_AUTH_TRUE; > - if (s1 == OPENVPN_PLUGIN_FUNC_DEFERRED) > + if (plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED > + || script_status == OPENVPN_PLUGIN_FUNC_DEFERRED) > { > ks->authenticated = KS_AUTH_DEFERRED; > } > I leave the code analysis to Gert who has already spent some time on it. However, I performed a couple of tests and I couldn't spot a anything wrong. Deferred authentication just worked as expected, as if I was using the plugin. Just with one 10th of the complexity. Tested-by: Antonio Quartulli <antonio@openvpn.net> My compile-test rig on GitLab is also happy. Regards,
Acked-by: Gert Doering <gert@greenie.muc.de> I've taken Antonio's ACK on previous versions, plus add my own. v6 differs only minimally from v5 (in the addition of the inotify for the script auth control file), so ACKs and test results of v5 are valid. I have subjected this to the server side cabinet of horrors, with a mixture of plugin and scripts active, including a new test instance that does deferred and synchronous "auth-user-pass-verify script". It does not break any of the existing tests, especially not the "plugin fails after delay while script auth succeeds right away", which broke v3 or v4, and the new functionality also works. Staring at the code, it also looks fine now. Rewriting all these "s1" and "s2" is really a good thing, though it makes the code take a bit more space. But it was really hard to understand before. As discussed on IRC, I've reworded the comment in ssl_verify.c /* auth_plugin and auth_man are either ACF_DISABLED or ACF_SUCCEEDED */ to include "auth_script" and a bit of extra clarification. Further, as discussed on IRC, we noticed that this lost the hunk in ssl_verify.c / verify_user_pass_script() that does error handling in the case of "platform_create_temp_file() failed". This is an unlikely case, but if you have a script that expects a file argument and just silently and occasionally get "no argument", this is bad - the old error handling was not right either (it reported the error, but still proceeded). To get on with this patchset, we've decided to merge v6 anyway, and clean this in a followup patch. Your patch has been applied to the master branch. commit 28e6103096ae8ba0a4498da1625a61150a50e6c1 Author: Arne Schwabe Date: Wed Apr 7 17:49:51 2021 +0200 Implement deferred auth for scripts Signed-off-by: Arne Schwabe <arne@rfc2549.org> Tested-by: Antonio Quartulli <antonio@openvpn.net> Acked-by: Antonio Quartulli <antonio@openvpn.net> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20210407154951.13330-1-arne@rfc2549.org> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg22072.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
diff --git a/Changes.rst b/Changes.rst index 457dfc07e..9185b55f7 100644 --- a/Changes.rst +++ b/Changes.rst @@ -30,6 +30,16 @@ TLS mode with self-signed certificates become optional. This allows for small OpenVPN setups without setting up a PKI with Easy-RSA or similar software. +Deferred auth support for scripts + The ``--auth-user-pass-verify`` script supports now deferred authentication. + +Pending auth support for plugins and scripts + Both auth plugin and script can now signal pending authentication to + the client when using deferred authentication. The new ``client-crresponse`` + script option and ``OPENVPN_PLUGIN_CLIENT_CRRESPONSE`` plugin function can + be used to parse a client response to a ``CR_TEXT`` two factor challenge. + + See ``sample/sample-scripts/totpauth.py`` for an example. Deprecated features ------------------- diff --git a/doc/man-sections/script-options.rst b/doc/man-sections/script-options.rst index 03b3dd77b..22990f4f4 100644 --- a/doc/man-sections/script-options.rst +++ b/doc/man-sections/script-options.rst @@ -90,7 +90,19 @@ SCRIPT HOOKS The script should examine the username and password, returning a success exit code (:code:`0`) if the client's authentication request is to be - accepted, or a failure code (:code:`1`) to reject the client. + accepted, a failure code (:code:`1`) to reject the client, or a that + the authentication is deferred (:code:`2`). If the authentication is + deferred, the script must fork/start a background or another non-blocking + operation to continue the authentication in the background. When finshing + the authentication, a :code:`1` or :code:`0` must be written to the + file specified by the :code:`auth_control_file`. + + When deferred authentication is in use, the script can also request + pending authentication by writing to the file specified by the + :code:`auth_pending_file`. The first line must be the timeout in + seconds, the required method on the second line (e.g. crtext) and + third line must be the EXTRA as documented in the + ``client-pending-auth`` section of `doc/management.txt`. This directive is designed to enable a plugin-style interface for extending OpenVPN's authentication capabilities. diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c index 7c9500f3e..e5e466631 100644 --- a/src/openvpn/multi.c +++ b/src/openvpn/multi.c @@ -2990,6 +2990,12 @@ multi_process_post(struct multi_context *m, struct multi_instance *mi, const uns add_inotify_file_watch(m, mi, m->top.c2.inotify_fd, ks->plugin_auth.auth_control_file); } + if (ks && ks->script_auth.auth_control_file && was_unauthenticated + && (ks->authenticated == KS_AUTH_DEFERRED)) + { + add_inotify_file_watch(m, mi, m->top.c2.inotify_fd, + ks->script_auth.auth_control_file); + } #endif if (!IS_SIG(&mi->context)) diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c index d8662d000..b27a78b3d 100644 --- a/src/openvpn/ssl.c +++ b/src/openvpn/ssl.c @@ -994,6 +994,7 @@ key_state_free(struct key_state *ks, bool clear) packet_id_free(&ks->crypto_options.packet_id); key_state_rm_auth_control_files(&ks->plugin_auth); + key_state_rm_auth_control_files(&ks->script_auth); if (clear) { diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h index b2fa44e56..acb815955 100644 --- a/src/openvpn/ssl_common.h +++ b/src/openvpn/ssl_common.h @@ -222,6 +222,7 @@ struct key_state time_t acf_last_mod; struct auth_deferred_status plugin_auth; + struct auth_deferred_status script_auth; }; /** Control channel wrapping (--tls-auth/--tls-crypt) context */ diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c index 7608155cd..7c02d46ce 100644 --- a/src/openvpn/ssl_verify.c +++ b/src/openvpn/ssl_verify.c @@ -1102,20 +1102,25 @@ tls_authentication_status(struct tls_multi *multi, const int latency) } else { - unsigned int s1 = ACF_DISABLED; - unsigned int s2 = ACF_DISABLED; - s1 = key_state_test_auth_control_file(&ks->plugin_auth); + unsigned int auth_plugin = ACF_DISABLED; + unsigned int auth_script = ACF_DISABLED; + unsigned int auth_man = ACF_DISABLED; + auth_plugin = key_state_test_auth_control_file(&ks->plugin_auth); + auth_script = key_state_test_auth_control_file(&ks->script_auth); #ifdef ENABLE_MANAGEMENT - s2 = man_def_auth_test(ks); + auth_man = man_def_auth_test(ks); #endif - ASSERT(s1 < 4 && s2 < 4); + ASSERT(auth_plugin < 4 && auth_script < 4 && auth_man < 4); - if (s1 == ACF_FAILED || s2 == ACF_FAILED) + if (auth_plugin == ACF_FAILED || auth_script == ACF_FAILED + || auth_man == ACF_FAILED) { ks->authenticated = KS_AUTH_FALSE; failed_auth = true; } - else if (s1 == ACF_UNDEFINED || s2 == ACF_UNDEFINED) + else if (auth_plugin == ACF_UNDEFINED + || auth_script == ACF_UNDEFINED + || auth_man == ACF_UNDEFINED) { if (now < ks->auth_deferred_expire) { @@ -1124,7 +1129,7 @@ tls_authentication_status(struct tls_multi *multi, const int latency) } else { - /* s1 and s2 are either ACF_DISABLED or ACF_SUCCEDED */ + /* auth_plugin and auth_man are either ACF_DISABLED or ACF_SUCCEDED */ success = true; ks->authenticated = KS_AUTH_TRUE; } @@ -1204,14 +1209,15 @@ tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, con /* * Verify the user name and password using a script */ -static bool +static int verify_user_pass_script(struct tls_session *session, struct tls_multi *multi, const struct user_pass *up) { struct gc_arena gc = gc_new(); struct argv argv = argv_new(); const char *tmp_file = ""; - bool ret = false; + int retval = OPENVPN_PLUGIN_FUNC_ERROR; + struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ /* Set environmental variables prior to calling script */ setenv_str(session->opt->es, "script_type", "user-pass-verify"); @@ -1239,25 +1245,58 @@ verify_user_pass_script(struct tls_session *session, struct tls_multi *multi, /* pass temp file name to script */ argv_printf_cat(&argv, "%s", tmp_file); } - else - { - msg(D_TLS_ERRORS, "TLS Auth Error: could not create write " - "username/password to temp file"); - } } else { + setenv_str(session->opt->es, "username", up->username); setenv_str(session->opt->es, "password", up->password); } + /* generate filename for deferred auth control file */ + if (!key_state_gen_auth_control_files(&ks->script_auth, session->opt)) + { + msg(D_TLS_ERRORS, "TLS Auth Error (%s): " + "could not create deferred auth control file", __func__); + return OPENVPN_PLUGIN_FUNC_ERROR; + } + /* call command */ - ret = openvpn_run_script(&argv, session->opt->es, 0, - "--auth-user-pass-verify"); + int script_ret = openvpn_run_script(&argv, session->opt->es, S_EXITCODE, + "--auth-user-pass-verify"); + switch (script_ret) + { + case 0: + retval = OPENVPN_PLUGIN_FUNC_SUCCESS; + break; + case 2: + retval = OPENVPN_PLUGIN_FUNC_DEFERRED; + break; + default: + retval = OPENVPN_PLUGIN_FUNC_ERROR; + break; + } + if (retval == OPENVPN_PLUGIN_FUNC_DEFERRED) + { + /* Check if we the plugin has written the pending auth control + * file and send the pending auth to the client */ + if(!key_state_check_auth_pending_file(&ks->script_auth, + multi)) + { + retval = OPENVPN_PLUGIN_FUNC_ERROR; + key_state_rm_auth_control_files(&ks->script_auth); + } + } + else + { + /* purge auth control filename (and file itself) for non-deferred returns */ + key_state_rm_auth_control_files(&ks->script_auth); + } if (!session->opt->auth_user_pass_verify_script_via_file) { setenv_del(session->opt->es, "password"); } + done: if (tmp_file && strlen(tmp_file) > 0) { @@ -1266,7 +1305,7 @@ done: argv_free(&argv); gc_free(&gc); - return ret; + return retval; } /* @@ -1387,8 +1426,6 @@ void verify_user_pass(struct user_pass *up, struct tls_multi *multi, struct tls_session *session) { - int s1 = OPENVPN_PLUGIN_FUNC_SUCCESS; - bool s2 = true; struct key_state *ks = &session->key[KS_PRIMARY]; /* primary key */ #ifdef ENABLE_MANAGEMENT @@ -1448,30 +1485,32 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, } } + int plugin_status = OPENVPN_PLUGIN_FUNC_SUCCESS; + int script_status = OPENVPN_PLUGIN_FUNC_SUCCESS; /* Set the environment variables used by all auth variants */ if (!set_verify_user_pass_env(up, multi, session)) { skip_auth = true; - s1 = OPENVPN_PLUGIN_FUNC_ERROR; + plugin_status = OPENVPN_PLUGIN_FUNC_ERROR; } /* call plugin(s) and/or script */ if (!skip_auth) { #ifdef ENABLE_MANAGEMENT - if (man_def_auth==KMDA_DEF) + if (man_def_auth == KMDA_DEF) { man_def_auth = verify_user_pass_management(session, multi, up); } #endif if (plugin_defined(session->opt->plugins, OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY)) { - s1 = verify_user_pass_plugin(session, multi, up); + plugin_status = verify_user_pass_plugin(session, multi, up); } if (session->opt->auth_user_pass_verify_script) { - s2 = verify_user_pass_script(session, multi, up); + script_status = verify_user_pass_script(session, multi, up); } } @@ -1482,19 +1521,25 @@ verify_user_pass(struct user_pass *up, struct tls_multi *multi, msg(D_TLS_ERRORS, "TLS Auth Error: --username-as-common name specified and username is longer than the maximum permitted Common Name length of %d characters", TLS_USERNAME_LEN); - s1 = OPENVPN_PLUGIN_FUNC_ERROR; + plugin_status = OPENVPN_PLUGIN_FUNC_ERROR; + script_status = OPENVPN_PLUGIN_FUNC_ERROR; } /* auth succeeded? */ - if ((s1 == OPENVPN_PLUGIN_FUNC_SUCCESS - || s1 == OPENVPN_PLUGIN_FUNC_DEFERRED - ) && s2 + bool plugin_ok = plugin_status == OPENVPN_PLUGIN_FUNC_SUCCESS + || plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED; + + bool script_ok = script_status == OPENVPN_PLUGIN_FUNC_SUCCESS + || script_status == OPENVPN_PLUGIN_FUNC_DEFERRED; + + if (script_ok && plugin_ok && tls_lock_username(multi, up->username) #ifdef ENABLE_MANAGEMENT && man_def_auth != KMDA_ERROR #endif - && tls_lock_username(multi, up->username)) + ) { ks->authenticated = KS_AUTH_TRUE; - if (s1 == OPENVPN_PLUGIN_FUNC_DEFERRED) + if (plugin_status == OPENVPN_PLUGIN_FUNC_DEFERRED + || script_status == OPENVPN_PLUGIN_FUNC_DEFERRED) { ks->authenticated = KS_AUTH_DEFERRED; }
This patch also refactors the if condition that checks the result of the authentication since that has become quite unreadable. It renames s1/s2 and extracts some parts of the condition into individual variables to make the condition better understandle Patch v2: add refactoring of the if condition Patch v4: fix documentation not mentioning method as 2nd line Patch v5: fix deferred auth used by both plugin and script not working Patch v6: Add missing async inotify for script deferred auth Signed-off-by: Arne Schwabe <arne@rfc2549.org> --- Changes.rst | 10 +++ doc/man-sections/script-options.rst | 14 +++- src/openvpn/multi.c | 6 ++ src/openvpn/ssl.c | 1 + src/openvpn/ssl_common.h | 1 + src/openvpn/ssl_verify.c | 105 ++++++++++++++++++++-------- 6 files changed, 106 insertions(+), 31 deletions(-)