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" <jhro...@redhat.com> > > > >>>>To: sssd-devel@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?= > > > >>>>>>>>>>>>>><pbrez...@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?= > > > >>>>>>>>>>>>>><pbrez...@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?= > > > >>>>>>>>>>>>>><pbrez...@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?= > > > >>>>>>>>>>>>>><pbrez...@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel