On Fri, Jul 08, 2011 at 12:34:44PM -0400, Stephen Gallagher wrote: > On Fri, 2011-07-08 at 14:59 +0200, Sumit Bose wrote: > > On Mon, Jul 04, 2011 at 07:27:17PM -0400, Jakub Hrozek wrote: > > > On 07/01/2011 04:44 PM, Stephen Gallagher wrote: > > > > And now with patches attached... > > > > > > > > On Fri, 2011-07-01 at 16:42 -0400, Stephen Gallagher wrote: > > > >> So many changes have been made since the original pass that I have > > > >> revisited the layout of my patches and have squashed many of them > > > >> together. With these patches, we are fully compatible with the change > > > >> to > > > >> disable DENY rules in FreeIPA. > > > >> > > > >> Patch 0001: Add helper function msgs2attrs_array > > > >> Unchanged from previous versions > > > >> > > > >> Patch 0002: Add HBAC evaluator and tests > > > >> The evaluator library has changed its interface slightly. It now always > > > >> returns an hbac_info object that contains the name of the rule that > > > >> matched (if one did). > > > >> > > > > > > I only had time to review these two patches as the bindings are built on > > > top of them -- sorry. > > > > > > 0001 - unchanged, ack > > > 0002 - ack, but please also fix the docstring of hbac_evaluate() before > > > pushing (error vs. info) > > Fixed > > > > > Hi, > > > > 0002 also adds an empty line at the end of configure.ac. > > > > I didn't see this in my patch.
ah, sorry it is not configure.ac, but ipa_hbac.pc.in, line 558 in your patch. If you find it, you can remove it before pushing, otherwise it is fine as well. > > > > > 0005-Add-new-HBAC-lookup-and-evaluation-routines.patch: > > + base_dn = sysdb_custom_subtree_dn(sysdb, NULL, > > + domain->name, > > + HBAC_RULES_SUBDIR); > > + if (base_dn == NULL) { > > + ipa_access_reply(hbac_ctx, PAM_SYSTEM_ERR); > > + return; > > + } > > > > sysdb_custom_subtree_dn() always fails, because ldb_dn_new_fmt() does > > not like a NULL memory context, additionally base_dn is never freed. > > > > Thanks, I added a tmp_ctx here and make sure everything gets freed. > > > 0006-Add-ipa_hbac_refresh-option.patch: > > I would prefer to save a reference to the ipa_access_ctx in hbac_ctx in > > ipa_access_handler() and use this later on. But this is just cosmetics. > > > > Done. > > > 0007-Add-ipa_hbac_treat_deny_as-option.patch > > + <para> > > + <emphasis>DENY_ALL</emphasis>: If any HBAC > > rules > > + are detected, all users will be denied access. > > + </para> > > > > I think you meant 'If any HBAC deny rules ...' > > > > Yup, good catch. > > > Although it is not the default I think it would make sense to have a > > level 5 or above debug message that tells that deny rules are ignored > > and not loaded from the server when the rules are evaluated. > > > > I decided not to bother with this. If this has been set, they're aware > of it. > > > I've done some testing and all the logic is working as expected. > > New patches attached. Thanks for the review! ACK to all, just push it :-) bye, Sumit _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel