[Freeipa-devel] Final OTP Review

[Simo Sorce]

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.


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