[Freeipa-devel] Final OTP Review

Simo Sorce simo at redhat.com
Thu May 2 21:39:30 UTC 2013


On Thu, 2013-05-02 at 15:24 -0400, Nathaniel McCallum wrote:
> On Thu, 2013-05-02 at 12:18 -0400, Nathaniel McCallum wrote:
> > Attached are the patches from the ongoing OTP review with rcrit. We
> > believe these to be ready to merge. Please review. The first two patches
> > just add the required schema. The third patch adds support for OTP to
> > kdb. The fourth adds ipa-otpd, the otp companion daemon. The fifth, adds
> > the 389DS bind plugin. The sixth patch is cosmetic (.gitignore).
> > 
> > Code for managing tokens (CLI or GUI) remains to be written, though I do
> > have a rudimentary script for adding tokens for testing.
> > 
> > KNOWN ISSUES
> > 1. ipa-otpd runs as root. This trade-off exists to permit autobinding
> > for this PoC. Ideally, ipa-otpd would run as its own unprivileged user.
> > I'd like to address this for the N+1 release.
> > 2. krb5 currently requires the top three patches here in order to
> > properly trigger the otp code path:
> > https://github.com/greghudson/krb5/commits/keycheck. These should
> > hopefully be merged upstream soon and will be backported to krb5 1.11 in
> > Fedora 19 shortly.
> > 3. krb5 tickets can't be issued. This is due to an upstream ticket
> > issuance bug that was discovered on Monday. This occurs *after* the OTP
> > has already been validated. We are working on a fix for this.
> 
> rcrit noticed that I wasn't using pkgconfig in patch #5, which I fixed.
> He also merged patch #6. Attached are the five remaining patches.
> 
> Nathaniel


Will do one patch at a time as these are huge.

I think you should have separate mail threads per patch so that each can
be independently tracked in patchwork.
These patches are so huge I am going to have to write separate mails for
each anyway.




Patch 1 NACK:

- I see GPLv2 boiler plate, but we should use v3.

- Bad indentation with 2 spaces indent in several places.

- not required, but I find much better to use braces around single line
ifs, not only for pure stylistic reasons but for defensive programming,
it's very common to make mistakes later when modifying existing code and
forget to add braces when adding lines to the if statement.

- we do not do a new line after a the return type when declaring
functions.

Not ok:
int
fn_name()
{

ok:
int fn_name()
{

- Constructs like the following are not good:

if (slapi_entry_attr_find(e, type, &attr) != 0 || !attr)

Makes debugging hard and reading hard. It should be:

ret = slapi_entry_attr_find(e, type, &attr);
if (ret != 0 || attr) {
    do something;
}

In general it is not ok to call a function and test its value within an
if statement except for trivial ones that return booleans.
always:
ret = fn()
if (ret == ?) { }

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

- Please do not use if (1) { ... } constructs, it makes no sense, simply
remove the if statement and leave the contents.

- Please if you can use 'done' as a label to get out of a function

- token_decode(), credentials_parse() and ipa_otp_do_otp_auth() are
effectively returning a boolean state, please make them return bool and
true for success, false for failure.
(ps: I see this all over, please use bool everywhere you return
effectively a boolean state, not going to point out every single
function from now on)

- indentation issues at lines 524 and 527 of the patch, both case should
align after the previous line '('

- another bad testing pattern:
do not do things like:
ret = foo() == 0
if (ret) { ... }

do:

ret = fn()
if (ret == 0) { ... }



- Using a single ipa_otp_postop() function instead of one function per
operation makes things less clear, as you have to create the boilerplate
for each function seaprately anyway and then most of the function is in
the switch case statements which are completely separate. The only
common code is the initial checks that you have already split off in
_stared()/_oktodo() functions anyway.
Having separate function per operation would be preferable I think.

- bad indentation line 1054,1055,1065,1074,1092 and so on ... they
should be indented after the preceding line(s) '('
Also this is often the case with slapi_log_error() functions too, please
indent after the opening '(' on previous lines, not at a random
indentation.


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


- What about the comments labeled NGK ? Some of them seem to indicate
the plugin still has some deficiencies we should fix ? If so they should
use the label FIXME so that it gets readily highlighetd when looking at
the code.


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


Continuing with otp.c:

- what does 'egress' mean ?
 (can you just use 'done' as a standard label for exceptions ?)

- Is it ok to call PK11_DestroyContext() if ctx is NULL ?
Can't find much documentation but see #276314 / #276311 on
bugzilla.mozilla.org


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


- Why do we have ipa_otp_hotp if we implement only totp ?



In general the code seem ok functionally, but the style must be fixed.

In general please read http://www.freeipa.org/page/Coding_Style and
conform to the style used in other plugins where they do not explicitly
contradict the above document for uniformity.


Simo.

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




More information about the Freeipa-devel mailing list