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

[Nathaniel McCallum]

On Fri, 2013-04-26 at 18:30 -0400, Rob Crittenden wrote:
> 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

It is there already. I updated the spec file to have a hard dependency
on krb5 >= 1.11.

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

Most of the functions are only needed in one other file. I thought this
was cleaner. I'm also open to suggestions on how to better split the
files so there are less includes...

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

I would hope it is assumed by const.

> 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)?

There are lots of clock related enhancements that should be low-hanging
fruit.

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

I did not fix this since strcasecmp() is available everywhere.

> ops.c:
> 
> pedantic: lack of spacing between if and parens

Fixed.

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

The RFC doesn't specify sha384 support. However, adding support for it
is somewhat obvious. In the spirit of "being liberal in what you
receive, strict in what you send" I added sha384 support in auth.c.

Nathaniel