On Wed, Oct 30, 2013 at 10:39:23PM +0100, Jakub Hrozek wrote: > On Wed, Oct 30, 2013 at 10:05:42PM +0100, Sumit Bose wrote: > > 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 > > I haven't found any bug in the patches either (just the issue with the > ad_filter) so ack++ > > For the record, I tested: > getent passwd user@root.domain > getent passwd user@sub.domain > getent group gr@root.domain (mixed membership) > getent group gr@sub.domain (mixed membership) > id user@root.domain (mixed membership) > id user@sub.domain (mixed membership)
Pushed the whole patchset to master and sssd-1-11 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
