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

[John Dennis]

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>