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.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to