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

Pavel Březina pbrezina at redhat.com
Wed Oct 30 14:21:16 UTC 2013


On 10/29/2013 01:43 PM, Pavel Březina wrote:
> On 10/28/2013 10:00 PM, Sumit Bose wrote:
>> 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 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.
>>>>
>>>> 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.
>
> Nice catch, I amended Jakub's patch.

I reworked the first patch and put the huge branch into separate 
function. See if you like it.

If not, I need to send another version of the original since it 
unfortunately contains two bugs that I found now - didn't free sid 
obtained from idmap and didn't set errno to zero before calling strtouint.


-------------- 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: 4959 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131030/0e3a6c9d/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/20131030/0e3a6c9d/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/20131030/0e3a6c9d/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/20131030/0e3a6c9d/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/20131030/0e3a6c9d/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/20131030/0e3a6c9d/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: 4124 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131030/0e3a6c9d/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/20131030/0e3a6c9d/attachment-0007.bin>


More information about the sssd-devel mailing list