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

[Simo Sorce]

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.

Simo.

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