Update memory and cached creds when changing password from gdm
or xdm
Jeremy Allison
jra at samba.org
Thu Jul 3 20:53:55 GMT 2008
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.
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.
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 ?
Thanks,
Jeremy.
-------------- next part --------------
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;
More information about the samba-technical
mailing list