[Freeipa-devel] Expired passwords cannot be changed via LDAP

[Martin Kosek]

On 06/09/2014 10:59 AM, Martin Kosek wrote:
> On 06/09/2014 08:21 AM, Martin Kosek wrote:
>> On 06/06/2014 05:47 PM, Nathaniel McCallum wrote:
>>> On Fri, 2014-06-06 at 11:43 -0400, Simo Sorce wrote:
>>>> On Fri, 2014-06-06 at 11:06 -0400, Nathaniel McCallum wrote:
>>>>> On Fri, 2014-06-06 at 08:00 -0400, Simo Sorce wrote:
>>>>>> On Fri, 2014-06-06 at 10:30 +0200, Martin Kosek wrote:
>>>>>>> On 05/31/2014 03:27 AM, Simo Sorce wrote:
>>>>>>>> I have rebased theold patch attached to the ticket, unfortunately I
>>>>>>>> haven't had time to test it yet, but didn't want to lose it in some
>>>>>>>> branch.
>>>>>>>>
>>>>>>>> Simo.
>>>>>>>
>>>>>>> I tested the patch and it worked fine, code also reads OK. Thus, I am willing
>>>>>>> to ACK it.
>>>>>>>
>>>>>>> I am just wondering if there is any scenario we could have missed, but I did
>>>>>>> not find any. In there is no push back against the patch I may just push it.
>>>>>
>>>>>> The only thing I would draw attention to is the fact that now I am
>>>>>> sending back the error directly once we have a negative return from the
>>>>>> function in which expiration is checked (ipapwd_authenticate).
>>>>>>
>>>>>> I could not see why we did, in fact, not do that before and I meant
>>>>>> asking Nathaniel if he had an explicit reason why we do not, as he is
>>>>>> the last one that did some significant refactoring in the bind preop
>>>>>> plugin.
>>>>>
>>>>> In the current design, if ipapwd_authenticate() fails, we defer to 389ds
>>>>> for the actual response to the client. The purpose for this is so that
>>>>> verification of the first factor always behaves the same, regardless of
>>>>> what is in pre-bind.
>>>>>
>>>>> So ipapwd_authenticate() is not actually the "final" authentication. It
>>>>> is a preliminary authentication to determine if the user should be able
>>>>> to write kerberos keys and perform OTP sync. So checking expiration in
>>>>> pre-bind only protects these two operations.
>>>>>
>>>>> I presume that 389ds also checks password expiration. If this assumption
>>>>> is true, ipapwd_authenticate() SHOULD NOT return any response to the
>>>>> client. It should simply fail key-write/otp-sync silently and let dirsrv
>>>>> return the error to the client.
>>>>
>>>> 389ds would check it if we were using 389ds native password policies but
>>>> we are not. So we need to check on our own, which is what this patch
>>>> implements.
>>>>
>>>>> The important point here is that so long as 389ds is verifying password
>>>>> expiration, using a correct-but-expired password will should not result
>>>>> in a bind in the current code. It will result in kerberos key writing
>>>>> and OTP sync. Do we actually care about protecting kerberos key writing
>>>>> and OTP sync from correct-but-expired passwords?
>>>>
>>>> No, but that's not the point.
>>>>
>>>>> If 389ds does not check password expiration, then we probably need to
>>>>> patch upstream 389ds or, at the very least, return an error to the
>>>>> client.
>>>>
>>>> It is not a 389ds bug, it is just an integration issue due to the fact
>>>> IPA uses a different schema for password policies (for compatibility
>>>> with the Kerberos schema).
>>>>
>>>>> Otherwise, if we don't care about protecting kerberos key writing and
>>>>> OTP sync from correct-but-expired passwords, this patch is entirely
>>>>> unnecessary.
>>>>>
>>>>> Otherwise, the patch is necessary, but should skip kerberos key writing
>>>>> and OTP sync and fall through silently to 389ds.
>>>>
>>>> If we fall through to 389ds then authentication will succeed.
>>>>
>>>> So from this discussion it seem to me we achieve the goal of the patch
>>>> and returning an error directly is ok.
>>>>
>>>> Unless Nathaniel has something *against* returning an error in this
>>>> place I think we are good to go.
>>>
>>> Looks good to me. ACK.
>>>
>>> Nathaniel
>>>
>>
>> Ok, thanks for discussion and double checking!
>>
>> Pushed to master: bfdbd3b6ad7c437e7dd293d2488b2d53f4ea7ba6
>>
>> Martin
> 
> I just read a question in the ticket from alonbl about how password change
> should work on the LDAP level:
> 
> https://fedorahosted.org/freeipa/ticket/1539#comment:29
> 
> Are we ready to say that from now on, expired passwords cannot be changed via
> LDAP? I.e. this workflow will no longer work:
> 
> $ ipa user-add --first=Foo --last Bar expired --random
> --------------------
> Added user "expired"
> --------------------
>   User login: expired
>   First name: Foo
>   Last name: Bar
>   Full name: Foo Bar
>   Display name: Foo Bar
>   Initials: FB
>   Home directory: /home/expired
>   GECOS: Foo Bar
>   Login shell: /bin/sh
>   Kerberos principal: expired at MKOSEK-FEDORA20.TEST
>   Email address: expired at mkosek-fedora20.test
>   Random password: qiCjofo.2pfT
>   UID: 1170600026
>   GID: 1170600026
>   Password: True
>   Member of groups: ipausers
>   Kerberos keys available: True
> 
> $ ldappasswd -h `hostname` -D
> uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -x -w qiCjofo.2pfT
> uid=expired,cn=users,cn=accounts,dc=mkosek-fedora20,dc=test -a qiCjofo.2pfT -s
> Secret123
> ldap_bind: Invalid credentials (49)
> 	additional info: The user password is expired
> 
> Thanks,
> Martin

As noted in other part of this thread, this also breaks password changes via
Web UI as the hooks use LDAP (and not kadmin) to update passwords.

Given all sort of issues we get, I am thinking we should just revert it unless
there is a quick fix available.

Martin