[Freeipa-devel] Patch for #1539

[Nathaniel McCallum]

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