<html> <head> <meta content="text/html; charset=ISO-8859-1" http-equiv="Content-Type"> </head> <body bgcolor="#FFFFFF" text="#000000"> <div class="moz-cite-prefix">On 05/26/2014 06:20 PM, Petr Viktorin wrote: </div> <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite">On 05/20/2014 06:15 PM, Tomas Babej wrote: <blockquote type="cite">Hi, the following set of patches fixes: <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4274">https://fedorahosted.org/freeipa/ticket/4274</a> <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4263">https://fedorahosted.org/freeipa/ticket/4263</a> <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4324">https://fedorahosted.org/freeipa/ticket/4324</a> <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4340">https://fedorahosted.org/freeipa/ticket/4340</a> <a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4341">https://fedorahosted.org/freeipa/ticket/4341</a> and additional minor issues. The improvemed CI test coverage for the sudorule plugin is added as a bonus. </blockquote> Thanks! I haven't tested yet; here are my comments on the code. 0187 - sudorule: PEP8 fixes in sudorule.py This change: <blockquote type="cite">@@ -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) </blockquote> 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 </blockquote> Right, it's gone. <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite"> (Speaking of PEP8, if you could remove the baseldap star import from sudorule.py, it would be great...) </blockquote> This one did hurt, but the star disappeared. <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite"> 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. </blockquote> I added Rob to cc. Rob, can you elaborate on this? <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite"> 0188 - sudorule: Allow using hostmasks for setting allowed hosts Here: <blockquote type="cite">--- 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 = <a class="moz-txt-link-rfc2396E" href="ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX">"ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX"</a>)(version 3.0;acl "permission:Add Sudo rule";allow (add) groupdn = <a class="moz-txt-link-rfc2396E" href="ldap:///cn=AddSudorule,cn=permissions,cn=pbac,$SUFFIX">"ldap:///cn=Add Sudo rule,cn=permissions,cn=pbac,$SUFFIX"</a>;)' add:aci: '(target = <a class="moz-txt-link-rfc2396E" href="ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX">"ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX"</a>)(version 3.0;acl "permission:Delete Sudo rule";allow (delete) groupdn = <a class="moz-txt-link-rfc2396E" href="ldap:///cn=DeleteSudorule,cn=permissions,cn=pbac,$SUFFIX">"ldap:///cn=Delete Sudo rule,cn=permissions,cn=pbac,$SUFFIX"</a>;)' -add:aci: '(targetattr = "description || ipaenabledflag || usercategory || hostcategory || cmdcategory || ipasudorunasusercategory || ipasudorunasgroupcategory || externaluser || ipasudorunasextuser || ipasudorunasextgroup || memberdenycmd || memberallowcmd || memberuser")(target = <a class="moz-txt-link-rfc2396E" href="ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX">"ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX"</a>)(version 3.0;acl "permission:Modify Sudo rule";allow (write) groupdn = <a class="moz-txt-link-rfc2396E" href="ldap:///cn=ModifySudorule,cn=permissions,cn=pbac,$SUFFIX">"ldap:///cn=Modify Sudo rule,cn=permissions,cn=pbac,$SUFFIX"</a>;)' +add:aci: '(targetattr = "description || ipaenabledflag || usercategory || hostcategory || cmdcategory || ipasudorunasusercategory || ipasudorunasgroupcategory || externaluser || ipasudorunasextuser || ipasudorunasextgroup || memberdenycmd || memberallowcmd || memberuser || hostmask")(target = <a class="moz-txt-link-rfc2396E" href="ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX">"ldap:///ipauniqueid=*,cn=sudorules,cn=sudo,$SUFFIX"</a>)(version 3.0;acl "permission:Modify Sudo rule";allow (write) groupdn = <a class="moz-txt-link-rfc2396E" href="ldap:///cn=ModifySudorule,cn=permissions,cn=pbac,$SUFFIX">"ldap:///cn=Modify Sudo rule,cn=permissions,cn=pbac,$SUFFIX"</a>;)' remove:aci: '(targetattr = "description")(target = <a class="moz-txt-link-rfc2396E" href="ldap:///sudocmd=*,cn=sudocmds,cn=sudo,$SUFFIX">"ldap:///sudocmd=*,cn=sudocmds,cn=sudo,$SUFFIX"</a>)(version 3.0;acl "permission:Modify Sudo command";allow (write) groupdn = <a class="moz-txt-link-rfc2396E" href="ldap:///cn=ModifySudocommand,cn=permissions,cn=pbac,$SUFFIX">"ldap:///cn=Modify Sudo command,cn=permissions,cn=pbac,$SUFFIX"</a>;)' remove:aci: '(target = <a class="moz-txt-link-rfc2396E" href="ldap:///sudocmd=*,cn=sudocmds,cn=sudo,$SUFFIX">"ldap:///sudocmd=*,cn=sudocmds,cn=sudo,$SUFFIX"</a>)(version 3.0;acl "permission:Delete Sudo command";allow (delete) groupdn = <a class="moz-txt-link-rfc2396E" href="ldap:///cn=DeleteSudocommand,cn=permissions,cn=pbac,$SUFFIX">"ldap:///cn=Delete Sudo command,cn=permissions,cn=pbac,$SUFFIX"</a>;)' </blockquote> you'll want to remove the old ACI before adding the new version. </blockquote> This is irrelevant since we're rebased on top of your ACI refactoring now. <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite">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: <blockquote type="cite">- _entry_attrs = ldap.get_entry(dn, self.obj.default_attributes) + self._entry_attrs = ldap.get_entry(dn, self.obj.default_attributes) </blockquote> 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. </blockquote> Fixed. <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite"> In: <blockquote type="cite">+ num_added = len(new_masks.difference(old_masks)) </blockquote> and: <blockquote type="cite">+ entry_attrs['hostmask'] = list(old_masks.union(new_masks)) </blockquote> and: <blockquote type="cite">+ num_added = len(removed_masks.intersection(old_masks)) </blockquote> etc.: with two sets, you can use the `-`, `|`, and `&` operators. </blockquote> Cool tip, replaced! <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite"> Since you're touching this: <blockquote type="cite">+ return add_external_post_callback('memberhost', 'host', 'externalhost', + ldap, completed, failed, dn, + entry_attrs, keys, options) </blockquote> 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. </blockquote> Done, part of patch 225. <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite"> 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.) </blockquote> This is done in the new attached patch 225. <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite">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")? </blockquote> 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. <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite"> 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. </blockquote> <meta http-equiv="Content-Type" content="text/html; charset=ISO-8859-1"> 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. <blockquote cite="mid:538369E1.5020300@redhat.com" type="cite"> 0199-0201 (test fixes) OK </blockquote> <pre class="moz-signature" cols="72">-- Tomas Babej Associate Software Engineer | Red Hat | Identity Management RHCE | Brno Site | IRC: tbabej | freeipa.org </pre> </body> </html>