Dirk van Deun <dvand...@wilma.vub.ac.be> writes: > On Wed, Mar 06, 2024 at 09:11:54PM +0100, Simon Josefsson wrote: >> Dirk van Deun <dvand...@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