[Openvpn-devel] Support client reason from auth plugin
Commit Message
Hi All,
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.
Example usage for this change can be found in a new plugin here modified
from the included PAM plugin -
https://github.com/thesparklabs/openvpn-two-factor-extensions/tree/master/yubikey-u2f-pam-plugin
Regards,
Eric
Comments
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
@@ -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)
{