Skip to contentSkip to conversation

Support client reason from auth plugin

PATCH In review Eric Thorpe 3 participants 4 messages Apr 20, 2018 – Apr 26, 2018

lore
Share & tools
lore

Eric Thorpe

author
#0 lore raw

PATCH 1/1 Support client reason from auth plugin

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

Signature · 6 lines
--
Eric Thorpe
SparkLabs Developer
https://www.sparklabs.com
https://twitter.com/sparklabs
[email protected]...

From 0970f16c27c2b90df04ad148bea84a9bfd3ad4a0 Mon Sep 17 00:00:00 2001
From: ethorpe <[email protected]>
Date: Wed, 18 Apr 2018 19:05:54 +1000
Subject: [PATCH] Modify verify_user_pass_plugin to support returning a
client_reason when

auth is rejected. This is backwards compatible with existing plugins.

---
 src/openvpn/ssl_verify.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)
src/openvpn/ssl_verify.c +22 -3
index 25395b2..6266fb3 100644
@@ -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)
     {
Signature · 1 line
2.7.4
#1 lore raw

Hi,

Quoted 6 lines · Eric Thorpe

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

Signature · 6 lines
"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...
#2 lore raw

Hi

Quoted 10 lines · Gert Doering

Hi,

Quoted 6 lines · Eric Thorpe

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.

Quoted 2 lines

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

#3 lore raw
In reply to Eric Thorpe

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:

Quoted 30 lines
src/openvpn/ssl_verify.c +6 -2
index 25395b2..6266fb3 100644
@@ -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);
+

src/openvpn/ssl_verify.c verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up, · line 1215

Quoted 10 lines
src/openvpn/ssl_verify.c +10 -0
+        /* 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.

Quoted 9 lines
src/openvpn/ssl_verify.c +5 -0
+                }
+            }
+        }
+
+        plugin_return_free(&pr);
 
 #ifdef PLUGIN_DEF_AUTH
         /* purge auth control filename (and file itself) for non-deferred returns */

src/openvpn/ssl_verify.c verify_user_pass(struct user_pass *up, struct tls_multi *multi, · line 1403

Quoted 8 lines
src/openvpn/ssl_verify.c +1 -1
@@ -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