[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]