diff mbox series

[Openvpn-devel,v3,4/4] Allow scripts and plugins to set a custom AUTH_FAILED message

Message ID 20220824140848.88013-1-arne@rfc2549.org
State Accepted
Headers show
Series None | expand

Commit Message

Arne Schwabe Aug. 24, 2022, 4:08 a.m. UTC 
This is currently only possible when using the management interface
and the client-deny functionality.

Patch v3: add missing gc_free

Signed-off-by: Arne Schwabe <arne@rfc2549.org>
---
 src/openvpn/ssl_common.h |  1 +
 src/openvpn/ssl_verify.c | 74 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 73 insertions(+), 2 deletions(-)

Comments

Heiko Hund Sept. 14, 2022, 5:13 a.m. UTC | #1 
On Mittwoch, 24. August 2022 16:08:48 CEST Arne Schwabe wrote:
> This is currently only possible when using the management interface
> and the client-deny functionality.
> 
> Patch v3: add missing gc_free

Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
Gert Doering Sept. 17, 2022, 7:04 a.m. UTC | #2 
My server test rig has a "--auth-user-pass-verify" script that already
does client-controlled success/failure returns (setenv UV...), and
this has now learned to return client-specific messages if 
$auth_failed_reason_file is set...

2022-09-17 17:44:53 AUTH: Received control message: AUTH_FAILED,you stink

.. works.

For the plugin case, I've tried to test this with my existing 
"--client-connect magic hooks plugin", but it seems this functionality
is not exported to client-connect (so, CC plugin fails can only return
basic AUTH_FAIL).  So I've hacked this into plugin-auth-pam, which is
used in a different server instance, and that one also works:

2022-09-17 18:07:47 AUTH: Received control message: AUTH_FAILED,my plugin does not like you

All the other tests (client+server) still works as well, no files
are left around in /tmp/, etc.


Staring at the code took me a bit, because of the two-fold way you
did the checks - half the locations call check_for_client_reason(),
while tls_authentication_status() prefers to do it "inline" (... leading
to the gc_free()... ;-) ).  Could this be unified, or am I overlooking
something?  Anyway, decided to not stop progress because of this.


Your patch has been applied to the master branch.

commit 8893fe49a4c593387d469ccc4a73ec0714f69315
Author: Arne Schwabe
Date:   Wed Aug 24 16:08:48 2022 +0200

     Allow scripts and plugins to set a custom AUTH_FAILED message

     Signed-off-by: Arne Schwabe <arne@rfc2549.org>
     Acked-by: Heiko Hund <heiko@ist.eigentlich.net>
     Message-Id: <20220824140848.88013-1-arne@rfc2549.org>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg25099.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
diff mbox series

Patch

diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index bf3ac67a5..f1cade2ef 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -155,6 +155,7 @@  struct auth_deferred_status
 {
     char *auth_control_file;
     char *auth_pending_file;
+    char *auth_failed_reason_file;
     unsigned int auth_control_status;
 };
 
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index b4608517f..76cb9f19b 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -989,6 +989,12 @@  key_state_rm_auth_control_files(struct auth_deferred_status *ads)
         free(ads->auth_control_file);
         ads->auth_control_file = NULL;
     }
+    if (ads->auth_failed_reason_file)
+    {
+        platform_unlink(ads->auth_failed_reason_file);
+        free(ads->auth_failed_reason_file);
+        ads->auth_failed_reason_file = NULL;
+    }
     key_state_rm_auth_pending_file(ads);
 }
 
@@ -1007,19 +1013,47 @@  key_state_gen_auth_control_files(struct auth_deferred_status *ads,
     key_state_rm_auth_control_files(ads);
     const char *acf = platform_create_temp_file(opt->tmp_dir, "acf", &gc);
     const char *apf = platform_create_temp_file(opt->tmp_dir, "apf", &gc);
+    const char *afr = platform_create_temp_file(opt->tmp_dir, "afr", &gc);
 
     if (acf && apf)
     {
         ads->auth_control_file = string_alloc(acf, NULL);
         ads->auth_pending_file = string_alloc(apf, NULL);
+        ads->auth_failed_reason_file = string_alloc(afr, NULL);
+
         setenv_str(opt->es, "auth_control_file", ads->auth_control_file);
         setenv_str(opt->es, "auth_pending_file", ads->auth_pending_file);
+        setenv_str(opt->es, "auth_failed_reason_file", ads->auth_failed_reason_file);
     }
 
     gc_free(&gc);
     return (acf && apf);
 }
 
+/**
+ * Checks if the auth failed reason file has any content and if yes it will
+ * be returned as string allocated in gc to the caller.
+ */
+static char *
+key_state_check_auth_failed_message_file(const struct auth_deferred_status *ads,
+                                         struct tls_multi *multi,
+                                         struct gc_arena *gc)
+{
+    char *ret = NULL;
+    if (ads->auth_failed_reason_file)
+    {
+        struct buffer reason = buffer_read_from_file(ads->auth_failed_reason_file, gc);
+
+        if (BLEN(&reason))
+        {
+            ret = BSTR(&reason);
+        }
+
+    }
+    return ret;
+}
+
+
 /**
  * Checks the auth control status from a file. The function will try
  * to read and update the cached status if the status is still pending
@@ -1184,12 +1218,27 @@  tls_authentication_status(struct tls_multi *multi)
 #endif
     if (failed_auth)
     {
+        struct gc_arena gc = gc_new();
+        const struct key_state *ks = get_primary_key(multi);
+        const char *plugin_message = key_state_check_auth_failed_message_file(&ks->plugin_auth, multi, &gc);
+        const char *script_message = key_state_check_auth_failed_message_file(&ks->script_auth, multi, &gc);
+
+        if (plugin_message)
+        {
+            auth_set_client_reason(multi, plugin_message);
+        }
+        if (script_message)
+        {
+            auth_set_client_reason(multi, script_message);
+        }
+
         /* We have at least one session that failed authentication. There
          * might be still another session with valid keys.
          * Although our protocol allows keeping the VPN session alive
          * with the other session (and we actually did that in earlier
          * version, this behaviour is really strange from a user (admin)
          * experience */
+        gc_free(&gc);
         return TLS_AUTHENTICATION_FAILED;
     }
     else if (success)
@@ -1248,6 +1297,21 @@  tls_authenticate_key(struct tls_multi *multi, const unsigned int mda_key_id, con
  * this is the place to start.
  *************************************************************************** */
 
+/**
+ * Check if the script/plugin left a message in the auth failed message
+ * file and relay it to the user */
+static void
+check_for_client_reason(struct tls_multi *multi,
+                        struct auth_deferred_status *status)
+{
+    struct gc_arena gc = gc_new();
+    const char *msg = key_state_check_auth_failed_message_file(status, multi, &gc);
+    if (msg)
+    {
+        auth_set_client_reason(multi, msg);
+    }
+    gc_free(&gc);
+}
 /*
  * Verify the user name and password using a script
  */
@@ -1316,6 +1380,7 @@  verify_user_pass_script(struct tls_session *session, struct tls_multi *multi,
             break;
 
         default:
+            check_for_client_reason(multi, &ks->script_auth);
             retval = OPENVPN_PLUGIN_FUNC_ERROR;
             break;
     }
@@ -1450,10 +1515,15 @@  verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,
         if (!key_state_check_auth_pending_file(&ks->plugin_auth, multi))
         {
             retval = OPENVPN_PLUGIN_FUNC_ERROR;
-            key_state_rm_auth_control_files(&ks->plugin_auth);
         }
     }
-    else
+
+    if (retval == OPENVPN_PLUGIN_FUNC_ERROR)
+    {
+        check_for_client_reason(multi, &ks->plugin_auth);
+    }
+
+    if (retval != OPENVPN_PLUGIN_FUNC_DEFERRED)
     {
         /* purge auth control filename (and file itself) for non-deferred returns */
         key_state_rm_auth_control_files(&ks->plugin_auth);