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

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

Commit Message

Gert Doering June 20, 2020, 4:22 p.m.
If OpenVPN signals deferred authentication support (by setting the
internal environment variable "auth_control_file"), 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>
---
 src/plugins/auth-pam/auth-pam.c | 88 ++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 2 deletions(-)

Comments

Gert Doering June 20, 2020, 4:27 p.m. | #1
Hi,

On Sat, Jun 20, 2020 at 06:22:06PM +0200, Gert Doering wrote:
> If OpenVPN signals deferred authentication support (by setting the
> internal environment variable "auth_control_file"), 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.
[..]
> Lightly tested on Linux.

This is sort of a "v1" of this patch, knowing well that it needs some
more polishing regarding logging etc. - but it would be good to have 
some feedback if I'm overlooking something crucial here.

"It seems to work, though" - as in "I've built a pam stack with 
pam_auth_radius talking to a non-existing radius server, which will
always add 15 seconds of delay - and with this patch, two clients can
authenticate concurrently, TLS handshakes do work while a client is
authenticating, and pings for client A do not stop while client B is
connecting".  And, no zombie processes.

I have not tested this in anger, like "100 clients connecting at the
same time" or "let it run for two weeks with 1000 clients connecting
and disconnecting all the time".

 
It goes on top of the patch that adds plugin_log() logging to auth-pam.c

gert
Selva Nair June 21, 2020, 10:23 p.m. | #2
Hi,

On Sat, Jun 20, 2020 at 12:23 PM Gert Doering <gert@greenie.muc.de> wrote:
>
> If OpenVPN signals deferred authentication support (by setting the
> internal environment variable "auth_control_file"), 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).

I have no doubt this will work, but can we avoid the double fork? Instead,
we could set a handler for SIGCHLD and reap all child pids there using
a non blocking waitpid(-1, NULL, WNOHANG). Will have to also check for
EINTR and retry in recv_control() and possibly recv_string().

Fork is probably not expensive, but two of them per auth event look
excessive and not very elegant, somehow.

>
> Lightly tested on Linux.
>
> Signed-off-by: Gert Doering <gert@greenie.muc.de>
> ---
>  src/plugins/auth-pam/auth-pam.c | 88 ++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
> index 9a11876d..777957fa 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,21 @@ 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) : "";
>
> +        /* get auth_control_file filename from envp string array*/
> +        const char *auth_control_file = get_env("auth_control_file", envp);
> +        if ( auth_control_file == NULL )
> +        {
> +            auth_control_file = "";
> +        }
> +        plugin_log(PLOG_NOTE, MODULE, "deferred auth '%s'", 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 +550,11 @@ openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha
>                  {
>                      return OPENVPN_PLUGIN_FUNC_SUCCESS;
>                  }
> +                if (status == RESPONSE_DEFER)
> +                {
> +                    plugin_log(PLOG_NOTE, MODULE, "deferred authentication active");
> +                    return OPENVPN_PLUGIN_FUNC_DEFERRED;
> +                }
>                  if (status == -1)
>                  {
>                      plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error receiving auth confirmation from background process");
> @@ -789,6 +804,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 +863,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 +884,73 @@ 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
> +                 * TODO: put this into its own function
> +                 */

TODOs seldom get attended to, it's now or never :)

> +                if ( strlen(ac_file_name) > 0 )
> +                {
> +                    if (send_control(fd, RESPONSE_DEFER) == -1)
> +                    {
> +                        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: write error on response socket [4]");
> +                        goto done;
> +                    }
> +
> +                    /* 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");
> +                        goto done;
> +                    }
> +                    if ( p1 != 0 )                         /* parent */
> +                    {
> +                        waitpid(p1, NULL, 0);
> +                        /* plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: fork(1), parent=%d, waitpid(%d) done, loop on", (int) getpid(), (int) p1 ); */
> +                        break;
> +                    }
> +
> +                    /* child */
> +                    pid_t p2 = fork();
> +
> +                    if ( p2 < 0 )
> +                    {
> +                        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: fork(2) failed");
> +                        goto done;
> +                    }
> +
> +                    if ( p2 != 0 )                        /* parent-child */
> +                    {
> +                        /* plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: fork(2), mid=%d, exiting", (int) getpid() ); */
> +                        exit(0);
> +                    }
> +
> +                    /* grandchild */
> +                    plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: deferred auth handler, pid=%d", (int) getpid() );

I think the scoket in the parent could be closed here. The child
doesn't need it.

> +
> +                    /* 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: deferred auth finished" );
> +                    exit(0);
> +                }
> +
> +                /* 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)
> --
> 2.26.2

Looks good otherwise.

As auth_control_file is always generated by the core built with
PLUGIN_DEF_AUTH, the patch appears to make deferred auth the default
with this plugin. So PLUGIN_DEF_AUTH takes a new meaning now.

Not a fault of this patch, but I noticed that the auth_control_file is
removed only during the next auth cycle for the same client, or while
restarting the client, not promptly after an auth success or failure.


Selva
Gert Doering June 22, 2020, 6:15 a.m. | #3
Good morning,

and thanks for the quick review :-)

On Sun, Jun 21, 2020 at 06:23:15PM -0400, Selva Nair wrote:
> On Sat, Jun 20, 2020 at 12:23 PM Gert Doering <gert@greenie.muc.de> wrote:
> > If OpenVPN signals deferred authentication support (by setting the
> > internal environment variable "auth_control_file"), 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).
> 
> I have no doubt this will work, but can we avoid the double fork? Instead,
> we could set a handler for SIGCHLD and reap all child pids there using
> a non blocking waitpid(-1, NULL, WNOHANG). Will have to also check for
> EINTR and retry in recv_control() and possibly recv_string().
> 
> Fork is probably not expensive, but two of them per auth event look
> excessive and not very elegant, somehow.

I have started with signal handlers, and then decided that this will
lead to more complications (EINTR) and possibly someone in the PAM stack
will also might fork(), so we need to reset signal handlers in the 
child, and then I found it all much more complicated than I liked...

fork() isn't as costly anymore as it used to be "in old unix legends" :-)
(it does have a cost, for setting up new memory mappings for COW, which
can get large if the memory footprint is large) - and I find the code
much simpler and much more straightforward, as in "everything related
to deferred auth happens *here*"...


[..]
> >                  split_scrv1_password(&up);
> >
> > +                /* client wants deferred auth
> > +                 * TODO: put this into its own function
> > +                 */
> 
> TODOs seldom get attended to, it's now or never :)

Right.  v2 is coming :-) - I wanted some quick initial feedback on whether
something here is a truly bad idea or if I had overlooked something 
crucial.

(In the meantime, the code has gotten some more excercise - I've let a
client connect in a loop, with "connect", "die with --inactive 30",
"be restarted from script", while another client was connected all the
time and pinging the server - and no zombies and no ping loss so far,
over ~30 hours)

[..]
> > +                    {
> > +                        /* plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: fork(2), mid=%d, exiting", (int) getpid() ); */
> > +                        exit(0);
> > +                    }
> > +
> > +                    /* grandchild */
> > +                    plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: deferred auth handler, pid=%d", (int) getpid() );
> 
> I think the scoket in the parent could be closed here. The child
> doesn't need it.

I had this initially, and then the socket got messed up - but that might
have been some other bug in the early code.  I'll re-add and re-test.

[..]
> As auth_control_file is always generated by the core built with
> PLUGIN_DEF_AUTH, the patch appears to make deferred auth the default
> with this plugin. So PLUGIN_DEF_AUTH takes a new meaning now.

Yes.  I plan on adding a new (internal) environment variable to control
this, like

  setenv deferred-auth-pam 1

or the like.  So the default would be "synchronous, as it is now", and
you get deferred if "auth_control_file *and* deferred-auth-pam are set"
(or we could do it the other way round, "no-deferred-auth-pam"?).


> Not a fault of this patch, but I noticed that the auth_control_file is
> removed only during the next auth cycle for the same client, or while
> restarting the client, not promptly after an auth success or failure.

Indeed

gentoo ~/t_server/tun-udp-p2mp-global-authpam # ls -l /tmp/*acf*
-rw------- 1 root root 0 Jun 22 08:14 /tmp/openvpn_acf_6d4bb9bdca44e95168015833e2ff3273.tmp
-rw------- 1 root root 1 Jun 22 08:13 /tmp/openvpn_acf_7055788f18a55f221abf8f9b10751dde.tmp
gentoo ~/t_server/tun-udp-p2mp-global-authpam # ls -l /tmp/*acf*
-rw------- 1 root root 1 Jun 22 08:14 /tmp/openvpn_acf_6d4bb9bdca44e95168015833e2ff3273.tmp

(that's the client-in-a-loop)


Not related to this patch, but it might be worth a look on whether this
can be easily fixed.

gert

Patch

diff --git a/src/plugins/auth-pam/auth-pam.c b/src/plugins/auth-pam/auth-pam.c
index 9a11876d..777957fa 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,21 @@  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) : "";
 
+        /* get auth_control_file filename from envp string array*/
+        const char *auth_control_file = get_env("auth_control_file", envp);
+        if ( auth_control_file == NULL )
+        {
+            auth_control_file = "";
+        }
+        plugin_log(PLOG_NOTE, MODULE, "deferred auth '%s'", 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 +550,11 @@  openvpn_plugin_func_v1(openvpn_plugin_handle_t handle, const int type, const cha
                 {
                     return OPENVPN_PLUGIN_FUNC_SUCCESS;
                 }
+                if (status == RESPONSE_DEFER)
+                {
+                    plugin_log(PLOG_NOTE, MODULE, "deferred authentication active");
+                    return OPENVPN_PLUGIN_FUNC_DEFERRED;
+                }
                 if (status == -1)
                 {
                     plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "Error receiving auth confirmation from background process");
@@ -789,6 +804,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 +863,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 +884,73 @@  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
+                 * TODO: put this into its own function
+                 */
+                if ( strlen(ac_file_name) > 0 )
+                {
+                    if (send_control(fd, RESPONSE_DEFER) == -1)
+                    {
+                        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: write error on response socket [4]");
+                        goto done;
+                    }
+
+                    /* 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");
+                        goto done;
+                    }
+                    if ( p1 != 0 )                         /* parent */
+                    {
+                        waitpid(p1, NULL, 0);
+                        /* plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: fork(1), parent=%d, waitpid(%d) done, loop on", (int) getpid(), (int) p1 ); */
+                        break;
+                    }
+
+                    /* child */
+                    pid_t p2 = fork();
+
+                    if ( p2 < 0 )
+                    {
+                        plugin_log(PLOG_ERR|PLOG_ERRNO, MODULE, "BACKGROUND: fork(2) failed");
+                        goto done;
+                    }
+
+                    if ( p2 != 0 )                        /* parent-child */
+                    {
+                        /* plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: fork(2), mid=%d, exiting", (int) getpid() ); */
+                        exit(0);
+                    }
+
+                    /* grandchild */
+                    plugin_log(PLOG_NOTE, MODULE, "BACKGROUND: deferred auth handler, pid=%d", (int) getpid() );
+
+                    /* 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: deferred auth finished" );
+                    exit(0);
+                }
+
+                /* 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)