On 09/11/2012 01:14 PM, Martin Kosek wrote:
On 09/06/2012 01:13 PM, Tomas Babej wrote:
On 09/05/2012 01:56 PM, Martin Kosek wrote:
On 09/03/2012 05:12 PM, Tomas Babej wrote:
Hi,

Both selinuxusermap-add and selinuxusermap-mod commands now behave
consistently in not allowing user/host category or user/host members
and HBAC rule being set at the same time. Also adds a bunch of unit
tests that check this behaviour.

https://fedorahosted.org/freeipa/ticket/2983

Tomas

I found few issues with this patch:

1) Patch needs a rebase

2) Patch does not expect attributes to be set to None, i.e. to be left empty or
to be deleted, e.g.:

# ipa selinuxusermap-add foo --selinuxuser=guest_u:s0 --usercat=all --hbacrule=
ipa: ERROR: HBAC rule and local members cannot both be set

# ipa selinuxusermap-add foo --selinuxuser=guest_u:s0 --usercat=all
----------------------------
Added SELinux User Map "foo"
----------------------------
    Rule name: foo
    SELinux User: guest_u:s0
    User category: all
    Enabled: TRUE

# ipa selinuxusermap-mod foo --usercat= --hbacrule=
ipa: ERROR: HBAC rule and local members cannot both be set

# ipa selinuxusermap-mod foo --usercat=
-------------------------------
Modified SELinux User Map "foo"
-------------------------------
    Rule name: foo
    SELinux User: guest_u:s0
    Enabled: TRUE

# ipa selinuxusermap-mod foo --hbacrule=foo
-------------------------------
Modified SELinux User Map "foo"
-------------------------------
    Rule name: foo
    SELinux User: guest_u:s0
    HBAC Rule: foo
    Enabled: TRUE

# ipa selinuxusermap-mod foo --hbacrule= --usercat=all
ipa: ERROR: HBAC rule and local members cannot both be set

All these validation failures are not valid.

3) Additionally, I think it would be more readable and less error prone that if
instead of this blob:

+        are_local_members_to_be_set  = 'usercategory' in _entry_attrs or \
+                                       'hostcategory' in _entry_attrs or \
+                                       'memberuser' in _entry_attrs or \
+                                       'memberhost' in _entry_attrs

You would use something like that:

are_local_members_to_be_set  = any(attr in _entry_attrs
                                     for attr in ('usercategory',
                                                  'hostcategory',
                                                  'memberuser',
                                                  'memberhost'))

Martin
1.) Done.
2.) Corrected.
3.) Fixed.

Tomas
1) There are some (corner) cases where this approach still does not work:

# ipa selinuxusermap-show foo
   Rule name: foo
   SELinux User: guest_u:s0
   HBAC Rule: foo
   Enabled: TRUE
# ipa selinuxusermap-mod foo --usercat=all --hbacrule=
ipa: ERROR: HBAC rule and local members cannot both be set

HBAC rule attribute is being deleted and user category set, so this should not
be rejected.

2) There are also some styling issues (you can use pep8 tool present in Fedora
to locate them on your own, e.g.:

ipalib/plugins/selinuxusermap.py:247:32: E203 whitespace before ':'
ipalib/plugins/selinuxusermap.py:247:70: E225 missing whitespace around operator
ipalib/plugins/selinuxusermap.py:249:36: E221 multiple spaces before operator
...

Martin
The corner case is fixed now and styling issues corrected as well.

Tomas
>From 003e340bceb2bbae614f07edf1dd3d25d1f1ac23 Mon Sep 17 00:00:00 2001
From: Tomas Babej <tba...@redhat.com>
Date: Thu, 6 Sep 2012 07:03:42 -0400
Subject: [PATCH] Make sure selinuxusemap behaves consistently to HBAC rule

Both selinuxusermap-add and selinuxusermap-mod commands now behave
consistently in not allowing user/host category or user/host members
and HBAC rule being set at the same time. Also adds a bunch of unit
tests that check this behaviour.

https://fedorahosted.org/freeipa/ticket/2983
---
 ipalib/plugins/selinuxusermap.py                |  76 +++++++---
 tests/test_xmlrpc/test_selinuxusermap_plugin.py | 179 ++++++++++++++++++++++++
 2 files changed, 237 insertions(+), 18 deletions(-)

diff --git a/ipalib/plugins/selinuxusermap.py b/ipalib/plugins/selinuxusermap.py
index 13bbb58ec0e6b7bd4275be17198c7452090a0781..32c55850b7d5b78f39cfae8960b8588a35b30251 100644
--- a/ipalib/plugins/selinuxusermap.py
+++ b/ipalib/plugins/selinuxusermap.py
@@ -70,6 +70,7 @@ SEEALSO:
 
 notboth_err = _('HBAC rule and local members cannot both be set')
 
+
 def validate_selinuxuser(ugettext, user):
     """
     An SELinux user has 3 components: user:MLS:MCS. user and MLS are required.
@@ -91,7 +92,7 @@ def validate_selinuxuser(ugettext, user):
 
     # If we add in ::: we don't have to check to see if some values are
     # empty
-    (name, mls, mcs, ignore) = (user + ':::').split(':',3)
+    (name, mls, mcs, ignore) = (user + ':::').split(':', 3)
 
     if not regex_name.match(name):
         return _('Invalid SELinux user name, only a-Z and _ are allowed')
@@ -99,10 +100,12 @@ def validate_selinuxuser(ugettext, user):
         return _('Invalid MLS value, must match s[0-15](-s[0-15])')
     m = regex_mcs.match(mcs)
     if mcs and (not m or (m.group(3) and (int(m.group(3)) > 1023))):
-        return _('Invalid MCS value, must match c[0-1023].c[0-1023] and/or c[0-1023]-c[0-c0123]')
+        return _('Invalid MCS value, must match c[0-1023].c[0-1023] '
+                 'and/or c[0-1023]-c[0-c0123]')
 
     return None
 
+
 def validate_selinuxuser_inlist(ldap, user):
     """
     Ensure the user is in the list of allowed SELinux users.
@@ -112,13 +115,17 @@ def validate_selinuxuser_inlist(ldap, user):
     config = ldap.get_ipa_config()[1]
     item = config.get('ipaselinuxusermaporder', [])
     if len(item) != 1:
-        raise errors.NotFound(reason=_('SELinux user map list not found in configuration'))
+        raise errors.NotFound(reason=_('SELinux user map list not '
+                                       'found in configuration'))
     userlist = item[0].split('$')
     if user not in userlist:
-        raise errors.NotFound(reason=_('SELinux user %(user)s not found in ordering list (in config)') % dict(user=user))
+        raise errors.NotFound(
+            reason=_('SELinux user %(user)s not found in '
+                     'ordering list (in config)') % dict(user=user))
 
     return
 
+
 class selinuxusermap(LDAPObject):
     """
     SELinux User Map object.
@@ -211,7 +218,11 @@ class selinuxusermap(LDAPObject):
         except ValueError:
             try:
                 (dn, entry_attrs) = self.backend.find_entry_by_attr(
-                    self.api.Object['hbacrule'].primary_key.name, seealso, self.api.Object['hbacrule'].object_class, [''], self.api.Object['hbacrule'].container_dn)
+                    self.api.Object['hbacrule'].primary_key.name,
+                    seealso,
+                    self.api.Object['hbacrule'].object_class,
+                    [''],
+                    self.api.Object['hbacrule'].container_dn)
                 seealso = dn
             except errors.NotFound:
                 raise errors.NotFound(reason=_('HBAC rule %(rule)s not found') % dict(rule=seealso))
@@ -223,7 +234,7 @@ class selinuxusermap(LDAPObject):
         Convert an HBAC rule dn into a name
         """
         if options.get('raw', False):
-             return
+            return
 
         if 'seealso' in entry_attrs:
             (hbac_dn, hbac_attrs) = ldap.get_entry(entry_attrs['seealso'][0], ['cn'])
@@ -242,8 +253,21 @@ class selinuxusermap_add(LDAPCreate):
         # rules are enabled by default
         entry_attrs['ipaenabledflag'] = 'TRUE'
         validate_selinuxuser_inlist(ldap, entry_attrs['ipaselinuxuser'])
-        if 'seealso' in options:
-            entry_attrs['seealso'] = self.obj._normalize_seealso(options['seealso'])
+
+        # hbacrule is not allowed when usercat or hostcat is set
+        is_to_be_set = lambda x: x in entry_attrs and entry_attrs[x] != None
+
+        are_local_members_to_be_set = any(is_to_be_set(attr)
+                                          for attr in ('usercategory',
+                                                       'hostcategory'))
+
+        is_hbacrule_to_be_set = is_to_be_set('seealso')
+
+        if is_hbacrule_to_be_set and are_local_members_to_be_set:
+            raise errors.MutuallyExclusiveError(reason=notboth_err)
+
+        if is_hbacrule_to_be_set:
+            entry_attrs['seealso'] = self.obj._normalize_seealso(entry_attrs['seealso'])
 
         return dn
 
@@ -276,18 +300,34 @@ class selinuxusermap_mod(LDAPUpdate):
         except errors.NotFound:
             self.obj.handle_not_found(*keys)
 
-        if 'seealso' in options and ('usercategory' in _entry_attrs or
-          'hostcategory' in _entry_attrs or
-          'memberuser' in _entry_attrs or
-          'memberhost' in _entry_attrs):
+        is_to_be_deleted = lambda x: (x in _entry_attrs and x in entry_attrs) and \
+                                     entry_attrs[x] == None
+
+        # makes sure the local members and hbacrule is not set at the same time
+        # memberuser or memberhost could have been set using --setattr
+        is_to_be_set = lambda x: ((x in _entry_attrs and _entry_attrs[x] != None) or \
+                                 (x in entry_attrs and entry_attrs[x] != None)) and \
+                                 not is_to_be_deleted(x)
+
+        are_local_members_to_be_set = any(is_to_be_set(attr)
+                                          for attr in ('usercategory',
+                                                       'hostcategory',
+                                                       'memberuser',
+                                                       'memberhost'))
+
+        is_hbacrule_to_be_set = is_to_be_set('seealso')
+
+        # this can disable all modifications if hbacrule and local members were
+        # set at the same time bypassing this commad, e.g. using ldapmodify
+        if are_local_members_to_be_set and is_hbacrule_to_be_set:
             raise errors.MutuallyExclusiveError(reason=notboth_err)
 
-        if is_all(options, 'usercategory') and 'memberuser' in entry_attrs:
-            raise errors.MutuallyExclusiveError(reason=_("user category "
-                "cannot be set to 'all' while there are allowed 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 allowed hosts"))
+        if is_all(entry_attrs, 'usercategory') and 'memberuser' in entry_attrs:
+            raise errors.MutuallyExclusiveError(reason="user category "
+                 "cannot be set to 'all' while there are allowed users")
+        if is_all(entry_attrs, 'hostcategory') and 'memberhost' in entry_attrs:
+            raise errors.MutuallyExclusiveError(reason="host category "
+                 "cannot be set to 'all' while there are allowed hosts")
 
         if 'ipaselinuxuser' in entry_attrs:
             validate_selinuxuser_inlist(ldap, entry_attrs['ipaselinuxuser'])
diff --git a/tests/test_xmlrpc/test_selinuxusermap_plugin.py b/tests/test_xmlrpc/test_selinuxusermap_plugin.py
index aa2d0cac92f0944653be87be5df1fbe96470b3bc..816e7673584cc5adba4e40de4fcc9527e19ee2b2 100644
--- a/tests/test_xmlrpc/test_selinuxusermap_plugin.py
+++ b/tests/test_xmlrpc/test_selinuxusermap_plugin.py
@@ -664,4 +664,183 @@ class test_selinuxusermap(Declarative):
                 error=u'Invalid MLS value, must match s[0-15](-s[0-15])'),
         ),
 
+        dict(
+            desc='Create rule with both --hbacrule and --usercat set',
+            command=(
+                'selinuxusermap_add', [rule1], dict(ipaselinuxuser=selinuxuser1,seealso=hbacrule1,usercategory=u'all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Create rule with both --hbacrule and --hostcat set',
+            command=(
+                'selinuxusermap_add', [rule1], dict(ipaselinuxuser=selinuxuser1,seealso=hbacrule1,hostcategory=u'all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Create rule with both --hbacrule and --usercat set via setattr',
+            command=(
+                'selinuxusermap_add', [rule1], dict(ipaselinuxuser=selinuxuser1,seealso=hbacrule1,setattr=u'usercategory=all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Create rule with both --hbacrule and --hostcat set via setattr',
+            command=(
+                'selinuxusermap_add', [rule1], dict(ipaselinuxuser=selinuxuser1,seealso=hbacrule1,setattr=u'hostcategory=all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Create rule %r with --hbacrule' % rule1,
+            command=(
+                'selinuxusermap_add', [rule1], dict(ipaselinuxuser=selinuxuser1,seealso=hbacrule1)
+            ),
+            expected=dict(
+                value=rule1,
+                summary=u'Added SELinux User Map "%s"' % rule1,
+                result=dict(
+                    cn=[rule1],
+                    ipaselinuxuser=[selinuxuser1],
+                    objectclass=objectclasses.selinuxusermap,
+                    ipauniqueid=[fuzzy_uuid],
+                    ipaenabledflag = [u'TRUE'],
+                    dn=fuzzy_selinuxusermapdn,
+                    seealso=hbacrule1
+                ),
+            ),
+        ),
+
+        dict(
+            desc='Add an --usercat to %r that has HBAC set' % rule1,
+            command=(
+                'selinuxusermap_mod', [rule1], dict(usercategory=u'all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Add an --hostcat to %r that has HBAC set' % rule1,
+            command=(
+                'selinuxusermap_mod', [rule1], dict(hostcategory=u'all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Add an usercat via setattr to %r that has HBAC set' % rule1,
+            command=(
+                'selinuxusermap_mod', [rule1], dict(setattr=u'usercategory=all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Add an hostcat via setattr to %r that has HBAC set' % rule1,
+            command=(
+                'selinuxusermap_mod', [rule1], dict(setattr=u'hostcategory=all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Delete %r' % rule1,
+            command=('selinuxusermap_del', [rule1], {}),
+            expected=dict(
+                result=dict(failed=u''),
+                value=rule1,
+                summary=u'Deleted SELinux User Map "%s"' % rule1,
+            )
+        ),
+
+        dict(
+            desc='Create rule %r with usercat and hostcat set' % rule1,
+            command=(
+                'selinuxusermap_add', [rule1], dict(ipaselinuxuser=selinuxuser1,usercategory=u'all',hostcategory=u'all')
+            ),
+            expected=dict(
+                value=rule1,
+                summary=u'Added SELinux User Map "%s"' % rule1,
+                result=dict(
+                    cn=[rule1],
+                    ipaselinuxuser=[selinuxuser1],
+                    objectclass=objectclasses.selinuxusermap,
+                    ipauniqueid=[fuzzy_uuid],
+                    ipaenabledflag = [u'TRUE'],
+                    dn=fuzzy_selinuxusermapdn,
+                    usercategory = [u'all'],
+                    hostcategory = [u'all']
+                ),
+            ),
+        ),
+
+        dict(
+            desc='Add HBAC rule to %r that has usercat and hostcat' % rule1,
+            command=(
+                'selinuxusermap_mod', [rule1], dict(seealso=hbacrule1)
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Delete %r' % rule1,
+            command=('selinuxusermap_del', [rule1], {}),
+            expected=dict(
+                result=dict(failed=u''),
+                value=rule1,
+                summary=u'Deleted SELinux User Map "%s"' % rule1,
+            )
+        ),
+
+        dict(
+            desc='Create rule %r' % rule1,
+            command=(
+                'selinuxusermap_add', [rule1], dict(ipaselinuxuser=selinuxuser1)
+            ),
+            expected=dict(
+                value=rule1,
+                summary=u'Added SELinux User Map "%s"' % rule1,
+                result=dict(
+                    cn=[rule1],
+                    ipaselinuxuser=[selinuxuser1],
+                    objectclass=objectclasses.selinuxusermap,
+                    ipauniqueid=[fuzzy_uuid],
+                    ipaenabledflag = [u'TRUE'],
+                    dn=fuzzy_selinuxusermapdn,
+                ),
+            ),
+        ),
+
+        dict(
+            desc='Add HBAC rule, hostcat and usercat to %r' % rule1,
+            command=(
+                'selinuxusermap_mod', [rule1], dict(seealso=hbacrule1,usercategory=u'all',hostcategory=u'all')
+            ),
+            expected=errors.MutuallyExclusiveError(
+                reason=u'HBAC rule and local members cannot both be set'),
+        ),
+
+        dict(
+            desc='Delete %r' % rule1,
+            command=('selinuxusermap_del', [rule1], {}),
+            expected=dict(
+                result=dict(failed=u''),
+                value=rule1,
+                summary=u'Deleted SELinux User Map "%s"' % rule1,
+            )
+        ),
     ]
-- 
1.7.11.4

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

Reply via email to