[Freeipa-devel] [PATCHES 187-201] Improvements and coverage for sudorule plugin

Tue Jun 17 10:25:44 UTC 2014

On 05/26/2014 06:20 PM, Petr Viktorin wrote:
> On 05/20/2014 06:15 PM, Tomas Babej wrote:
>> Hi,
>>
>> the following set of patches fixes:
>>
>> https://fedorahosted.org/freeipa/ticket/4274
>> https://fedorahosted.org/freeipa/ticket/4263
>> https://fedorahosted.org/freeipa/ticket/4324
>> https://fedorahosted.org/freeipa/ticket/4340
>> https://fedorahosted.org/freeipa/ticket/4341
>>
>> and additional minor issues.
>>
>> The improvemed CI test coverage for the sudorule plugin is added as a
>> bonus.
>
> Thanks!
> I haven't tested yet; here are my comments on the code.
>
>
> 0187 - sudorule: PEP8 fixes in sudorule.py
>
> This change:
>
>> @@ -527,11 +551,15 @@ def pre_callback(self, ldap, dn, found,
>> not_found, *keys, **options):
>>              _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'"))
>> +
>> +        if is_all(self._entry_attrs, 'hostcategory'):
>> +            raise errors.MutuallyExclusiveError(
>> +                reason=_("hosts cannot be added when host
>> category='all'"))
>> +
>>          return add_external_pre_callback('host', ldap, dn, keys,
>> options)
>
> adds `self` to `self._entry_attrs`. That should be a part of the next
> patch.
> Git tip: `git log --word-diff` helps a lot when you play with whitespace
Right, it's gone.

>
> (Speaking of PEP8, if you could remove the baseldap star import from
> sudorule.py, it would be great...)
>
>
This one did hurt, but the star disappeared.

>
> General thoughts:
>
> Would it be possible to merge schema_compat.uldif and
> install/updates/10-schema_compat.update into one file? Is the uldif
> special somehow? I guess this is a question for Rob.
> It would be nice to add a link to some schema-compat-entry-attribute
> documentation to these files.
>
I added Rob to cc. Rob, can you elaborate on this?

>
>
> 0188 - sudorule: Allow using hostmasks for setting allowed hosts
>
> Here:
>
>> --- a/install/updates/40-delegation.update
>> +++ b/install/updates/40-delegation.update
>> @@ -174,7 +174,7 @@ default:member: cn=Sudo
>> Administrator,cn=privileges,cn=pbac,$SUFFIX
>>  dn: $SUFFIX
>>  add:aci: '(target =
>> "ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX")(version 3.0;acl
>> "permission:Add Sudo rule";allow (add) groupdn = "ldap:///cn=Add Sudo
>> rule,cn=permissions,cn=pbac,$SUFFIX";)'
>>  add:aci: '(target =
>> "ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX")(version 3.0;acl
>> "permission:Delete Sudo rule";allow (delete) groupdn =
>> "ldap:///cn=Delete Sudo rule,cn=permissions,cn=pbac,$SUFFIX";)'
>> -add:aci: '(targetattr = "description || ipaenabledflag ||
>> usercategory || hostcategory || cmdcategory ||
>> ipasudorunasusercategory || ipasudorunasgroupcategory || externaluser
>> || ipasudorunasextuser || ipasudorunasextgroup || memberdenycmd ||
>> memberallowcmd || memberuser")(target =
>> "ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX")(version 3.0;acl
>> "permission:Modify Sudo rule";allow (write) groupdn =
>> "ldap:///cn=Modify Sudo rule,cn=permissions,cn=pbac,$SUFFIX";)'
>> +add:aci: '(targetattr = "description || ipaenabledflag ||
>> usercategory || hostcategory || cmdcategory ||
>> ipasudorunasusercategory || ipasudorunasgroupcategory || externaluser
>> || ipasudorunasextuser  || ipasudorunasextgroup || memberdenycmd ||
>> memberallowcmd || memberuser || hostmask")(target =
>> "ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX")(version 3.0;acl
>> "permission:Modify Sudo rule";allow (write) groupdn =
>> "ldap:///cn=Modify Sudo rule,cn=permissions,cn=pbac,$SUFFIX";)'
>>
>>  remove:aci: '(targetattr = "description")(target =
>> "ldap:///sudocmd=*,cn=sudocmds,cn=sudo,$SUFFIX")(version 3.0;acl
>> "permission:Modify Sudo command";allow (write) groupdn =
>> "ldap:///cn=Modify Sudo command,cn=permissions,cn=pbac,$SUFFIX";)'
>>  remove:aci: '(target =
>> "ldap:///sudocmd=*,cn=sudocmds,cn=sudo,$SUFFIX")(version 3.0;acl
>> "permission:Delete Sudo command";allow (delete) groupdn =
>> "ldap:///cn=Delete Sudo command,cn=permissions,cn=pbac,$SUFFIX";)'
>
> you'll want to remove the old ACI before adding the new version.
>
This is irrelevant since we're rebased on top of your ACI refactoring now.

> In get_options of sudorule_add_host and sudorule_remove_host, it would
> be DRY-er to put the hostmask in a shared variable.
>
> You write:
>> -            _entry_attrs = ldap.get_entry(dn,
>> self.obj.default_attributes)
>> +            self._entry_attrs = ldap.get_entry(dn,
>> self.obj.default_attributes)
>
> Never store anything on the Command object. They're currently
> singletons. Another thread could change _entry_attrs to a different
> value.
> This affects all the other uses of self._entry_attrs, of course.
Fixed.

>
> In:
>> +            num_added = len(new_masks.difference(old_masks))
> and:
>> +                entry_attrs['hostmask'] =
>> list(old_masks.union(new_masks))
> and:
>> +            num_added = len(removed_masks.intersection(old_masks))
> etc.: with two sets, you can use the `-`, `|`, and `&` operators.
>
Cool tip, replaced!

>
> Since you're touching this:
>> +        return add_external_post_callback('memberhost', 'host',
>> 'externalhost',
>> +                                          ldap, completed, failed, dn,
>> +                                          entry_attrs, keys, options)
> it would be nice to pass the arguments by name, for clarity and to
> avoid mistakes. I guess this should be in the PEP8 patch, too.
>
Done, part of patch 225.

>
>
>
> 0189 sudorule: Allow using external groups as groups of runAsUsers
>
> In sudorule_add_runasuser and sudorule_remove_runasuser, could you
> again pass the arguments by name?
> I'd welcome comment explaining why completed=0. If you could throw in
> a comment for add_external_post_callback explaining the inputs and
> outputs, it would be great -- I'm getting quite lost in this code.
> (I'd even go as far as changing the signature of
> *_external_post_callback -- AFAICS, in all the places we call it, we
> do `*keys, **options` wrong, and the `completed` argument is not
> useful at all. Most of the calls are from sudorule so it would make
> sense to do it in this patchset.)
>
This is done in the new attached patch 225.

> And a heads-up: the change to "permission:Modify Sudo rule" conflicts
> with my ACI work. Let me know if there are problems.
>
>
>
>
> 0190 sudorule: Make sure sudoRunAsGroup is dereferencing the correct
> attribute
>
> Looks good
>
>
>
>
> 0191 sudorule: Include externalhost and ipasudorunasextgroup in  the
> list of default attributes
>
> Looks good
>
>
>
>
> 0192 sudorule: Allow adding deny commands when command category set to
> ALL
>
> Looks good
>
>
>
>
> 0193 sudorule: Make sure all the relevant attributes are checked when
> setting category to ALL
>
> Wow, haven't seen a copyright year change in some time. We are sloppy
> with those. Portions of the file are still from 2010 so you should
> definitely leave that year in, though. Actually I think the proper
> year field would read "2010,2011,2012,2013,2014".
> I am not a lawyer.
>
> You're missing the `_` for the hostcategory error message.
> Did you think about using something like _("%s cannot be set to 'all'
> while there are %s")?
>
Fixed. Initially, I changed the message as you suggested, but then I
realized, that this might pose a problem for translations that do not
follow the word order in the sentence as it is defined in English language.

>
>
> 0194 sudorule: Fix the order of the parameters to have less chaotic
> output
>
> Looks fine
>
>
>
>
> 0195-sudorule-Enforce-category-ALL-checks-on-dirsrv-level.patch
>
> Looks fine
>
>
>
>
> 0196-0198 (Add tests)
>
> Looks fine. Just remind me, why is this all in integration tests?
> Couldn't it at least some of it be in the main test suite? It looks to
> me like most of it doesn't need multiple machines.
>
>
We need to check that the rules are actually enforced on the client
setup to look them up on the IPA server. This tests compat tree entry
generation, SSSD-sudo integration and the actual rule enforcement. All
sudo commands are executed on the client machine.


>
>
> 0199-0201 (test fixes)
>
> OK
>
>

-- 
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0225-sudorule-Refactor-add-and-remove-external_post_callb.patch
Type: text/x-patch
Size: 21184 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0187-3-sudorule-PEP8-fixes-in-sudorule.py.patch
Type: text/x-patch
Size: 18390 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0188-3-sudorule-Allow-using-hostmasks-for-setting-allowed-h.patch
Type: text/x-patch
Size: 11076 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0189-3-sudorule-Allow-using-external-groups-as-groups-of-ru.patch
Type: text/x-patch
Size: 13876 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0190-3-sudorule-Make-sure-sudoRunAsGroup-is-dereferencing-t.patch
Type: text/x-patch
Size: 2736 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0191-3-sudorule-Include-externalhost-and-ipasudorunasextgro.patch
Type: text/x-patch
Size: 1079 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0192-3-sudorule-Allow-adding-deny-commands-when-command-cat.patch
Type: text/x-patch
Size: 1092 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0193-3-sudorule-Make-sure-all-the-relevant-attributes-are-c.patch
Type: text/x-patch
Size: 4336 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0007.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0194-3-sudorule-Fix-the-order-of-the-parameters-to-have-les.patch
Type: text/x-patch
Size: 2984 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0195-3-sudorule-Enforce-category-ALL-checks-on-dirsrv-level.patch
Type: text/x-patch
Size: 4898 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0196-3-ipatests-test_sudo-Add-tests-for-allowing-hosts-via-.patch
Type: text/x-patch
Size: 2625 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0197-3-ipatests-test_sudo-Add-coverage-for-external-entries.patch
Type: text/x-patch
Size: 6890 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0198-3-ipatests-test_sudo-Add-coverage-for-category-ALL-val.patch
Type: text/x-patch
Size: 15765 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0199-3-ipatests-test_sudo-Fix-assertions-not-assuming-runas.patch
Type: text/x-patch
Size: 4708 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0200-3-ipatests-test_sudo-Do-not-expect-enumeration-of-runa.patch
Type: text/x-patch
Size: 1089 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0201-3-ipatests-test_sudo-Expect-root-listed-out-if-no-RunA.patch
Type: text/x-patch
Size: 1544 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140617/f4db7a79/attachment-0015.bin>