Re: Update memory and cached creds when changing password from gdmor xdm - Samba

This is a discussion on Re: Update memory and cached creds when changing password from gdmor xdm - Samba ; 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 ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: Re: Update memory and cached creds when changing password from gdmor xdm

  1. Re: Update memory and cached creds when changing password from gdmor xdm

    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.





  2. Re: Update memory and cached creds when changing password from gdmor xdm

    On 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.


  3. Re: Update memory and cached creds when changing password from gdmor xdm

    On 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.


+ Reply to Thread