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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel