On 06/09/2014 12:15 PM, Alon Bar-Lev wrote: > > > ----- Original Message ----- >> From: "Martin Kosek" <mko...@redhat.com> >> To: "Nathaniel McCallum" <npmccal...@redhat.com>, "Simo Sorce" >> <s...@redhat.com> >> Cc: "freeipa-devel" <freeipa-devel@redhat.com> >> Sent: Monday, June 9, 2014 1:11:17 PM >> Subject: Re: [Freeipa-devel] Expired passwords cannot be changed via LDAP >> >> 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: expi...@mkosek-fedora20.test >>> Email address: expi...@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. > > The fix should be for the password modify to work within anonymous bind if > old password is specified. I am not sure why IPA enforces non anonymous bind > for this extended request. > > Applications should also be modified to perform anonymous bind, exactly per > this reason. > > Searching why IPA requires non anonymous bind is what led me to this bug... :)
Simo, do you know the historical reason why this is enforced in ipapwd_chpwop? By quickly looking at the code it should not be difficult to fix, but devil is in details and we need to be very cautious in this function. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
