[Freeipa-devel] [PATCH 0016] Add RADIUS proxy support to ipalib CLI

[Nathaniel McCallum]

On Fri, 2013-11-15 at 12:34 +0100, Petr Viktorin wrote:
> On 11/12/2013 12:17 AM, Nathaniel McCallum wrote:
> > On Fri, 2013-11-08 at 13:26 +0100, Petr Viktorin wrote:
> >> >On 09/25/2013 10:56 PM, Nathaniel McCallum wrote:
> >>> > >On Fri, 2013-09-20 at 12:38 -0400, Nathaniel McCallum wrote:
> >>>> > >>On Thu, 2013-09-12 at 16:48 -0400, Nathaniel McCallum wrote:
> >>>>> > >>>On Thu, 2013-09-05 at 00:06 -0400, Nathaniel McCallum wrote:
> >>>>>> > >>>>patch attached
> >>>>> > >>>
> >>>>> > >>>Update for ./makeapi attached.
> >>>> > >>
> >>>> > >>Version 3. This should fix all the current review issues, including the
> >>>> > >>use of the referential integrity plugin. I had to make one schema change
> >>>> > >>in order to make the referential integrity modification work. Note also
> >>>> > >>that the command name prefix is changed from radius to radiusproxy.
> >>> > >
> >>> > >Version 4. This patch fixes my failure to increment the minor version
> >>> > >number in the VERSION file.
> >>> > >
> >>> > >Nathaniel
> >> >
> >> >We've since decided that we'll carry LDAP "content" updates only in
> >> >update files, so you can leave indices.ldif & referint-conf.ldif unchanged.
> >> >Schema, on the other hand, will still be in ldif files (and soon*only*
> >> >in ldif files).
> > Fixed.
> >
> >> >The patch needs a rebase.
> > Fixed.
> >
> > Also fixed: two other bugs I found when testing the above fixes. Tests
> > pass.
> >
> > Nathaniel
> 
> Thanks for the patch!
> 
> The design page needs an update: radius-* are renamed to radiusproxy-*, 
> several options marked in the doc as optional are mandatory.

Fixed.

> The password can be retrieved with radiusproxy-show --all, because it is 
> not blocked by LDAP ACIs. Is that intended?

Yes. But I'm torn as to whether or not this is a good idea. Regular
users can't see radius proxy servers at all. Admins can see all
attributes.

It is common in radius server deployments to have a text file readable
by root with the radius secret. The current LDAP policy replicates this
"expected" behavior. It may be wise to block all reads of the secret
though. I'm open to suggestions.

> ipatokenradiusserver is not validated. See validate_searchtimelimit in 
> the config plugin for an example validator. You can use validate_ipaddr 
> and validate_hostname from ipalib.util.

Fixed.

> ipatokenusermapattribute is also not validated. Not sure if it needs to be.

I don't think validation is really possible outside of the permitted
characters for an LDAP attribute.

> The --secret CLI option does nothing (it doesn't take a value, and the 
> secret is prompted for whether or not the option is given). It should be 
> flagged no_option. (Or file an RFE for better Password semantics)

Fixed.

> For the user commands, --radius takes the name on input, but a DN is 
> output. Is that expected?

Fixed.

> I'm attaching tests I used.
> 
> > @@ -640,16 +663,29 @@ class user_mod(LDAPUpdate):
> >               entry_attrs['userpassword'] = ipa_generate_password(user_pwdchars)
> >               # save the password so it can be displayed in post_callback
> >               setattr(context, 'randompassword', entry_attrs['userpassword'])
> > -        if 'ipasshpubkey' in entry_attrs or 'ipauserauthtype' in entry_attrs:
> > +        if 'ipasshpubkey' in entry_attrs or 'ipauserauthtype' in entry_attrs or \
> > +           'ipatokenradiusconfiglink' in entry_attrs:
> >               if 'objectclass' in entry_attrs:
> >                   obj_classes = entry_attrs['objectclass']
> >               else:
> >                   (_dn, _entry_attrs) = ldap.get_entry(dn, ['objectclass'])
> 
> This form is deprecated. Since you don't need _dn, just do
>      _entry_attrs = ldap.get_entry(dn, ['objectclass'])

Fixed.

> >                   obj_classes = entry_attrs['objectclass'] = _entry_attrs['objectclass']
> > +
> >               if 'ipasshpubkey' in entry_attrs and 'ipasshuser' not in obj_classes:
> >                   obj_classes.append('ipasshuser')
> > +
> >               if 'ipauserauthtype' in entry_attrs and 'ipauserauthtype' not in obj_classes:
> >                   obj_classes.append('ipauserauthtypeclass')
> 
> That should be `and 'ipauserauthtypeclass' not in obj_classes`, right? 
> It must have slipped an earlier review.

Fixed.

>  > +
>  > +            if 'ipatokenradiusconfiglink' in entry_attrs:
>  > +                cl = entry_attrs['ipatokenradiusconfiglink']
>  > +                if cl:
>  > +                    if 'ipatokenradiusproxyuser' not in 
> entry_attrs['objectclass']:
>  > + 
> entry_attrs['objectclass'].append('ipatokenradiusproxyuser')
> 
> Nitpick: entry_attrs['objectclass'] is stored in obj_classes, you can 
> use that.

Fixed.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-npmccallum-0016-6-Add-RADIUS-proxy-support-to-ipalib-CLI.patch
Type: text/x-patch
Size: 32178 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20131121/5eec8dec/attachment.bin>