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 <g...@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
--
"If was one thing all people took for granted, was conviction that if you
feed honest figures into a computer, honest figures come out. Never doubted
it myself till I met a computer with a sense of humor."
Robert A. Heinlein, The Moon is a Harsh Mistress
Gert Doering - Munich, Germany g...@greenie.muc.de
signature.asc
Description: PGP signature
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
