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
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@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@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@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@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.
I haven't tested these patches yet.
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@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@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@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@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.
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 :-)
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@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@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@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@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.
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..
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@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@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@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@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.
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.
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@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@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@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@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.
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.
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@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@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@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@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.
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.
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@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@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@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@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.
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@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@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@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@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.
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@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@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@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@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).
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@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@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@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@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.
----- Original Message -----
From: "Jakub Hrozek" jhrozek@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?= >>>>>pbrezina@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@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@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@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.
On Fri, Oct 25, 2013 at 01:47:51PM -0400, Pavel Brezina wrote:
----- Original Message -----
From: "Jakub Hrozek" jhrozek@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?= >>>>>>pbrezina@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@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@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@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.
On Fri, Oct 25, 2013 at 08:10:18PM +0200, Jakub Hrozek wrote:
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?
I meant something like the attached patch.
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@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?= >>>>>>> pbrezina@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@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@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@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.
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@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?= >>>>>>>> pbrezina@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@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@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@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.
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@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?= >>>>>>>>>pbrezina@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@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@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@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.
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@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?= >>>>>>>>>>pbrezina@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@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@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@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.
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@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?= >>>>>>>>>>>pbrezina@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@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@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@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
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@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?= >>>>>>>>>>>> pbrezina@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@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@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@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.
bye, Sumit
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@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?= >>>>>>>>>>>>> pbrezina@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@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@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@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.
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@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?= >>>>>>>>>>>>>>pbrezina@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@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@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@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
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" jhrozek@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?= >>>>>>>>>>>>>>>pbrezina@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@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@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@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)
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" jhrozek@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?= >>>>>>>>>>>>>>>>pbrezina@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@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@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@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
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@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@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@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@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.
From d2224259b331c2e6887ce7b8816f1949342e539a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= pbrezina@redhat.com Date: Mon, 9 Sep 2013 15:52:03 +0200 Subject: [PATCH 1/5] 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.
src/providers/ad/ad_id.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)
diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c index f09b9c6fe239b9b602f53e8afce641a4d07be6e9..6ac10bba82ba71fde2a963f3ade23a49a20d705e 100644 --- a/src/providers/ad/ad_id.c +++ b/src/providers/ad/ad_id.c @@ -20,10 +20,12 @@ along with this program. If not, see http://www.gnu.org/licenses/. */ #include "util/util.h" +#include "util/strtonum.h" #include "providers/ad/ad_common.h" #include "providers/ad/ad_id.h" #include "providers/ad/ad_domain_info.h" #include "providers/ldap/sdap_async_enum.h" +#include "providers/ldap/sdap_idmap.h"
struct ad_handle_acct_info_state { struct be_req *breq; @@ -224,17 +226,61 @@ ad_account_info_handler(struct be_req *be_req) struct sss_domain_info *dom; struct sdap_domain *sdom; struct sdap_id_conn_ctx **clist;
struct sdap_idmap_ctx *idmap_ctx;
enum idmap_error_code map_err;
uint32_t id;
char *sid = NULL; errno_t ret;
ad_ctx = talloc_get_type(be_ctx->bet_info[BET_ID].pvt_bet_data, struct ad_id_ctx); ar = talloc_get_type(be_req_get_data(be_req), struct be_acct_req); sdap_id_ctx = ad_ctx->sdap_id_ctx;
idmap_ctx = sdap_id_ctx->opts->idmap_ctx;
if (be_is_offline(be_ctx)) { return be_req_terminate(be_req, DP_ERR_OFFLINE, EAGAIN, "Offline"); }
/* Try to shortcut if this is ID or SID search and it belongs to
* other domain range than is in ar->domain. */if (dp_opt_get_bool(idmap_ctx->id_ctx->opts->basic, SDAP_ID_MAPPING)) {
Please use sdap_idmap_domain_has_algorithmic_mapping() instead. In the case of trusted AD domains SDAP_ID_MAPPING is not set and the mapping type depends on the type to the related IPA idranges.
bye, Sumit
if (ar->filter_type == BE_FILTER_IDNUM|| ar->filter_type == BE_FILTER_SECID) {if (ar->filter_type == BE_FILTER_IDNUM) {id = strtouint32(ar->filter_value, NULL, 10);if (errno != EOK) {ret = EINVAL;goto fail;}
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Sumit Bose