[Freeipa-devel] [PATCH] krb 1.12's OTP-Over-RADIUS

[Rob Crittenden]

Nathaniel McCallum wrote:
> On Fri, 2013-04-12 at 17:39 -0400, Nathaniel McCallum wrote:
>> On Fri, 2013-04-12 at 11:53 -0400, Nathaniel McCallum wrote:
>>> On Fri, 2013-04-12 at 11:34 -0400, Nathaniel McCallum wrote:
>>>> On Thu, 2013-04-11 at 14:48 -0400, Rob Crittenden wrote:
>>>>> Nathaniel McCallum wrote:
>>>>>> On Wed, 2013-04-10 at 15:35 -0400, Rob Crittenden wrote:
>>>>>>> I'm not sure how I'd test it if I got it built.
>>>>>>
>>>>>> I'm working on this. I hope to have a clear answer next week. Bear with
>>>>>> me...
>>>>>>
>>>>>>> Overall looks really good.
>>>>>>
>>>>>> I've split up the patch into multiple commits. I've also added .update
>>>>>> files and a patch for ipa-kdb to feed krb5 the right user string.
>>>>>>
>>>>>> https://github.com/npmccallum/freeipa/commits/otp
>>>>>>
>>>>>> Please take a look. I *think* I've got everything worked out so far with
>>>>>> the exception of bug numbers / urls. Should every patch have a separate
>>>>>> bug and a link to the design page?
>>>>>
>>>>> The ticket should go into every commit. I'd probably put the design link
>>>>> there too, just for completeness. Future bug fixes, et all aren't going
>>>>> to require the design page, but since these commits are all related to
>>>>> the initial feature it will be nice to have.
>>>>>
>>>>> You can have multiple patches on the same ticket/bug.
>>>>
>>>> https://github.com/npmccallum/freeipa/commits/otp
>>>>
>>>> All four commits now have bug numbers and design page links. I'm adding
>>>> the design page link to the tickets as we speak.
>>>>
>>>> Remaining issues (AFAICS):
>>>> 1. The daemon (ipa-otpd) runs as root and binds anonymously
>>>> 2. ipatokenRadiusSecret is readable by an anonymous bind
>>> 3. ipatokenT?OTP.* are readable by an anonymous bind
>>>
>>> In the case of both #2 and #3, only admins should have RW. ipa-otpd
>>> needs read access to ipatokenRadiusSecret. The DS bind plugin below (#2)
>>> needs read access to ipatokenT?OTP.*.
>>>
>>>> Outstanding pieces:
>>>> 1. CLI tool -- https://fedorahosted.org/freeipa/ticket/3368
>>>> 2. DS bind plugin -- https://fedorahosted.org/freeipa/ticket/3367
>>>> 3. UI -- https://fedorahosted.org/freeipa/ticket/3369
>>>> 4. Self Service UI -- https://fedorahosted.org/freeipa/ticket/3370
>>>>
>>>> #1 and #2 are within the scope of F19 and should hopefully land shortly
>>>> (in separate commits). #3 and #4 are probably $nextrelease.
>>>>
>>
>> FYI - Here is an RPM with all of the code so far:
>> http://koji.fedoraproject.org/koji/taskinfo?taskID=5247029
>
> Updated RPMs, containing the new 389DS bind plugin and build for F19,
> are here:
> http://koji.fedoraproject.org/koji/taskinfo?taskID=5270926
>
> Nathaniel

BuildRequires needed for whatever provides krad.h

A bunch of the files declare functions in each other. Is it cleaner to 
put these into an include file? I'm gathering that this will always be 
self-contained, so maybe this is ok.

In entry_attr_get_berval() is it worth pointing out that there is no 
need to free the value, or is that assumed because it uses 
slapi_value_get_berval()?

If we detect that there is clock drift should we log it? Will we ever 
try to report to the client (e.g. future enhancement)?

I wonder if the NSPR-version of some functions should be used since this 
is running inside 389-ds, like PL_strcasecmp for strcasecmp()

ops.c:

pedantic: lack of spacing between if and parens

sha384 is an allowed type only in otp.c. Is that needed?

rob