[Freeipa-devel] Final OTP Review

Rob Crittenden rcritten at redhat.com
Fri May 3 02:29:26 UTC 2013


Simo Sorce wrote:
> On Thu, 2013-05-02 at 17:57 -0400, Rob Crittenden wrote:
>> 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.
>
> The second patch is big, I need more time to look at it.
>
> 3 and 5 look ok to me.
>
> On 4:
> - I would have called the file otp-aci.ldif (we have default-aci.ldif)
> not just otp.ldif

I named it that because I snuck in the container as well. If the file is 
renamed we should really break that part out.

> - I really do not like the deny acis that much.
> We may need to fix this with the post 3.2 ticket to handle allows
> differently. The problem is that adding denies here will make that work
> more difficult and is also tricky top get right, see below:
>
> - Is it right to deny all ipatokenRadiusConfigLink,
> ipatokenRadiusUserName for reading ?
> If nobody can access this information what do we store it for ?
> IIRC deny-first wins and the following allow will simply be ignored ?
> 9.7.2.2 of RHDS manual seem to confirm this.
>
> Same comment with the rest of the ACIs whic I see follow the same
> pattern.
>
> Make sure to tyest with a user that is not cn=Directory Manager

I've tested them all with real data with the exception of the Radius 
acis, but since they follow the same pattern it should be ok. No user 
can read any attribute but ipaUserAuthType, and only a member of the 
admins group can write it.

We are limited because we have a blanket read-all aci, so I had to go in 
and remove read access for each, then grant it back to admins. We will 
add in delegatable permissions in the future.

This code is testable against 389-ds as-is. I was able to create a user, 
configure it for OTP, create a token, add it to my Android phone, and 
use that to do a simple bind against 389-ds.

rob




More information about the Freeipa-devel mailing list