[Freeipa-devel] Final OTP Review

[Simo Sorce]

On Thu, 2013-05-02 at 23:39 -0700, Nathan Kinder wrote:
> 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.

Ok, but this does not answer my question.
We definitely need to *always* run our other preop plugin as we do
sanity checks like verifying if the user is enabled/disabled etc...
Also we need to understand how to deal with migrating password auth when
OTP is enabeld.

TBH I think we should not have a separate OTP-auth plugin but we should
probably have a single plugin that handles authentication and the 2
should be merged. Keeping them separate is going to cause more harm than
good with unexpected interactions.

We could still have 2 plugins and simply move the prebind action
currently don in ipa-pwd-extop to the otp plugin by making some more
code common. But it is probably easier to just merge OTP into
ipa-pwd-extop right now than try to do a huge refactoring. We can always
refactor the ipa-pwd-extop plugin later.


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

I took a second stab afresh this morning and grokked it. No more
concerns from me. NSS'a API does look *very* ugly...

Simo.

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