[Freeipa-devel] Final OTP Review

[Nathan Kinder]

On 05/02/2013 10:27 PM, Nathaniel McCallum wrote:
> 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...
We would need to set the precedence if you want a predictable/guaranteed 
execution order.  If a pre-BIND plug-in callback returns non-zero (which 
you should do when the plug-in sends the result to the client directly), 
it will cause other pre-bind plug-ins to not be called.  This is 
actually how all pre-op plug-ins work.  If a pre-op callback returns an 
error, we don't call the rest of the pre-op plug-ins in the list.
>
>> 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...
We should have Bob Relyea (cc'd) from the NSS development team take a 
look at it.

-NGK
>
> 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
>