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

Jakub Hrozek jhrozek at redhat.com
Wed Oct 30 22:03:19 UTC 2013


On Wed, Oct 30, 2013 at 10:39:23PM +0100, Jakub Hrozek wrote:
> On Wed, Oct 30, 2013 at 10:05:42PM +0100, Sumit Bose wrote:
> > On Wed, Oct 30, 2013 at 03:21:16PM +0100, Pavel Březina wrote:
> > > On 10/29/2013 01:43 PM, Pavel Březina wrote:
> > > >On 10/28/2013 10:00 PM, Sumit Bose wrote:
> > > >>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.
> > > >
> > > >Thanks. I forgot to commit this change.
> > > >
> > > >>
> > > >>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 don't think so. We are searching for an sdap_domain that is converted
> > > >to sss_domain_info by caller. The default sdap domain is not available
> > > >in the code that references this function so we would have to amend the
> > > >code in order to obtain it and pass it to sdap_domain_get_by_dn() (more
> > > >code) or hard code sdap_domain_get_by_dn() to return the head of sdap
> > > >domain list (not nice).
> > > >
> > > >I think it is better this way, that it returns NULL if it cannot match
> > > >dn with domain and let the caller decide how to continue.
> > > >
> > > >>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.
> > > >
> > > >Well, since we don't support other subdomains than AD I don't think it
> > > >is needed. Such documentation may actually imply that we support other
> > > >LDAP servers as subdomains as well.
> > > >
> > > >>If the DLIST_FOR_EACH loop in sdap_nested_member_is_ent() is never run
> > > >>ret is uninitialized.
> > > >
> > > >Nice catch, I amended Jakub's patch.
> > > 
> > > I reworked the first patch and put the huge branch into separate
> > > function. See if you like it.
> > > 
> > > If not, I need to send another version of the original since it
> > > unfortunately contains two bugs that I found now - didn't free sid
> > > obtained from idmap and didn't set errno to zero before calling
> > > strtouint.
> > > 
> > > 
> > 
> > I like the new one better and didn't found any regression during my
> > testing, so ACK.
> > 
> > bye,
> > Sumit
> 
> I haven't found any bug in the patches either (just the issue with the
> ad_filter) so ack++
> 
> For the record, I tested:
>     getent passwd user at root.domain
>     getent passwd user at sub.domain
>     getent group gr at root.domain (mixed membership)
>     getent group gr at sub.domain  (mixed membership)
>     id user at root.domain (mixed membership)
>     id user at sub.domain (mixed membership)

Pushed the whole patchset to master and sssd-1-11



More information about the sssd-devel mailing list