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

[Rob Crittenden]

Nathaniel McCallum wrote:
> 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).

Ok. Kerberos can be twitchy about FQDN :-)

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

I suppose not. Then again someone might see this in ps output and wonder 
what the heck it is :-) Not a show stopper IMHO.

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

You don't actually seem to use the rv from setup_ldap() other than 
whether it is non-zero so it may not be important. Think about where 
problems might occur and what debugging would be useful in tracking down 
the fixes. It may be that simply adding logging is ok and returning 1/0 
is enough.

My problem with systemd is that it seems to make things more opaque. It 
is hard to do even simple things like an strace on process startup 
because the shell is completely detached from execution. Which means 
that your logging needs to be top-notch so when things go wrong we can 
look somewhere and say "aha, it's X"

>> 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 wasn't a leading question. If there won't be multiple entries 
returned then I think we're ok.

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

When upgrading an IPA server new LDIF files are not copied around. We 
are actually working on changing this in the future, but for now you'll 
need to create a .update file in install/updates which creates the 
schema. You can use install/updates/10-selinuxusermap.update as an example.

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

With commit messages the format should be something like:

One-line description of reason for commit (you're good there).

One or more paragraphs describing the problem and solution. Anything 
that can be useful. Space is cheap, so write a book if you want (see 
some of John's commits).

URL for trac ticket

Design URL (if any)

When we close the ticket we will include the commit messages. This way 
it is easy to find a ticket or a fix multiple ways, and to some extent 
helps us figure out what release a change dropped into.

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

You might want to add a README with either a pointer to the design URL 
or the architecture statement from the design page. That pretty well 
describes what this is supposed to do. Again, not a show-stopper, just 
think of the poor developer who comes along in a year to try to 
understand how this works. I was a little baffled by the reason for the 
whole stdio stuff until I saw the design page.

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

I'm not sure how I'd test it if I got it built.

Overall looks really good.

rob