[SSSD] [PATCHES] HBAC access control rewrite

Stephen Gallagher sgallagh at redhat.com
Fri Jul 8 19:12:47 UTC 2011


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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20110708/463ccb49/attachment.sig>


More information about the sssd-devel mailing list