[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Ilkka Virta
Subject: Re: [OATH-Toolkit-help] pam-oath, private usersfiles (feature request)
Date: Mon, 08 Dec 2014 13:39:33 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Thunderbird/24.6.0

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>

reply via email to

[Prev in Thread] Current Thread [Next in Thread]