New and improved pam_exec module

[Daniel Richard G.]

On Mon, 2006 Jul 31 11:50:04 +0200, Thorsten Kukuk wrote:
> 
> > * Instead of directly exec'ing a named program, the module invokes a 
> >   /bin/sh command string, for greater flexibility.
> 
> Which flexibility should this give?

You can use redirections, shell variables, environment assignments, even 
pipes in the command string...

	LANG=C make -C /var/yp >/var/log/yp.log 2>&1
	beep -f $PW_UID
	who | grep "^$PW_USER\$" | awk '{print $2}' | ...

...without needing to wrap them in a small, separate shell script. I was 
originally using execv(), and building up the array of arguments for it, 
but the benefits of the /bin/sh approach seemed much more compelling.

Oh, and it avoids some annoying parse issues, like if you have a quoted 
string with spaces:

	make CFLAGS="-O3 -g" foo

Here, PAM will give you an argument vector of

	make
	CFLAGS="-O3
	-g"
	foo

which doesn't work so well in execv() :-)

> The pam_exec line is from a pam_exec implementation calling openlog/closelog.
> This is very bad. You loose the information, which application called
> pam_exec, you destroy the syslog options of the main applications and you
> will get problems with threaded applications.
>  
> The pam_unix one looks more correct. You should always use pam_syslog.
> We will not accpet self written logging functions again in Linux-PAM,
> we are happy that all are replaced with pam_*() functions now.

Hmm. I had refrained from using Linux-PAM-specific functions, bearing in 
mind compatibility with OpenPAM et al., but I suppose the log() function 
can be #ifdef'ed appropriately. I'll make this change.

> > * The module will accept the following options, but I'm not sure what 
> >   exactly it should do (if anything) with them:
> > 
> > 	expose_account
> > 	try_first_pass
> > 	use_first_pass
> > 	use_mapped_pass
> 
> We don't need them at all.

Okay. Should the module accept-but-ignore them?

(I see the "4. Generic optional arguments" section in the manual states 
"Here we list the generic arguments that all modules can expect to be 
passed," which implies that this ought to be the case.)

> > * I'm a bit unclear on how pam_fail_delay() is supposed to work. Is the 
> >   manner in which I handled it---and described it in the documentation---at 
> >   all incorrect?
> 
> You should not call pam_fail_delay() from a module. The application has to
> set it. Else you will overwrite defaults which the sysadmin set.

Erm... the "2.2 Other functions provided by libpam" section states "The 
pam_fail_delay() function provides the mechanism by which an application or 
module can suggest a minimum delay (of micro_sec micro-seconds). Linux-PAM 
keeps a record of the longest time requested with this function."

Am I missing something, or is the manual out of date?

> I would prefer to handle such large changes like the kernel developers
> do: A lot of small patches for every new functionality, so that it
> is possible to review and discuss every change and not one very big
> patch.

I'm not quite sure how to approach that. The original code was fairly small 
and simple to begin with, and was refactored anyhow in the course of this 
work. Would you figure something of a reverse progression, starting from 
this code and removing some major feature at each turn?

Alternately... I did aim for good function delegation; all the routines 
(with the exception of the main do_exec() function) serve a well-defined 
purpose. Would a function-by-function review be workable?

This new implementation is not complicated in concept---for all the talk of 
"features," there really isn't that much new machinery here. The main bit 
of novelty is the code dealing with file descriptors and IPC.


--Daniel


-- 
NAME   = Daniel Richard G.       ##  Remember, skunks       _\|/_  meef?
EMAIL1 = skunk at iskunk.org        ##  don't smell bad---    (/o|o\) /
EMAIL2 = skunk at alum.mit.edu      ##  it's the people who   < (^),>
WWW    = http://www.******.org/  ##  annoy them that do!    /   \
--
(****** = site not yet online)