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

[Ilkka Virta]

On 6.12. 17:32, Felix Salfelder wrote:
> Hi there.
>
> 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.

> 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?

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.

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?


> 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.)
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)

> please consider the attached commit.

> +prep_cfg (struct cfg *cfg, const char* user, char* scratch)

scratch is not used in the function?

> +       D (("found %h for %s", user));

Should this be %%h ?

> +    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.


--
Ilkka Virta <address@hidden>