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

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

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 redhat com>

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