Re: [OATH-Toolkit-help] pam-oath, private usersfiles (feature request)

[Felix Salfelder]

Hi Ilkka.

On Mon, Dec 08, 2014 at 01:39:33PM +0200, Ilkka Virta wrote:
> >it might make sense to not put user credentials into a global
> >configuration file. similarly, ~/.ssh/authorized_keys is per-user.
> 
> Of course this opens the usersfile handling code to potentially
> dangerous input from the users.

good point.

> >the current usersfile argument only specifies one global path. this may
> >be extended allowing some magic substring, like "%h" to point to the
> >home directory of the user that tries to authenticate.
> 
> Isn't ~ traditional for home directories?

yes. %h was inspired by
#AuthorizedKeysFile  %h/.ssh/authorized_keys
in my sshd_config. perhaps "~" is better -- i don't really care.

> There's probably no need to ever fill the home directory to the
> _middle_ of a path? Just supporting paths like the above
> ~/.ssh/authorized keys should be enough if the goal is to let users
> control their own authentication files.

i'm not too sure if "%h" approach is a good idea at all. but yes, in
that case a prefix should be sufficient. how about "%u" for the login
name?

> Another questions is if the file in the user's home directory should
> be treated differently from a global one. There's no need for the
> username to appear there, so should it be removed totally?

maybe pass username=NULL to oath_authenticate_usersfile & al.?

> >this will lock out users that do not provide a usersfile, an additional
> >"optional" flag may then bypass the authorization (but there might be a
> >better way).
> 
> I first thought if the name should be better, but actually, perhaps
> the PAM module should support optional authentication with the
> global file too. (i.e. return ok if the user is not found in the
> file.)

i did not do the "optional" thing on a higher level because i don't know
how. can you give me a pointer?

> Then one would not need a separate pam module to filter the users,
> though making a typo in the usersfile would be more severe (the same
> goes for a per-user file if one creates it with a wrong name).

roughly the (optional) userfile was meant to lock out users, so there
would be no lock-out if it does not exist. similarly, i'd like to get
PAM_SUCCESS in case the userfile is broken/incomplete/unparseable...
clearly, YMMV.

> > +prep_cfg (struct cfg *cfg, const char* user, char* scratch)
> 
> scratch is not used in the function?

i wasn't sure where to allocate the scratch. i haven't implemented the
other case (scratch!=NULL). maybe it's just pointless.

> >+    D (("found %h for %s", user));
> 
> Should this be %%h ?

yes.

> 
> >+    scratch = prep_cfg(&cfg, user, scratch);
> >+
> >+    if(cfg.optional)
> >+      {
> >+        infh = fopen (cfg.usersfile, "r");
> >+        if (!infh)
> >+      {
> >+        DBG (("optional usersfile does not exist, PAM_SUCCESS"));
> >+        return PAM_SUCCESS;
> 
> The return will leak the memory allocation made to scratch.

indeed.

thanks
felix