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
>From 9426423635d3cb527216859370b5b1cbdd77cc32 Mon Sep 17 00:00:00 2001
From: Rob Crittenden <rcrit...@redhat.com>
Date: Fri, 13 Jan 2012 11:34:04 -0500
Subject: [PATCH] In sudo when the category is all do not allow members, and
 vice versa.

This is what we already do in the HBAC plugin, this ports it to Sudo.

If a category (user, host, etc) is u'all' then we don't allow individual
members be added. Conversely if there are members we don't allow the
category be set to u'all'.

https://fedorahosted.org/freeipa/ticket/1440
---
 ipalib/plugins/hbacrule.py                |   11 ++-
 ipalib/plugins/sudorule.py                |   75 ++++++++++++++++++++++
 tests/test_xmlrpc/test_sudorule_plugin.py |   98 ++++++++++++++++++++++++++++-
 3 files changed, 177 insertions(+), 7 deletions(-)

diff --git a/ipalib/plugins/hbacrule.py b/ipalib/plugins/hbacrule.py
index 92b656d66f971b0d2b6fa1d4f1ff8413b45cc819..0fa44a5903cf35715d0d97acb5aa0a5eb58ddf76 100644
--- a/ipalib/plugins/hbacrule.py
+++ b/ipalib/plugins/hbacrule.py
@@ -96,10 +96,13 @@ def is_all(options, attribute):
     """
     See if options[attribute] is lower-case 'all' in a safe way.
     """
-    if attribute in options and \
-        options[attribute] is not None and \
-        options[attribute].lower() == 'all':
-        return True
+    if attribute in options and options[attribute] is not None:
+        if type(options[attribute]) in (list, tuple):
+            value = options[attribute][0].lower()
+        else:
+            value = options[attribute].lower()
+        if value == 'all':
+            return True
     else:
         return False
 
diff --git a/ipalib/plugins/sudorule.py b/ipalib/plugins/sudorule.py
index 65a1d8541cd52c89e7fcb7b70adccf73490bdcbc..df395ead2e0380c3f9e15533f8e18904d9f697af 100644
--- a/ipalib/plugins/sudorule.py
+++ b/ipalib/plugins/sudorule.py
@@ -20,6 +20,7 @@
 from ipalib import api, errors
 from ipalib import Str, StrEnum
 from ipalib.plugins.baseldap import *
+from ipalib.plugins.hbacrule import is_all
 from ipalib import _, ngettext
 
 __doc__ = _("""
@@ -77,6 +78,8 @@ class sudorule(LDAPObject):
         'description', 'usercategory', 'hostcategory',
         'cmdcategory', 'memberuser', 'memberhost',
         'memberallowcmd', 'memberdenycmd', 'ipasudoopt',
+        'ipasudorunas', 'ipasudorunasgroup',
+        'ipasudorunasusercategory', 'ipasudorunasgroupcategory',
     ]
     uuid_attribute = 'ipauniqueid'
     rdn_attribute = 'ipauniqueid'
@@ -232,6 +235,25 @@ class sudorule_mod(LDAPUpdate):
     __doc__ = _('Modify Sudo Rule.')
 
     msg_summary = _('Modified Sudo Rule "%(value)s"')
+    def pre_callback(self, ldap, dn, entry_attrs, attrs_list, *keys, **options):
+        try:
+            (_dn, _entry_attrs) = ldap.get_entry(dn, self.obj.default_attributes)
+        except errors.NotFound:
+            self.obj.handle_not_found(*keys)
+
+        if is_all(options, 'usercategory') and 'memberuser' in _entry_attrs:
+            raise errors.MutuallyExclusiveError(reason=_("user category cannot be set to 'all' while there are users"))
+        if is_all(options, 'hostcategory') and 'memberhost' in _entry_attrs:
+            raise errors.MutuallyExclusiveError(reason=_("host category cannot be set to 'all' while there are hosts"))
+        if is_all(options, 'cmdcategory') and ('memberallowcmd' or
+            'memberdenywcmd') in _entry_attrs:
+            raise errors.MutuallyExclusiveError(reason=_("command category cannot be set to 'all' while there are allow or deny commands"))
+        if is_all(options, 'ipasudorunasusercategory') and 'ipasudorunas' in _entry_attrs:
+            raise errors.MutuallyExclusiveError(reason=_("user runAs category cannot be set to 'all' while there are users"))
+        if is_all(options, 'ipasudorunasgroupcategory') and 'ipasudorunasgroup' in _entry_attrs:
+            raise errors.MutuallyExclusiveError(reason=_("group runAs category cannot be set to 'all' while there are groups"))
+
+        return dn
 
 api.register(sudorule_mod)
 
@@ -306,6 +328,16 @@ class sudorule_add_allow_command(LDAPAddMember):
     member_attributes = ['memberallowcmd']
     member_count_out = ('%i object added.', '%i objects added.')
 
+    def pre_callback(self, ldap, dn, found, not_found, *keys, **options):
+        try:
+            (_dn, _entry_attrs) = ldap.get_entry(dn, self.obj.default_attributes)
+        except errors.NotFound:
+            self.obj.handle_not_found(*keys)
+        if is_all(_entry_attrs, 'cmdcategory'):
+            raise errors.MutuallyExclusiveError(reason=_("commands cannot be added when command category='all'"))
+
+        return dn
+
 api.register(sudorule_add_allow_command)
 
 
@@ -324,6 +356,15 @@ class sudorule_add_deny_command(LDAPAddMember):
     member_attributes = ['memberdenycmd']
     member_count_out = ('%i object added.', '%i objects added.')
 
+    def pre_callback(self, ldap, dn, found, not_found, *keys, **options):
+        try:
+            (_dn, _entry_attrs) = ldap.get_entry(dn, self.obj.default_attributes)
+        except errors.NotFound:
+            self.obj.handle_not_found(*keys)
+        if is_all(_entry_attrs, 'cmdcategory'):
+            raise errors.MutuallyExclusiveError(reason=_("commands cannot be added when command category='all'"))
+        return dn
+
 api.register(sudorule_add_deny_command)
 
 
@@ -342,6 +383,15 @@ class sudorule_add_user(LDAPAddMember):
     member_attributes = ['memberuser']
     member_count_out = ('%i object added.', '%i objects added.')
 
+    def pre_callback(self, ldap, dn, found, not_found, *keys, **options):
+        try:
+            (_dn, _entry_attrs) = ldap.get_entry(dn, self.obj.default_attributes)
+        except errors.NotFound:
+            self.obj.handle_not_found(*keys)
+        if is_all(_entry_attrs, 'usercategory'):
+            raise errors.MutuallyExclusiveError(reason=_("users cannot be added when user category='all'"))
+        return dn
+
     def post_callback(self, ldap, completed, failed, dn, entry_attrs, *keys, **options):
         completed_external = 0
         # Sift through the user failures. We assume that these are all
@@ -410,6 +460,15 @@ class sudorule_add_host(LDAPAddMember):
     member_attributes = ['memberhost']
     member_count_out = ('%i object added.', '%i objects added.')
 
+    def pre_callback(self, ldap, dn, found, not_found, *keys, **options):
+        try:
+            (_dn, _entry_attrs) = ldap.get_entry(dn, self.obj.default_attributes)
+        except errors.NotFound:
+            self.obj.handle_not_found(*keys)
+        if is_all(_entry_attrs, 'hostcategory'):
+            raise errors.MutuallyExclusiveError(reason=_("hosts cannot be added when host category='all'"))
+        return dn
+
     def post_callback(self, ldap, completed, failed, dn, entry_attrs, *keys, **options):
         completed_external = 0
         # Sift through the host failures. We assume that these are all
@@ -485,6 +544,14 @@ class sudorule_add_runasuser(LDAPAddMember):
                 return False
             return True
 
+        try:
+            (_dn, _entry_attrs) = ldap.get_entry(dn, self.obj.default_attributes)
+        except errors.NotFound:
+            self.obj.handle_not_found(*keys)
+        if is_all(_entry_attrs, 'ipasudorunasusercategory') or \
+          is_all(_entry_attrs, 'ipasudorunasgroupcategory'):
+            raise errors.MutuallyExclusiveError(reason=_("users cannot be added when runAs user or runAs group category='all'"))
+
         if 'user' in options:
             for name in options['user']:
                 if not check_validity(name):
@@ -575,6 +642,14 @@ class sudorule_add_runasgroup(LDAPAddMember):
                 return False
             return True
 
+        try:
+            (_dn, _entry_attrs) = ldap.get_entry(dn, self.obj.default_attributes)
+        except errors.NotFound:
+            self.obj.handle_not_found(*keys)
+        if is_all(_entry_attrs, 'ipasudorunasusercategory') or \
+          is_all(_entry_attrs, 'ipasudorunasgroupcategory'):
+            raise errors.MutuallyExclusiveError(reason=_("users cannot be added when runAs user or runAs group category='all'"))
+
         if 'group' in options:
             for name in options['group']:
                 if not check_validity(name):
diff --git a/tests/test_xmlrpc/test_sudorule_plugin.py b/tests/test_xmlrpc/test_sudorule_plugin.py
index 88e31c72c5b4c823c11649c18417e51d67d77f9b..07d23c3d207151f063f2884d453bb019107aac2a 100644
--- a/tests/test_xmlrpc/test_sudorule_plugin.py
+++ b/tests/test_xmlrpc/test_sudorule_plugin.py
@@ -47,7 +47,7 @@ class test_sudorule(XMLRPC_test):
     test_denycommand = u'/usr/bin/testdenysudocmd1'
     test_runasuser = u'manager'
     test_runasgroup = u'manager'
-    test_catagory = u'all'
+    test_category = u'all'
     test_option = u'authenticate'
 
     def test_0_sudorule_add(self):
@@ -520,7 +520,99 @@ class test_sudorule(XMLRPC_test):
         assert 'memberdenycmd_sudocmd' not in entry
         assert 'memberdenycmd_sudocmdgroup' not in entry
 
-    def test_c_sudorule_clear_testing_data(self):
+    def test_c_sudorule_exclusiveuser(self):
+        """
+        Test adding a user to an Sudo rule when usercat='all'
+        """
+        api.Command['sudorule_mod'](self.rule_name, usercategory=u'all')
+        try:
+            api.Command['sudorule_add_user'](self.rule_name, users='admin')
+        except errors.MutuallyExclusiveError:
+            pass
+        api.Command['sudorule_mod'](self.rule_name, usercategory=u'')
+
+    def test_d_sudorule_exclusiveuser(self):
+        """
+        Test setting usercat='all' in an Sudo rule when there are users
+        """
+        api.Command['sudorule_add_user'](self.rule_name, users='admin')
+        try:
+            api.Command['sudorule_mod'](self.rule_name, usercategory=u'all')
+        except errors.MutuallyExclusiveError:
+            pass
+        finally:
+            api.Command['sudorule_remove_user'](self.rule_name, users='admin')
+
+    def test_e_sudorule_exclusivehost(self):
+        """
+        Test adding a host to an Sudo rule when hostcat='all'
+        """
+        api.Command['sudorule_mod'](self.rule_name, hostcategory=u'all')
+        try:
+            api.Command['sudorule_add_host'](self.rule_name, host=self.test_host)
+        except errors.MutuallyExclusiveError:
+            pass
+        api.Command['sudorule_mod'](self.rule_name, hostcategory=u'')
+
+    def test_f_sudorule_exclusivehost(self):
+        """
+        Test setting hostcat='all' in an Sudo rule when there are hosts
+        """
+        api.Command['sudorule_add_host'](self.rule_name, host=self.test_host)
+        try:
+            api.Command['sudorule_mod'](self.rule_name, hostcategory=u'all')
+        except errors.MutuallyExclusiveError:
+            pass
+        finally:
+            api.Command['sudorule_remove_host'](self.rule_name, host=self.test_host)
+
+    def test_g_sudorule_exclusivecommand(self):
+        """
+        Test adding a command to an Sudo rule when cmdcategory='all'
+        """
+        api.Command['sudorule_mod'](self.rule_name, cmdcategory=u'all')
+        try:
+            api.Command['sudorule_add_allow_command'](self.rule_name, sudocmd=self.test_command)
+        except errors.MutuallyExclusiveError:
+            pass
+        api.Command['sudorule_mod'](self.rule_name, cmdcategory=u'')
+
+    def test_h_sudorule_exclusivecommand(self):
+        """
+        Test setting cmdcategory='all' in an Sudo rule when there are commands
+        """
+        api.Command['sudorule_add_allow_command'](self.rule_name, sudocmd=self.test_command)
+        try:
+            api.Command['sudorule_mod'](self.rule_name, cmdcategory=u'all')
+        except errors.MutuallyExclusiveError:
+            pass
+        finally:
+            api.Command['sudorule_remove_allow_command'](self.rule_name, sudocmd=self.test_command)
+
+    def test_i_sudorule_exclusiverunas(self):
+        """
+        Test adding a runasuser to an Sudo rule when ipasudorunasusercategory='all'
+        """
+        api.Command['sudorule_mod'](self.rule_name, ipasudorunasusercategory=u'all')
+        try:
+            api.Command['sudorule_add_runasuser'](self.rule_name, sudocmd=self.test_user)
+        except errors.MutuallyExclusiveError:
+            pass
+        api.Command['sudorule_mod'](self.rule_name, ipasudorunasusercategory=u'')
+
+    def test_j_sudorule_exclusiverunas(self):
+        """
+        Test setting ipasudorunasusercategory='all' in an Sudo rule when there are runas users
+        """
+        api.Command['sudorule_add_runasuser'](self.rule_name, user=self.test_user)
+        try:
+            api.Command['sudorule_mod'](self.rule_name, ipasudorunasusercategory=u'all')
+        except errors.MutuallyExclusiveError:
+            pass
+        finally:
+            api.Command['sudorule_remove_runasuser'](self.rule_name, user=self.test_command)
+
+    def test_k_sudorule_clear_testing_data(self):
         """
         Clear data for Sudo rule plugin testing.
         """
@@ -534,7 +626,7 @@ class test_sudorule(XMLRPC_test):
         api.Command['sudocmdgroup_del'](self.test_sudodenycmdgroup)
 
 
-    def test_f_sudorule_del(self):
+    def test_l_sudorule_del(self):
         """
         Test deleting a Sudo rule using `xmlrpc.sudorule_del`.
         """
-- 
1.7.6

_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to