[Freeipa-devel] Final OTP Review

Fri May 3 05:27:45 UTC 2013

All issues fixed unless noted below... The attached patches are tested
to work.

On Thu, 2013-05-02 at 17:39 -0400, Simo Sorce wrote:

> - (nit) slapi_ch_malloc/slapi_ch_strdup are not checked for failure
> (although I know slapi_ch_malloc() currently just aborts on failure, I
> think that is plain wrong which is why I would prefer using
> malloc/strdup, but well, I guess this is not something I am going to
> care too much about for now).

Not fixed.

> - Is the logic with auth_type_used correct ?
> At least the name of the variable sounds misleading, because you set it
> only if the authentication was successful but not set if it was 'used'
> but was unsuccessful. Made me look at it multiple times to reconstuct
> the logic. The var name could be better, however I also want a comment
> that explain exactly how we are going to manage authentication and
> fallbacks here as that logic is critical and is useful for anyone that
> is going to have to change this code later in order not to break it.

The variable is now gone. I re-factored the section to make the logic
clearer and put a nice big comment up top.

> - General question: how does this PRE_BIND plugin interact with
> ipapwd_pre_bind() in the ipa-pwd-extop() plugin ?
> Are you going to cause that plugin not to run by returning a result
> directly in this function ?
> Or is this plugin configured to run only after the previous one went
> through ?
> I ask because I do not see any ordering information in the cn=config
> plugin configuration so it is not immediately clear to me.

That is a good question for Nathan since he wrote this part of the
code...

> Continuing with otp.c:
> 
> - what does 'egress' mean ?
>  (can you just use 'done' as a standard label for exceptions ?)

Egress:
Noun - The action of going out of or leaving a place: "direct means of
access and egress".
Verb - Go out of or leave (a place).

In short: ingress means to enter and egress means to exit.

I have changed all 'egress' labels to 'done'.

> - Is it ok to call PK11_DestroyContext() if ctx is NULL ?
> Can't find much documentation but see #276314 / #276311 on
> bugzilla.mozilla.org

I added if's for all of these just to be defensive.

> - Can you please add a comment that describe which HMAC algorithm are
> you using (or a reference to a RFC of it ?) Unfortunately NSS makes
> thins a lot more cryptic than it should :(
> Also adding comments before the various NSS invocation to explain what
> they do would help, this code is obscure.

Unfortunately, that codes is mostly copy/paste from an NSS example of
how to do HMAC. It is just as unclear to me as it is to you. I have
added a link to the example with a note about me not understanding how
it works...

The good news is that it passes all the unit tests which use values
defined in the RFC. Also, valgrind reports no leaks or other errors. So
even if I don't know *how* it works, I do know that it does, in fact,
work.

> - Why do we have ipa_otp_hotp if we implement only totp ?

TOTP is a specialized case of HOTP. It is simply an alternative
mechanism for calculating the counter input to HOTP. Note that
ipa_otp_totp() is basically a one-liner. Since you *have* to implement
HOTP to get TOTP, you might as well expose the HOTP implementation for
future use.

Nathaniel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-Add-ipa-otp-389DS-bind-plugin.patch
Type: text/x-patch
Size: 76544 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130503/ea539454/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-Add-the-krb5-FreeIPA-RADIUS-companion-daemon.patch
Type: text/x-patch
Size: 59994 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130503/ea539454/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-ipa-kdb-Add-OTP-support.patch
Type: text/x-patch
Size: 6525 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130503/ea539454/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-IPA-OTP-schema-and-ACLs.patch
Type: text/x-patch
Size: 22741 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130503/ea539454/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-ipaUserAuthType-and-ipaUserAuthTypeClass.patch
Type: text/x-patch
Size: 4215 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130503/ea539454/attachment-0004.bin>