[Openvpn-devel,v2] Add deferred authentication support to plugin-auth-pam

Message ID 20200623092848.21214-1-gert@greenie.muc.de
State New
Headers show
Series
  • [Openvpn-devel,v2] Add deferred authentication support to plugin-auth-pam
Related show

Commit Message

Gert Doering June 23, 2020, 9:28 a.m.
If OpenVPN signals deferred authentication support (by setting
the internal environment variables "auth_control_file" and
"deferred_auth_pam"), do not wait for PAM stack to finish.  Instead,
the privileged PAM process returns RESPONSE_DEFER via the control
socket, which gets turned into OPENVPN_PLUGIN_FUNC_DEFERRED towards
openvpn.

The PAM process will then fork() and handle all the PAM auth in
the new process, signalling success/failure back by means of the
auth_control_file (forking twice, to simplify wait() handling).

With the extra fork(), multiple deferred authentications can run at
the same time - otherwise the first one would block the next auth
call (because the child would not be ready again to read from the
control socket).

Lightly tested on Linux.

Signed-off-by: Gert Doering <gert@greenie.muc.de>

--
v2:
  - only do deferred auth if "deferred_auth_pam" is set (env)
  - put deferred auth logic into do_deferred_pam_auth()
  - line-wrap lines where needed
  - close "background end" of socketpair in deferred auth process
  - remove leftover /* plugin_log() */ lines from initial testing
  - tested over a few hundred "15s delayed" authentication cycles
---
 src/plugins/auth-pam/auth-pam.c | 124 +++++++++++++++++++++++++++++++-
 1 file changed, 122 insertions(+), 2 deletions(-)

Comments

Selva Nair July 14, 2020, 5:47 p.m. | #1
Hi,

Sorry for the long delay in getting back to this.. A few minor
nitpicks on style follows:

On Tue, Jun 23, 2020 at 5:29 AM Gert Doering <gert@greenie.muc.de> wrote:
>
> If OpenVPN signals deferred authentication support (by setting
> the internal environment variables "auth_control_file" and
> "deferred_auth_pam"), do not wait for PAM stack to finish.  Instead,
> the privileged PAM process returns RESPONSE_DEFER via the control
> socket, which gets turned into OPENVPN_PLUGIN_FUNC_DEFERRED towards
> openvpn.
>
> The PAM process will then fork() and handle all the PAM auth in
> the new process, signalling success/failure back by means of the
> auth_control_file (forking twice, to simplify wait() handling).
>
> With the extra fork(), multiple deferred authentications can run at
> the same time - otherwise the first one would block the next auth
> call (because the child would not be ready again to read from the
> control socket).
>
> Lightly tested on Linux.
>
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
>
> --
> v2:
>   - only do deferred auth if "deferred_auth_pam" is set (env)
>   - put deferred auth logic into do_deferred_pam_auth()
>   - line-wrap lines where needed
>   - close "background end" of socketpair in deferred auth process
>   - remove leftover /* plugin_log() */ lines from initial testing
>   - tested over a few hundred "15s delayed" authentication cycles
> ---
>  src/plugins/auth-pam/auth-pam.c | 124 +++++++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
> index 9a11876d..2a75e5f0 100644
> --- a/src/plugins/auth-pam/auth-pam.c
> +++ b/src/plugins/auth-pam/auth-pam.c
> @@ -62,6 +62,7 @@
>  #define RESPONSE_INIT_FAILED      11
>  #define RESPONSE_VERIFY_SUCCEEDED 12
>  #define RESPONSE_VERIFY_FAILED    13
> +#define RESPONSE_DEFER            14
>
>  /* Pointers to functions exported from openvpn */
>  static plugin_log_t plugin_log = NULL;
> @@ -524,12 +525,31 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha
>          const char *password = get_env("password", envp);
>          const char *common_name = get_env("common_name", envp) ? get_env("common_name", envp) : "";
>
> +        /* should we do deferred auth?
> +         *  yes, if there is "auth_control_file" and "deferred_auth_pam" env
> +         */
> +        const char *auth_control_file = get_env("auth_control_file", envp);
> +        const char *deferred_auth_pam = get_env("deferred_auth_pam", envp);
> +        if ( auth_control_file != NULL && deferred_auth_pam != NULL)

We don't add spaces around parentheses, do we? A few such cases below..

> +        {
> +            if (DEBUG(context->verb))
> +            {
> +                plugin_log(PLOG_NOTE, MODULE, "do deferred auth '%s'",
> +                           auth_control_file);
> +            }
> +        }
> +        else
> +        {
> +            auth_control_file = "";
> +        }
> +
>          if (username && strlen(username) > 0 && password)
>          {
>              if (send_control(context->foreground_fd, COMMAND_VERIFY) == -1
>                  || send_string(context->foreground_fd, username) == -1
>                  || send_string(context->foreground_fd, password) == -1
> -                || send_string(context->foreground_fd, common_name) == -1)
> +                || send_string(context->foreground_fd, common_name) == -1
> +                || send_string(context->foreground_fd, auth_control_file) == -1)
>              {
>                  plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error sending auth info to background process");
>              }
> @@ -540,6 +560,14 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha
>                  {
>                      return OPENVPN_PLUGIN_FUNC_SUCCESS;
>                  }
> +                if (status == RESPONSE_DEFER)
> +                {
> +                    if (DEBUG(context->verb))
> +                    {
> +                        plugin_log(PLOG_NOTE, MODULE, "deferred authentication");
> +                    }
> +                    return OPENVPN_PLUGIN_FUNC_DEFERRED;
> +                }
>                  if (status == -1)
>                  {
>                      plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error receiving auth confirmation from background process");
> @@ -782,6 +810,80 @@ pam_auth(const char *service, const struct user_pass *up)
>      return ret;
>  }
>
> +/*
> + * deferred auth handler
> + *   - fork() (twice, to avoid the need for async wait / SIGCHLD handling)
> + *   - query PAM stack via pam_auth()
> + *   - send response back to OpenVPN via "ac_file_name"
> + *
> + * parent process returns "0" for "fork() and wait() succeeded",
> + *                        "-1" for "something went wrong, abort program"
> + */
> +
> +static int
> +do_deferred_pam_auth(int fd, const char *ac_file_name,
> +                     const char *service, const struct user_pass *up)
> +{
> +    if (send_control(fd, RESPONSE_DEFER) == -1)
> +    {
> +        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: write error on response socket [4]");
> +        return -1;
> +    }
> +
> +    /* double forking so we do not need to wait() for async auth kids */
> +    pid_t p1 = fork();
> +
> +    if ( p1 < 0 )
> +    {
> +        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: fork(1) failed");
> +        return -1;
> +    }
> +    if ( p1 != 0 )                         /* parent */

Same here..

> +    {
> +        waitpid(p1, NULL, 0);
> +        return 0;                          /* parent's job succeeded */
> +    }
> +
> +    /* child */
> +    close(fd);                              /* socketpair no longer needed */
> +
> +    pid_t p2 = fork();
> +    if ( p2 < 0 )

and here

> +    {
> +        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: fork(2) failed");
> +        exit(1);
> +    }
> +
> +    if ( p2 != 0 )                          /* new parent: exit right away */

ditto..

> +    {
> +        exit(0);
> +    }
> +
> +    /* grandchild */
> +    plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: deferred auth for '%s', pid=%d",
> +               up->username, (int) getpid() );

extra space before the closing )

> +
> +    /* the rest is very simple: do PAM, write status byte to file, done */
> +    int ac_fd = open( ac_file_name, O_WRONLY );

and around here

> +    if ( ac_fd < 0 )

and here

> +    {
> +        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "cannot open '%s' for writing",
> +                   ac_file_name );

Space before the closing )

> +        exit(1);
> +    }
> +    int pam_success = pam_auth(service, up);
> +
> +    if ( write( ac_fd, pam_success? "1": "0", 1 ) != 1 )

and here

> +    {
> +        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "cannot write to '%s'",
> +                   ac_file_name );

and before the last )

> +    }
> +    close(ac_fd);
> +    plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: %s: deferred auth: PAM %s",
> +               up->username, pam_success? "succeeded": "rejected" );

Same here before the closing ")"

> +    exit(0);
> +}
> +
>  /*
>   * Background process -- runs with privilege.
>   */
> @@ -789,6 +891,7 @@ static void
>  pam_server(int fd, const char *service, int verb, const struct name_value_list *name_value_list)
>  {
>      struct user_pass up;
> +    char ac_file_name[PATH_MAX];
>      int command;
>  #ifdef USE_PAM_DLOPEN
>      static const char pam_so[] = "libpam.so";
> @@ -847,7 +950,8 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *
>              case COMMAND_VERIFY:
>                  if (recv_string(fd, up.username, sizeof(up.username)) == -1
>                      || recv_string(fd, up.password, sizeof(up.password)) == -1
> -                    || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1)
> +                    || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1
> +                    || recv_string(fd, ac_file_name, sizeof(ac_file_name)) == -1)
>                  {
>                      plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: read error on command channel: code=%d, exiting",
>                              command);
> @@ -867,6 +971,22 @@ pam_server(int fd, const char *service, int verb, const struct name_value_list *
>                  /* If password is of the form SCRV1:base64:base64 split it up */
>                  split_scrv1_password(&up);
>
> +                /* client wants deferred auth
> +                 */
> +                if ( strlen(ac_file_name) > 0 )
> +                {
> +                    if (do_deferred_pam_auth(fd, ac_file_name,
> +                                             service, &up) < 0)
> +                    {
> +                        goto done;

Do we have to abort in this case? This will exit the background
process and cripple the server while this could be a temporary memory
pressure causing the fork to fail. Why not just break and plough
along? The core will fail to get a response via the ac_file, but that
could happen if the grand-child fails as well -- the server is
supposed to cope with such failures.

> +                    }
> +                    break;
> +                }
> +
> +
> +                /* non-deferred auth: wait for pam result and send
> +                 * result back via control socketpair
> +                 */
>                  if (pam_auth(service, &up)) /* Succeeded */
>                  {
>                      if (send_control(fd, RESPONSE_VERIFY_SUCCEEDED) == -1)
> --

Apart from these minor issues that could be corrected or ignored at
merge time, all look good.

We should put the usage info into README.auth-pam as that seems to be
the only documentation of the plugin. Also an entry in changelog?
Could be a separate patch.

Acked-by: Selva Nair <selva.nair@gmail.com>
Gert Doering July 14, 2020, 8:47 p.m. | #2
Hi,

On Tue, Jul 14, 2020 at 01:47:06PM -0400, Selva Nair wrote:
> Sorry for the long delay in getting back to this.. A few minor
> nitpicks on style follows:

Thanks for the review.  Indeed, my style fu was sort of absent when
I wrote this :-) (it's "mgetty+sendfax" style, with "I must remember
to use vim and :set expandtab!!!", but the braces always escape me...
and Antonio was busy elsewhere :) )

This...:

> On Tue, Jun 23, 2020 at 5:29 AM Gert Doering <gert@greenie.muc.de> wrote:
> > +                /* client wants deferred auth
> > +                 */
> > +                if ( strlen(ac_file_name) > 0 )
> > +                {
> > +                    if (do_deferred_pam_auth(fd, ac_file_name,
> > +                                             service, &up) < 0)
> > +                    {
> > +                        goto done;
> 
> Do we have to abort in this case? This will exit the background
> process and cripple the server while this could be a temporary memory
> pressure causing the fork to fail. Why not just break and plough
> along? The core will fail to get a response via the ac_file, but that
> could happen if the grand-child fails as well -- the server is
> supposed to cope with such failures.

... is something I need to look more closely at.  I remember that the
general approach to "mmmh, something failed!" here is "goto done", 
but your argument about "yeah, fork() failed, so what - try again
later!" has merits.

> Apart from these minor issues that could be corrected or ignored at
> merge time, all look good.

This is actually a code change, so I think I'll send a v3 with all
the nitpicks fixed (uncrustify is my friend :-) ) and this one changed.

> We should put the usage info into README.auth-pam as that seems to be
> the only documentation of the plugin. Also an entry in changelog?
> Could be a separate patch.

When I do a v3 anyway, I can add this as well.  Makes sense to have it
one commit.

gert

Patch

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index 9a11876d..2a75e5f0 100644
--- a/src/plugins/auth-pam/auth-pam.c
+++ b/src/plugins/auth-pam/auth-pam.c
@@ -62,6 +62,7 @@ 
 #define RESPONSE_INIT_FAILED      11
 #define RESPONSE_VERIFY_SUCCEEDED 12
 #define RESPONSE_VERIFY_FAILED    13
+#define RESPONSE_DEFER            14
 
 /* Pointers to functions exported from openvpn */
 static plugin_log_t plugin_log = NULL;
@@ -524,12 +525,31 @@  openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha
         const char *password = get_env("password", envp);
         const char *common_name = get_env("common_name", envp) ? get_env("common_name", envp) : "";
 
+        /* should we do deferred auth?
+         *  yes, if there is "auth_control_file" and "deferred_auth_pam" env
+         */
+        const char *auth_control_file = get_env("auth_control_file", envp);
+        const char *deferred_auth_pam = get_env("deferred_auth_pam", envp);
+        if ( auth_control_file != NULL && deferred_auth_pam != NULL)
+        {
+            if (DEBUG(context->verb))
+            {
+                plugin_log(PLOG_NOTE, MODULE, "do deferred auth '%s'",
+                           auth_control_file);
+            }
+        }
+        else
+        {
+            auth_control_file = "";
+        }
+
         if (username && strlen(username) > 0 && password)
         {
             if (send_control(context->foreground_fd, COMMAND_VERIFY) == -1
                 || send_string(context->foreground_fd, username) == -1
                 || send_string(context->foreground_fd, password) == -1
-                || send_string(context->foreground_fd, common_name) == -1)
+                || send_string(context->foreground_fd, common_name) == -1
+                || send_string(context->foreground_fd, auth_control_file) == -1)
             {
                 plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error sending auth info to background process");
             }
@@ -540,6 +560,14 @@  openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha
                 {
                     return OPENVPN_PLUGIN_FUNC_SUCCESS;
                 }
+                if (status == RESPONSE_DEFER)
+                {
+                    if (DEBUG(context->verb))
+                    {
+                        plugin_log(PLOG_NOTE, MODULE, "deferred authentication");
+                    }
+                    return OPENVPN_PLUGIN_FUNC_DEFERRED;
+                }
                 if (status == -1)
                 {
                     plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error receiving auth confirmation from background process");
@@ -782,6 +810,80 @@  pam_auth(const char *service, const struct user_pass *up)
     return ret;
 }
 
+/*
+ * deferred auth handler
+ *   - fork() (twice, to avoid the need for async wait / SIGCHLD handling)
+ *   - query PAM stack via pam_auth()
+ *   - send response back to OpenVPN via "ac_file_name"
+ *
+ * parent process returns "0" for "fork() and wait() succeeded",
+ *                        "-1" for "something went wrong, abort program"
+ */
+
+static int
+do_deferred_pam_auth(int fd, const char *ac_file_name,
+                     const char *service, const struct user_pass *up)
+{
+    if (send_control(fd, RESPONSE_DEFER) == -1)
+    {
+        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: write error on response socket [4]");
+        return -1;
+    }
+
+    /* double forking so we do not need to wait() for async auth kids */
+    pid_t p1 = fork();
+
+    if ( p1 < 0 )
+    {
+        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: fork(1) failed");
+        return -1;
+    }
+    if ( p1 != 0 )                         /* parent */
+    {
+        waitpid(p1, NULL, 0);
+        return 0;                          /* parent's job succeeded */
+    }
+
+    /* child */
+    close(fd);                              /* socketpair no longer needed */
+
+    pid_t p2 = fork();
+    if ( p2 < 0 )
+    {
+        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: fork(2) failed");
+        exit(1);
+    }
+
+    if ( p2 != 0 )                          /* new parent: exit right away */
+    {
+        exit(0);
+    }
+
+    /* grandchild */
+    plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: deferred auth for '%s', pid=%d",
+               up->username, (int) getpid() );
+
+    /* the rest is very simple: do PAM, write status byte to file, done */
+    int ac_fd = open( ac_file_name, O_WRONLY );
+    if ( ac_fd < 0 )
+    {
+        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "cannot open '%s' for writing",
+                   ac_file_name );
+        exit(1);
+    }
+    int pam_success = pam_auth(service, up);
+
+    if ( write( ac_fd, pam_success? "1": "0", 1 ) != 1 )
+    {
+        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "cannot write to '%s'",
+                   ac_file_name );
+    }
+    close(ac_fd);
+    plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: %s: deferred auth: PAM %s",
+               up->username, pam_success? "succeeded": "rejected" );
+    exit(0);
+}
+
 /*
  * Background process -- runs with privilege.
  */
@@ -789,6 +891,7 @@  static void
 pam_server(int fd, const char *service, int verb, const struct name_value_list *name_value_list)
 {
     struct user_pass up;
+    char ac_file_name[PATH_MAX];
     int command;
 #ifdef USE_PAM_DLOPEN
     static const char pam_so[] = "libpam.so";
@@ -847,7 +950,8 @@  pam_server(int fd, const char *service, int verb, const struct name_value_list *
             case COMMAND_VERIFY:
                 if (recv_string(fd, up.username, sizeof(up.username)) == -1
                     || recv_string(fd, up.password, sizeof(up.password)) == -1
-                    || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1)
+                    || recv_string(fd, up.common_name, sizeof(up.common_name)) == -1
+                    || recv_string(fd, ac_file_name, sizeof(ac_file_name)) == -1)
                 {
                     plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: read error on command channel: code=%d, exiting",
                             command);
@@ -867,6 +971,22 @@  pam_server(int fd, const char *service, int verb, const struct name_value_list *
                 /* If password is of the form SCRV1:base64:base64 split it up */
                 split_scrv1_password(&up);
 
+                /* client wants deferred auth
+                 */
+                if ( strlen(ac_file_name) > 0 )
+                {
+                    if (do_deferred_pam_auth(fd, ac_file_name,
+                                             service, &up) < 0)
+                    {
+                        goto done;
+                    }
+                    break;
+                }
+
+
+                /* non-deferred auth: wait for pam result and send
+                 * result back via control socketpair
+                 */
                 if (pam_auth(service, &up)) /* Succeeded */
                 {
                     if (send_control(fd, RESPONSE_VERIFY_SUCCEEDED) == -1)