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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
