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