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

[Rob Crittenden]

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