----- Original Message -----
> From: "Jakub Hrozek" <jhrozek(a)redhat.com>
> To: sssd-devel(a)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(a)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(a)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(a)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(a)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@subad : user@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.