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

Sumit Bose sbose at redhat.com
Mon Oct 28 21:00:32 UTC 2013


On Mon, Oct 28, 2013 at 04:51:27PM +0100, Jakub Hrozek wrote:
> On Fri, Oct 25, 2013 at 10:40:28PM +0200, Jakub Hrozek wrote:
> > 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.
> 
> The testing itself went fine, but patch #1 didn't even compile. Attached
> are patches after I amended them, only patch #1 changed.

After sdap_domain_get_by_dn() is called it is always checked if NULL is
returned and if yes the current domain is set. Maybe it would make the
code easier if a default return value is added to the parameter list of
sdap_domain_get_by_dn().

I think we should mention somewhere that cross domain group memberships
are supported but will currently only work for AD GC lookups or other
environments which use dc=-style based DNs.

If the DLIST_FOR_EACH loop in sdap_nested_member_is_ent() is never run
ret is uninitialized.

bye,
Sumit



More information about the sssd-devel mailing list