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

Sumit Bose sbose at redhat.com
Wed Oct 30 21:05:42 UTC 2013


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



More information about the sssd-devel mailing list