Hi
Thanks for the patch.This feature (and a similar support for plugins)
is something very nice to have
But this implementation is inadequate.
The main problem is that multi->client_reason is sent back to the client only
during the initial auth not during reauth (renegotiations). So this will work
in the first round and will fail during renegotiation. The client will not get
the "AUTH_FAILED,reason" message and stall until ping-restart.
This is the same issue that affects gen-auth-token expiry as well although
there the reason text is not CRV1:foo but SESSION:foo
Management-def-auth with dynamic CR works because of its support
for calling send_auth_failed() during reauth.
See management_client_auth() in multi.c (~line 3270)
That is not to say that dynamic challenge need to be raised during every
reauth --- in most cases not. But the plugin or script decides how to handle
that. The daemon should be ready to send back the reason to the client
whenever the plugin asks it to.
And, a minor comment below:
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 25395b2..6266fb3 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -1167,7 +1167,7 @@ done:
> * Verify the username and password using a plugin
> */
> static int
> -verify_user_pass_plugin(struct tls_session *session, const struct user_pass
> *up, const char *raw_username)
> +verify_user_pass_plugin(struct tls_session *session, struct tls_multi
> *multi, const struct user_pass *up, const char *raw_username)
> {
> int retval = OPENVPN_PLUGIN_FUNC_ERROR;
> #ifdef PLUGIN_DEF_AUTH
> @@ -1177,6 +1177,9 @@ verify_user_pass_plugin(struct tls_session *session,
> const struct user_pass *up,
> /* Is username defined? */
> if ((session->opt->ssl_flags & SSLF_AUTH_USER_PASS_OPTIONAL) ||
> strlen(up->username))
> {
> + struct plugin_return pr, prfetch;
> + plugin_return_init(&pr);
> +
> /* set username/password in private env space */
> setenv_str(session->opt->es, "username", (raw_username ?
> raw_username : up->username));
> setenv_str(session->opt->es, "password", up->password);
> @@ -1198,7 +1201,23 @@ verify_user_pass_plugin(struct tls_session *session,
> const struct user_pass *up,
> #endif
>
> /* call command */
> - retval = plugin_call(session->opt->plugins,
> OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, NULL, session->opt->es);
> + retval = plugin_call(session->opt->plugins,
> OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, &pr, session->opt->es);
> +
> + /* Fetch client reason */
> + plugin_return_get_column(&pr, &prfetch, "client_reason");
> + if (plugin_return_defined(&prfetch))
> + {
> + int i;
> + for (i = 0; i < prfetch.n; ++i)
> + {
> + if (prfetch.list[i] && prfetch.list[i]->value)
> + {
> + man_def_auth_set_client_reason(multi,
> prfetch.list[i]->value);
man_def_auth_set_client_reason() is defined only when
MANAGEMENT_DEF_AUTH is defined. So this will fail to build in
some cases.
> + }
> + }
> + }
> +
> + plugin_return_free(&pr);
>
> #ifdef PLUGIN_DEF_AUTH
> /* purge auth control filename (and file itself) for non-deferred
> returns */
> @@ -1378,7 +1397,7 @@ verify_user_pass(struct user_pass *up, struct
> tls_multi *multi,
> #endif
> if (plugin_defined(session->opt->plugins,
> OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY))
> {
> - s1 = verify_user_pass_plugin(session, up, raw_username);
> + s1 = verify_user_pass_plugin(session, multi, up, raw_username);
> }
> if (session->opt->auth_user_pass_verify_script)
> {
Selva
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Openvpn-devel mailing list
Openvpn-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
