Update memory and cached creds when changing password from gdm
or xdm
boyang
boyang at novell.com
Fri Jul 4 03:00:45 GMT 2008
Jeremy Allison wrote:
> On 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_PASSWORD_EXPIRED), thus ther is no memory
>> creds, which causes winbindd_replace_memory_creds()
>> fail. It will return NT_STATUS_OBJECT_NAME_NOT_FOUND,
>> 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_NO_SUCH_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_LOGIN
>> 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.
>>
>
> Ok, 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_LOGIN;
> + }
>
> 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 (_pam_require_krb5_auth_after_chauthtok(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_STATUS_OK and
> cache_ret is an error not equal to NT_STATUS_OBJECT_NAME_NOT_FOUND
> or NT_STATUS_NO_SUCH_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 WINBINDD_OK.
>
> Also, this logic :
>
> + if (!(NT_STATUS_IS_OK(cache_ret)
> + || NT_STATUS_EQUAL(cache_ret, NT_STATUS_OBJECT_NAME_NOT_FOUND))) {
>
> for the error case looks wrong to me. I think it should be :
>
> + if (!(NT_STATUS_IS_OK(cache_ret)
> + && !NT_STATUS_EQUAL(cache_ret, NT_STATUS_OBJECT_NAME_NOT_FOUND))) {
>
> 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_IS_OK(cache_ret)
|| NT_STATUS_EQUAL(cache_ret, NT_STATUS_OBJECT_NAME_NOT_FOUND)))
is any error except NT_STATUS_OBJECT_NAME_NOT_FOUND. 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_STATUS_OBJECT_NAME_NOT_FOUND, 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(Oh, 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_NO_SUCH_USER)) {
+ result = NT_STATUS_OK;
+ }
@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 reliable....ugly?)
@@:
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_STATUS_OBJECT_NAME_
NOT_FOUND or NT_STATUS_NO_SUCH_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 --git a/source/nsswitch/pam_winbind.c b/source/nsswitch/pam_winbind.c
> index eff9101..57f7b86 100644
> --- a/source/nsswitch/pam_winbind.c
> +++ b/source/nsswitch/pam_winbind.c
> @@ -1914,15 +1914,16 @@ static BOOL _pam_require_krb5_auth_after_chauthtok(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, PAM_WINBIND_NEW_AUTHTOK_REQD_DURING_AUTH, &new_authtok_reqd_during_auth);
> pam_set_data(pamh, PAM_WINBIND_NEW_AUTHTOK_REQD_DURING_AUTH, 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_LOGIN;
> + /* Keep the WINBIND_CACHED_LOGIN 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 --git a/source/nsswitch/winbindd_pam.c b/source/nsswitch/winbindd_pam.c
> index 42540a6..27af806 100644
> --- a/source/nsswitch/winbindd_pam.c
> +++ b/source/nsswitch/winbindd_pam.c
> @@ -2048,20 +2048,44 @@ enum winbindd_result winbindd_dual_pam_chauthtok(struct winbindd_domain *contact
> done:
>
> if (NT_STATUS_IS_OK(result) && (state->request.flags & WBFLAG_PAM_CACHED_LOGIN)) {
> -
> +
> /* Update the single sign-on memory creds. */
> result = winbindd_replace_memory_creds(state->request.data.chauthtok.user,
> newpass);
>
> - if (!NT_STATUS_IS_OK(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_STATUS_OBJECT_NAME_NOT_FOUND, which is not a failure.
> + * --- BoYang
> + */
> + if (NT_STATUS_EQUAL(result, NT_STATUS_OBJECT_NAME_NOT_FOUND)) {
> + result = NT_STATUS_OK;
> + }
> +
> + if (!(NT_STATUS_IS_OK(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_NO_SUCH_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_NO_SUCH_USER)) {
> + result = NT_STATUS_OK;
> + }
> +
> if (!NT_STATUS_IS_OK(result)) {
> DEBUG(10,("Failed to store creds: %s\n", nt_errstr(result)));
> goto process_result;
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: boyang.vcf
Type: text/x-vcard
Size: 209 bytes
Desc: not available
Url : http://lists.samba.org/archive/samba-technical/attachments/20080704/c1a665d7/boyang.vcf
More information about the samba-technical
mailing list