Re: pam_oath with user_unknown=ignore

[Simon Josefsson]

Dirk van Deun <dvandeun@wilma.vub.ac.be> writes:

> On Wed, Mar 06, 2024 at 09:11:54PM +0100, Simon Josefsson wrote:
>> Dirk van Deun <dvandeun@wilma.vub.ac.be> writes:
>> 
>> > Hi,
>> >
>> > I really like the fact that you can use user_unknown=ignore to
>> > introduce pam_oath gradually, and it works fine if you use one users
>> > file to store all the secrets; but when you use a file per user 
>> > (like with usersfile=/oath/${USER}), users that do not have a secret
>> > yet do need an empty file for PAM_USER_UNKNOWN to be returned;
>> > without, you get a file-not-found kind of error instead.
>> >
>> > It is debatable whether this is a bug or just unexpected behaviour,
>> > but to me it would make more sense if in such a configuration, a
>> > missing file would also lead to PAM_USER_UNKNOWN.  
>> 
>> Yes this makes somewhat sense.  The patch to get your behaviour would
>> probably be something like this:
>> 
>> diff --git a/pam_oath/pam_oath.c b/pam_oath/pam_oath.c
>> index 25eb83e..f5ae490 100644
>> --- a/pam_oath/pam_oath.c
>> +++ b/pam_oath/pam_oath.c
>> @@ -303,7 +303,7 @@ pam_sm_authenticate (pam_handle_t *pamh,
>>      DBG (("authenticate first pass rc %d (%s: %s) last otp %s", rc,
>>        oath_strerror_name (rc) ? oath_strerror_name (rc) : "UNKNOWN",
>>        oath_strerror (rc), ctime (&last_otp)));
>> -    if (rc == OATH_UNKNOWN_USER)
>> +    if (rc == OATH_NO_SUCH_FILE || rc == OATH_UNKNOWN_USER)
>>        {
>>      retval = PAM_USER_UNKNOWN;
>>      goto done;
>> 
>> Although since (IIRC) 'user_unknown=ignore' is handled by the PAM layer
>> and not the pam_oath module, I worry that if we start to return
>> PAM_USER_UNKNOWN on file-not-found errors, we will cause unwanted
>> behaviour in other situations.
>> 
>> One ugly way to resolve this is to introduce a new keyword that cause
>> this particular behaviour to happen, for example
>> 'missing_usersfile_leads_to_user_unknown' causes OATH_NO_SUCH_FILE
>> errors to be translated into PAM_USER_UNKNOWN.  What do you think?
>> Would you like to make an attempt to make a patch?
>> 
>> /Simon
>
> We can try to do the right thing based on the usersfile parameter:
>
> - if usersfile does not contain a variable, then OATH_NO_SUCH_FILE
> is still an error;
>
> - if usersfile starts with ${HOME}, OATH_NO_SUCH_FILE gives rise to
> PAM_USER_UNKNOWN, except if the home directory in question does
> not exist, as then it is safer to assume that something went wrong
> (like homes not being mounted) and keep calling it an error;
>
> - if usersfile starts with a literal path that ends with a slash,
> followed by a variable (presumably ${USER}), followed by whatever,
> OATH_NO_SUCH_FILE gives rise to PAM_USER_UNKNOWN, except if the
> literal path part does not exist OR is an empty directory -- the
> is-empty test can be added because we can reasonably assume that at
> least one user on the system has configured oath; 
>
> - other cases are just weird, so the error may stay an error.
>
> Agree ?

This sounds complicated to me since it assumes even further semantic
understanding of the usersfile path value.  Understanding the difference
between a missing and unreadable home directory can be difficult for
network file systems.  Right now the error is returned on all fopen()
failures, which includes a number of different error conditions.  It
isn't clear to me that there aren't reasonable scenarios where a missing
usersfile should NOT lead PAM_USER_UNKNOWN errors, but a hard error:
maybe someone is using the current semantics and has a hierarchy of
usersfile for users, and expect the current behaviour.

It still seems simpler to introduce a new keyword that triggers
PAM_USER_UNKNOWN for OATH_NO_SUCH_FILE usersfile error.  Then this can
be documented as a new supported feature.

/Simon

signature.asc
Description: PGP signature