[SSSD] [PATCH] Backport ipa_hbac_support_srchost option to 1.5.x

Stephen Gallagher sgallagh at redhat.com
Wed Aug 15 17:45:35 UTC 2012


On Wed, 2012-08-15 at 11:12 +0200, Jakub Hrozek wrote:
> On Tue, Aug 14, 2012 at 04:47:04PM -0400, Stephen Gallagher wrote:
> > On Tue, 2012-08-14 at 18:12 +0200, Jakub Hrozek wrote:
> > > On Mon, Aug 13, 2012 at 07:27:42PM -0400, Stephen Gallagher wrote:
> > > > On Mon, 2012-08-13 at 15:03 -0400, Stephen Gallagher wrote:
> > > > > This patch backports commit 6fb75e297bf7fc83e3db1f5ae8560624656ef319 to
> > > > > the 1.5.x branch. It makes one major change: 1.5.x does not have support
> > > > > for deref/ASQ searches, so the deref lookup for hostgroups has been
> > > > > replaced by parallel LDAP "base" searches.
> > > > > 
> > > > > I'm attaching two patches. One is the complete patch to backport, the
> > > > > other is just an interdiff of the changes from Jan's original patch to
> > > > > support the parallel lookups.
> > > > > 
> > > > > I'm still in the process of testing this patch, but I wanted to get it
> > > > > out for review in the meantime.
> > > > 
> > > > Ok, during testing I found one bug where I forgot to initialize a
> > > > loop-control/array-index variable. Oops.
> > > > 
> > > > My basic smoketesting is now complete. Both modes (srchost enabled and
> > > > disabled) are working as expected for a small set of hosts. I don't have
> > > > the infrastructure to test large numbers, but I suspect the only
> > > > difference will be performance, not function.
> > > > 
> > > > New patches attached. As before, one is the complete patch ("Add
> > > > ipa_hbac_support_srchost option to IPA provider") and the other is just
> > > > the changes from Jan Zeleny's original patch (to simplify the review
> > > > process).
> > > 
> > > I don't understand this part:
> > > 
> > > > +    if (queue_len == 0) {
> > > > +        /* This host is not in any hostgroups */
> > > > +        ret = ENOMEM;
> > > > +        goto error;
> > > > +    }
> > > 
> > > This seems to be preventing me from logging into a host that is not part
> > > of any host group:
> > > 
> > > [ipa_hbac_hostgroup_info_done] (3): Error [12][Cannot allocate memory]
> > > [sdap_id_op_done] (9): releasing operation connection
> > > 
> > 
> > Thanks, there were two bugs there. The first was that this should have
> > been ENOENT, not ENOMEM. The second was that I forgot to special-case
> > ENOENT in ipa_hbac_hostgroup_info_done(). Both are now fixed.
> > 
> > New patches attached.
> > 
> > > The rest looks good. Thank you for the interdiff, indeed it made the
> > > review easier.
> > 
> > 
> 
> There's one more problem - if the host is not a member of any host
> group, it has no memberof attribute. However, this call would add an
> empty attribute in that case:
> 
> > +    ret = sysdb_attrs_get_el(state->hosts[0],
> > +                             SYSDB_ORIG_MEMBEROF,
> > +                             &parent_el);
> > +    if (ret != EOK) goto error;
> > +
> 
> and later saving that host would fail:
> 
> [sysdb_store_custom] (1): Failed to store custmo entry: Constraint violation(19)[attribute originalMemberOf on name=HOSTNAME,cn=hbac_hosts,cn=custom,cn=idm.lab.bos.redhat.com,cn=sysdb specified, but with 0 values (illegal)]
> [sysdb_store_custom] (6): Error: 14 (Bad address)
> cancel ldb transaction (nesting: 2)
> [ipa_hbac_save_list] (1): sysdb_store_custom failed.
> [ipa_hbac_sysdb_save] (1): Could not save hbac_hosts. [14][Bad address]
> 
> Does anyone remember why does sysdb_attrs_get_el internally create the
> attribute if it doesn't exist? To ease error checking? I think we should
> either change it or create another getter that wouldn't create the
> attribute automatically.

I don't remember the reasoning either, but I don't want to go through
the hassle of trying to change that during this backport. I added a new
patch (that will work on 1.5.x and master) that just exposes the
internal get_el() routine that takes an option to create or not create
the value. I then changed the above so it detects the ENOENT case
appropriately.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Convert-deref-searches-to-parallel-searches.patch
Type: text/x-patch
Size: 17930 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120815/d26bcb2f/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-ipa_hbac_support_srchost-option-to-IPA-provider.patch
Type: text/x-patch
Size: 26356 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120815/d26bcb2f/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SYSDB-Make-sysdb_attrs_get_el_int-public.patch
Type: text/x-patch
Size: 3591 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120815/d26bcb2f/attachment-0002.bin>
-------------- 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/20120815/d26bcb2f/attachment.sig>


More information about the sssd-devel mailing list