[Freeipa-devel] Final OTP Review

[Rob Crittenden]

Simo Sorce wrote:
> On Fri, 2013-05-03 at 01:27 -0400, 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...
>>
>>> 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.
>
> Ok I took a deper look and now understand what it is doing.
> I think it is implementing RFC 6234 HMAC, but can't say for sure.
>
> The first NSS call creates a key container, the second initializes the
> context and tells NSS which HMAC algorythm to use and what is the key.
>
> Then the 3 calls simply (1) start the hmac calculation, (2) add in the
> data to be signed and (3) extracts the final signature on the data to be
> returned.
>
>>> - 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.
>
> Yeah I've seen that, looks a bit weird but makes perfect sense.
>
> I do not have any more concerns on patch 1, so it's an ACK from me for
> that one.
>
> I haven't yet gone through the whole companion daemon patch :/
>
> The otp ACIs one I think is wrong though, so still no full ack on the
> whole patchset.
>
> Simo.

This patch should fix things up.

rob

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-otp_aci.patch
Type: text/x-diff
Size: 13297 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20130503/f8152d7b/attachment.bin>