On Fri, 2011-07-08 at 20:31 +0200, Sumit Bose wrote:
> 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 :-)Fixed the extra line and pushed to master.
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
