| Message ID | d75b57da-d1ca-d4ca-41c1-f9af06ec54bb@sparklabs.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [Openvpn-devel] Support client reason from auth plugin | expand |
Hi, On Fri, Apr 20, 2018 at 03:20:26PM +1000, Eric Thorpe wrote: > This patch allows for a client reason to be returned from an auth plugin > and sent to the connecting client on an auth fail. This change is > backwards compatible with existing plugins and hasn't caused issues with > existing plugins like the included pam plugin in our testing. The main > purpose of this change is to support dynamic challenge/response from > plugins, currently this is only possible from the management interface. I find this highly interesting, so "feature ACK". No idea about the code though... David and Selva understand plugins :-) gert
Hi On Fri, Apr 20, 2018 at 4:00 AM, Gert Doering <gert@greenie.muc.de> wrote: > Hi, > > On Fri, Apr 20, 2018 at 03:20:26PM +1000, Eric Thorpe wrote: >> This patch allows for a client reason to be returned from an auth plugin >> and sent to the connecting client on an auth fail. This change is >> backwards compatible with existing plugins and hasn't caused issues with >> existing plugins like the included pam plugin in our testing. The main >> purpose of this change is to support dynamic challenge/response from >> plugins, currently this is only possible from the management interface. > > I find this highly interesting, so "feature ACK". No idea about the > code though... David and Selva understand plugins :-) Just to say "me too" for the highly interesting part. Its in my TODO, hopefully I will get to it soon unless David provides a review in the mean time. Selva ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot
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
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); + } + } + } + + 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) {