[Freeipa-devel] Final OTP Review

[Rob Crittenden]

Simo Sorce wrote:
> 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, I've done an initial review of these and give them an ACK. If 
you'd like to take a second look it'd be great. At this point though I'm 
not sure I want to break this out into separate e-mails. I don't want 
process to get in the way of progress.

rob