Update memory and cached creds when changing password from gdm orxdm

Hi, All:
There is a lot of pain when changing password from
gdm or xdm. Ie, When users try to login from gdm or
xdm, and password expires.

1. because user didn't login(PAM_AUTH returns
NT_STATUS_PASSWRD_EXPIRED), thus ther is no memory
creds, which causes winbindd_replace_memory_creds()
fail. It will return NT_STATUSBJECT_NAME_NT_FUND,
which is not a real failure. Because changing password
succeeded.

2. And there can be no cached creds(If it has been deleted
if cached creds reach the maximum cached number. Thus
Updating cached creds will probably fail with NT_STATUS_NSUCH_USER.
It is not a real failure too because changing password succeed.

3. When login from gdm or xdm with passthrough authentication.
there is no memory creds. Therefore, we should authenticate with
new password even for passthrough authentication to update memory
creds.

4. because updating cached creds in winbindd_dual_pam_chauthtok()
can probably fail. Therefore we should set WINBIND_CACHED_LGIN
bit in the authentication immediately after changing password
to cover the hole of the possible failure of updating creds
in winbindd_dual_pam_chauthtok.

Please correct if there is anything wrong.

Patch for v3-[023]-test in the attachment. Please review them.

Thanks very much!

Best
Regards
BoYang
First, July.

Tue, Jul 01, 2008 at 01:29:39PM +0800, boyang wrote:
Hi, All:
There is a lot of pain when changing password from
gdm or xdm. Ie, When users try to login from gdm or
xdm, and password expires.

1. because user didn't login(PAM_AUTH returns
NT_STATUS_PASSWRD_EXPIRED), thus ther is no memory
creds, which causes winbindd_replace_memory_creds()
fail. It will return NT_STATUSBJECT_NAME_NT_FUND,
which is not a real failure. Because changing password
succeeded.

2. And there can be no cached creds(If it has been deleted
if cached creds reach the maximum cached number. Thus
Updating cached creds will probably fail with NT_STATUS_NSUCH_USER.
It is not a real failure too because changing password succeed.

3. When login from gdm or xdm with passthrough authentication.
there is no memory creds. Therefore, we should authenticate with
new password even for passthrough authentication to update memory
creds.

4. because updating cached creds in winbindd_dual_pam_chauthtok()
can probably fail. Therefore we should set WINBIND_CACHED_LGIN
bit in the authentication immediately after changing password
to cover the hole of the possible failure of updating creds
in winbindd_dual_pam_chauthtok.

Please correct if there is anything wrong.

Patch for v3-[023]-test in the attachment. Please review them.

I'll review this tomorrow (2nd July Pacific time). Hopefully
we'll get this done for 3.0.31.

Thanks,

Jeremy.

Jeremy Allison wrote:
Tue, Jul 01, 2008 at 01:29:39PM +0800, boyang wrote:

>Hi, All:
>There is a lot of pain when changing password from
>gdm or xdm. Ie, When users try to login from gdm or
>xdm, and password expires.
>>
>1. because user didn't login(PAM_AUTH returns
>NT_STATUS_PASSWRD_EXPIRED), thus ther is no memory
>creds, which causes winbindd_replace_memory_creds()
>fail. It will return NT_STATUSBJECT_NAME_NT_FUND,
>which is not a real failure. Because changing password
>succeeded.
>
>2. And there can be no cached creds(If it has been deleted
>if cached creds reach the maximum cached number. Thus
>Updating cached creds will probably fail with NT_STATUS_NSUCH_USER.
>It is not a real failure too because changing password succeed.
>
>3. When login from gdm or xdm with passthrough authentication.
>there is no memory creds. Therefore, we should authenticate with
>new password even for passthrough authentication to update memory
>creds.
>>
>4. because updating cached creds in winbindd_dual_pam_chauthtok()
>can probably fail. Therefore we should set WINBIND_CACHED_LGIN
>bit in the authentication immediately after changing password
>to cover the hole of the possible failure of updating creds
>in winbindd_dual_pam_chauthtok.
>>
>Please correct if there is anything wrong.
>>
>Patch for v3-[023]-test in the attachment. Please review them.
>
>
I'll review this tomorrow (2nd July Pacific time). Hopefully
we'll get this done for 3.0.31.


Thanks for spending time on this. :-)

Thanks,
>
Jeremy.
>
>

Tue, Jul 01, 2008 at 01:29:39PM +0800, boyang wrote:
Hi, All:
There is a lot of pain when changing password from
gdm or xdm. Ie, When users try to login from gdm or
xdm, and password expires.

1. because user didn't login(PAM_AUTH returns
NT_STATUS_PASSWRD_EXPIRED), thus ther is no memory
creds, which causes winbindd_replace_memory_creds()
fail. It will return NT_STATUSBJECT_NAME_NT_FUND,
which is not a real failure. Because changing password
succeeded.

2. And there can be no cached creds(If it has been deleted
if cached creds reach the maximum cached number. Thus
Updating cached creds will probably fail with NT_STATUS_NSUCH_USER.
It is not a real failure too because changing password succeed.

3. When login from gdm or xdm with passthrough authentication.
there is no memory creds. Therefore, we should authenticate with
new password even for passthrough authentication to update memory
creds.

4. because updating cached creds in winbindd_dual_pam_chauthtok()
can probably fail. Therefore we should set WINBIND_CACHED_LGIN
bit in the authentication immediately after changing password
to cover the hole of the possible failure of updating creds
in winbindd_dual_pam_chauthtok.

Please correct if there is anything wrong.

, I've checked this patch, and there are some things wrong
with the logic I think. The idea is good though.

Problems I found (referring to the 3.0.x version of the patch) :

In nsswitch/pam_winbind.c, the added code :

+ if (cached_login) {
+ ctrl |= WINBIND_CACHED_LGIN;
+ }

is redundent. Exactly the same logic is being done above
at around 2133 before we get into this clause. the value
of 'cached_login' or 'ctrl' aren't changed between this
code (at 2133) and your addition after the line :

2145 if ((pamh, ctrl, user)) {

and so there's no need for the added code there.

Secondly, in nsswitch/winbindd_pam.c, the added logic
looks suspect to me. When result == NT_STATUSK and
cache_ret is an error not equal to NT_STATUSBJECT_NAME_NT_FUND
or NT_STATUS_NSUCH_USER, you jump to the exit condition
(process_result) but don't update the 'result' exit code.
Thus even in the fail case you'll return WINBINDDK.

Also, this logic :

+ if (!(NT_STATUS_ISK(cache_ret)
+ || NT_STATUS_EQUAL(cache_ret, NT_STATUSBJECT_NAME_NT_FUND))) {

for the error case looks wrong to me. I think it should be :

+ if (!(NT_STATUS_ISK(cache_ret)
+ && !NT_STATUS_EQUAL(cache_ret, NT_STATUSBJECT_NAME_NT_FUND))) {

This makes me worried that these checks may not be
correct. How did you test this ? With your logic
you jump to the error case when cache_ret is
*any* error, which is not what you want.

My guess is you got away with it due to the first error
w.r.t. 'result' I noted above.

I rewrote the patch for 3.0.x to do what I *thought*
you intended. Can you check this over please ?

What concerns me is that you've added logic into
nsswitch/pam_winbind.c that says:

"If we login from gdm or xdm and password expires,
we change password, but there is no memory cache.
Thus, even for passthrough login, we should do
authentication again to update memory cache."

But then you add logic in nsswitch/winbindd_pam.c
logic that claims :

"When login from gdm or xdm and password expires,
we change password, but there is no memory crendentials
So, winbindd_replace_memory_creds() returns
NT_STATUSBJECT_NAME_NT_FUND, it is not failure."

But haven't you just addressed that case with
the patch to nsswitch/pam_winbind.c ? In other
words, isn't the second part of your patch
not needed as in the first part you've already
assured that there will be memory credentials
even in the "login from gdm or xdm and password expires"
case ?

Thanks,

Jeremy.

Jeremy Allison wrote:
Tue, Jul 01, 2008 at 01:29:39PM +0800, boyang wrote:

>Hi, All:
>There is a lot of pain when changing password from
>gdm or xdm. Ie, When users try to login from gdm or
>xdm, and password expires.
>>
>1. because user didn't login(PAM_AUTH returns
>NT_STATUS_PASSWRD_EXPIRED), thus ther is no memory
>creds, which causes winbindd_replace_memory_creds()
>fail. It will return NT_STATUSBJECT_NAME_NT_FUND,
>which is not a real failure. Because changing password
>succeeded.
>
>2. And there can be no cached creds(If it has been deleted
>if cached creds reach the maximum cached number. Thus
>Updating cached creds will probably fail with NT_STATUS_NSUCH_USER.
>It is not a real failure too because changing password succeed.
>
>3. When login from gdm or xdm with passthrough authentication.
>there is no memory creds. Therefore, we should authenticate with
>new password even for passthrough authentication to update memory
>creds.
>>
>4. because updating cached creds in winbindd_dual_pam_chauthtok()
>can probably fail. Therefore we should set WINBIND_CACHED_LGIN
>bit in the authentication immediately after changing password
>to cover the hole of the possible failure of updating creds
>in winbindd_dual_pam_chauthtok.
>>
>Please correct if there is anything wrong.
>
>
, I've checked this patch, and there are some things wrong
with the logic I think. The idea is good though.
>
Problems I found (referring to the 3.0.x version of the patch) :
>
In nsswitch/pam_winbind.c, the added code :
>
+ if (cached_login) {
+ ctrl |= WINBIND_CACHED_LGIN;
+ }
>
is redundent. Exactly the same logic is being done above
at around 2133 before we get into this clause. the value
of 'cached_login' or 'ctrl' aren't changed between this
code (at 2133) and your addition after the line :
>
2145 if ((pamh, ctrl, user)) {
>
and so there's no need for the added code there.

I agree with you.
Secondly, in nsswitch/winbindd_pam.c, the added logic
looks suspect to me. When result == NT_STATUSK and
cache_ret is an error not equal to NT_STATUSBJECT_NAME_NT_FUND
or NT_STATUS_NSUCH_USER, you jump to the exit condition
(process_result) but don't update the 'result' exit code.
Thus even in the fail case you'll return WINBINDDK.
>
Also, this logic :
>
+ if (!(NT_STATUS_ISK(cache_ret)
+ || NT_STATUS_EQUAL(cache_ret, NT_STATUSBJECT_NAME_NT_FUND))) {
>
for the error case looks wrong to me. I think it should be :
>
+ if (!(NT_STATUS_ISK(cache_ret)
+ && !NT_STATUS_EQUAL(cache_ret, NT_STATUSBJECT_NAME_NT_FUND))) {
>
This makes me worried that these checks may not be
correct. How did you test this ? With your logic
you jump to the error case when cache_ret is
*any* error, which is not what you want.

(!(NT_STATUS_ISK(cache_ret)
|| NT_STATUS_EQUAL(cache_ret, NT_STATUSBJECT_NAME_NT_FUND)))
is any error except NT_STATUSBJECT_NAME_NT_FUND. The pairs of () seems
to lead misunderstanding.

My guess is you got away with it due to the first error
w.r.t. 'result' I noted above.
>
I rewrote the patch for 3.0.x to do what I *thought*
you intended. Can you check this over please ?
>
What concerns me is that you've added logic into
nsswitch/pam_winbind.c that says:
>
"If we login from gdm or xdm and password expires,
we change password, but there is no memory cache.
Thus, even for passthrough login, we should do
authentication again to update memory cache."
>
But then you add logic in nsswitch/winbindd_pam.c
logic that claims :
>
"When login from gdm or xdm and password expires,
we change password, but there is no memory crendentials
So, winbindd_replace_memory_creds() returns
NT_STATUSBJECT_NAME_NT_FUND, it is not failure."
>
But haven't you just addressed that case with
the patch to nsswitch/pam_winbind.c ? In other
words, isn't the second part of your patch
not needed as in the first part you've already
assured that there will be memory credentials
even in the "login from gdm or xdm and password expires"
case ?

Reauthentication happens after changing password. Therefore,
when we change password, there is no memory creds. *BUT*
after changing password, we have to update memory creds.
In winbindd_dual_pam_chauthtok(), update memory creds
will definitely fail if password is changed from gdm or xdm
because there is no memory creds at that time.

So, I think you've looked the way around. It worked as:
changing password updated memory creds fail >
reauthentication to update memory creds successfully.
*BUT* you look it as:
authentication(, yes. memory creds exists) >
changing password update memory creds(memory
creds is there, so update succeed.)
:-).
Did I understand you correctly?

@About patches:
I think there is only one careless mistake. cache_ret is not
replaced with result. :-)
here:

+ if (NT_STATUS_EQUAL(cache_ret, NT_STATUS_NSUCH_USER)) {
+ result = NT_STATUSK;
+ }


@Plus:
There seems to be some duplicate in pam_sm_chauthtok() and
winbindd_dual_pam_chauthtok(). That is to say, why we not abandon
memory creds and cached creds updating in winbindd_dual_pam
_chauthtok()? And just force one reauthentication after password change?

The idea behind the duplication is password can be changed by user
other than the user itself, eg, administrator. So, we give a chance to
update
cached creds if it is admin that change the password. (even the updating
of cached creds is not quite reliableugly?)

@@:
Sorry. It seems I was out of order when write the patch. There is
so many
logic problems. :-).
And my test had not return errors other than NT_STATUSBJECT_NAME_
NT_FUND or NT_STATUS_NSUCH_USER. Sorry.
Thank you very much for your patient guidance! thank you!
I can resubmit the patch at 7th, July. To avoid duplicate work, who
will rewrite
the patch, you or I?
Thanks,
>
Jeremy.
>
>
>


>
diff
index eff910157f7b86 100644


@@ -1914,15 +1914,16 @@ static BL (pam_handle_t *pamh, int ctrl,
/* Make sure that we only do this if
* a) the chauthtok got initiated during a logon attempt (authenticate->acct_mgmt->chauthtok)
* b) any later password change via the "passwd" command if done by the user itself
+ *
+ * NB. If we login from gdm or xdm and password expires, we change
+ * the password, but there is no memory cache. Thus, even for
+ * passthrough login, we should login again to update the memory cache.
+ * BoYang
*/

char *new_authtok_reqd_during_auth = NULL;
struct passwd *pwd = NULL;

- if (!(ctrl & WINBIND_KRB5_AUTH)) {
- return False;
- }
-
_pam_get_data(pamh, , &new_authtok_reqd_during_auth);
pam_set_data(pamh, , NULL, NULL);

@@ -2146,8 +2147,14 @@ int pam_sm_chauthtok(pam_handle_t * pamh, int flags,
const char *member = get_member_from_config(pamh, argc, argv, ctrl, d);
const char *cctype = get_krb5_cc_type_from_config(pamh, argc, argv, ctrl, d);

- /* clearing offline bit for auth */
- ctrl &= ~WINBIND_CACHED_LGIN;
+ /* Keep the WINBIND_CACHED_LGIN bit for
+ * authentication after changing the password
+ * if cached logins were enabled.
+ * This will update the cached credentials in case
+ * that winbindd_dual_pam_chauthtok() failed
+ * to update them.
+ * BoYang
+ */

ret = winbind_auth_request(pamh, ctrl, user, pass_new,
member, cctype, &response, NULL, &username_ret);
diff
index 42540a627af806 100644


@@ -2048,20 +2048,44 @@ enum winbindd_result winbindd_dual_pam_chauthtok(struct winbindd_domain *contact
done:

if (NT_STATUS_ISK(result) && (state->request.flags & WBFLAG_PAM_CACHED_LGIN)) {
-
+
/* Update the single sign-on memory creds. */
result = winbindd_replace_memory_creds(state->request.data.chauthtok.user,
newpass);

- if (!NT_STATUS_ISK(result)) {
- DEBUG(10,("Failed to replace memory creds: %s\n", nt_errstr(result)));
+ /*
+ * When we login from gdm or xdm and the password expires,
+ * we change the password, but there are no memory crendentials.
+ * So, winbindd_replace_memory_creds() returns
+ * NT_STATUSBJECT_NAME_NT_FUND, which is not a failure.
+ * BoYang
+ */
+ if (NT_STATUS_EQUAL(result, NT_STATUSBJECT_NAME_NT_FUND)) {
+ result = NT_STATUSK;
+ }
+
+ if (!(NT_STATUS_ISK(result))) {
+ DEBUG(10,("Failed to replace memory creds: %s\n",
+ nt_errstr(result)));
goto process_result;
}

if (lp_winbind_offline_logon()) {
+
+ /* Again, if we login from gdm or xdm
+ * and the password expires, cached crendentials
+ * doesn't exist. winbindd_update_creds_by_name()
+ * returns NT_STATUS_NSUCH_USER.
+ * This is not a failure. BoYang
+ */
+
result = winbindd_update_creds_by_name(contact_domain,
state->mem_ctx, user,
newpass);
+ if (NT_STATUS_EQUAL(cache_ret, NT_STATUS_NSUCH_USER)) {
+ result = NT_STATUSK;
+ }
+
if (!NT_STATUS_ISK(result)) {
DEBUG(10,("Failed to store creds: %s\n", nt_errstr(result)));
goto process_result;
>

Fri, Jul 04, 2008 at 11:00:45AM +0800, boyang wrote:

(!(NT_STATUS_ISK(cache_ret)
|| NT_STATUS_EQUAL(cache_ret, NT_STATUSBJECT_NAME_NT_FUND)))
is any error except NT_STATUSBJECT_NAME_NT_FUND. The pairs of () seems
to lead misunderstanding.

Ah, yes - now I see - thanks. I missed the placing of the
()'s. I prefer my way of doing it though :-). Less likely
to lead to misunderstandings :-).

Reauthentication happens after changing password. Therefore,
when we change password, there is no memory creds. *BUT*
after changing password, we have to update memory creds.
In winbindd_dual_pam_chauthtok(), update memory creds
will definitely fail if password is changed from gdm or xdm
because there is no memory creds at that time.

So, I think you've looked the way around. It worked as:
changing password updated memory creds fail >
reauthentication to update memory creds successfully.
*BUT* you look it as:
authentication(, yes. memory creds exists) >
changing password update memory creds(memory
creds is there, so update succeed.)
:-).
Did I understand you correctly?

No, I thought the logic looked like :

change password, followed by authentication,
followed by update memory creds.

but your explaination makes sense instead. Thanks.

@About patches:
I think there is only one careless mistake. cache_ret is not
replaced with result. :-)
here:

+ if (NT_STATUS_EQUAL(cache_ret, NT_STATUS_NSUCH_USER)) {
+ result = NT_STATUSK;
+ }

Yes - well caught. That should be "result" instead of
cache_ret.

@Plus:
There seems to be some duplicate in pam_sm_chauthtok() and
winbindd_dual_pam_chauthtok(). That is to say, why we not abandon
memory creds and cached creds updating in winbindd_dual_pam
_chauthtok()? And just force one reauthentication after password change?

The idea behind the duplication is password can be changed by user
other than the user itself, eg, administrator. So, we give a chance to
update
cached creds if it is admin that change the password. (even the updating
of cached creds is not quite reliableugly?)

Don't want to mess with this. It's complicated :-). In order to
do so I'd have to go through this entire code again with testing,
I don't have time for that right now :-(.

@@:
Sorry. It seems I was out of order when write the patch. There is
so many
logic problems. :-).
And my test had not return errors other than NT_STATUSBJECT_NAME_
NT_FUND or NT_STATUS_NSUCH_USER. Sorry.
Thank you very much for your patient guidance! thank you!
I can resubmit the patch at 7th, July. To avoid duplicate work, who
will rewrite
the patch, you or I?

Apart from the "cache_ret" should be "result" error you pointed
out above, does my patch functionally fix the issues I pointed
out before ? If so, then I'll fix that one error and commit it.

If not, let me know what other problems you see (let's not
go into fixing the duplication right now, too much change to
take on for a simple fix :-).

I'm happy to finish this up and commit, but I'd appreciate
you being able to test it for me.

Thanks,

Jeremy.

Jeremy Allison wrote:
Fri, Jul 04, 2008 at 11:00:45AM +0800, boyang wrote:
>

>(!(NT_STATUS_ISK(cache_ret)
>|| NT_STATUS_EQUAL(cache_ret, NT_STATUSBJECT_NAME_NT_FUND)))
>is any error except NT_STATUSBJECT_NAME_NT_FUND. The pairs of () seems
>to lead misunderstanding.
>
>
Ah, yes - now I see - thanks. I missed the placing of the
()'s. I prefer my way of doing it though :-). Less likely
to lead to misunderstandings :-).
>

>Reauthentication happens after changing password. Therefore,
>when we change password, there is no memory creds. *BUT*
>after changing password, we have to update memory creds.
>In winbindd_dual_pam_chauthtok(), update memory creds
>will definitely fail if password is changed from gdm or xdm
>because there is no memory creds at that time.
>>
>So, I think you've looked the way around. It worked as:
>changing password updated memory creds fail >
>reauthentication to update memory creds successfully.
>*BUT* you look it as:
>authentication(, yes. memory creds exists) >
>changing password update memory creds(memory
>creds is there, so update succeed.)
>:-).
>Did I understand you correctly?
>
>
No, I thought the logic looked like :
>
change password, followed by authentication,
followed by update memory creds.
>
but your explaination makes sense instead. Thanks.
>

>@About patches:
>I think there is only one careless mistake. cache_ret is not
>replaced with result. :-)
>here:
>>
>+ if (NT_STATUS_EQUAL(cache_ret, NT_STATUS_NSUCH_USER)) {
>+ result = NT_STATUSK;
>+ }
>
>
Yes - well caught. That should be "result" instead of
cache_ret.
>

>@Plus:
>There seems to be some duplicate in pam_sm_chauthtok() and
>winbindd_dual_pam_chauthtok(). That is to say, why we not abandon
>memory creds and cached creds updating in winbindd_dual_pam
>_chauthtok()? And just force one reauthentication after password change?
>>
>The idea behind the duplication is password can be changed by user
>other than the user itself, eg, administrator. So, we give a chance to
>update
>cached creds if it is admin that change the password. (even the updating
>of cached creds is not quite reliableugly?)
>
>
Don't want to mess with this. It's complicated :-). In order to
do so I'd have to go through this entire code again with testing,
I don't have time for that right now :-(.

:-(. I will figure out how to test this.

>@@:
>Sorry. It seems I was out of order when write the patch. There is
>so many
>logic problems. :-).
>And my test had not return errors other than NT_STATUSBJECT_NAME_
>NT_FUND or NT_STATUS_NSUCH_USER. Sorry.
>Thank you very much for your patient guidance! thank you!
>I can resubmit the patch at 7th, July. To avoid duplicate work, who
>will rewrite
>the patch, you or I?
>
>
Apart from the "cache_ret" should be "result" error you pointed
out above, does my patch functionally fix the issues I pointed
out before ? If so, then I'll fix that one error and commit it.

Yes. It fixed the issue you pointed out before.
If not, let me know what other problems you see (let's not
go into fixing the duplication right now, too much change to
take on for a simple fix :-).
>
I'm happy to finish this up and commit, but I'd appreciate
you being able to test it for me.

Sure. I'll test it.
Thanks,
>
Jeremy.
>
>

[GOOGLE]8nike.com has been totally updated which has many new items in stock,such as Nike,Addidas,
Gucci,Chanel,LV,Lacoste,Boss,Evisu,Puma,Bape,Polo and so on .They are all with most fashion,top quality and nice
price.We offer the fastest shipping with EMS,UPS,DHL,TNT,FEDEX ,and the payment way as Western Union,
Money Booker,Bank Transfer and Paypal,PAYPAL accept large order now!!!
We will always offer you the best price and nice service,if you prefer some,please contact us!Thanks!
Let's do best for you!
msn: china8nike at live.cn
e-mail: sara at cn-fr.com [/GOOGLE]

Mon, Jul 07, 2008 at 02:44:12PM +0800, boyang wrote:

I have tested it with kdm/xdm as well as from command line("passwd" command,
Both as the user itself and root), it worked as expected.

I rewrote the patch for v3-[023]-test, which arrives in the attachment.

Please review them.

Thank you very much!

No problem, I'll review these today. Last weekend
was a long weekend (which is why I didn't get to
this friday).

Cheers,

Jeremy.

Mon, Jul 07, 2008 at 02:44:12PM +0800, boyang wrote:

I have tested it with kdm/xdm as well as from command line("passwd" command,
Both as the user itself and root), it worked as expected.

I rewrote the patch for v3-[023]-test, which arrives in the attachment.

Please review them.

Thank you very much!

Pushed to all branches - thanks a lot !

Jeremy.