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