[SSSD] [PATCHES] HBAC access control rewrite

Sumit Bose sbose at redhat.com
Fri Jul 8 18:31:02 UTC 2011


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



More information about the sssd-devel mailing list