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(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.
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.