[Freeipa-devel] [PATCH] 916 make category and members mutually exclusive in Sudo

[Rob Crittenden]

Martin Kosek wrote:
> On Mon, 2012-01-16 at 22:20 -0500, Rob Crittenden wrote:
>> Rob Crittenden wrote:
>>> Martin Kosek wrote:
>>>> On Mon, 2011-12-12 at 17:14 -0500, Rob Crittenden wrote:
>>>>> This patch makes all categories and their equivalent members mutually
>>>>> exclusive like in the HBAC plugin. So if you have usercat='all' you
>>>>> can't add users.
>>>>>
>>>>> Added test cases for these as well.
>>>>>
>>>>> I also modified the default list of attributes to include the RunAs
>>>>> attributes.
>>>>>
>>>>> rob
>>>>
>>>> NACK. I see several issues in this patch:
>>>>
>>>> 1) Error messages should be internationalized since they can be read by
>>>> a user (this problem is in HBAC too)
>>>>
>>>> 2) All constructs like this one can be simplified (and thus made less
>>>> error prone).
>>>>
>>>> + if 'cmdcategory' in _entry_attrs and \
>>>> + _entry_attrs['cmdcategory'][0].lower() == 'all':
>>>> + raise errors.MutuallyExclusiveError(reason="commands cannot be added
>>>> when command category='all'")
>>>>
>>>> can be changed to:
>>>>
>>>> + if _entry_attrs.get('cmdcategory', [''])[0].lower() == 'all':
>>>> + raise errors.MutuallyExclusiveError(...)
>>>>
>>>> I think the code would be then also more readable.
>>>>
>>>> 3) I think this code only works by an accident :-)
>>>>
>>>> + if ('ipasudorunasusercategory' or 'ipasudorunasgroupcategory') \
>>>> + in _entry_attrs and \
>>>> + (_entry_attrs['ipasudorunasusercategory'][0].lower() == 'all' or \
>>>> + _entry_attrs['ipasudorunasgroupcategory'][0].lower() == 'all'):
>>>> + raise errors.MutuallyExclusiveError(reason="users cannot be added
>>>> when runAs user or runAs group category='all'")
>>>>
>>>> ('ipasudorunasusercategory' or 'ipasudorunasgroupcategory') simply
>>>> returns 'ipasudorunasusercategory'. Thus the check for
>>>> 'ipasudorunasgroupcategory' in _entry_attrs is not performed at all.
>>>> Thanks to this bug, user is then able to pass a runAsGroup to sudorule
>>>> with groupcat == 'all':
>>>>
>>>> # ipa sudorule-add foo --runasgroupcat=all
>>>> ---------------------
>>>> Added Sudo Rule "foo"
>>>> ---------------------
>>>> Rule name: foo
>>>> Enabled: TRUE
>>>> RunAs Group category: all
>>>> # ipa sudorule-add-runasuser foo --groups=admins
>>>> Rule name: foo
>>>> Enabled: TRUE
>>>> RunAs Group category: all
>>>> Groups of RunAs Users: admins
>>>> -------------------------
>>>> Number of members added 1
>>>> -------------------------
>>>>
>>>> A change proposed in 1) could make the change simpler:
>>>>
>>>> + if _entry_attrs.get('ipasudorunasusercategory', [''])[0].lower() ==
>>>> 'all' or \
>>>> + _entry_attrs.get('ipasudorunasgroupcategory', [''])[0].lower() ==
>>>> 'all':
>>>> + raise ...
>>>>
>>>> Martin
>>>>
>>>>
>>>
>>> Updated patch attached. Using the is_all() function instead. Opened
>>> separate ticket to internationalize HBAC exceptions,
>>> https://fedorahosted.org/freeipa/ticket/2267
>>>
>>> rob
>>
>> Rebased against ipa-2-2.
>>
>> rob
>
> There are still few issues:
>
> 1) test_sudorule_plugin.py is missing in the new commit
>
> 2) The patch does not work for ipasudorunasusercategory or
> ipasudorunasgroupcategory as they are not in self.obj.default_attributes
>
> 3) I also saw some internal errors:
>
> # ipa sudorule-show foo --all
>    dn:
> ipauniqueid=a0e84cda-40e5-11e1-a35b-00163e7228ea,cn=sudorules,cn=sudo,dc=idm,dc=lab,dc=bos,dc=redhat,dc=com
>    Rule name: foo
>    Enabled: TRUE
>    User category: all
>    RunAs Group category: all
>    Groups of RunAs Users: admins
>    ipauniqueid: a0e84cda-40e5-11e1-a35b-00163e7228ea
>    objectclass: ipaassociation, ipasudorul
>
> # ipa sudorule-add-user foo --users=admin
> ipa: ERROR: an internal error has occurred
>
> # ipa sudorule-add-user foo --groups=admins
> ipa: ERROR: an internal error has occurred
>
> # ipa sudorule-mod foo --hostcat=all
> # ipa sudorule-add-host foo --hosts=`hostname`
> ipa: ERROR: an internal error has occurred
>
> Martin
>

Somehow sent the wrong version of the patch. This one should be better.

rob
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-916-3-sudo.patch
Type: text/x-diff
Size: 13426 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120117/9b62c617/attachment.bin>