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

[Nathaniel McCallum]

On Thu, 2013-04-04 at 11:59 -0400, Rob Crittenden wrote:
> Nathaniel McCallum wrote:
> > On Fri, 2013-03-08 at 09:50 -0500, Simo Sorce wrote:
> >> On Wed, 2013-03-06 at 13:04 -0500, Nathaniel McCallum wrote:
> >>> On Wed, 2013-03-06 at 12:56 -0500, Nathaniel McCallum wrote:
> >>>> Patch is attached.
> >>>>
> >>>> There are currently a few security downsides to this patch:
> >>>> 1. The daemon (ipa-otpd) runs as root and binds anonymously
> >>>> 2. ipatokenRadiusSecret is readable by an anonymous bind
> >>>>
> >>>> This patch also adds some new dependencies, namely:
> >>>> 1. libverto (a dependency of krb5)
> >>>> 2. systemd
> >>>> 3. a krb5 patched for libk5radius support [1]
> >>>>
> >>>> In the interest of trying to meet the Fedora Features deadline, I am
> >>>> providing the patch in spite of the above issues.
> >>>>
> >>>> Nathaniel
> >>>>
> >>>> 1 - http://bit.ly/ZqtK79
> >>>
> >>> Also, I assumed the usability of 2.16.840.1.113730.3.8.16 for the
> >>> schema. This will need to be verified and finalized.
> >>
> >> I reserved this OID space for ipa OTP schema.
> >
> > Thanks! For posterity, where is this documented?
> >
> > Nathaniel
> 
> In daemons/configure.ac you fix up the inconsistent tab/spacing we use 
> when displaying a summary for IPA Server, except you add a tab in the 
> CFLAGS line.

Fixed.

> Trailing white space in 70ipaotp.ldif

Fixed.

> Just a style thing, you use tmp as a variable a lot to do somewhat beefy 
> things (e.g. find_base())

Fixed.

> Does hostname need to be fully-qualified? We enforce this during the 
> install but things change. I don't know if that is important for this 
> daemon.

Hostname is only used for looking up results in the RADIUS configuration
on the server side. It is up to the admin to ensure a match here (the
admin will have to do it manually anyway).

> ENOMEM is used as an error code when some things fail, sometimes in 
> favor of the real error message:
> 
> +    /* Set Service-Type. */
> +    retval = k5_radius_attrset_add_number(ctx.attrs,
> +                                   k5_radius_attr_name2num("Service-Type"),
> + 
> K5_RADIUS_SERVICE_TYPE_AUTHENTICATE_ONLY);
> +    if (retval != 0) {
> +        log_err(ENOMEM, "Unable to set Service-Type");
> +        goto error;
> +    }

Fixed.

> No man pages

Do we make man pages for socket-activated processes in /usr/libexec?
Running this directly never makes sense...

> In setup_ldap() there might be an inconsequential memory leak. If there 
> is no base, and one is found, but some later part of the LDAP sequence 
> fails an error will be returned but the base will remain set. It seems 
> that you quit on all setup_ldap() failures so this is probably no big deal.

Fixed.

> In on_query_readable() is it an error if multiple entries are returned? 
> And there is tmp again. I had to do a double-take on the return value of 
> the parse_ commands to realize that tmp was the error string.

This is a good question. In theory there should never be multiple
entries. Is there something that looks broken here? I'm not sure if we
should error in this case or if we should silently proceed... So far
I've chosen the second option and I fixed a memory leak that occurs if
multiple entries are received.

> It doesn't look like the LDAP entry is freed in on_query_readable().

Fixed.

> We'll need to handle upgrades eventually too.

I'm not sure of the implications here. I thought this was done by
copy-schema-to-ca.py. But I guess I'm wrong.

> Can you add the ticket(s) to the git commit message.
> 
> If there are any wiki design pages it would help to point to those.

http://freeipa.org/page/V3/OTP

> You'll need to add the new BuildRequires to freeipa.spec.in too.

Fixed.

> There isn't a lot of documentation on what each of these files/functions 
> are supposed to do.

Fixed.

The latest version of the patch is here:
https://github.com/npmccallum/freeipa/commit/0d126d4c2449787527f7b507029ceba8aea1eb4c

Also note that you can now build this patch against krb5 1.11 in Fedora
rawhide (and f19 when the alpha freeze lifts).

Nathaniel