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
>From e8dd66cc3fc82b5b6cdf71804073cee3a2c1d091 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                |   67 ++++++++++++++++++--
 tests/test_xmlrpc/test_sudorule_plugin.py |   96 ++++++++++++++++++++++++++++-
 3 files changed, 163 insertions(+), 11 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 281f6b4ddd4db69b2e53d83b7550ff9d2c9948d6..9082fa5bd13fb04c9f89eb881d997fc9cf9e92ec 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, Int, DateTime
 from ipalib.plugins.baseldap import *
+from ipalib.plugins.hbacrule import is_all
 from ipalib import _, ngettext
 
 __doc__ = _("""
@@ -300,16 +301,16 @@ class sudorule_mod(LDAPUpdate):
             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")
+            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")
+            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")
+            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")
+            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")
+            raise errors.MutuallyExclusiveError(reason=_("group runAs category cannot be set to 'all' while there are groups"))
 
         return dn
 
@@ -386,6 +387,15 @@ 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)
 
 
@@ -404,6 +414,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)
 
 
@@ -422,6 +441,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
@@ -490,6 +518,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
@@ -580,6 +617,16 @@ class sudorule_add_runasuser(LDAPAddMember):
 
         return dn
 
+    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, 'ipasudorunasusercategory') or \
+          is_all(_entry_attrs, 'ipasudorunasgroupcategory'):
+            raise errors.MutuallyExclusiveError(reason=_("users cannot be added when runAs user or runAs group 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
@@ -664,6 +711,16 @@ class sudorule_add_runasgroup(LDAPAddMember):
 
         return dn
 
+    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, 'ipasudorunasusercategory') or \
+          is_all(_entry_attrs, 'ipasudorunasgroupcategory'):
+            raise errors.MutuallyExclusiveError(reason=_("users cannot be added when runAs user or runAs group category='all'"))
+        return dn
+
     def post_callback(self, ldap, completed, failed, dn, entry_attrs, *keys, **options):
         completed_external = 0
         # Sift through the group failures. We assume that these are all
diff --git a/tests/test_xmlrpc/test_sudorule_plugin.py b/tests/test_xmlrpc/test_sudorule_plugin.py
index aa405fadd6a93bca93ddb013c1f9abff1e2e7508..706bc77eab38deb6ee4d631de22a656b4de2d00a 100644
--- a/tests/test_xmlrpc/test_sudorule_plugin.py
+++ b/tests/test_xmlrpc/test_sudorule_plugin.py
@@ -48,7 +48,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):
@@ -521,7 +521,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.
         """
-- 
1.7.6

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

Reply via email to