[Freeipa-devel] [PATCH] radius work, please review
John Dennis
jdennis at redhat.com
Thu Nov 29 22:44:14 UTC 2007
Rob Crittenden wrote:
> 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?
GSSAPI (e.g. kerberos) is just one of many possible SASL mechanisms
which is why I separated the two as build options, not so much for us
but more for the pleasure of upstream and other builders of freeradius.
One shouldn't be required to compile and link with kerberos simply to
use SASL.
I had contemplated forcing the SASL build option on if kerberos is
specified but that was inconsistent with how other build options are
handled. You'll get an error and it's up to you to then reconfigure.
> 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?
Rob wins the prize!!! You found the file which was different by only a
blank line. I was going to fix it but decided it wasn't worth the trouble.
Yes, it was because I used to have the validation code in there, but
moved it to a radius specific file. Why? Because it really is freeradius
specific. FreeRADIUS has a specifc format they will accept ip addresses
in, including an optional mask. For instance the mask has to be
specified as a "bit shift" you can't use a colon followed by dotted
octets which is another popular mask format. The length values are also
freeradius specific (not even radius specific). That's why I moved the
validation code out of the shared file into something radius specific, I
didn't think it was general enough.
> 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.
I understand the reasoning. It would seem to be easier on the caller not
to have to be aware of how to validate what they are about to submit. It
also seems better to keep all the validation logic in just one place so
it's easier to fix and update. I'm not sure the cost of the futile round
trip when the validation fails is a huge issue and warrants duplicating
the validation. But like I say, I understand the rationale, just two
different approaches, each with their merits.
> 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.
I think you're always going to have to validate on the back end because
you don't know if the caller did so a priori. You're right, in many
instances LDAP will catch bad data, but not always, at least not when
it's application specific. For instance the internal free radius code
will skip any clients whose client data fails to meet any of its
internal requirements. It will log an error, but that happens outside of
IPA's awareness. One could spend a lot of time tracking down why
something is not working because the error won't perculate back up and
report itself to IPA so it's better to make sure it never gets accepted
in the first place. A different rev of freeradius or a different radius
implementation will have different requirements and you don't want to
embed those version specific limitations in the schema so LDAP won't be
able to catch it.
> 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?
Yes, I was becoming painfully aware of the code duplication. In other
languages perhaps the ideal way to deal with this would be macros or
templates, neither of which are available in Python.
Thus the best way to do this in Python I believe is to create a generic
class and then sub-class it for specific requirements.
What I noticed was often there was only 1 or 2 lines different and a
subclass override might not be the most elegant way to express that
difference, that's I why I would think macros or templates would be
better, but I think most things can be condensed and share common code
reasonably well by subclassing if we're careful about partitioning the
logic into smaller class methods which can easily be overriden to
accommodate the small unique differences.
> 5. Can you write some man pages for the tools?
Yeah it occurred to me I was missing man pages. Damn you caught me :-)
Yes, I'll write the missing man pages.
> Nothing else jumped out at me. If I wanted to test this how might I go
> about it?
Well, there are two things you might want to test.
1) Verify the data gets created in LDAP
use ipa-addradiusclient to create a client, test by either doing a ldap
search from the command line or use ipa-findradiusclient. Note,
ipa-findradiusclient will take a series of client addresses or you can
use wildcards. I often just do "ipa-findradiusclient \*". Test again by
modifying an attribute of the client and dumping the results. Delete the
client, verify by dumping again.
Same holds true with radius profiles. Note with radius profiles they can
be attached to a user -or- live on their on as a shared global profile.
All the *radiusprofile* commands take a -s argument to specify you're
operating on a shared profile as opposed to one attached to a user. I
don't permit adding a radius profile on a user because all users get a
default empty profile, thus ipa-addradiusprofile can only be used to
create a shared global profile (same holds true for
ipa-delradiusprofile). But ipa-radiusprofilemod will operate on either
per-user or shared profiles. Try adding an attribute, dumping the
profile, try deleting the attribute later.
2) Verify the radius daemon (/usr/sbin/radiusd) can authenticate to the
IPA LDAP server using kerberos and that it successfully reads the client
data you created in step 1 above. The easiest way to do that is to stop
the running radius server (service stop radiusd) and run the daemon in
verbose non-forked debug mode like this: /usr/sbin/radiusd -X
Along with other output it should print something about attempting a
SASL bind to the directory server, the result, and the client data it
read from LDAP.
More advanced testing can be done with radius test code that emulates
clients and NAS devices, but that's out of scope. If it does the above
things, we're pretty far down the road.
--
John Dennis <jdennis at redhat.com>
More information about the Freeipa-devel
mailing list