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.