[SSSD] [PATCH] ad: support cross domain membership

Jakub Hrozek jhrozek at redhat.com
Fri Oct 25 20:40:28 UTC 2013


On Fri, Oct 25, 2013 at 10:33:42PM +0200, Pavel Březina wrote:
> On 10/25/2013 10:24 PM, Pavel Březina wrote:
> >On 10/25/2013 08:10 PM, Jakub Hrozek wrote:
> >>On Fri, Oct 25, 2013 at 01:47:51PM -0400, Pavel Brezina wrote:
> >>>
> >>>
> >>>----- Original Message -----
> >>>>From: "Jakub Hrozek" <jhrozek at redhat.com>
> >>>>To: sssd-devel at lists.fedorahosted.org
> >>>>Sent: Friday, October 25, 2013 4:01:03 PM
> >>>>Subject: Re: [SSSD] [PATCH] ad: support cross domain membership
> >>>>
> >>>>On Fri, Oct 25, 2013 at 03:27:40PM +0200, Pavel Březina wrote:
> >>>>>On 10/25/2013 02:22 PM, Jakub Hrozek wrote:
> >>>>>>On Fri, Oct 25, 2013 at 12:58:23PM +0200, Pavel Březina wrote:
> >>>>>>>On 10/25/2013 10:55 AM, Pavel Březina wrote:
> >>>>>>>>On 10/24/2013 08:40 PM, Jakub Hrozek wrote:
> >>>>>>>>>On Wed, Oct 02, 2013 at 04:13:32PM +0200, Pavel Březina wrote:
> >>>>>>>>>>On 10/01/2013 09:54 PM, Jakub Hrozek wrote:
> >>>>>>>>>>>On Tue, Sep 24, 2013 at 03:17:47PM +0200, Pavel Březina wrote:
> >>>>>>>>>>>>On 09/24/2013 01:32 PM, Jakub Hrozek wrote:
> >>>>>>>>>>>>>On Wed, Sep 11, 2013 at 02:40:14PM +0200, Pavel Březina wrote:
> >>>>>>>>>>>>>>https://fedorahosted.org/sssd/ticket/2064
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>These patch set depends on: [PATCH] ad: store group in
> >>>>>>>>>>>>>>correct
> >>>>>>>>>>>>>>tree on initgroups via tokenGroups
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>You can also pull it with all dependencies from my
> >>>>>>>>>>>>>>repository:
> >>>>>>>>>>>>>>fedorapeople.org:public_git/sssd.git #ad-groups
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>The fundamental changes in this patch set are: - lookup
> >>>>>>>>>>>>>>groups
> >>>>>>>>>>>>>>in global catalog - pick up member domain from its originalDN
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>From 0273d17f24eac7b60dfc0515a9e3b97ad16d1199 Mon Sep 17
> >>>>>>>>>>>>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>>>>>>>>>>>>><pbrezina at redhat.com> Date: Mon, 9 Sep 2013 15:52:03 +0200
> >>>>>>>>>>>>>>Subject: [PATCH 1/9] ad: shortcut if possible during get
> >>>>>>>>>>>>>>object
> >>>>>>>>>>>>>>by ID or SID
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>When getByID or getBySID comes from responder, the request
> >>>>>>>>>>>>>>doesn't necessarily have to contain correct domain, since
> >>>>>>>>>>>>>>responder iterates over all domains until it finds a match.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>Every domain has its own ID range, so we can simply shortcut
> >>>>>>>>>>>>>>if domain does not match and avoid LDAP round trip. Responder
> >>>>>>>>>>>>>>will continue with next domain until it finds the correct
> >>>>>>>>>>>>>>one.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>This patch seems OK to me, but I'd like a second look from
> >>>>>>>>>>>>>someone who understands the ranges better (which is probably
> >>>>>>>>>>>>>Sumit)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>From f74d4637980438032649dfbf079fa6c839862586 Mon Sep 17
> >>>>>>>>>>>>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>>>>>>>>>>>>><pbrezina at redhat.com> Date: Tue, 10 Sep 2013 10:40:06 +0200
> >>>>>>>>>>>>>>Subject: [PATCH 2/9] ad: simplify get_conn_list()
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>It was originally design to return list of connection
> >>>>>>>>>>>>>>objects,
> >>>>>>>>>>>>>>it really always work with only one connection.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>I'd like to review this patch and the following along with my
> >>>>>>>>>>>>>patches to look up POSIX IDs in GC, they touch the same code.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>From ad5dc9e7557ef605fc5d7fc759e5cb6c2f9a148c Mon Sep 17
> >>>>>>>>>>>>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>>>>>>>>>>>>><pbrezina at redhat.com> Date: Tue, 10 Sep 2013 14:45:50 +0200
> >>>>>>>>>>>>>>Subject: [PATCH 4/9] sdap_domain_add(): fix possible memory
> >>>>>>>>>>>>>>leak
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>ACK.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>>From 9f2c212e01700289d70002c8c39b732ca6c11cee Mon Sep 17
> >>>>>>>>>>>>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?=
> >>>>>>>>>>>>>><pbrezina at redhat.com> Date: Tue, 10 Sep 2013 14:45:52 +0200
> >>>>>>>>>>>>>>Subject: [PATCH 5/9] sdap: store base dn in sdap_domain
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>Groups may contain members from different domains.
> >>>>>>>>>>>>>>Remembering
> >>>>>>>>>>>>>>base dn in domain object gives us the ability to simply
> >>>>>>>>>>>>>>lookup
> >>>>>>>>>>>>>>correct domain by comparing object dn with domain base dn.
> >>>>>>>>>>>>>>
> >>>>>>>>>>>>>>Resolves: https://fedorahosted.org/sssd/ticket/2064
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>I haven't tested these patches yet.
> >>>>>>>>>>>>
> >>>>>>>>>>>>I'm sending rebased version of my patches.
> >>>>>>>>>>>>
> >>>>>>>>>>>>[PATCH 4/9] sdap_domain_add(): fix possible memory leak was
> >>>>>>>>>>>>removed
> >>>>>>>>>>>>from the patch set since recent Sumit's patch removed the
> >>>>>>>>>>>code I
> >>>>>>>>>>>>fixed :-)
> >>>>>>>>>>>
> >>>>>>>>>>>Hi,
> >>>>>>>>>>>
> >>>>>>>>>>>can you check if patches #6 and #7 still apply after the recent
> >>>>>>>>>>>changes in 1.11 ? We actually do use the LDAP fallback now..
> >>>>>>>>>>
> >>>>>>>>>>They won't apply, but I think it is quite all right to just
> >>>>>>>>>>skip them.
> >>>>>>>>>>The purpose of these patches was to always contact GC for
> >>>>>>>>>>get_group
> >>>>>>>>>>and
> >>>>>>>>>>initgroups.
> >>>>>>>>>>
> >>>>>>>>>>We always contact GC first at the moment and having LDAP as
> >>>>>>>>>>fallback
> >>>>>>>>>>is
> >>>>>>>>>>fine for groups. If there will be a member from different
> >>>>>>>>>>domain, we
> >>>>>>>>>>will just fail - but if there won't be foreign member it will
> >>>>>>>>>>work.
> >>>>>>>>>
> >>>>>>>>>Hi,
> >>>>>>>>>
> >>>>>>>>>In general the patches look good to me but I think the first
> >>>>>>>>>two can be
> >>>>>>>>>simplified.
> >>>>>>>>>
> >>>>>>>>>Instead of magically trying to find the base DN from the entry's
> >>>>>>>>>original DN, I think we can simplify it to:
> >>>>>>>>>
> >>>>>>>>>struct sdap_domain *
> >>>>>>>>>sdap_domain_get_by_dn(struct sdap_options *opts,
> >>>>>>>>>                       const char *dn)
> >>>>>>>>>{
> >>>>>>>>>     struct sdap_domain *sditer = NULL;
> >>>>>>>>>
> >>>>>>>>>     DLIST_FOR_EACH(sditer, opts->sdom) {
> >>>>>>>>>         if (sss_ldap_dn_in_search_bases(tmp_ctx, dn,
> >>>>>>>>>sditer->search_bases, NULL) ||
> >>>>>>>>>             sss_ldap_dn_in_search_bases(tmp_ctx, dn,
> >>>>>>>>>sditer->user_search_bases, NULL) ||
> >>>>>>>>>             sss_ldap_dn_in_search_bases(tmp_ctx, dn,
> >>>>>>>>>sditer->group_search_bases, NULL)) {
> >>>>>>>>>             return sditer;
> >>>>>>>>>         }
> >>>>>>>>>     }
> >>>>>>>>>
> >>>>>>>>>     return NULL;
> >>>>>>>>>}
> >>>>>>>>>
> >>>>>>>>>Then you won't need to store the base DN, no need for the magic
> >>>>>>>>>with
> >>>>>>>>>strstr
> >>>>>>>>>and the code will work even for custom search bases.
> >>>>>>>>
> >>>>>>>>Hi,
> >>>>>>>>new patches are attached. I just added all of the search bases.
> >>>>>>>
> >>>>>>>And here's one more iteration. While working on token groups patches
> >>>>>>>I found a bug in the first patch. I tried to shortcut using id
> >>>>>>>mapping even if id mapping is disabled. This made the posix groups
> >>>>>>>unresolvable since id mapping returned an error when trying to map
> >>>>>>>GID to SID.
> >>>>>>>
> >>>>>>
> >>>>>>Thanks. See also one more attached patch I applied on top of yours to
> >>>>>>make cross-domain group resolution work. I'm still chasing one bug in
> >>>>>>fill_members that might cause some members to be not qualified,
> >>>>>>though.
> >>>>>
> >>>>>Hi,
> >>>>>ack to this patch. The problem was when you have:
> >>>>>group at subad : user at parentad
> >>>>>it did not find it. Jakub's patch fixes it.
> >>>>>
> >>>>>There is still one problem that was introduce by the change of
> >>>>>sdap_domain_get_by_dn to search bases instead of string comparison.
> >>>>>
> >>>>>We have domains:
> >>>>>ad.pb -> sub.ad.pb
> >>>>>
> >>>>>Search bases:
> >>>>>ad.pb: dc=ad,dc=pb
> >>>>>sub.ad.pb: dc=sub,dc=ad,dc=pb
> >>>>>
> >>>>>User:
> >>>>>cn=user,dc=sub,dc=ad,dc=pb
> >>>>>
> >>>>>This user belongs to sub.ad.pb but its dn match with dc=ad,dc=pb and
> >>>>>as a result we will assign him to domain ad.pb.
> >>>>>
> >>>>>We can either swap back to my original patches or tune
> >>>>>sss_ldap_dn_in_search_bases() so it returns expected result for this
> >>>>>situation (it will require all dc parts to match).
> >>>>
> >>>>I would prefer the latter. Just remove the RDN and return true only if
> >>>>the rest matches.
> >>>
> >>>Comparing only the part after RDN is not sufficient.
> >>>
> >>>dn: cn=Joe,cn=Manager,cn=Users,dc=example,dc=com
> >>>rdn: cn=Joe
> >>>search base: dc=example,dc=com
> >>>
> >>>strcmp(cn=Manager,cn=Users,dc=example,dc=com, dc=example,dc=com) != 0
> >>>
> >>>We would have to use similar "magic" to locate domain part of both dn
> >>>that you did not like.
> >>
> >>Well, the main reason I didn't like the original patches was that they
> >>were hardcoding "DC=". Sure, that works for AD, but I don't like these
> >>hacks in the generic LDAP code.
> >>
> >>Could we modify sdap_domain_get_by_dn() so that it would try to match as
> >>much as possible?
> >>
> >>btw latest patches based on your patchset are attached. I added two
> >>fixes in the NSS responder that I needed to get getent reporting the
> >>right members.
> >
> >We agreed with Jakub to go with original approach for the moment. Here
> >is the current working patchset. The latest Jakub's patch contains a bug
> >when no members are printed. We didn't find the root of this bug yet,
> >but we need to leave it for a fresh day.
> 
> And one more time, now with Sumit's note addressed.

I have filed a ticket for fixing the nss responder -
https://fedorahosted.org/sssd/ticket/2131

And one more to improve detecting the search bases - 
https://fedorahosted.org/sssd/ticket/2132

I bypassed triage and put them directly to the 1.11.x bucket as they
improve on existing feature.



More information about the sssd-devel mailing list