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

[Petr Viktorin]

On 11/28/2013 04:59 PM, Nathaniel McCallum wrote:

Everything looks good to me. +1


Pushed to master: a1f32fa9369109235dba041de9c972da09d8448a


--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Nathaniel McCallum]

Everything looks good to me. +1

On Thu, 2013-11-28 at 12:18 +0100, Petr Viktorin wrote:
> Thanks!
> Just a bit of cleaning up now, sending a patch with proposed changes to 
> speed things up.
> 
> Patch needs a tiny rebase.
> Points I missed:
> - There are some unused imports.
> - ValidationError takes the attribute name in `name` rather than the 
> name of the CLI option.
> 
> >> Now the validation is too strict, a port is not accepted.
> >
> > Fixed.
> 
> "invalid!" is pretty bad for an error message. I put it in as a 
> placeholder, but I wasn't clear about that, sorry!
> 
> >> Should non-FQDN hostnames be allowed?
> >
> > I agree they should not. Fixed.
> 
> validate_hostname() has a check_fqdn argument, no need to do this manually.
> 
>  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.
> >>
> >> I think if "$%^&*" is allowed, we'll get a bug from QA soon enough.
> >
> > Fixed.
> 
> The `sre` module is named `re` since Python 2.5.
> 
> >> We generally output lists; this should also be a list with one element.
> >
> > Fixed.
> >
> >> Attaching updated tests.
> >
> > A few of these tests are still failing for me, but it is not immediately
> > obvious why. They seem to be getting answers from previous queries. I'm
> > not sure if this is something wrong with my code or the tests. Can you
> > take a look at it?
> 
> My bad, I've used a wrong variable name.
> 


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Nathaniel McCallum]

On Wed, 2013-11-27 at 12:28 +0100, Petr Viktorin wrote:
> >> 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.
> 
> Now the validation is too strict, a port is not accepted.

Fixed.

> Should non-FQDN hostnames be allowed?

I agree they should not. 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.
> 
> I think if "$%^&*" is allowed, we'll get a bug from QA soon enough.

Fixed.

> We generally output lists; this should also be a list with one element.

Fixed.

> Attaching updated tests.

A few of these tests are still failing for me, but it is not immediately
obvious why. They seem to be getting answers from previous queries. I'm
not sure if this is something wrong with my code or the tests. Can you
take a look at it?

Nathaniel
>From beb846295d68d2c5ce958bcf08790279761aaf58 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum 
Date: Mon, 11 Nov 2013 17:58:02 -0400
Subject: [PATCH] Add RADIUS proxy support to ipalib CLI

https://fedorahosted.org/freeipa/ticket/3368
---
 API.txt|  95 +++--
 VERSION|   2 +-
 install/share/70ipaotp.ldif|   2 +-
 install/updates/20-indices.update  |   7 ++
 install/updates/25-referint.update |   1 +
 install/updates/40-otp.update  |   5 ++
 ipalib/constants.py|   1 +
 ipalib/plugins/config.py   |   2 +-
 ipalib/plugins/radiusproxy.py  | 166 +
 ipalib/plugins/user.py |  65 +--
 10 files changed, 328 insertions(+), 18 deletions(-)
 create mode 100644 ipalib/plugins/radiusproxy.py

diff --git a/API.txt b/API.txt
index c29efad3382ff59e4753eff5354cba72bc1fe027..328fcf76ac6c8a8f5349e9f5631ec61bae7b3ea4 100644
--- a/API.txt
+++ b/API.txt
@@ -523,7 +523,7 @@ option: Int('ipasearchrecordslimit', attribute=True, autofill=False, cli_name='s
 option: Int('ipasearchtimelimit', attribute=True, autofill=False, cli_name='searchtimelimit', minvalue=-1, multivalue=False, required=False)
 option: Str('ipaselinuxusermapdefault', attribute=True, autofill=False, cli_name='ipaselinuxusermapdefault', multivalue=False, required=False)
 option: Str('ipaselinuxusermaporder', attribute=True, autofill=False, cli_name='ipaselinuxusermaporder', multivalue=False, required=False)
-option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password',))
+option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password', u'radius'))
 option: Str('ipauserobjectclasses', attribute=True, autofill=False, cli_name='userobjectclasses', csv=True, multivalue=True, required=False)
 option: IA5Str('ipausersearchfields', attribute=True, autofill=False, cli_name='usersearch', multivalue=False, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
@@ -2551,6 +2551,81 @@ option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
 output: Output('value', , None)
+command: radiusproxy_add
+args: 1,11,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+option: Str('addattr*', cli_name='addattr', exclude='webui')
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
+option: Int('ipatokenradiusretries', attribute=True, cli_name='retries', maxvalue=10, minvalue=0, multivalue=False, required=False)
+option: Password('ipatokenradiussecret', attribute=True, cli_name='secret', confirm=True, multivalue=False, required=True)
+option: Str('ipatokenradiusserver', attribute=True, cli_name='server', multivalue=True, required=True)
+option: Int('ipatokenradiustimeout', attribute=True, cli_name='timeout', minvalue=1, multivalue=False, required=False)
+option: Str('ipatokenusermapattribute', attribute=True, cli_name='userattr', multivalue=False, required=False)
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('setattr*', cli_name='setattr', exclude='webui')
+option: Str('version?', exclude='webui')
+output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+output: Output('summary', (, ), None)
+output: Output('value', , None)
+command: radiusproxy_del
+args: 1,2,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=Tr

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

[Simo Sorce]

On Wed, 2013-11-27 at 15:12 -0500, Nathaniel McCallum wrote:
> On Wed, 2013-11-27 at 14:34 +, Simo Sorce wrote:
> > On Thu, 2013-11-21 at 15:54 -0500, Dmitri Pal wrote:
> > > On 11/21/2013 01:34 PM, Nathaniel McCallum wrote:
> > > >> 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.
> > > >
> > > If it is readable by admin only I would leave it as is for now and
> > > address later when we redo ACIs.
> > 
> > Is this specific to the one and only admin account or does it extend to
> > any user in the admins group ?
> 
> All admins. See ipatokenRadiusConfiguration in
> install/share/default-aci.ldif. Read access is denied to everyone except
> admins. The entire class is hidden from normal users. See below.

Oh I see it now, sorry, my fault in reading that ACI.
I can't wait to finally straighten out our ACI system...

> > Looking at the current master it seem *any* user except anonymous can
> > read secrets ? Or is there a patch I am missing ?
> > I think this is too broad.
> 
> [root@freeipa ~]# kinit admin
> Password for ad...@example.com: 
> [root@freeipa ~]# ipa radiusproxy-find
> -
> 1 RADIUS proxy server matched
> -
>   RADIUS proxy server name: foo
>   Server: foo
> 
> Number of entries returned 1
> 
> 
> [root@freeipa ~]# kinit test
> Password for t...@example.com: 
> kinit: Password incorrect while getting initial credentials
> [root@freeipa ~]# kinit test
> Password for t...@example.com: 
> [root@freeipa ~]# ipa radiusproxy-find
> --
> 0 RADIUS proxy servers matched
> --
> 
> Number of entries returned 0
> 

Looks good, I would still prefer to not make the password readable by
default to admins, but I think we can further restrict this later once
we have the new ACI system in place, which should make it easier to
handle these cases.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Nathaniel McCallum]

On Wed, 2013-11-27 at 14:34 +, Simo Sorce wrote:
> On Thu, 2013-11-21 at 15:54 -0500, Dmitri Pal wrote:
> > On 11/21/2013 01:34 PM, Nathaniel McCallum wrote:
> > >> 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.
> > >
> > If it is readable by admin only I would leave it as is for now and
> > address later when we redo ACIs.
> 
> Is this specific to the one and only admin account or does it extend to
> any user in the admins group ?

All admins. See ipatokenRadiusConfiguration in
install/share/default-aci.ldif. Read access is denied to everyone except
admins. The entire class is hidden from normal users. See below.

> Looking at the current master it seem *any* user except anonymous can
> read secrets ? Or is there a patch I am missing ?
> I think this is too broad.

[root@freeipa ~]# kinit admin
Password for ad...@example.com: 
[root@freeipa ~]# ipa radiusproxy-find
-
1 RADIUS proxy server matched
-
  RADIUS proxy server name: foo
  Server: foo

Number of entries returned 1


[root@freeipa ~]# kinit test
Password for t...@example.com: 
kinit: Password incorrect while getting initial credentials
[root@freeipa ~]# kinit test
Password for t...@example.com: 
[root@freeipa ~]# ipa radiusproxy-find
--
0 RADIUS proxy servers matched
--

Number of entries returned 0




___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Simo Sorce]

On Thu, 2013-11-21 at 15:54 -0500, Dmitri Pal wrote:
> On 11/21/2013 01:34 PM, Nathaniel McCallum wrote:
> >> 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.
> >
> If it is readable by admin only I would leave it as is for now and
> address later when we redo ACIs.

Is this specific to the one and only admin account or does it extend to
any user in the admins group ?

Looking at the current master it seem *any* user except anonymous can
read secrets ? Or is there a patch I am missing ?
I think this is too broad.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Petr Viktorin]

Sorry for the late review!

On 11/21/2013 07:34 PM, Nathaniel McCallum wrote:

On Fri, 2013-11-15 at 12:34 +0100, Petr Viktorin wrote:



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.


I'm fine either way, just making sure it gets some thought.
Let's see what Simo has to say on this.


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.


Now the validation is too strict, a port is not accepted.

Should non-FQDN hostnames be allowed?


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.


I think if "$%^&*" is allowed, we'll get a bug from QA soon enough.


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.


We generally output lists; this should also be a list with one element.

Attaching updated tests.

--
Petr³

From a8145b2531222604e7883b298e00929727319a5a Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Fri, 15 Nov 2013 12:26:54 +0100
Subject: [PATCH] Add tests for the radiusproxy plugin

---
 ipatests/test_xmlrpc/objectclasses.py   |   5 +
 ipatests/test_xmlrpc/test_radiusproxy_plugin.py | 384 
 2 files changed, 389 insertions(+)
 create mode 100644 ipatests/test_xmlrpc/test_radiusproxy_plugin.py

diff --git a/ipatests/test_xmlrpc/objectclasses.py b/ipatests/test_xmlrpc/objectclasses.py
index 75ac3eb174aaa50eecfda875024b62dbdab238a5..089ee69a38b37f11de539e60b9ecacdec7a7de0b 100644
--- a/ipatests/test_xmlrpc/objectclasses.py
+++ b/ipatests/test_xmlrpc/objectclasses.py
@@ -161,3 +161,8 @@
 u'nsContainer',
 u'domainRelatedObject',
 ]
+
+radiusproxy = [
+u'ipatokenradiusconfiguration',
+u'top',
+]
diff --git a/ipatests/test_xmlrpc/test_radiusproxy_plugin.py b/ipatests/test_xmlrpc/test_radiusproxy_plugin.py
new file mode 100644
index ..4a10b393c8a1500c1bdf354f72b962cda6ab984a
--- /dev/null
+++ b/ipatests/test_xmlrpc/test_radiusproxy_plugin.py
@@ -0,0 +1,384 @@
+# Authors:
+#   Petr Viktorin 
+#
+# Copyright (C) 2013  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see .
+
+
+from ipalib import errors, api, _
+from ipapython.dn import DN
+from ipatests.test_xmlrpc.xmlrpc_test import Declarative
+from ipatests.test_xmlrpc.test_user_plugin import get_user_result
+from ipatests.test_xmlrpc import objectclasses
+
+radius1 = u'testradius'
+radius1_fqdn = u'testradius.test'
+radius1_dn = DN(('cn=testradius'), ('cn=radiusproxy'), api.env.basedn)
+user1 = u'tuser1'
+password1 = u'very*secure123'
+
+
+class test_raduisproxy(Declarative):
+
+cleanup_commands = [
+('radiusproxy_del', [radius1], {}),
+('user_del', [user1], {}),
+]
+
+tests = [
+
+dict(
+desc='Try to retrieve non-existent %r' % radius1,
+command=('radiusproxy_show', [radius1], {}),
+expected=errors.NotFound(
+reason=u'%s: RADIUS proxy server not found' % radius1),
+),
+
+
+dict(
+desc='Try to update non-existent %r' % radius1,
+command=('radiusproxy_mod', [radius1], {}),
+expected=errors.NotFound(
+reason=_('%s: RADIUS proxy server not found') % radius1),
+),
+
+
+dict(
+desc='Try to delete non-existent %r' % radius1,
+command=('radiusproxy_del', [radius1], {}),
+expected=errors.NotFound(
+reason=_('%s: RADIU

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

[Petr Viktorin]

On 11/21/2013 09:54 PM, Dmitri Pal wrote:

On 11/21/2013 01:34 PM, Nathaniel McCallum wrote:

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.


If it is readable by admin only I would leave it as is for now and
address later when we redo ACIs.


CCing Simo since this is ACI-related


--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Dmitri Pal]

On 11/21/2013 01:34 PM, Nathaniel McCallum wrote:
>> 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.
>
If it is readable by admin only I would leave it as is for now and
address later when we redo ACIs.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


---
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/



___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Re: [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.

>From 838b75c95d30888d72d5120a1b2e5f348deb686c Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum 
Date: Mon, 11 Nov 2013 17:58:02 -0400
Subject: [PATCH] Add RADIUS proxy support to ipal

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

[Petr Viktorin]

On 11/15/2013 12:34 PM, 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:



>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).


Heads up: this was merged. When you get a merge conflict just remove the 
schema update file.


--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Petr Viktorin]

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.


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


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.


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

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)


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


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'])


  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.


> +
> +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.



--
Petr³

From da40f65c4805dff4def4628df189f4b7a9de413c Mon Sep 17 00:00:00 2001
From: Petr Viktorin 
Date: Fri, 15 Nov 2013 12:26:54 +0100
Subject: [PATCH] Add tests for the radiusproxy plugin

---
 ipatests/test_xmlrpc/test_radiusproxy_plugin.py | 384 
 1 file changed, 384 insertions(+)
 create mode 100644 ipatests/test_xmlrpc/test_radiusproxy_plugin.py

diff --git a/ipatests/test_xmlrpc/test_radiusproxy_plugin.py b/ipatests/test_xmlrpc/test_radiusproxy_plugin.py
new file mode 100644
index ..7a15a0ff0f19a3e6c0233577b2f5f16bc262f5d6
--- /dev/null
+++ b/ipatests/test_xmlrpc/test_radiusproxy_plugin.py
@@ -0,0 +1,384 @@
+# Authors:
+#   Petr Viktorin 
+#
+# Copyright (C) 2013  Red Hat
+# see file 'COPYING' for use and warranty information
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; wit

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

[Nathaniel McCallum]

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

>From aab00ba19773064a5ec43b73d5761c7145117ed8 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum 
Date: Mon, 11 Nov 2013 17:58:02 -0400
Subject: [PATCH] Add RADIUS proxy support to ipalib CLI

https://fedorahosted.org/freeipa/ticket/3368
---
 API.txt|  95 +++--
 VERSION|   2 +-
 install/share/70ipaotp.ldif|   2 +-
 install/updates/10-70ipaotp.update |   2 +-
 install/updates/20-indices.update  |   7 ++
 install/updates/25-referint.update |   1 +
 install/updates/40-otp.update  |   5 ++
 ipalib/constants.py|   1 +
 ipalib/plugins/config.py   |   2 +-
 ipalib/plugins/radiusproxy.py  | 138 +
 ipalib/plugins/user.py |  50 --
 11 files changed, 290 insertions(+), 15 deletions(-)
 create mode 100644 ipalib/plugins/radiusproxy.py

diff --git a/API.txt b/API.txt
index cddb9d719f38dd3d89e2633148311e437aed0bc9..bee8db1fc218cd63dbb2196a38ce4086f9a4d7bb 100644
--- a/API.txt
+++ b/API.txt
@@ -514,7 +514,7 @@ option: Int('ipasearchrecordslimit', attribute=True, autofill=False, cli_name='s
 option: Int('ipasearchtimelimit', attribute=True, autofill=False, cli_name='searchtimelimit', minvalue=-1, multivalue=False, required=False)
 option: Str('ipaselinuxusermapdefault', attribute=True, autofill=False, cli_name='ipaselinuxusermapdefault', multivalue=False, required=False)
 option: Str('ipaselinuxusermaporder', attribute=True, autofill=False, cli_name='ipaselinuxusermaporder', multivalue=False, required=False)
-option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password',))
+option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password', u'radius'))
 option: Str('ipauserobjectclasses', attribute=True, autofill=False, cli_name='userobjectclasses', csv=True, multivalue=True, required=False)
 option: IA5Str('ipausersearchfields', attribute=True, autofill=False, cli_name='usersearch', multivalue=False, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
@@ -2542,6 +2542,81 @@ option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
 output: Output('value', , None)
+command: radiusproxy_add
+args: 1,11,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+option: Str('addattr*', cli_name='addattr', exclude='webui')
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
+option: Int('ipatokenradiusretries', attribute=True, cli_name='retries', maxvalue=10, minvalue=0, multivalue=False, required=False)
+option: Password('ipatokenradiussecret', attribute=True, cli_name='secret', confirm=True, multivalue=False, required=True)
+option: Str('ipatokenradiusserver', attribute=True, cli_name='server', multivalue=True, required=True)
+option: Int('ipatokenradiustimeout', attribute=True, cli_name='timeout', minvalue=1, multivalue=False, required=False)
+option: Str('ipatokenusermapattribute', attribute=True, cli_name='userattr', multivalue=False, required=False)
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('setattr*', cli_name='setattr', exclude='webui')
+option: Str('version?', exclude='webui')
+output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+output: Output(

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

[Petr Viktorin]

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).


The patch needs a rebase.

--
Petr³

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Nathaniel McCallum]

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
>From 58d6c5672fa3b8de1e71a3c5cbec7dc90434cbac Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum 
Date: Wed, 4 Sep 2013 23:45:00 -0400
Subject: [PATCH] Add RADIUS proxy support to ipalib CLI

https://fedorahosted.org/freeipa/ticket/3368
---
 API.txt|  95 +++--
 VERSION|   2 +-
 install/share/70ipaotp.ldif|   2 +-
 install/share/indices.ldif |  10 +++
 install/share/referint-conf.ldif   |   3 +
 install/updates/10-70ipaotp.update |   2 +-
 install/updates/20-indices.update  |   7 ++
 install/updates/25-referint.update |   1 +
 install/updates/40-otp.update  |   5 ++
 ipalib/constants.py|   1 +
 ipalib/plugins/config.py   |   2 +-
 ipalib/plugins/radiusproxy.py  | 138 +
 ipalib/plugins/user.py |  44 +++-
 13 files changed, 298 insertions(+), 14 deletions(-)
 create mode 100644 ipalib/plugins/radiusproxy.py

diff --git a/API.txt b/API.txt
index b49493f33af0f7d0192df8318bda12df94c9567b..e662cc53e84cc3cb66e78c444e12e615bf7e3a7f 100644
--- a/API.txt
+++ b/API.txt
@@ -514,7 +514,7 @@ option: Int('ipasearchrecordslimit', attribute=True, autofill=False, cli_name='s
 option: Int('ipasearchtimelimit', attribute=True, autofill=False, cli_name='searchtimelimit', minvalue=-1, multivalue=False, required=False)
 option: Str('ipaselinuxusermapdefault', attribute=True, autofill=False, cli_name='ipaselinuxusermapdefault', multivalue=False, required=False)
 option: Str('ipaselinuxusermaporder', attribute=True, autofill=False, cli_name='ipaselinuxusermaporder', multivalue=False, required=False)
-option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password',))
+option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password', u'radius'))
 option: Str('ipauserobjectclasses', attribute=True, autofill=False, cli_name='userobjectclasses', csv=True, multivalue=True, required=False)
 option: IA5Str('ipausersearchfields', attribute=True, autofill=False, cli_name='usersearch', multivalue=False, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
@@ -2542,6 +2542,81 @@ option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
 output: Output('value', , None)
+command: radiusproxy_add
+args: 1,11,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+option: Str('addattr*', cli_name='addattr', exclude='webui')
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
+option: Int('ipatokenradiusretries', attribute=True, cli_name='retries', maxvalue=10, minvalue=0, multivalue=False, required=False)
+option: Password('ipatokenradiussecret', attribute=True, cli_name='secret', confirm=True, multivalue=False, required=True)
+option: Str('ipatokenradiusserver', attribute=True, cli_name='server', multivalue=True, required=True)
+option: Int('ipatokenradiustimeout', attribute=True, cli_name='timeout', minvalue=1, multivalue=False, required=False)
+option: Str('ipatokenusermapattribute', attribute=True, cli_name='userattr', multivalue=False, required=False)
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('setattr*', cli_name='setattr', exclude='webui')
+option: Str('version?', exclude='webui')
+output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+output: Output('summary', (, ), None)
+output: Output('value', , None)
+command: radiusproxy_del
+args: 1,2,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=True, required=True)
+option: Flag('continue', autofill=True, cli_name='continue', default=False)
+option: Str('version?', exclude='webui')
+output: Output('result', , None)
+output: Output('summary', (, ), None)
+output: Output('value', , None)
+command: radiu

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

[Nathan Kinder]

On 09/20/2013 09:38 AM, 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.
The LDIF and update related changes look good to me.  I'm not familiar 
with the ipalib code myself, so someone else should review that portion 
of your patch.


Thanks,
-NGK


Nathaniel


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel


___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Nathaniel McCallum]

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.

Nathaniel
>From ddcdef7f62fe364c1081eecd9f254e8e1c0c6ee6 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum 
Date: Wed, 4 Sep 2013 23:45:00 -0400
Subject: [PATCH] Add RADIUS proxy support to ipalib CLI

https://fedorahosted.org/freeipa/ticket/3368
---
 API.txt|  95 +++--
 install/share/70ipaotp.ldif|   2 +-
 install/share/indices.ldif |  10 +++
 install/share/referint-conf.ldif   |   3 +
 install/updates/10-70ipaotp.update |   2 +-
 install/updates/20-indices.update  |   7 ++
 install/updates/25-referint.update |   1 +
 install/updates/40-otp.update  |   5 ++
 ipalib/constants.py|   1 +
 ipalib/plugins/config.py   |   2 +-
 ipalib/plugins/radiusproxy.py  | 138 +
 ipalib/plugins/user.py |  44 +++-
 12 files changed, 297 insertions(+), 13 deletions(-)
 create mode 100644 ipalib/plugins/radiusproxy.py

diff --git a/API.txt b/API.txt
index b49493f33af0f7d0192df8318bda12df94c9567b..e662cc53e84cc3cb66e78c444e12e615bf7e3a7f 100644
--- a/API.txt
+++ b/API.txt
@@ -514,7 +514,7 @@ option: Int('ipasearchrecordslimit', attribute=True, autofill=False, cli_name='s
 option: Int('ipasearchtimelimit', attribute=True, autofill=False, cli_name='searchtimelimit', minvalue=-1, multivalue=False, required=False)
 option: Str('ipaselinuxusermapdefault', attribute=True, autofill=False, cli_name='ipaselinuxusermapdefault', multivalue=False, required=False)
 option: Str('ipaselinuxusermaporder', attribute=True, autofill=False, cli_name='ipaselinuxusermaporder', multivalue=False, required=False)
-option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password',))
+option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password', u'radius'))
 option: Str('ipauserobjectclasses', attribute=True, autofill=False, cli_name='userobjectclasses', csv=True, multivalue=True, required=False)
 option: IA5Str('ipausersearchfields', attribute=True, autofill=False, cli_name='usersearch', multivalue=False, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
@@ -2542,6 +2542,81 @@ option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
 output: Output('value', , None)
+command: radiusproxy_add
+args: 1,11,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+option: Str('addattr*', cli_name='addattr', exclude='webui')
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
+option: Int('ipatokenradiusretries', attribute=True, cli_name='retries', maxvalue=10, minvalue=0, multivalue=False, required=False)
+option: Password('ipatokenradiussecret', attribute=True, cli_name='secret', confirm=True, multivalue=False, required=True)
+option: Str('ipatokenradiusserver', attribute=True, cli_name='server', multivalue=True, required=True)
+option: Int('ipatokenradiustimeout', attribute=True, cli_name='timeout', minvalue=1, multivalue=False, required=False)
+option: Str('ipatokenusermapattribute', attribute=True, cli_name='userattr', multivalue=False, required=False)
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('setattr*', cli_name='setattr', exclude='webui')
+option: Str('version?', exclude='webui')
+output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+output: Output('summary', (, ), None)
+output: Output('value', , None)
+command: radiusproxy_del
+args: 1,2,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=True, required=True)
+option: Flag('continue', autofill=True, cli_name='continue', default=False)
+option: Str('version?', exclude='webui')
+output: Output('result', , None)
+output: Output('summary', (, ), None)
+output: Output('value', , None)
+command: radiusproxy_find
+args: 1,13,4
+arg: Str('criteria?', noextrawhitespace=False)
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('cn', attribute=True, autofill=False, cli_name='name',

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

[Jan Cholasta]

On 13.9.2013 09:21, Jan Cholasta wrote:

Hi,

On 12.9.2013 22:48, Nathaniel McCallum wrote:

On Thu, 2013-09-05 at 00:06 -0400, Nathaniel McCallum wrote:

patch attached


Update for ./makeapi attached.



+if 'ipatokenradiusconfiglink' in entry_attrs:
+cl = entry_attrs['ipatokenradiusconfiglink']
+if not cl:
+entry_attrs['ipatokenradiususername'] = None
+if 'ipatokenradiusproxyuser' in
entry_attrs['objectclass']:
+ entry_attrs['objectclass'].remove('ipatokenradiusproxyuser')

Is there are particular reason to remove the object class? I think you
can just leave it there, that is what we do in other plugins.

+else:
+if 'ipatokenradiusproxyuser' not in
entry_attrs['objectclass']:
+ entry_attrs['objectclass'].append('ipatokenradiusproxyuser')
+
+answer = self.api.Command.radius_show(cl)
+entry_attrs['ipatokenradiusconfiglink'] =
answer['result']['dn']

Please use self.api.Object.radius.get_dn_if_exists(cl) to get the DN
instead of radius_show.

The whole code block should be added to user_add as well.


+radius = options.get('ipatokenradiusconfiglink', None)
+if radius is not None:
+answer = self.api.Command.radius_show(radius)
+filter = filter.replace('(ipatokenradiusconfiglink=%s)' %
radius,
+'(ipatokenradiusconfiglink=%s)' %
answer['result']['dn'])

Again, use get_dn_if_exists instead of radius_show to get the DN.

As for the filter processing, I think it would be safer to override
args_options_2_entry in user_find and do it in there:

 def args_options_2_entry(self, *keys, **options):
 if 'ipatokenradiusconfiglink' in options:
 options['ipatokenradiusconfiglink'] =
self.api.Object.radius.get_dn(options['ipatokenradiusconfiglink'])
 return super(user_find, self).args_options_2_entry(


... or you can do this in user_find.execute, as there already is 
something similar done for the "manager" attribute.





Honza



BTW, I think you should configure the referential integrity plugin so 
that when a radius object is deleted, all ipatokenradiusconfiglink's to 
it are deleted as well.


Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Jan Cholasta]

Hi,

On 12.9.2013 22:48, Nathaniel McCallum wrote:

On Thu, 2013-09-05 at 00:06 -0400, Nathaniel McCallum wrote:

patch attached


Update for ./makeapi attached.



+if 'ipatokenradiusconfiglink' in entry_attrs:
+cl = entry_attrs['ipatokenradiusconfiglink']
+if not cl:
+entry_attrs['ipatokenradiususername'] = None
+if 'ipatokenradiusproxyuser' in entry_attrs['objectclass']:
+ 
entry_attrs['objectclass'].remove('ipatokenradiusproxyuser')


Is there are particular reason to remove the object class? I think you 
can just leave it there, that is what we do in other plugins.


+else:
+if 'ipatokenradiusproxyuser' not in 
entry_attrs['objectclass']:
+ 
entry_attrs['objectclass'].append('ipatokenradiusproxyuser')

+
+answer = self.api.Command.radius_show(cl)
+entry_attrs['ipatokenradiusconfiglink'] = 
answer['result']['dn']


Please use self.api.Object.radius.get_dn_if_exists(cl) to get the DN 
instead of radius_show.


The whole code block should be added to user_add as well.


+radius = options.get('ipatokenradiusconfiglink', None)
+if radius is not None:
+answer = self.api.Command.radius_show(radius)
+filter = filter.replace('(ipatokenradiusconfiglink=%s)' % 
radius,
+'(ipatokenradiusconfiglink=%s)' % 
answer['result']['dn'])


Again, use get_dn_if_exists instead of radius_show to get the DN.

As for the filter processing, I think it would be safer to override 
args_options_2_entry in user_find and do it in there:


def args_options_2_entry(self, *keys, **options):
if 'ipatokenradiusconfiglink' in options:
options['ipatokenradiusconfiglink'] = 
self.api.Object.radius.get_dn(options['ipatokenradiusconfiglink'])

return super(user_find, self).args_options_2_entry(


Honza

--
Jan Cholasta

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Nathaniel McCallum]

On Thu, 2013-09-05 at 00:06 -0400, Nathaniel McCallum wrote:
> patch attached

Update for ./makeapi attached.
>From 8add59e470606caec4fe5ef87f5261a2b7f7f52f Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum 
Date: Wed, 4 Sep 2013 23:45:00 -0400
Subject: [PATCH] Add RADIUS proxy support to ipalib CLI

https://fedorahosted.org/freeipa/ticket/3368
---
 API.txt   |  95 ++---
 install/updates/40-otp.update |   5 ++
 ipalib/constants.py   |   1 +
 ipalib/plugins/config.py  |   2 +-
 ipalib/plugins/radius.py  | 138 ++
 ipalib/plugins/user.py|  35 ++-
 6 files changed, 265 insertions(+), 11 deletions(-)
 create mode 100644 ipalib/plugins/radius.py

diff --git a/API.txt b/API.txt
index b49493f33af0f7d0192df8318bda12df94c9567b..70c4f2accf6e5822260fcd3c871eb16977540c1e 100644
--- a/API.txt
+++ b/API.txt
@@ -514,7 +514,7 @@ option: Int('ipasearchrecordslimit', attribute=True, autofill=False, cli_name='s
 option: Int('ipasearchtimelimit', attribute=True, autofill=False, cli_name='searchtimelimit', minvalue=-1, multivalue=False, required=False)
 option: Str('ipaselinuxusermapdefault', attribute=True, autofill=False, cli_name='ipaselinuxusermapdefault', multivalue=False, required=False)
 option: Str('ipaselinuxusermaporder', attribute=True, autofill=False, cli_name='ipaselinuxusermaporder', multivalue=False, required=False)
-option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password',))
+option: StrEnum('ipauserauthtype', attribute=True, autofill=False, cli_name='user_auth_type', csv=True, multivalue=True, required=False, values=(u'password', u'radius'))
 option: Str('ipauserobjectclasses', attribute=True, autofill=False, cli_name='userobjectclasses', csv=True, multivalue=True, required=False)
 option: IA5Str('ipausersearchfields', attribute=True, autofill=False, cli_name='usersearch', multivalue=False, required=False)
 option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
@@ -2542,6 +2542,81 @@ option: Str('version?', exclude='webui')
 output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
 output: Output('summary', (, ), None)
 output: Output('value', , None)
+command: radius_add
+args: 1,11,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=False, primary_key=True, required=True)
+option: Str('addattr*', cli_name='addattr', exclude='webui')
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('description', attribute=True, cli_name='desc', multivalue=False, required=False)
+option: Int('ipatokenradiusretries', attribute=True, cli_name='retries', maxvalue=10, minvalue=0, multivalue=False, required=False)
+option: Password('ipatokenradiussecret', attribute=True, cli_name='secret', confirm=True, multivalue=False, required=True)
+option: Str('ipatokenradiusserver', attribute=True, cli_name='server', multivalue=True, required=True)
+option: Int('ipatokenradiustimeout', attribute=True, cli_name='timeout', minvalue=1, multivalue=False, required=False)
+option: Str('ipatokenusermapattribute', attribute=True, cli_name='userattr', multivalue=False, required=False)
+option: Flag('raw', autofill=True, cli_name='raw', default=False, exclude='webui')
+option: Str('setattr*', cli_name='setattr', exclude='webui')
+option: Str('version?', exclude='webui')
+output: Entry('result', , Gettext('A dictionary representing an LDAP entry', domain='ipa', localedir=None))
+output: Output('summary', (, ), None)
+output: Output('value', , None)
+command: radius_del
+args: 1,2,3
+arg: Str('cn', attribute=True, cli_name='name', multivalue=True, primary_key=True, query=True, required=True)
+option: Flag('continue', autofill=True, cli_name='continue', default=False)
+option: Str('version?', exclude='webui')
+output: Output('result', , None)
+output: Output('summary', (, ), None)
+output: Output('value', , None)
+command: radius_find
+args: 1,13,4
+arg: Str('criteria?', noextrawhitespace=False)
+option: Flag('all', autofill=True, cli_name='all', default=False, exclude='webui')
+option: Str('cn', attribute=True, autofill=False, cli_name='name', multivalue=False, primary_key=True, query=True, required=False)
+option: Str('description', attribute=True, autofill=False, cli_name='desc', multivalue=False, query=True, required=False)
+option: Int('ipatokenradiusretries', attribute=True, autofill=False, cli_name='retries', maxvalue=10, minvalue=0, multivalue=False, query=True, required=False)
+option: Password('ipatokenradiussecret', attribute=True, autofill=False, cli_name='secret', confirm=True, multivalue=False, query=True, required=False)
+option: Str('ipatokenradiusserver', attribute=True, autofill=False, cli_name='server', multivalue=True, query=True, required=False)
+option: Int('ipatokenradiustimeout', attri

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

[Dmitri Pal]

On 09/05/2013 12:29 AM, Nathaniel McCallum wrote:
> I forgot to mention that this code ignores the design page in one area:
> radius-show does not list the users attached to this server. How
> important is this? user-find --radius=MyRADIUSServer should find all the
> users.
>
> Nathaniel
>
> ___
> Freeipa-devel mailing list
> Freeipa-devel@redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
This is probably OK but then the design needs to be updated.
Also I would suggest documenting that to get all users associated with a
radius server you need to run the command above.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager for IdM portfolio
Red Hat Inc.


---
Looking to carve out IT costs?
www.redhat.com/carveoutcosts/



___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

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

[Nathaniel McCallum]

I forgot to mention that this code ignores the design page in one area:
radius-show does not list the users attached to this server. How
important is this? user-find --radius=MyRADIUSServer should find all the
users.

Nathaniel

___
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel