[Openvpn-devel] Enable deferred auth for multiple plugins (RFC).

Message ID 20220310115718.11188-1-gert@greenie.muc.de
State New
Headers show
Series [Openvpn-devel] Enable deferred auth for multiple plugins (RFC). | expand

Commit Message

Gert Doering March 10, 2022, 11:57 a.m. UTC
Without this patch, OpenVPN behaviour if more than one plugin wants
to do deferred user/password authentication not well-defined, as
there is just one set of auth control files and a single plugin state.

This patch changes "key state -> plugin_auth" from a single struct
to an array of MAX_PLUGINS elements that have per-plugin auth-control
files and auth status.

All direct access is changed to iterating over this array.

The actual plugin calls are no longer done with the "do them all"
function plugin_call() (or plugin_call_ssl()) but plugin.c/plugin.h
is changed to expose the "call one" function plugin_call_item(), and
verify_user_pass_plugin() calls this for each loaded plugin,
keeping track of "overall" state.

key_state_test_plugin_auth() is introduced to do the
"key_state_test_auth_control_file()" test for all plugins, and
return "FAIL if one fails, PENDING if one is pending, SUCCESS
if one was pending and succeeded now".

This was tested with the "auth-multi" test plugin, with 5-7 plugins
loaded (some deferred, some direct) and "some of them failing" or
"all succeeding".  Testing included "will it leak files if multiple
deferred plugins are ongoing, and one of the earlier ones rejects
authentication".

This patch is not suitable for production use:
 - it's full of debug output
 - it will break compilation without ENABLE_PLUGINS
 - it stands to argue whether plugin_call_item() should be exposed,
   or key_state_test_plugin_auth() might be moved to plugin.c instead
   (with a null function if ENABLE_PLUGINS is not defined)

Signed-off-by: Gert Doering <gert@greenie.muc.de>
---
 src/openvpn/plugin.c     |   2 +-
 src/openvpn/plugin.h     |   9 +++
 src/openvpn/ssl.c        |   5 +-
 src/openvpn/ssl_common.h |   4 +-
 src/openvpn/ssl_verify.c | 127 ++++++++++++++++++++++++++++++++-------
 5 files changed, 123 insertions(+), 24 deletions(-)

Comments

Pete Nelson March 10, 2022, 1:57 p.m. UTC | #1
One of the behaviors that brought this to light was a user who had an LDAP
(non-deferred) plugin followed by a Duo MFA (deferred) plugin.  He noted
that, even if the LDAP call returned failure, the Duo plugin was still
called.  That would generate a push notification to his phone even though
the authentication was already destined to fail due to LDAP.  It looks like
this patch returns failure when the first plugin fails, so would resolve
that scenario by not calling the Duo plugin at all if LDAP returned failure.

Is the source for the "auth-multi" plugin you tested with available
anywhere?  I could not find it, and had wanted something like it for my own
testing.

On Thu, Mar 10, 2022 at 12:18 PM Gert Doering <gert@greenie.muc.de> wrote:

> Without this patch, OpenVPN behaviour if more than one plugin wants
> to do deferred user/password authentication not well-defined, as
> there is just one set of auth control files and a single plugin state.
>
> This patch changes "key state -> plugin_auth" from a single struct
> to an array of MAX_PLUGINS elements that have per-plugin auth-control
> files and auth status.
>
> All direct access is changed to iterating over this array.
>
> The actual plugin calls are no longer done with the "do them all"
> function plugin_call() (or plugin_call_ssl()) but plugin.c/plugin.h
> is changed to expose the "call one" function plugin_call_item(), and
> verify_user_pass_plugin() calls this for each loaded plugin,
> keeping track of "overall" state.
>
> key_state_test_plugin_auth() is introduced to do the
> "key_state_test_auth_control_file()" test for all plugins, and
> return "FAIL if one fails, PENDING if one is pending, SUCCESS
> if one was pending and succeeded now".
>
> This was tested with the "auth-multi" test plugin, with 5-7 plugins
> loaded (some deferred, some direct) and "some of them failing" or
> "all succeeding".  Testing included "will it leak files if multiple
> deferred plugins are ongoing, and one of the earlier ones rejects
> authentication".
>
> This patch is not suitable for production use:
>  - it's full of debug output
>  - it will break compilation without ENABLE_PLUGINS
>  - it stands to argue whether plugin_call_item() should be exposed,
>    or key_state_test_plugin_auth() might be moved to plugin.c instead
>    (with a null function if ENABLE_PLUGINS is not defined)
>
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  src/openvpn/plugin.c     |   2 +-
>  src/openvpn/plugin.h     |   9 +++
>  src/openvpn/ssl.c        |   5 +-
>  src/openvpn/ssl_common.h |   4 +-
>  src/openvpn/ssl_verify.c | 127 ++++++++++++++++++++++++++++++++-------
>  5 files changed, 123 insertions(+), 24 deletions(-)
>
> diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
> index e3a89293..74b57033 100644
> --- a/src/openvpn/plugin.c
> +++ b/src/openvpn/plugin.c
> @@ -518,7 +518,7 @@ plugin_open_item(struct plugin *p,
>      }
>  }
>
> -static int
> +int
>  plugin_call_item(const struct plugin *p,
>                   void *per_client_context,
>                   const int type,
> diff --git a/src/openvpn/plugin.h b/src/openvpn/plugin.h
> index c6fa206a..799a646e 100644
> --- a/src/openvpn/plugin.h
> +++ b/src/openvpn/plugin.h
> @@ -132,6 +132,15 @@ int plugin_call_ssl(const struct plugin_list *pl,
>                      int current_cert_depth,
>                      openvpn_x509_cert_t *current_cert
>                      );
> +int plugin_call_item(const struct plugin *p,
> +                     void *per_client_context,
> +                     const int type,
> +                     const struct argv *av,
> +                     struct openvpn_plugin_string_list **retlist,
> +                     const char **envp,
> +                     int certdepth,
> +                     openvpn_x509_cert_t *current_cert
> +                     );
>
>  void plugin_list_close(struct plugin_list *pl);
>
> diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
> index 14a943a7..ce6de9a0 100644
> --- a/src/openvpn/ssl.c
> +++ b/src/openvpn/ssl.c
> @@ -1036,7 +1036,10 @@ key_state_free(struct key_state *ks, bool clear)
>
>      packet_id_free(&ks->crypto_options.packet_id);
>
> -    key_state_rm_auth_control_files(&ks->plugin_auth);
> +    for (int i=0; i<MAX_PLUGINS; i++)
> +    {
> +        key_state_rm_auth_control_files(&ks->plugin_auth[i]);
> +    }
>      key_state_rm_auth_control_files(&ks->script_auth);
>
>      if (clear)
> diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
> index 8a077c74..0de3290a 100644
> --- a/src/openvpn/ssl_common.h
> +++ b/src/openvpn/ssl_common.h
> @@ -37,6 +37,8 @@
>
>  #include "ssl_backend.h"
>
> +#include "plugin.h"
> +
>  /* passwords */
>  #define UP_TYPE_AUTH        "Auth"
>  #define UP_TYPE_PRIVATE_KEY "Private Key"
> @@ -239,7 +241,7 @@ struct key_state
>  #endif
>      time_t acf_last_mod;
>
> -    struct auth_deferred_status plugin_auth;
> +    struct auth_deferred_status plugin_auth[MAX_PLUGINS];
>      struct auth_deferred_status script_auth;
>  };
>
> diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
> index 3b6b58fa..38c8a830 100644
> --- a/src/openvpn/ssl_verify.c
> +++ b/src/openvpn/ssl_verify.c
> @@ -1059,6 +1059,50 @@ key_state_test_auth_control_file(struct
> auth_deferred_status *ads, bool cached)
>      return ACF_DISABLED;
>  }
>
> +/**
> + * Checks the auth control status for all plugins.
> + *
> + * returns:
> + * - ACF_PENDING  if any plugin is still waiting for results.
> + * - ACF_SUCCEEDED if there were pending plugins AND all succeeded
> + * - ACF_FAILED   if any plugin fails
> + * - ACF_DISABLED if no plugin is waiting
> + *
> + * @param ads       array of deferred status control structures
> + * @param cached    Return only cached status
> + * @return          ACF_* as per enum
> + */
> +static enum auth_deferred_result
> +key_state_test_plugin_auth(struct auth_deferred_status *ads, bool cached)
> +{
> +    unsigned int ret = ACF_DISABLED;
> +
> +    for (int i=0;i<MAX_PLUGINS;i++)
> +    {
> +        unsigned int ret_one =
> key_state_test_auth_control_file(&(ads[i]), cached);
> +msg(M_INFO, "GERT(9a): kstpa: i=%d, ret_one=%d", i, ret_one);
> +
> +        /* if at least one plugin fails, we fail all */
> +        if ( ret_one == ACF_FAILED )
> +        {
> +            ret = ret_one;
> +            break;
> +        }
> +        /* if at least one plugin is still pending, we're still pending */
> +        if ( ret_one == ACF_PENDING )
> +        {
> +            ret = ret_one;
> +        }
> +        /* if no plugins are pending AND we have success, we succeed */
> +        if ( ret_one == ACF_SUCCEEDED && ret != ACF_PENDING )
> +        {
> +            ret = ret_one;
> +        }
> +    }
> +msg(M_INFO, "GERT(9b): kstpa: ret=%d", ret);
> +    return ret;
> +}
> +
>  /**
>   * This method takes a key_state and if updates the state
>   * of the key if it is deferred.
> @@ -1069,6 +1113,7 @@ key_state_test_auth_control_file(struct
> auth_deferred_status *ads, bool cached)
>  static void
>  update_key_auth_status(bool cached, struct key_state *ks)
>  {
> +msg(M_INFO, "GERT(10) update_key_auth_status authenticated=%d",
> ks->authenticated);
>      if (ks->authenticated == KS_AUTH_FALSE)
>      {
>          return;
> @@ -1078,7 +1123,7 @@ update_key_auth_status(bool cached, struct key_state
> *ks)
>          enum auth_deferred_result auth_plugin = ACF_DISABLED;
>          enum auth_deferred_result auth_script = ACF_DISABLED;
>          enum auth_deferred_result auth_man = ACF_DISABLED;
> -        auth_plugin = key_state_test_auth_control_file(&ks->plugin_auth,
> cached);
> +        auth_plugin = key_state_test_plugin_auth(ks->plugin_auth, cached);
>          auth_script = key_state_test_auth_control_file(&ks->script_auth,
> cached);
>  #ifdef ENABLE_MANAGEMENT
>          auth_man = man_def_auth_test(ks);
> @@ -1357,40 +1402,80 @@ static int
>  verify_user_pass_plugin(struct tls_session *session, struct tls_multi
> *multi,
>                          const struct user_pass *up)
>  {
> -    int retval = OPENVPN_PLUGIN_FUNC_ERROR;
> +    int retval = OPENVPN_PLUGIN_FUNC_SUCCESS;
>      struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key
> */
>
> +    struct gc_arena gc = gc_new();
> +
>      /* set password in private env space */
>      setenv_str(session->opt->es, "password", up->password);
>
> -    /* generate filename for deferred auth control file */
> -    if (!key_state_gen_auth_control_files(&ks->plugin_auth, session->opt))
> +    /* iterate over list of plugins here, because deferred auth needs
> +     * per-plugin auth control files
> +     */
> +    const struct plugin_list *pl = session->opt->plugins;
> +    int plugins_active = plugin_n(pl);
> +    ASSERT( plugins_active < SIZE(ks->plugin_auth) );
> +    msg(M_INFO, "GERT(1): plugins_active=%d, SIZE=%d", plugins_active,
> (int)SIZE(ks->plugin_auth) );
> +
> +    for (int i=0; i<plugins_active; i++)
>      {
> -        msg(D_TLS_ERRORS, "TLS Auth Error (%s): "
> -            "could not create deferred auth control file", __func__);
> -        return retval;
> -    }
> +        /* generate filename for deferred auth control file */
> +        if (!key_state_gen_auth_control_files(&ks->plugin_auth[i],
> session->opt))
> +        {
> +            msg(D_TLS_ERRORS, "TLS Auth Error (%s): "
> +                "could not create deferred auth control file", __func__);
> +            retval = OPENVPN_PLUGIN_FUNC_ERROR;
> +            break;
> +        }
>
> -    /* call command */
> -    retval = plugin_call(session->opt->plugins,
> OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, NULL, session->opt->es);
> +        msg(M_INFO, "GERT(4): plugin #%d, acf=%s acp=%s", i,
> +                ks->plugin_auth[i].auth_control_file,
> ks->plugin_auth[i].auth_pending_file);
>
> -    if (retval == OPENVPN_PLUGIN_FUNC_DEFERRED)
> -    {
> -        /* Check if the plugin has written the pending auth control
> -         * file and send the pending auth to the client */
> -        if(!key_state_check_auth_pending_file(&ks->plugin_auth, multi))
> +        /* call command */
> +        // retval = plugin_call_item(session->opt->plugins,
> OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, NULL, session->opt->es);
> +        //                                pl                    type
>                        av    pr    es   [crt -1, NULL]
> +
> +        const char **envp = make_env_array(session->opt->es, false, &gc);
> +        const int status = plugin_call_item(&pl->common->plugins[i],
> +
> pl->per_client.per_client_context[i],
> +
> OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY,
> +                                            NULL, NULL, envp, -1, NULL );
> +
> +        if (status == OPENVPN_PLUGIN_FUNC_DEFERRED)
> +        {
> +            /* Check if the plugin has written the pending auth control
> +             * file and send the pending auth to the client */
> +            if(!key_state_check_auth_pending_file(&(ks->plugin_auth[i]),
> multi))
> +            {
> +                retval = OPENVPN_PLUGIN_FUNC_ERROR;
> +                key_state_rm_auth_control_files(&(ks->plugin_auth[i]));
> +                break;
> +            }
> +            else
> +            {
> +                retval = OPENVPN_PLUGIN_FUNC_DEFERRED;
> +            }
> +        }
> +        else
> +        {
> +            /* purge auth control filename (and file itself) for
> non-deferred returns */
> +            key_state_rm_auth_control_files(&(ks->plugin_auth[i]));
> +            ks->plugin_auth[i].auth_control_status = ACF_DISABLED;
> +        }
> +
> +        /* if a single plugin fails, we fail all */
> +        if (status == OPENVPN_PLUGIN_FUNC_ERROR)
>          {
>              retval = OPENVPN_PLUGIN_FUNC_ERROR;
> -            key_state_rm_auth_control_files(&ks->plugin_auth);
> +            break;
>          }
> -    }
> -    else
> -    {
> -        /* purge auth control filename (and file itself) for non-deferred
> returns */
> -        key_state_rm_auth_control_files(&ks->plugin_auth);
> +
> +        msg(M_INFO, "GERT(3): plugin #%d status=%d -> retval=%d", i,
> status, retval );
>      }
>
>      setenv_del(session->opt->es, "password");
> +    gc_free(&gc);
>
>      return retval;
>  }
> --
> 2.34.1
>
>
>
> _______________________________________________
> Openvpn-devel mailing list
> Openvpn-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openvpn-devel
>
<div dir="ltr"><div>One of the behaviors that brought this to light was a user who had an LDAP (non-deferred) plugin followed by a Duo MFA (deferred) plugin.  He noted that, even if the LDAP call returned failure, the Duo plugin was still called.  That would generate a push notification to his phone even though the authentication was already destined to fail due to LDAP.  It looks like this patch returns failure when the first plugin fails, so would resolve that scenario by not calling the Duo plugin at all if LDAP returned failure.<br></div><div><br></div><div>Is the source for the &quot;auth-multi&quot; plugin you tested with available anywhere?  I could not find it, and had wanted something like it for my own testing.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 10, 2022 at 12:18 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Without this patch, OpenVPN behaviour if more than one plugin wants<br>
to do deferred user/password authentication not well-defined, as<br>
there is just one set of auth control files and a single plugin state.<br>
<br>
This patch changes &quot;key state -&gt; plugin_auth&quot; from a single struct<br>
to an array of MAX_PLUGINS elements that have per-plugin auth-control<br>
files and auth status.<br>
<br>
All direct access is changed to iterating over this array.<br>
<br>
The actual plugin calls are no longer done with the &quot;do them all&quot;<br>
function plugin_call() (or plugin_call_ssl()) but plugin.c/plugin.h<br>
is changed to expose the &quot;call one&quot; function plugin_call_item(), and<br>
verify_user_pass_plugin() calls this for each loaded plugin,<br>
keeping track of &quot;overall&quot; state.<br>
<br>
key_state_test_plugin_auth() is introduced to do the<br>
&quot;key_state_test_auth_control_file()&quot; test for all plugins, and<br>
return &quot;FAIL if one fails, PENDING if one is pending, SUCCESS<br>
if one was pending and succeeded now&quot;.<br>
<br>
This was tested with the &quot;auth-multi&quot; test plugin, with 5-7 plugins<br>
loaded (some deferred, some direct) and &quot;some of them failing&quot; or<br>
&quot;all succeeding&quot;.  Testing included &quot;will it leak files if multiple<br>
deferred plugins are ongoing, and one of the earlier ones rejects<br>
authentication&quot;.<br>
<br>
This patch is not suitable for production use:<br>
 - it&#39;s full of debug output<br>
 - it will break compilation without ENABLE_PLUGINS<br>
 - it stands to argue whether plugin_call_item() should be exposed,<br>
   or key_state_test_plugin_auth() might be moved to plugin.c instead<br>
   (with a null function if ENABLE_PLUGINS is not defined)<br>
<br>
Signed-off-by: Gert Doering &lt;<a href="mailto:gert@greenie.muc.de" target="_blank">gert@greenie.muc.de</a>&gt;<br>
---<br>
 src/openvpn/plugin.c     |   2 +-<br>
 src/openvpn/plugin.h     |   9 +++<br>
 src/openvpn/ssl.c        |   5 +-<br>
 src/openvpn/ssl_common.h |   4 +-<br>
 src/openvpn/ssl_verify.c | 127 ++++++++++++++++++++++++++++++++-------<br>
 5 files changed, 123 insertions(+), 24 deletions(-)<br>
<br>
diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c<br>
index e3a89293..74b57033 100644<br>
--- a/src/openvpn/plugin.c<br>
+++ b/src/openvpn/plugin.c<br>
@@ -518,7 +518,7 @@ plugin_open_item(struct plugin *p,<br>
     }<br>
 }<br>
<br>
-static int<br>
+int<br>
 plugin_call_item(const struct plugin *p,<br>
                  void *per_client_context,<br>
                  const int type,<br>
diff --git a/src/openvpn/plugin.h b/src/openvpn/plugin.h<br>
index c6fa206a..799a646e 100644<br>
--- a/src/openvpn/plugin.h<br>
+++ b/src/openvpn/plugin.h<br>
@@ -132,6 +132,15 @@ int plugin_call_ssl(const struct plugin_list *pl,<br>
                     int current_cert_depth,<br>
                     openvpn_x509_cert_t *current_cert<br>
                     );<br>
+int plugin_call_item(const struct plugin *p,<br>
+                     void *per_client_context,<br>
+                     const int type,<br>
+                     const struct argv *av,<br>
+                     struct openvpn_plugin_string_list **retlist,<br>
+                     const char **envp,<br>
+                     int certdepth,<br>
+                     openvpn_x509_cert_t *current_cert<br>
+                     );<br>
<br>
 void plugin_list_close(struct plugin_list *pl);<br>
<br>
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c<br>
index 14a943a7..ce6de9a0 100644<br>
--- a/src/openvpn/ssl.c<br>
+++ b/src/openvpn/ssl.c<br>
@@ -1036,7 +1036,10 @@ key_state_free(struct key_state *ks, bool clear)<br>
<br>
     packet_id_free(&amp;ks-&gt;crypto_options.packet_id);<br>
<br>
-    key_state_rm_auth_control_files(&amp;ks-&gt;plugin_auth);<br>
+    for (int i=0; i&lt;MAX_PLUGINS; i++)<br>
+    {<br>
+        key_state_rm_auth_control_files(&amp;ks-&gt;plugin_auth[i]);<br>
+    }<br>
     key_state_rm_auth_control_files(&amp;ks-&gt;script_auth);<br>
<br>
     if (clear)<br>
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h<br>
index 8a077c74..0de3290a 100644<br>
--- a/src/openvpn/ssl_common.h<br>
+++ b/src/openvpn/ssl_common.h<br>
@@ -37,6 +37,8 @@<br>
<br>
 #include &quot;ssl_backend.h&quot;<br>
<br>
+#include &quot;plugin.h&quot;<br>
+<br>
 /* passwords */<br>
 #define UP_TYPE_AUTH        &quot;Auth&quot;<br>
 #define UP_TYPE_PRIVATE_KEY &quot;Private Key&quot;<br>
@@ -239,7 +241,7 @@ struct key_state<br>
 #endif<br>
     time_t acf_last_mod;<br>
<br>
-    struct auth_deferred_status plugin_auth;<br>
+    struct auth_deferred_status plugin_auth[MAX_PLUGINS];<br>
     struct auth_deferred_status script_auth;<br>
 };<br>
<br>
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c<br>
index 3b6b58fa..38c8a830 100644<br>
--- a/src/openvpn/ssl_verify.c<br>
+++ b/src/openvpn/ssl_verify.c<br>
@@ -1059,6 +1059,50 @@ key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached)<br>
     return ACF_DISABLED;<br>
 }<br>
<br>
+/**<br>
+ * Checks the auth control status for all plugins.<br>
+ *<br>
+ * returns:<br>
+ * - ACF_PENDING  if any plugin is still waiting for results.<br>
+ * - ACF_SUCCEEDED if there were pending plugins AND all succeeded<br>
+ * - ACF_FAILED   if any plugin fails<br>
+ * - ACF_DISABLED if no plugin is waiting<br>
+ *<br>
+ * @param ads       array of deferred status control structures<br>
+ * @param cached    Return only cached status<br>
+ * @return          ACF_* as per enum<br>
+ */<br>
+static enum auth_deferred_result<br>
+key_state_test_plugin_auth(struct auth_deferred_status *ads, bool cached)<br>
+{<br>
+    unsigned int ret = ACF_DISABLED;<br>
+<br>
+    for (int i=0;i&lt;MAX_PLUGINS;i++)<br>
+    {<br>
+        unsigned int ret_one = key_state_test_auth_control_file(&amp;(ads[i]), cached);<br>
+msg(M_INFO, &quot;GERT(9a): kstpa: i=%d, ret_one=%d&quot;, i, ret_one);<br>
+<br>
+        /* if at least one plugin fails, we fail all */<br>
+        if ( ret_one == ACF_FAILED )<br>
+        {<br>
+            ret = ret_one;<br>
+            break;<br>
+        }<br>
+        /* if at least one plugin is still pending, we&#39;re still pending */<br>
+        if ( ret_one == ACF_PENDING )<br>
+        {<br>
+            ret = ret_one;<br>
+        }<br>
+        /* if no plugins are pending AND we have success, we succeed */<br>
+        if ( ret_one == ACF_SUCCEEDED &amp;&amp; ret != ACF_PENDING )<br>
+        {<br>
+            ret = ret_one;<br>
+        }<br>
+    }<br>
+msg(M_INFO, &quot;GERT(9b): kstpa: ret=%d&quot;, ret);<br>
+    return ret;<br>
+}<br>
+<br>
 /**<br>
  * This method takes a key_state and if updates the state<br>
  * of the key if it is deferred.<br>
@@ -1069,6 +1113,7 @@ key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached)<br>
 static void<br>
 update_key_auth_status(bool cached, struct key_state *ks)<br>
 {<br>
+msg(M_INFO, &quot;GERT(10) update_key_auth_status authenticated=%d&quot;, ks-&gt;authenticated);<br>
     if (ks-&gt;authenticated == KS_AUTH_FALSE)<br>
     {<br>
         return;<br>
@@ -1078,7 +1123,7 @@ update_key_auth_status(bool cached, struct key_state *ks)<br>
         enum auth_deferred_result auth_plugin = ACF_DISABLED;<br>
         enum auth_deferred_result auth_script = ACF_DISABLED;<br>
         enum auth_deferred_result auth_man = ACF_DISABLED;<br>
-        auth_plugin = key_state_test_auth_control_file(&amp;ks-&gt;plugin_auth, cached);<br>
+        auth_plugin = key_state_test_plugin_auth(ks-&gt;plugin_auth, cached);<br>
         auth_script = key_state_test_auth_control_file(&amp;ks-&gt;script_auth, cached);<br>
 #ifdef ENABLE_MANAGEMENT<br>
         auth_man = man_def_auth_test(ks);<br>
@@ -1357,40 +1402,80 @@ static int<br>
 verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,<br>
                         const struct user_pass *up)<br>
 {<br>
-    int retval = OPENVPN_PLUGIN_FUNC_ERROR;<br>
+    int retval = OPENVPN_PLUGIN_FUNC_SUCCESS;<br>
     struct key_state *ks = &amp;session-&gt;key[KS_PRIMARY];      /* primary key */<br>
<br>
+    struct gc_arena gc = gc_new();<br>
+<br>
     /* set password in private env space */<br>
     setenv_str(session-&gt;opt-&gt;es, &quot;password&quot;, up-&gt;password);<br>
<br>
-    /* generate filename for deferred auth control file */<br>
-    if (!key_state_gen_auth_control_files(&amp;ks-&gt;plugin_auth, session-&gt;opt))<br>
+    /* iterate over list of plugins here, because deferred auth needs<br>
+     * per-plugin auth control files<br>
+     */<br>
+    const struct plugin_list *pl = session-&gt;opt-&gt;plugins;<br>
+    int plugins_active = plugin_n(pl);<br>
+    ASSERT( plugins_active &lt; SIZE(ks-&gt;plugin_auth) );<br>
+    msg(M_INFO, &quot;GERT(1): plugins_active=%d, SIZE=%d&quot;, plugins_active, (int)SIZE(ks-&gt;plugin_auth) );<br>
+<br>
+    for (int i=0; i&lt;plugins_active; i++)<br>
     {<br>
-        msg(D_TLS_ERRORS, &quot;TLS Auth Error (%s): &quot;<br>
-            &quot;could not create deferred auth control file&quot;, __func__);<br>
-        return retval;<br>
-    }<br>
+        /* generate filename for deferred auth control file */<br>
+        if (!key_state_gen_auth_control_files(&amp;ks-&gt;plugin_auth[i], session-&gt;opt))<br>
+        {<br>
+            msg(D_TLS_ERRORS, &quot;TLS Auth Error (%s): &quot;<br>
+                &quot;could not create deferred auth control file&quot;, __func__);<br>
+            retval = OPENVPN_PLUGIN_FUNC_ERROR;<br>
+            break;<br>
+        }<br>
<br>
-    /* call command */<br>
-    retval = plugin_call(session-&gt;opt-&gt;plugins, OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, NULL, session-&gt;opt-&gt;es);<br>
+        msg(M_INFO, &quot;GERT(4): plugin #%d, acf=%s acp=%s&quot;, i, <br>
+                ks-&gt;plugin_auth[i].auth_control_file, ks-&gt;plugin_auth[i].auth_pending_file);<br>
<br>
-    if (retval == OPENVPN_PLUGIN_FUNC_DEFERRED)<br>
-    {<br>
-        /* Check if the plugin has written the pending auth control<br>
-         * file and send the pending auth to the client */<br>
-        if(!key_state_check_auth_pending_file(&amp;ks-&gt;plugin_auth, multi))<br>
+        /* call command */<br>
+        // retval = plugin_call_item(session-&gt;opt-&gt;plugins, OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, NULL, session-&gt;opt-&gt;es);<br>
+        //                                pl                    type                             av    pr    es   [crt -1, NULL]<br>
+<br>
+        const char **envp = make_env_array(session-&gt;opt-&gt;es, false, &amp;gc);<br>
+        const int status = plugin_call_item(&amp;pl-&gt;common-&gt;plugins[i],<br>
+                                            pl-&gt;per_client.per_client_context[i],<br>
+                                            OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY,<br>
+                                            NULL, NULL, envp, -1, NULL );<br>
+<br>
+        if (status == OPENVPN_PLUGIN_FUNC_DEFERRED)<br>
+        {<br>
+            /* Check if the plugin has written the pending auth control<br>
+             * file and send the pending auth to the client */<br>
+            if(!key_state_check_auth_pending_file(&amp;(ks-&gt;plugin_auth[i]), multi))<br>
+            {<br>
+                retval = OPENVPN_PLUGIN_FUNC_ERROR;<br>
+                key_state_rm_auth_control_files(&amp;(ks-&gt;plugin_auth[i]));<br>
+                break;<br>
+            }<br>
+            else<br>
+            {<br>
+                retval = OPENVPN_PLUGIN_FUNC_DEFERRED;<br>
+            }<br>
+        }<br>
+        else<br>
+        {<br>
+            /* purge auth control filename (and file itself) for non-deferred returns */<br>
+            key_state_rm_auth_control_files(&amp;(ks-&gt;plugin_auth[i]));<br>
+            ks-&gt;plugin_auth[i].auth_control_status = ACF_DISABLED;<br>
+        }<br>
+<br>
+        /* if a single plugin fails, we fail all */<br>
+        if (status == OPENVPN_PLUGIN_FUNC_ERROR)<br>
         {<br>
             retval = OPENVPN_PLUGIN_FUNC_ERROR;<br>
-            key_state_rm_auth_control_files(&amp;ks-&gt;plugin_auth);<br>
+            break;<br>
         }<br>
-    }<br>
-    else<br>
-    {<br>
-        /* purge auth control filename (and file itself) for non-deferred returns */<br>
-        key_state_rm_auth_control_files(&amp;ks-&gt;plugin_auth);<br>
+<br>
+        msg(M_INFO, &quot;GERT(3): plugin #%d status=%d -&gt; retval=%d&quot;, i, status, retval );<br>
     }<br>
<br>
     setenv_del(session-&gt;opt-&gt;es, &quot;password&quot;);<br>
+    gc_free(&amp;gc);<br>
<br>
     return retval;<br>
 }<br>
-- <br>
2.34.1<br>
<br>
<br>
<br>
_______________________________________________<br>
Openvpn-devel mailing list<br>
<a href="mailto:Openvpn-devel@lists.sourceforge.net" target="_blank">Openvpn-devel@lists.sourceforge.net</a><br>
<a href="https://lists.sourceforge.net/lists/listinfo/openvpn-devel" rel="noreferrer" target="_blank">https://lists.sourceforge.net/lists/listinfo/openvpn-devel</a><br>
</blockquote></div>
Gert Doering March 10, 2022, 3:13 p.m. UTC | #2
Hi,

On Thu, Mar 10, 2022 at 01:57:01PM +0000, Pete Nelson wrote:
> One of the behaviors that brought this to light was a user who had an LDAP
> (non-deferred) plugin followed by a Duo MFA (deferred) plugin.  He noted
> that, even if the LDAP call returned failure, the Duo plugin was still
> called.  That would generate a push notification to his phone even though
> the authentication was already destined to fail due to LDAP.  It looks like
> this patch returns failure when the first plugin fails, so would resolve
> that scenario by not calling the Duo plugin at all if LDAP returned failure.

Yes, your report triggered this work.  I think I misunderstood the 
original scenario (I thought it involved two plugins both going 
"deferred"), but this patch introduces a "short circuit" approach 
to plugin authentication where anything that fails will immediately
abort all other plugin calls.  So, it will fix that scenario(*).

Now, if you have two deferred plugins, OpenVPN wouldn't know that
"the deferred LDAP will fail in 0.1s", so would still fire up the
DUO plugin.  I thought about sequentializing them (the moment the
first plugin goes "deferred", wait for that plugin to finish, and
only then call the next one) but have not found a good way to do that
in our current code.  Might be worth having another look.


(*) for that scenario ("first plugin is direct, fails, then do not
call second plugin at all") the possible fix is much easier, though -
in plugin.c, plugin_call_ssl(), just leave the inner for() loop
whenever "error" becomes true.  So line 810 would change from

        for (i = 0; i < n; ++i)

into

        for (i = 0; i < n && !error; ++i)

... the existing behaviour is "run all plugins, always, and report
if any of them failed".  That change would it make "abort on the first
failure, do not run the rest".  Which sounds like it makes sense for
authentication related, but maybe not so much for housekeeping 
(client-connect etc.)...  to be discussed further :-)


> Is the source for the "auth-multi" plugin you tested with available
> anywhere?  I could not find it, and had wanted something like it for my own
> testing.

It is part of the patchset that David has prepared for the "two plugins 
going deferred is not well defined" problem.  It will hit the list
"real soon now", but I'll send it to you unicast.  It comes with detailed
instructions how to run multiple instances that have their own "log id"
and defined direct/deferred success/failure behaviour.

gert
Pete Nelson March 10, 2022, 6:32 p.m. UTC | #3
Hi Gert.

On Thu, Mar 10, 2022 at 3:13 PM Gert Doering <gert@greenie.muc.de> wrote:

> Yes, your report triggered this work.  I think I misunderstood the
> original scenario (I thought it involved two plugins both going
> "deferred"), but this patch introduces a "short circuit" approach
> to plugin authentication where anything that fails will immediately
> abort all other plugin calls.  So, it will fix that scenario(*).
>

You didn't misunderstand; they were always two separate issues.  I just
noticed the multiple-deferred issue when I was reviewing the code for why
it wasn't failing on the first fail.  The multiple-deferred issue I brought
to the sec list, and I submitted https://patchwork.openvpn.net/patch/2071/
as a possible fix for the other.  It's likely moot now with your other
updates.

Thanks for the multi-auth plugin source.  I'll try to make the community
IRC on the 12th; it's just real early for me.
--
Pete
<div dir="ltr"><div>Hi Gert.<br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 10, 2022 at 3:13 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Yes, your report triggered this work.  I think I misunderstood the <br>
original scenario (I thought it involved two plugins both going <br>
&quot;deferred&quot;), but this patch introduces a &quot;short circuit&quot; approach <br>
to plugin authentication where anything that fails will immediately<br>
abort all other plugin calls.  So, it will fix that scenario(*).<br></blockquote><div><br></div><div>You didn&#39;t misunderstand; they were always two separate issues.  I just noticed the multiple-deferred issue when I was reviewing the code for why it wasn&#39;t failing on the first fail.  The multiple-deferred issue I brought to the sec list, and I submitted <a href="https://patchwork.openvpn.net/patch/2071/">https://patchwork.openvpn.net/patch/2071/</a> as a possible fix for the other.  It&#39;s likely moot now with your other updates.<br></div><div> </div><div>Thanks for the multi-auth plugin source.  I&#39;ll try to make the community IRC on the 12th; it&#39;s just real early for me.<br></div><div>--</div><div>Pete<br></div></div></div>
Pete Nelson March 11, 2022, 4:29 p.m. UTC | #4
On Thu, Mar 10, 2022 at 12:18 PM Gert Doering <gert@greenie.muc.de> wrote:

> The actual plugin calls are no longer done with the "do them all"
> function plugin_call() (or plugin_call_ssl()) but plugin.c/plugin.h
> is changed to expose the "call one" function plugin_call_item(), and
> verify_user_pass_plugin() calls this for each loaded plugin,
> keeping track of "overall" state.
>
> key_state_test_plugin_auth() is introduced to do the
> "key_state_test_auth_control_file()" test for all plugins, and
> return "FAIL if one fails, PENDING if one is pending, SUCCESS
> if one was pending and succeeded now".
>

Since the handling is changing from "do them all' to "call one and FAIL on
first failure," may I suggest a feature enhancement?

Linux PAM uses auth plugins too, and each plugin has a control flag of
required, requisite, sufficient, or optional.  To date, OpenVPN handles
auth plugins as if each were required (proceed with next regardless of
failure), but with this patch, that will change to requisite (fail
immediately on first failure).  What if, as part of the openvpn plugin
config, we could set the behavior explicitly, much like PAM allows?

For reference:
http://linux-pam.org/Linux-PAM-html/sag-configuration-file.html

This would allow OpenVPN to handle alternate authentication paths, such as:

> plugin sufficient /usr/share/openvpn/plugin/lib/openvpn-auth-pam.so login
> plugin requisite /usr/share/openvpn/plugin/lib/openvpn-auth-ldap.so login
> plugin required /usr/share/openvpn/plugin/lib/openvpn-auth-duo.so login
> skey=abc ...
>

This setup would succeed immediately if the user successfully authenticates
with a local account; otherwise it would require LDAP and Duo MFA, but
would not query Duo if LDAP failed.

Such control would be constrained by the deferral mechanism, as the
deferred result cannot be immediately evaluated, so I think it makes sense
that all deferred plugins are assumed to be required and must follow any
plugins marked as requisite/sufficient.  Alternatively, if a deferred
plugin is marked as sufficient or requisite, it could hold further
processing until the deferred result is returned, but that negates much of
the asynchronous advantage that deferral gives us.

So that config files don't need updating between versions, I think it's
also wise to use a default flag if one is not specified.  I would suggest
non-deferred results default to requisite and deferred to required.  Or if
you want strict compatibility with existing behavior, you could default all
to required.

On that note, regarding serial vs. parallel launching of multiple deferred
plugins, unless PAM-like control is implemented and plugins are allowed to
be sufficient or requisite, I vote for launching them all in parallel and
returning FAIL if any one fails.

Thanks,
--
Pete
<div dir="ltr"><div dir="ltr">On Thu, Mar 10, 2022 at 12:18 PM Gert Doering &lt;<a href="mailto:gert@greenie.muc.de">gert@greenie.muc.de</a>&gt; wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The actual plugin calls are no longer done with the &quot;do them all&quot;<br>
function plugin_call() (or plugin_call_ssl()) but plugin.c/plugin.h<br>
is changed to expose the &quot;call one&quot; function plugin_call_item(), and<br>
verify_user_pass_plugin() calls this for each loaded plugin,<br>
keeping track of &quot;overall&quot; state.<br>
<br>
key_state_test_plugin_auth() is introduced to do the<br>
&quot;key_state_test_auth_control_file()&quot; test for all plugins, and<br>
return &quot;FAIL if one fails, PENDING if one is pending, SUCCESS<br>
if one was pending and succeeded now&quot;.<br></blockquote><div> </div><div>Since the handling is changing from &quot;do them all&#39; to &quot;call one and FAIL on first failure,&quot; may I suggest a feature enhancement?</div><div><br></div><div>Linux PAM uses auth plugins too, and each plugin has a control flag of required, requisite, sufficient, or optional.  To date, OpenVPN handles auth plugins as if each were required (proceed with next regardless of failure), but with this patch, that will change to requisite (fail immediately on first failure).  What if, as part of the openvpn plugin config, we could set the behavior explicitly, much like PAM allows?<br><br></div><div>For reference: <a href="http://linux-pam.org/Linux-PAM-html/sag-configuration-file.html">http://linux-pam.org/Linux-PAM-html/sag-configuration-file.html</a></div><div><br></div><div>This would allow OpenVPN to handle alternate authentication paths, such as:</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>plugin sufficient /usr/share/openvpn/plugin/lib/openvpn-auth-pam.so login<br>plugin requisite /usr/share/openvpn/plugin/lib/openvpn-auth-ldap.so login<br>plugin required /usr/share/openvpn/plugin/lib/openvpn-auth-duo.so login skey=abc ...</div></blockquote><div><br><div>This setup would succeed immediately if the user successfully authenticates with a local account; otherwise it would require LDAP and Duo MFA, but would not query Duo if LDAP failed.</div><div><br></div><div>Such control would be constrained by the deferral mechanism, as the deferred result cannot be immediately evaluated, so I think it makes sense that all deferred plugins are assumed to be required and must follow any plugins marked as requisite/sufficient.  Alternatively, if a deferred plugin is marked as sufficient or requisite, it could hold further processing until the deferred result is returned, but that negates much of the asynchronous advantage that deferral gives us.</div><div><br></div><div>So that config files don&#39;t need updating between versions, I think it&#39;s also wise to use a default flag if one is not specified.  I would suggest non-deferred results default to requisite and deferred to required.  Or if you want strict compatibility with existing behavior, you could default all to required.<br></div><div><br></div><div>On that note, regarding serial vs. parallel launching of multiple deferred plugins, unless PAM-like control is implemented and plugins are allowed to be sufficient or requisite, I vote for launching them all in parallel and returning FAIL if any one fails.<br></div><div><br></div><div>Thanks,</div><div>--</div><div>Pete<br></div></div></div></div>
David Sommerseth April 7, 2022, 10:24 a.m. UTC | #5
On 10/03/2022 12:57, Gert Doering wrote:
> Without this patch, OpenVPN behaviour if more than one plugin wants
> to do deferred user/password authentication not well-defined, as
> there is just one set of auth control files and a single plugin state.
> 
> This patch changes "key state -> plugin_auth" from a single struct
> to an array of MAX_PLUGINS elements that have per-plugin auth-control
> files and auth status.
> 
> All direct access is changed to iterating over this array.
> 
> The actual plugin calls are no longer done with the "do them all"
> function plugin_call() (or plugin_call_ssl()) but plugin.c/plugin.h
> is changed to expose the "call one" function plugin_call_item(), and
> verify_user_pass_plugin() calls this for each loaded plugin,
> keeping track of "overall" state.
> 
> key_state_test_plugin_auth() is introduced to do the
> "key_state_test_auth_control_file()" test for all plugins, and
> return "FAIL if one fails, PENDING if one is pending, SUCCESS
> if one was pending and succeeded now".
> 
> This was tested with the "auth-multi" test plugin, with 5-7 plugins
> loaded (some deferred, some direct) and "some of them failing" or
> "all succeeding".  Testing included "will it leak files if multiple
> deferred plugins are ongoing, and one of the earlier ones rejects
> authentication".
> 
> This patch is not suitable for production use:
>   - it's full of debug output
>   - it will break compilation without ENABLE_PLUGINS
>   - it stands to argue whether plugin_call_item() should be exposed,
>     or key_state_test_plugin_auth() might be moved to plugin.c instead
>     (with a null function if ENABLE_PLUGINS is not defined)
> 
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>   src/openvpn/plugin.c     |   2 +-
>   src/openvpn/plugin.h     |   9 +++
>   src/openvpn/ssl.c        |   5 +-
>   src/openvpn/ssl_common.h |   4 +-
>   src/openvpn/ssl_verify.c | 127 ++++++++++++++++++++++++++++++++-------
>   5 files changed, 123 insertions(+), 24 deletions(-)
Hi,

So I've had a deeper dive into this proposal.  At the moment, I have 
only glared at the code and considered the overall implementation idea.

TL;DR: This makes a lot of sense, and we should pursue this further. But 
there are room for some adjustments and further improvements.

When it comes to exposing plugin_call_item(), I think it makes sense to 
refactor the changes you've added to verify_user_pass_plugin() into 
plugin_call_ssl() instead.  This will make this implementation more 
generic and not having a special behavior only for auth-user-pass.

I also had a quick look at verify_user_pass_script() as well, and I 
think this needs to be re-evaluated with this change as well.  Scripts 
can now also do deferred authentication (which I believe is a new 
feature in git master/coming 2.6).  We may hit the same undefined 
behavior here as well with at least --auth-user-pass-verify, possibly 
other too.

The critical code path in regards to the authentication is in the new 
key_state_test_plugin_auth().  This implementation looks sane.  I would 
probably suggest to use a different variable name than 'ret_one'. 
Perhaps 'plugin_state' is a better name?  It was especially the last 
if() clause requiring me to re-read it a few times, as I missed the 
'_one' part and didn't quite understand the logic behind "ret = 
ACF_SUCCEEDED && ret != ACF_PENDING"; it made more sense when I grasped 
the first "ret" was supposed to be "ret_one".

Also, in that function "&(ads[i])" isn't the prettiest approach, but 
will work.  Could we do this nicer?

Another improvement I think can be reasonable is to refactor plugin_n() 
to return an internal counter of loaded plug-ins instead of passing a 
pointer to a plugin struct.  This allows the proposed changes to use 
plugin_n() more freely and to avoid iterating over MAX_PLUGINS.  Now 
there is a mixture between iterating plugin_n() and MAX_PLUGINS, and in 
most configurations plugin_n() will return a lower value than MAX_PLUGINS.

Patch

diff --git a/src/openvpn/plugin.c b/src/openvpn/plugin.c
index e3a89293..74b57033 100644
--- a/src/openvpn/plugin.c
+++ b/src/openvpn/plugin.c
@@ -518,7 +518,7 @@  plugin_open_item(struct plugin *p,
     }
 }
 
-static int
+int
 plugin_call_item(const struct plugin *p,
                  void *per_client_context,
                  const int type,
diff --git a/src/openvpn/plugin.h b/src/openvpn/plugin.h
index c6fa206a..799a646e 100644
--- a/src/openvpn/plugin.h
+++ b/src/openvpn/plugin.h
@@ -132,6 +132,15 @@  int plugin_call_ssl(const struct plugin_list *pl,
                     int current_cert_depth,
                     openvpn_x509_cert_t *current_cert
                     );
+int plugin_call_item(const struct plugin *p,
+                     void *per_client_context,
+                     const int type,
+                     const struct argv *av,
+                     struct openvpn_plugin_string_list **retlist,
+                     const char **envp,
+                     int certdepth,
+                     openvpn_x509_cert_t *current_cert
+                     );
 
 void plugin_list_close(struct plugin_list *pl);
 
diff --git a/src/openvpn/ssl.c b/src/openvpn/ssl.c
index 14a943a7..ce6de9a0 100644
--- a/src/openvpn/ssl.c
+++ b/src/openvpn/ssl.c
@@ -1036,7 +1036,10 @@  key_state_free(struct key_state *ks, bool clear)
 
     packet_id_free(&ks->crypto_options.packet_id);
 
-    key_state_rm_auth_control_files(&ks->plugin_auth);
+    for (int i=0; i<MAX_PLUGINS; i++)
+    {
+        key_state_rm_auth_control_files(&ks->plugin_auth[i]);
+    }
     key_state_rm_auth_control_files(&ks->script_auth);
 
     if (clear)
diff --git a/src/openvpn/ssl_common.h b/src/openvpn/ssl_common.h
index 8a077c74..0de3290a 100644
--- a/src/openvpn/ssl_common.h
+++ b/src/openvpn/ssl_common.h
@@ -37,6 +37,8 @@ 
 
 #include "ssl_backend.h"
 
+#include "plugin.h"
+
 /* passwords */
 #define UP_TYPE_AUTH        "Auth"
 #define UP_TYPE_PRIVATE_KEY "Private Key"
@@ -239,7 +241,7 @@  struct key_state
 #endif
     time_t acf_last_mod;
 
-    struct auth_deferred_status plugin_auth;
+    struct auth_deferred_status plugin_auth[MAX_PLUGINS];
     struct auth_deferred_status script_auth;
 };
 
diff --git a/src/openvpn/ssl_verify.c b/src/openvpn/ssl_verify.c
index 3b6b58fa..38c8a830 100644
--- a/src/openvpn/ssl_verify.c
+++ b/src/openvpn/ssl_verify.c
@@ -1059,6 +1059,50 @@  key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached)
     return ACF_DISABLED;
 }
 
+/**
+ * Checks the auth control status for all plugins.
+ *
+ * returns:
+ * - ACF_PENDING  if any plugin is still waiting for results.
+ * - ACF_SUCCEEDED if there were pending plugins AND all succeeded
+ * - ACF_FAILED   if any plugin fails
+ * - ACF_DISABLED if no plugin is waiting
+ *
+ * @param ads       array of deferred status control structures
+ * @param cached    Return only cached status
+ * @return          ACF_* as per enum
+ */
+static enum auth_deferred_result
+key_state_test_plugin_auth(struct auth_deferred_status *ads, bool cached)
+{
+    unsigned int ret = ACF_DISABLED;
+
+    for (int i=0;i<MAX_PLUGINS;i++)
+    {
+        unsigned int ret_one = key_state_test_auth_control_file(&(ads[i]), cached);
+msg(M_INFO, "GERT(9a): kstpa: i=%d, ret_one=%d", i, ret_one);
+
+        /* if at least one plugin fails, we fail all */
+        if ( ret_one == ACF_FAILED )
+        {
+            ret = ret_one;
+            break;
+        }
+        /* if at least one plugin is still pending, we're still pending */
+        if ( ret_one == ACF_PENDING )
+        {
+            ret = ret_one;
+        }
+        /* if no plugins are pending AND we have success, we succeed */
+        if ( ret_one == ACF_SUCCEEDED && ret != ACF_PENDING )
+        {
+            ret = ret_one;
+        }
+    }
+msg(M_INFO, "GERT(9b): kstpa: ret=%d", ret);
+    return ret;
+}
+
 /**
  * This method takes a key_state and if updates the state
  * of the key if it is deferred.
@@ -1069,6 +1113,7 @@  key_state_test_auth_control_file(struct auth_deferred_status *ads, bool cached)
 static void
 update_key_auth_status(bool cached, struct key_state *ks)
 {
+msg(M_INFO, "GERT(10) update_key_auth_status authenticated=%d", ks->authenticated);
     if (ks->authenticated == KS_AUTH_FALSE)
     {
         return;
@@ -1078,7 +1123,7 @@  update_key_auth_status(bool cached, struct key_state *ks)
         enum auth_deferred_result auth_plugin = ACF_DISABLED;
         enum auth_deferred_result auth_script = ACF_DISABLED;
         enum auth_deferred_result auth_man = ACF_DISABLED;
-        auth_plugin = key_state_test_auth_control_file(&ks->plugin_auth, cached);
+        auth_plugin = key_state_test_plugin_auth(ks->plugin_auth, cached);
         auth_script = key_state_test_auth_control_file(&ks->script_auth, cached);
 #ifdef ENABLE_MANAGEMENT
         auth_man = man_def_auth_test(ks);
@@ -1357,40 +1402,80 @@  static int
 verify_user_pass_plugin(struct tls_session *session, struct tls_multi *multi,
                         const struct user_pass *up)
 {
-    int retval = OPENVPN_PLUGIN_FUNC_ERROR;
+    int retval = OPENVPN_PLUGIN_FUNC_SUCCESS;
     struct key_state *ks = &session->key[KS_PRIMARY];      /* primary key */
 
+    struct gc_arena gc = gc_new();
+
     /* set password in private env space */
     setenv_str(session->opt->es, "password", up->password);
 
-    /* generate filename for deferred auth control file */
-    if (!key_state_gen_auth_control_files(&ks->plugin_auth, session->opt))
+    /* iterate over list of plugins here, because deferred auth needs
+     * per-plugin auth control files
+     */
+    const struct plugin_list *pl = session->opt->plugins;
+    int plugins_active = plugin_n(pl);
+    ASSERT( plugins_active < SIZE(ks->plugin_auth) );
+    msg(M_INFO, "GERT(1): plugins_active=%d, SIZE=%d", plugins_active, (int)SIZE(ks->plugin_auth) );
+
+    for (int i=0; i<plugins_active; i++)
     {
-        msg(D_TLS_ERRORS, "TLS Auth Error (%s): "
-            "could not create deferred auth control file", __func__);
-        return retval;
-    }
+        /* generate filename for deferred auth control file */
+        if (!key_state_gen_auth_control_files(&ks->plugin_auth[i], session->opt))
+        {
+            msg(D_TLS_ERRORS, "TLS Auth Error (%s): "
+                "could not create deferred auth control file", __func__);
+            retval = OPENVPN_PLUGIN_FUNC_ERROR;
+            break;
+        }
 
-    /* call command */
-    retval = plugin_call(session->opt->plugins, OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, NULL, session->opt->es);
+        msg(M_INFO, "GERT(4): plugin #%d, acf=%s acp=%s", i, 
+                ks->plugin_auth[i].auth_control_file, ks->plugin_auth[i].auth_pending_file);
 
-    if (retval == OPENVPN_PLUGIN_FUNC_DEFERRED)
-    {
-        /* Check if the plugin has written the pending auth control
-         * file and send the pending auth to the client */
-        if(!key_state_check_auth_pending_file(&ks->plugin_auth, multi))
+        /* call command */
+        // retval = plugin_call_item(session->opt->plugins, OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY, NULL, NULL, session->opt->es);
+        //                                pl                    type                             av    pr    es   [crt -1, NULL]
+
+        const char **envp = make_env_array(session->opt->es, false, &gc);
+        const int status = plugin_call_item(&pl->common->plugins[i],
+                                            pl->per_client.per_client_context[i],
+                                            OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY,
+                                            NULL, NULL, envp, -1, NULL );
+
+        if (status == OPENVPN_PLUGIN_FUNC_DEFERRED)
+        {
+            /* Check if the plugin has written the pending auth control
+             * file and send the pending auth to the client */
+            if(!key_state_check_auth_pending_file(&(ks->plugin_auth[i]), multi))
+            {
+                retval = OPENVPN_PLUGIN_FUNC_ERROR;
+                key_state_rm_auth_control_files(&(ks->plugin_auth[i]));
+                break;
+            }
+            else
+            {
+                retval = OPENVPN_PLUGIN_FUNC_DEFERRED;
+            }
+        }
+        else
+        {
+            /* purge auth control filename (and file itself) for non-deferred returns */
+            key_state_rm_auth_control_files(&(ks->plugin_auth[i]));
+            ks->plugin_auth[i].auth_control_status = ACF_DISABLED;
+        }
+
+        /* if a single plugin fails, we fail all */
+        if (status == OPENVPN_PLUGIN_FUNC_ERROR)
         {
             retval = OPENVPN_PLUGIN_FUNC_ERROR;
-            key_state_rm_auth_control_files(&ks->plugin_auth);
+            break;
         }
-    }
-    else
-    {
-        /* purge auth control filename (and file itself) for non-deferred returns */
-        key_state_rm_auth_control_files(&ks->plugin_auth);
+
+        msg(M_INFO, "GERT(3): plugin #%d status=%d -> retval=%d", i, status, retval );
     }
 
     setenv_del(session->opt->es, "password");
+    gc_free(&gc);
 
     return retval;
 }