[Openvpn-devel] Support client reason from auth plugin

Message ID d75b57da-d1ca-d4ca-41c1-f9af06ec54bb@sparklabs.com
State New
Headers show
Series
  • [Openvpn-devel] Support client reason from auth plugin
Related show

Commit Message

Eric Thorpe April 20, 2018, 5:20 a.m.
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

Gert Doering April 20, 2018, 8 a.m. | #1
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
Selva Nair April 20, 2018, 2:12 p.m. | #2
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
Selva Nair April 26, 2018, 3:47 a.m. | #3
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

Patch

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)
     {