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

[Rob Crittenden]

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.

Trailing white space in 70ipaotp.ldif

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

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.

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;
+    }

No man pages

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.

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.

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

We'll need to handle upgrades eventually too.

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.

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

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

rob