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.