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.
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 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.
If the DLIST_FOR_EACH loop in sdap_nested_member_is_ent() is never run
ret is uninitialized.
bye,
Sumit