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

[Martin Kosek]

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