On Fri, Oct 25, 2013 at 12:58:23PM +0200, Pavel Březina wrote: > On 10/25/2013 10:55 AM, Pavel Březina wrote: > >On 10/24/2013 08:40 PM, Jakub Hrozek wrote: > >>On Wed, Oct 02, 2013 at 04:13:32PM +0200, Pavel Březina wrote: > >>>On 10/01/2013 09:54 PM, Jakub Hrozek wrote: > >>>>On Tue, Sep 24, 2013 at 03:17:47PM +0200, Pavel Březina wrote: > >>>>>On 09/24/2013 01:32 PM, Jakub Hrozek wrote: > >>>>>>On Wed, Sep 11, 2013 at 02:40:14PM +0200, Pavel Březina wrote: > >>>>>>>https://fedorahosted.org/sssd/ticket/2064 > >>>>>>> > >>>>>>>These patch set depends on: [PATCH] ad: store group in correct > >>>>>>>tree on initgroups via tokenGroups > >>>>>>> > >>>>>>>You can also pull it with all dependencies from my repository: > >>>>>>>fedorapeople.org:public_git/sssd.git #ad-groups > >>>>>>> > >>>>>>>The fundamental changes in this patch set are: - lookup groups > >>>>>>>in global catalog - pick up member domain from its originalDN > >>>>>> > >>>>>>>From 0273d17f24eac7b60dfc0515a9e3b97ad16d1199 Mon Sep 17 > >>>>>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= > >>>>>>><pbrez...@redhat.com> Date: Mon, 9 Sep 2013 15:52:03 +0200 > >>>>>>>Subject: [PATCH 1/9] ad: shortcut if possible during get object > >>>>>>>by ID or SID > >>>>>>> > >>>>>>>When getByID or getBySID comes from responder, the request > >>>>>>>doesn't necessarily have to contain correct domain, since > >>>>>>>responder iterates over all domains until it finds a match. > >>>>>>> > >>>>>>>Every domain has its own ID range, so we can simply shortcut > >>>>>>>if domain does not match and avoid LDAP round trip. Responder > >>>>>>>will continue with next domain until it finds the correct one. > >>>>>> > >>>>>>This patch seems OK to me, but I'd like a second look from > >>>>>>someone who understands the ranges better (which is probably > >>>>>>Sumit) > >>>>>> > >>>>>>>From f74d4637980438032649dfbf079fa6c839862586 Mon Sep 17 > >>>>>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= > >>>>>>><pbrez...@redhat.com> Date: Tue, 10 Sep 2013 10:40:06 +0200 > >>>>>>>Subject: [PATCH 2/9] ad: simplify get_conn_list() > >>>>>>> > >>>>>>>It was originally design to return list of connection objects, > >>>>>>>it really always work with only one connection. > >>>>>> > >>>>>>I'd like to review this patch and the following along with my > >>>>>>patches to look up POSIX IDs in GC, they touch the same code. > >>>>>> > >>>>>>>From ad5dc9e7557ef605fc5d7fc759e5cb6c2f9a148c Mon Sep 17 > >>>>>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= > >>>>>>><pbrez...@redhat.com> Date: Tue, 10 Sep 2013 14:45:50 +0200 > >>>>>>>Subject: [PATCH 4/9] sdap_domain_add(): fix possible memory > >>>>>>>leak > >>>>>> > >>>>>>ACK. > >>>>>> > >>>>>>>From 9f2c212e01700289d70002c8c39b732ca6c11cee Mon Sep 17 > >>>>>>>00:00:00 2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= > >>>>>>><pbrez...@redhat.com> Date: Tue, 10 Sep 2013 14:45:52 +0200 > >>>>>>>Subject: [PATCH 5/9] sdap: store base dn in sdap_domain > >>>>>>> > >>>>>>>Groups may contain members from different domains. Remembering > >>>>>>>base dn in domain object gives us the ability to simply lookup > >>>>>>>correct domain by comparing object dn with domain base dn. > >>>>>>> > >>>>>>>Resolves: https://fedorahosted.org/sssd/ticket/2064 > >>>>>> > >>>>>>I haven't tested these patches yet. > >>>>> > >>>>>I'm sending rebased version of my patches. > >>>>> > >>>>>[PATCH 4/9] sdap_domain_add(): fix possible memory leak was removed > >>>>>from the patch set since recent Sumit's patch removed the code I > >>>>>fixed :-) > >>>> > >>>>Hi, > >>>> > >>>>can you check if patches #6 and #7 still apply after the recent > >>>>changes in 1.11 ? We actually do use the LDAP fallback now.. > >>> > >>>They won't apply, but I think it is quite all right to just skip them. > >>>The purpose of these patches was to always contact GC for get_group and > >>>initgroups. > >>> > >>>We always contact GC first at the moment and having LDAP as fallback is > >>>fine for groups. If there will be a member from different domain, we > >>>will just fail - but if there won't be foreign member it will work. > >> > >>Hi, > >> > >>In general the patches look good to me but I think the first two can be > >>simplified. > >> > >>Instead of magically trying to find the base DN from the entry's > >>original DN, I think we can simplify it to: > >> > >>struct sdap_domain * > >>sdap_domain_get_by_dn(struct sdap_options *opts, > >> const char *dn) > >>{ > >> struct sdap_domain *sditer = NULL; > >> > >> DLIST_FOR_EACH(sditer, opts->sdom) { > >> if (sss_ldap_dn_in_search_bases(tmp_ctx, dn, > >>sditer->search_bases, NULL) || > >> sss_ldap_dn_in_search_bases(tmp_ctx, dn, > >>sditer->user_search_bases, NULL) || > >> sss_ldap_dn_in_search_bases(tmp_ctx, dn, > >>sditer->group_search_bases, NULL)) { > >> return sditer; > >> } > >> } > >> > >> return NULL; > >>} > >> > >>Then you won't need to store the base DN, no need for the magic with > >>strstr > >>and the code will work even for custom search bases. > > > >Hi, > >new patches are attached. I just added all of the search bases. > > And here's one more iteration. While working on token groups patches > I found a bug in the first patch. I tried to shortcut using id > mapping even if id mapping is disabled. This made the posix groups > unresolvable since id mapping returned an error when trying to map > GID to SID. >
> From d2224259b331c2e6887ce7b8816f1949342e539a Mon Sep 17 00:00:00 2001 > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrez...@redhat.com> > Date: Mon, 9 Sep 2013 15:52:03 +0200 > Subject: [PATCH 1/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 mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
