[Freeipa-devel] [PATCH] radius work, please review

[Rob Crittenden]

John Dennis wrote:
> Attached are the patches for the radius work, please review.
> 
> * The first patch is for freeradius, it contains the C code
>   modifications for performing a SASL GSSAPI bind to the IPA directory
>   server using the radius service principal's keytab, for querying the
>   radius client information from LDAP, and the autoconf modifications
>   to support conditional compilation of the new krb and sasl code.
> 
>   The rpms have been build using Koji and can be accessed here:
> 
>   http://koji.fedoraproject.org/koji/buildinfo?buildID=26067
> 
> * The second patch is against our IPA source tree, please note this is
>   a patch showing the final cumulative differences for easier review,
>   not the 23 mercurial changesets comprising it. I will send Karl
>   the changeset file or he can pull the changes from my repo
>   depending on which he thinks is easier (contact me directly for the
>   repo address and password).
> 
> * These are the files changed in the patch:
> 
>   ipa-admintools/ipa-addradiusclient
>   ipa-admintools/ipa-addradiusprofile
>   ipa-admintools/ipa-delradiusclient
>   ipa-admintools/ipa-delradiusprofile
>   ipa-admintools/ipa-findradiusclient
>   ipa-admintools/ipa-findradiusprofile
>   ipa-admintools/ipa-radiusclientmod
>   ipa-admintools/ipa-radiusprofilemod
>   ipa-admintools/Makefile
>   ipa-python/ipaclient.py
>   ipa-python/ipautil.py
>   ipa-python/ipavalidate.py
>   ipa-python/radius_util.py
>   ipa-python/rpcclient.py
>   ipa-server/ipa-install/share/60radius.ldif
>   ipa-server/ipa-install/share/bootstrap-template.ldif
>   ipa-server/ipa-install/share/default-aci.ldif
>   ipa-server/ipa-install/share/encrypted_attribute.ldif
>   ipa-server/ipa-install/share/radius.radiusd.conf.template
>   ipa-server/ipaserver/radiusinstance.py
>   ipa-server/xmlrpc-server/funcs.py
>   ipa-server/xmlrpc-server/ipaxmlrpc.py
> 
> * I believe all the changes are exclusively related to radius and no
>   code in the general IPA code base was touched otherwise.
> 
> * During review I would like to draw your attention to the following
>   items:
> 
>   bootstrap-template.ldif: adds radius clients and profiles containers
>   under cn=services,cn=etc
> 
>   encrypted_attribute.ldif: sets the secret attribute in the radius
>   client objectclass to be encrypted using AES. At some point the ldiff
>   should be replaced with calls to perform an ldap modify as discussed
>   on the mailing list.
> 
>   default-aci.ldif: The ACI's were modified in two ways. First the
>   radiusprofile object class was added to the "Account Admins can manage
>   Users and Groups" acl. Second, the "Only radius and admin can access
>   radius service data" acl was added which denies all access to radius
>   service data except for the admin and the radius service principal.
> 
> * The command line utilities which manipulate LDAP attrbutes can:
> 
>   - take the attribute/value as a command line arg
> 
>   - take any number of argument strings containing any number of
>     attribute/value pairs
> 
>   - can read attribute/value pairs from a file or stdin
> 
>   - can interactively prompt for attribute/values with auto completion
>     of the attribute name and auto completion of the default value.
> 
>   - can delete attributes (critical as radius sometimes uses the
>     presence or absense of an attribute as a flag, setting the
>     attribute value to the empty string is not sufficient).
> 
> * Some general utility code was added to ipautil.py which provides the
>   following:
> 
>   - perform attribute/value pair auto completion on a TTY, returns set
>     of attribute/value pairs, values will auto complete to their
>     defaults or their previous value.
> 
>   - perform name auto completion on a TTY, returns a list of names.
> 
>   - parse attribute/value pairs from text strings (properly handles
>     quoting and escaped quotes)
> 
>   - read attribute/value pairs from a file (file can contain comments)
> 
>   - parse name list and read names from a file (can contain comments).
> 
>   - format for output a list of names in standard column format
>

Just a few comments.

1. In freeradius is it possible to have SASL2 without Kerberos? And 
vice-versa. What would happen in IPA if one or the other (or both) was 
missing? Should configure require both if either is found?

2. ipavalidate just adds a line at the end? I'm guessing you had your 
validation stuff in here originally and pulled it out? Would it be too 
much work to move some of it back in, such as the IP validation and 
length validation?

3. I like your UI for auto-completion. I may see if I can use that for 
other interactive work. The reason for validation on the front-end is to 
  error out without having to submit the entire work. We do need 
additional validation on the receiving end. LDAP does impose some of 
this though. Most of our values are user-type data though so there isn't 
much to validate (e.g. phone, street, etc). Not that there isn't a lot 
of work to do there.

4. As you can see in each tool there is a bunch of duplicated code (all 
the error catching at the end for example). Do you know of a way we can 
generalize that in Python and put it into a client library?

5. Can you write some man pages for the tools?

Nothing else jumped out at me. If I wanted to test this how might I go 
about it?

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 3245 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20071129/4309058d/attachment.bin>