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

[Rob Crittenden]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-rcrit-916-2-sudo.patch
Type: text/x-diff
Size: 5396 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120116/b83ddcad/attachment.bin>