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"
<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.
> >
> >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(a)root.domain
getent passwd user(a)sub.domain
getent group gr(a)root.domain (mixed membership)
getent group gr(a)sub.domain (mixed membership)
id user(a)root.domain (mixed membership)
id user(a)sub.domain (mixed membership)