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