[Freeipa-devel] [PATCH 0030] Add OTP sync plugin

[Simo Sorce]

On Thu, 2014-01-23 at 17:42 -0500, Nathaniel McCallum wrote:
> On Thu, 2014-01-23 at 14:23 -0500, Simo Sorce wrote:
> > On Thu, 2014-01-09 at 16:28 -0500, Nathaniel McCallum wrote:
> > > This plugin adds an extended operation for synchronizing tokens. This
> > > operation is availalbe both with and without bind. In the latter case,
> > > the first factor is required. This operation can also be performed
> > > on a per-token or per-user level. In the latter case, we will attempt
> > > to find the token automatically.
> > > 
> > > Thanks to Mark Reynolds for helping me with this patch.
> > 
> > 
> > Huge NACK,
> > 
> > otp_sync_request_authenticate() is not ok, it allows an attacker to do
> > password bruteforcing without any check.
> > 
> > This is the result of trying to re-implement what is basically a bind
> > but w/o using the ind code.
> > 
> > No checks to see if the user is enabled/disabled/locked
> > No updates for locking account after N wrong tries, etc...
> > 
> > It also duplicates the places to change should we wish to not depend on
> > having userPassword as an actual password field anymore (which I
> > considered multiple times as we already have kerberos keys against which
> > we could test binds in theory).
> > 
> > You should either extend the bind operation so all checks are used there
> > and re-sync is done at last if all checks pass, and as a bonus you end
> > up with an authenticated LDAP connection should you need it, or the bind
> > code that does all the checks need to be abstracted into helper
> > functions that can be reused by this plugin.
> 
> After discussing the details with nkinder, we have three options here.
> 
> 1. FreeIPA implements all of the bind operation from 389ds in
> ipa-pwd-extop. This is, I think, a bad idea.
> 
> 2. 389DS needs to grow a new plugin type which hooks immediately after
> the password validation completes, but before any changes are made to
> the connection state/pblock.
> 
> 3. Manually validate the password like we are doing now, but only for
> the synchronization operation. In this case, if the password doesn't
> match the normal bind process will fail and no synchronization will be
> performed. If the password does match, it will be validated twice: once
> for the synchronization operation and once for the bind. This could
> potentially allow a user who is locked out, for instance, to synchronize
> tokens. It would not, however, permit them to bind. Nor would it permit
> brute-forcing.
> 
> Thoughts?

So the proposal in (3) is to check the manually for the password first
(in case of synchronization), but then whether it matches or not keep
going and let 389ds finish the bind operation so that all accounting is
done ?

I think this is an acceptable compromise, but I think we may want to
open bugs to get (2) eventually, and then change the code.

Simo.

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