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

Pavel Březina pbrezina at redhat.com
Fri Oct 25 20:33:42 UTC 2013


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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-ad-shortcut-if-possible-during-get-object-by-ID-or-S.patch
Type: text/x-patch
Size: 3745 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131025/4ac829ae/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-sdap-store-base-dn-in-sdap_domain.patch
Type: text/x-patch
Size: 3853 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131025/4ac829ae/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-sdap-add-sdap_domain_get_by_dn.patch
Type: text/x-patch
Size: 2158 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131025/4ac829ae/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-ghosts-pick-correct-domain-for-every-member.patch
Type: text/x-patch
Size: 3364 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131025/4ac829ae/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-sdap_fill_memberships-pick-correct-domain-for-every-.patch
Type: text/x-patch
Size: 3232 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131025/4ac829ae/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-nested-groups-pick-correct-domain-for-cache-lookups.patch
Type: text/x-patch
Size: 2870 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131025/4ac829ae/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-LDAP-Check-all-search-bases-during-nested-group-proc.patch
Type: text/x-patch
Size: 4116 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131025/4ac829ae/attachment-0006.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0008-NSS-Fix-parenthesis.patch
Type: text/x-patch
Size: 1015 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131025/4ac829ae/attachment-0007.bin>


More information about the sssd-devel mailing list