On Thu, 2014-10-23 at 11:28 +0200, Lukas Slebodnik wrote: > On (22/10/14 14:55), Nathaniel McCallum wrote: > >On Wed, 2014-10-22 at 17:43 +0200, Lukas Slebodnik wrote: > >> On (22/10/14 10:58), Nathaniel McCallum wrote: > >> >On Tue, 2014-10-21 at 20:34 +0200, Lukas Slebodnik wrote: > >> >> On (21/10/14 20:06), Jakub Hrozek wrote: > >> >> >On Tue, Oct 21, 2014 at 01:29:53PM -0400, Nathaniel McCallum wrote: > >> >> >> On Tue, 2014-10-21 at 00:44 +0200, Lukas Slebodnik wrote: > >> >> >> > ehlo, > >> >> >> > > >> >> >> > We remove the password from the PAM stack when OTP is used to make > >> >> >> > sure > >> >> >> > that other pam modules (pam-gnome-keyring, pam_mount) cannot use > >> >> >> > it anymore > >> >> >> > and have to request a password on their own. > >> >> >> > > >> >> >> > Resolves: https://fedorahosted.org/sssd/ticket/2287 > >> >> >> > > >> >> >> > Simple patch is attached. > >> >> >> > >> >> >> I may be wrong, but I think that making the pam_add_response() and > >> >> >> pam_set_item() errors non-fatal is incorrect. Attempting to use the > >> >> >> OTP > >> >> >> credentials again could result in further errors, keyring problems or > >> >> >> account locking. It seems to me that it would better to fail the > >> >> >> authentication if you cannot guarantee that OTP credentials will not > >> >> >> be > >> >> >> reused. > >> >> > > >> >> >On the other hand, logging in as the user in question (and then letting > >> >> >him to sudo) might be the only way of getting access into the system at > >> >> >all.. > >> >> Should I change it or no? > >> >> > >> >> It would be very simple change :-) > >> > > >> >I'm not sure I understand Jakub's objection. Could someone clarify? > >> > > >> >As I understand it, a failure in these functions is largely restricted > >> >to thinks like OOM. In such a case, I wonder if login will be possible > >> >at all. > >> > > >> Sorry, I don't understand you. > >> Do you mean client part(src/sss_client/pam_sss.c) or responder part? > >> > >> Because on client part, OOM is not threated as failure. > >> 976 env_item = strdup((char *)&buf[p]); > >> 977 if (env_item == NULL) { > >> 978 D(("strdup failed")); > >> 979 break; > >> 980 } > >> 981 ret = putenv(env_item); > >> 982 if (ret == -1) { > >> 983 D(("putenv failed.")); > >> 984 break; > >> 985 } > >> > >> //break will cause jump out of switch and if there are no more data in > >> buffer > >> then PAM_SUCCESS will be returned from function eval_response > > > >Looking at the code again, I think the problem only exists in the > >handling of the return value from pam_add_response(). See your > >comment: /* Not fatal */ > > > I'm fine with changing code in back end. > I was against a change in client code. > > >I believe this block should be fatal. The function pam_add_response() > >only fails in the case of OOM (see src/providers/dp_pam_data_util.c). It > >is extremely likely that an OOM here, if ignored, will appear somewhere > >later in the chain. Since it is safe to fail the authentication here, we > >should do so. > > > >This is especially true because leaving the credentials on the pam stack > >may cause server-side issues like account locking. This is more > >difficult to recover from than an OOM-caused authentication failure. > > > >In short, if authentication succeeds but we cannot remove the > >credentials from the pam stack due to OOM (extremely unlikely), we > >should fail the authentication. > > > Updated version is attached.
I just finished testing this. Works for me. ACK Nathaniel _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
