[Freeipa-devel] Final OTP Review

[Rob Crittenden]

Rob Crittenden wrote:
> 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

The ACIs to let a user manage their own OTP needed some tweaking. It was 
loading in the wrong place for new installs and in both cases it lacked 
read access to objectclass so nothing was actually being granted.

rob

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