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(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.
>
>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.
I like the new one better and didn't found any regression during my
testing, so ACK.
bye,
Sumit