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

[Martin Kosek]

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