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

Jakub Hrozek jhrozek at redhat.com
Fri Oct 25 14:01:03 UTC 2013


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.



More information about the sssd-devel mailing list