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.
LS
On Tue, Oct 21, 2014 at 12:44:00AM +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.
LS
Hi Nathaniel,
is this something you can help us test or shall we do the testing ourselves?
On Tue, 2014-10-21 at 09:59 +0200, Jakub Hrozek wrote:
On Tue, Oct 21, 2014 at 12:44:00AM +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.
LS
Hi Nathaniel,
is this something you can help us test or shall we do the testing ourselves?
Yes, I will help test. I will probably need a day or two though.
Nathaniel
On Tue, Oct 21, 2014 at 01:22:55PM -0400, Nathaniel McCallum wrote:
On Tue, 2014-10-21 at 09:59 +0200, Jakub Hrozek wrote:
On Tue, Oct 21, 2014 at 12:44:00AM +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.
LS
Hi Nathaniel,
is this something you can help us test or shall we do the testing ourselves?
Yes, I will help test. I will probably need a day or two though.
Nathaniel
No problem and thank you for testing!
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.
Nathaniel
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..
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 :-)
LS
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.
Nathaniel
On Wed, Oct 22, 2014 at 10:58:07AM -0400, 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?
I was just suggesting to attempt to proceed with login if possible...
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.
..but after some more thinking I agree with you. If those two functions fail, we are looking at a genuine bug, so it's better to abort.
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
LS
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 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.
Nathaniel
On Wed, Oct 22, 2014 at 02:55:09PM -0400, Nathaniel McCallum wrote:
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.
Nathaniel
And as I said before, I agree after some more pondering the code.
Does this mean an ack? :-) Or are there any further comments about the code?
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.
LS
On Thu, Oct 23, 2014 at 11:28:44AM +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.
LS
This patch looks good to me.
Nathaniel, do you have any further comments?
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
On Thu, Nov 06, 2014 at 04:37:31PM -0500, Nathaniel McCallum wrote:
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
Thank you for the review!
master: 2368a0fc19bcd56581eccd8397289e4513a383a5
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Nathaniel McCallum