On Wed, Oct 30, 2013 at 03:21:16PM +0100, Pavel Březina wrote: > 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" <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. > > > >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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
