[PATCHES] IPA: add trusted domains with missing idrange

[Sumit Bose]

Hi,

this patch set tries to fix https://fedorahosted.org/sssd/ticket/2101 .
Currently we rely on the fact that the external mapping is the default
in the case of an error. But this leads to a loot of useless CPU cycles
and maybe irritating error messages. With this patch set each trusted AD
member domain gets a proper range assigned if the forest root is
configured to manage the POSIX attributes.

bye,
Sumit

[Jakub Hrozek]

On Thu, Oct 24, 2013 at 02:14:59PM +0200, Sumit Bose wrote:
> Hi,
>
> this patch set tries to fix https://fedorahosted.org/sssd/ticket/2101 .
> Currently we rely on the fact that the external mapping is the default
> in the case of an error. But this leads to a loot of useless CPU cycles
> and maybe irritating error messages. With this patch set each trusted AD
> member domain gets a proper range assigned if the forest root is
> configured to manage the POSIX attributes.
>
> bye,
> Sumit

Hi,

I only have two minor comments, see below:
> From e1a86771137f67e0e8d24c4bbb014bfc21d85538 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Wed, 23 Oct 2013 14:39:55 +0200
> Subject: [PATCH 1/4] find_subdomain_by_sid: skip domains with missing
>  domain_id

ACK. Thanks for the unit test.
> From dc0115802904c954c79d90cd846da9f5bd965019 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Thu, 24 Oct 2013 11:44:11 +0200
> Subject: [PATCH 2/4] idmap: add
>  sss_idmap_domain_by_name_has_algorithmic_mapping()

The code itself is good, but we need to bump the version number. See the
attached patch, can you squash it in if you agree?
> From dffdd7cc99025cb8790f34f79bdf294762f3468d Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Thu, 24 Oct 2013 11:45:57 +0200
> Subject: [PATCH 3/4] sdap_idmap_domain_has_algorithmic_mapping: add domain
>  name argument

ACK
> From 3e7283a39994ac24a9ff9dfede0d735cc89b4bb0 Mon Sep 17 00:00:00 2001
> From: Sumit Bose 
> Date: Wed, 23 Oct 2013 13:30:57 +0200
> Subject: [PATCH 4/4] IPA: add trusted domains with missing idrange
>
> If the forest root of a trusted forest is managing POSIX IDs for its
> users and groups the same is assumed for all member domains in the
> forest which do not have explicitly have an idrange set.
>
> To reflect this SSSD will create the matching ranges automatically.

One nitpick:
> tmp_ctx = talloc_new(NULL);
> if (tmp_ctx == NULL) {
>    DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
>    return ENOMEM;
> }
> for (c = 0; c < range_count; c++) {
>    r = range_list[c];
>    if (r->trusted_dom_sid != NULL
>            && strcmp(r->trusted_dom_sid, forest_root->domain_id) == 0) {
>        if (r->range_type == NULL
>                || strcmp(r->range_type, IPA_RANGE_AD_TRUST_POSIX) != 0) {
>            DEBUG(SSSDBG_MINOR_FAILURE,
>                  ("Forest root does not have range type [%s].\n",
>                   IPA_RANGE_AD_TRUST_POSIX));

we would leak tmp_ctx here, we should goto done instead.
>            return EINVAL;
>        }
>        range.min = r->base_id;
>        range.max = r->base_id + r->id_range_size -1;
>        range_id = talloc_asprintf(tmp_ctx, "%s-%s", dom_sid_str, r->name);
>        if (range_id == NULL) {
>            DEBUG(SSSDBG_OP_FAILURE, ("talloc_asprintf failed.\n"));
>            ret = ENOMEM;
>            goto done;
>        }
>        err = sss_idmap_add_domain_ex(idmap_ctx->map, dom_name, dom_sid_str,
>                                      &range, range_id, 0, true);
>        if (err != IDMAP_SUCCESS && err != IDMAP_COLLISION) {
>            DEBUG(SSSDBG_CRIT_FAILURE, ("Could not add range [%s] to ID map\n",
>                                        range_id));

If you're changing the patch, can you also reflow this line? It's past
80 characters and my editor highlights the line..
>            ret = EIO;
>            goto done;
>        }
>        found = true;
>    }
> }

[Sumit Bose]

On Fri, Oct 25, 2013 at 11:21:28AM +0200, Jakub Hrozek wrote:
> On Thu, Oct 24, 2013 at 02:14:59PM +0200, Sumit Bose wrote:
> > Hi,
> >
> > this patch set tries to fix https://fedorahosted.org/sssd/ticket/2101 .
> > Currently we rely on the fact that the external mapping is the default
> > in the case of an error. But this leads to a loot of useless CPU cycles
> > and maybe irritating error messages. With this patch set each trusted AD
> > member domain gets a proper range assigned if the forest root is
> > configured to manage the POSIX attributes.
> >
> > bye,
> > Sumit
>
> Hi,
>
> I only have two minor comments, see below:
> > From e1a86771137f67e0e8d24c4bbb014bfc21d85538 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Wed, 23 Oct 2013 14:39:55 +0200
> > Subject: [PATCH 1/4] find_subdomain_by_sid: skip domains with missing
> >  domain_id
>
> ACK. Thanks for the unit test.
> > From dc0115802904c954c79d90cd846da9f5bd965019 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Thu, 24 Oct 2013 11:44:11 +0200
> > Subject: [PATCH 2/4] idmap: add
> >  sss_idmap_domain_by_name_has_algorithmic_mapping()
>
> The code itself is good, but we need to bump the version number. See the
> attached patch, can you squash it in if you agree?

There was no patch but I changed version-info to 2:0:2. Is this what you
would suggest?
> > From dffdd7cc99025cb8790f34f79bdf294762f3468d Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Thu, 24 Oct 2013 11:45:57 +0200
> > Subject: [PATCH 3/4] sdap_idmap_domain_has_algorithmic_mapping: add domain
> >  name argument
>
> ACK
> > From 3e7283a39994ac24a9ff9dfede0d735cc89b4bb0 Mon Sep 17 00:00:00 2001
> > From: Sumit Bose 
> > Date: Wed, 23 Oct 2013 13:30:57 +0200
> > Subject: [PATCH 4/4] IPA: add trusted domains with missing idrange
> >
> > If the forest root of a trusted forest is managing POSIX IDs for its
> > users and groups the same is assumed for all member domains in the
> > forest which do not have explicitly have an idrange set.
> >
> > To reflect this SSSD will create the matching ranges automatically.
>
> One nitpick:
> > tmp_ctx = talloc_new(NULL);
> > if (tmp_ctx == NULL) {
> >    DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
> >    return ENOMEM;
> > }
> > for (c = 0; c < range_count; c++) {
> >    r = range_list[c];
> >    if (r->trusted_dom_sid != NULL
> >            && strcmp(r->trusted_dom_sid, forest_root->domain_id) == 0) {
> >        if (r->range_type == NULL
> >                || strcmp(r->range_type, IPA_RANGE_AD_TRUST_POSIX) != 0) {
> >            DEBUG(SSSDBG_MINOR_FAILURE,
> >                  ("Forest root does not have range type [%s].\n",
> >                   IPA_RANGE_AD_TRUST_POSIX));
>
> we would leak tmp_ctx here, we should goto done instead.

good catch, fixed.
> >            return EINVAL;
> >        }
> >        range.min = r->base_id;
> >        range.max = r->base_id + r->id_range_size -1;
> >        range_id = talloc_asprintf(tmp_ctx, "%s-%s", dom_sid_str, r->name);
> >        if (range_id == NULL) {
> >            DEBUG(SSSDBG_OP_FAILURE, ("talloc_asprintf failed.\n"));
> >            ret = ENOMEM;
> >            goto done;
> >        }
> >        err = sss_idmap_add_domain_ex(idmap_ctx->map, dom_name, dom_sid_str,
> >                                      &range, range_id, 0, true);
> >        if (err != IDMAP_SUCCESS && err != IDMAP_COLLISION) {
> >            DEBUG(SSSDBG_CRIT_FAILURE, ("Could not add range [%s] to ID map\n",
> >                                        range_id));
>
> If you're changing the patch, can you also reflow this line? It's past
> 80 characters and my editor highlights the line..

fixed.

Thank you for the review, new versions attached.

bye,
Sumit
> >            ret = EIO;
> >            goto done;
> >        }
> >        found = true;
> >    }
> > }
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Jakub Hrozek]

On Fri, Oct 25, 2013 at 12:06:25PM +0200, Sumit Bose wrote:
> On Fri, Oct 25, 2013 at 11:21:28AM +0200, Jakub Hrozek wrote:
> > On Thu, Oct 24, 2013 at 02:14:59PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch set tries to fix https://fedorahosted.org/sssd/ticket/2101 .
> > > Currently we rely on the fact that the external mapping is the default
> > > in the case of an error. But this leads to a loot of useless CPU cycles
> > > and maybe irritating error messages. With this patch set each trusted AD
> > > member domain gets a proper range assigned if the forest root is
> > > configured to manage the POSIX attributes.
> > >
> > > bye,
> > > Sumit
> >
> > Hi,
> >
> > I only have two minor comments, see below:
> > > From e1a86771137f67e0e8d24c4bbb014bfc21d85538 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Wed, 23 Oct 2013 14:39:55 +0200
> > > Subject: [PATCH 1/4] find_subdomain_by_sid: skip domains with missing
> > >  domain_id
> >
> > ACK. Thanks for the unit test.
> > > From dc0115802904c954c79d90cd846da9f5bd965019 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Thu, 24 Oct 2013 11:44:11 +0200
> > > Subject: [PATCH 2/4] idmap: add
> > >  sss_idmap_domain_by_name_has_algorithmic_mapping()
> >
> > The code itself is good, but we need to bump the version number. See the
> > attached patch, can you squash it in if you agree?
>
> There was no patch but I changed version-info to 2:0:2. Is this what you
> would suggest?

Oops, sorry. Yes, that was how I read the libtool manual as well. The
resulting soname (not libtool info) will be 0.2.0

[Jakub Hrozek]

On Fri, Oct 25, 2013 at 12:06:25PM +0200, Sumit Bose wrote:
> On Fri, Oct 25, 2013 at 11:21:28AM +0200, Jakub Hrozek wrote:
> > On Thu, Oct 24, 2013 at 02:14:59PM +0200, Sumit Bose wrote:
> > > Hi,
> > >
> > > this patch set tries to fix https://fedorahosted.org/sssd/ticket/2101 .
> > > Currently we rely on the fact that the external mapping is the default
> > > in the case of an error. But this leads to a loot of useless CPU cycles
> > > and maybe irritating error messages. With this patch set each trusted AD
> > > member domain gets a proper range assigned if the forest root is
> > > configured to manage the POSIX attributes.
> > >
> > > bye,
> > > Sumit
> >
> > Hi,
> >
> > I only have two minor comments, see below:
> > > From e1a86771137f67e0e8d24c4bbb014bfc21d85538 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Wed, 23 Oct 2013 14:39:55 +0200
> > > Subject: [PATCH 1/4] find_subdomain_by_sid: skip domains with missing
> > >  domain_id
> >
> > ACK. Thanks for the unit test.
> > > From dc0115802904c954c79d90cd846da9f5bd965019 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Thu, 24 Oct 2013 11:44:11 +0200
> > > Subject: [PATCH 2/4] idmap: add
> > >  sss_idmap_domain_by_name_has_algorithmic_mapping()
> >
> > The code itself is good, but we need to bump the version number. See the
> > attached patch, can you squash it in if you agree?
>
> There was no patch but I changed version-info to 2:0:2. Is this what you
> would suggest?
> > > From dffdd7cc99025cb8790f34f79bdf294762f3468d Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Thu, 24 Oct 2013 11:45:57 +0200
> > > Subject: [PATCH 3/4] sdap_idmap_domain_has_algorithmic_mapping: add domain
> > >  name argument
> >
> > ACK
> > > From 3e7283a39994ac24a9ff9dfede0d735cc89b4bb0 Mon Sep 17 00:00:00 2001
> > > From: Sumit Bose 
> > > Date: Wed, 23 Oct 2013 13:30:57 +0200
> > > Subject: [PATCH 4/4] IPA: add trusted domains with missing idrange
> > >
> > > If the forest root of a trusted forest is managing POSIX IDs for its
> > > users and groups the same is assumed for all member domains in the
> > > forest which do not have explicitly have an idrange set.
> > >
> > > To reflect this SSSD will create the matching ranges automatically.
> >
> > One nitpick:
> > > tmp_ctx = talloc_new(NULL);
> > > if (tmp_ctx == NULL) {
> > >    DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
> > >    return ENOMEM;
> > > }
> > > for (c = 0; c < range_count; c++) {
> > >    r = range_list[c];
> > >    if (r->trusted_dom_sid != NULL
> > >            && strcmp(r->trusted_dom_sid, forest_root->domain_id) == 0) {
> > >        if (r->range_type == NULL
> > >                || strcmp(r->range_type, IPA_RANGE_AD_TRUST_POSIX) != 0) {
> > >            DEBUG(SSSDBG_MINOR_FAILURE,
> > >                  ("Forest root does not have range type [%s].\n",
> > >                   IPA_RANGE_AD_TRUST_POSIX));
> >
> > we would leak tmp_ctx here, we should goto done instead.
>
> good catch, fixed.
> > >            return EINVAL;
> > >        }
> > >        range.min = r->base_id;
> > >        range.max = r->base_id + r->id_range_size -1;
> > >        range_id = talloc_asprintf(tmp_ctx, "%s-%s", dom_sid_str, r->name);
> > >        if (range_id == NULL) {
> > >            DEBUG(SSSDBG_OP_FAILURE, ("talloc_asprintf failed.\n"));
> > >            ret = ENOMEM;
> > >            goto done;
> > >        }
> > >        err = sss_idmap_add_domain_ex(idmap_ctx->map, dom_name, dom_sid_str,
> > >                                      &range, range_id, 0, true);
> > >        if (err != IDMAP_SUCCESS && err != IDMAP_COLLISION) {
> > >            DEBUG(SSSDBG_CRIT_FAILURE, ("Could not add range [%s] to ID map\n",
> > >                                        range_id));
> >
> > If you're changing the patch, can you also reflow this line? It's past
> > 80 characters and my editor highlights the line..
>
> fixed.
>
> Thank you for the review, new versions attached.
>
> bye,
> Sumit

ACK to these patches.

[Jakub Hrozek]

On Fri, Oct 25, 2013 at 02:51:47PM +0200, Jakub Hrozek wrote:
> On Fri, Oct 25, 2013 at 12:06:25PM +0200, Sumit Bose wrote:
> > On Fri, Oct 25, 2013 at 11:21:28AM +0200, Jakub Hrozek wrote:
> > > On Thu, Oct 24, 2013 at 02:14:59PM +0200, Sumit Bose wrote:
> > > > Hi,
> > > >
> > > > this patch set tries to fix https://fedorahosted.org/sssd/ticket/2101 .
> > > > Currently we rely on the fact that the external mapping is the default
> > > > in the case of an error. But this leads to a loot of useless CPU cycles
> > > > and maybe irritating error messages. With this patch set each trusted AD
> > > > member domain gets a proper range assigned if the forest root is
> > > > configured to manage the POSIX attributes.
> > > >
> > > > bye,
> > > > Sumit
> > >
> > > Hi,
> > >
> > > I only have two minor comments, see below:
> > > > From e1a86771137f67e0e8d24c4bbb014bfc21d85538 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose 
> > > > Date: Wed, 23 Oct 2013 14:39:55 +0200
> > > > Subject: [PATCH 1/4] find_subdomain_by_sid: skip domains with missing
> > > >  domain_id
> > >
> > > ACK. Thanks for the unit test.
> > > > From dc0115802904c954c79d90cd846da9f5bd965019 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose 
> > > > Date: Thu, 24 Oct 2013 11:44:11 +0200
> > > > Subject: [PATCH 2/4] idmap: add
> > > >  sss_idmap_domain_by_name_has_algorithmic_mapping()
> > >
> > > The code itself is good, but we need to bump the version number. See the
> > > attached patch, can you squash it in if you agree?
> >
> > There was no patch but I changed version-info to 2:0:2. Is this what you
> > would suggest?
> > > > From dffdd7cc99025cb8790f34f79bdf294762f3468d Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose 
> > > > Date: Thu, 24 Oct 2013 11:45:57 +0200
> > > > Subject: [PATCH 3/4] sdap_idmap_domain_has_algorithmic_mapping: add domain
> > > >  name argument
> > >
> > > ACK
> > > > From 3e7283a39994ac24a9ff9dfede0d735cc89b4bb0 Mon Sep 17 00:00:00 2001
> > > > From: Sumit Bose 
> > > > Date: Wed, 23 Oct 2013 13:30:57 +0200
> > > > Subject: [PATCH 4/4] IPA: add trusted domains with missing idrange
> > > >
> > > > If the forest root of a trusted forest is managing POSIX IDs for its
> > > > users and groups the same is assumed for all member domains in the
> > > > forest which do not have explicitly have an idrange set.
> > > >
> > > > To reflect this SSSD will create the matching ranges automatically.
> > >
> > > One nitpick:
> > > > tmp_ctx = talloc_new(NULL);
> > > > if (tmp_ctx == NULL) {
> > > >    DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
> > > >    return ENOMEM;
> > > > }
> > > > for (c = 0; c < range_count; c++) {
> > > >    r = range_list[c];
> > > >    if (r->trusted_dom_sid != NULL
> > > >            && strcmp(r->trusted_dom_sid, forest_root->domain_id) == 0) {
> > > >        if (r->range_type == NULL
> > > >                || strcmp(r->range_type, IPA_RANGE_AD_TRUST_POSIX) != 0) {
> > > >            DEBUG(SSSDBG_MINOR_FAILURE,
> > > >                  ("Forest root does not have range type [%s].\n",
> > > >                   IPA_RANGE_AD_TRUST_POSIX));
> > >
> > > we would leak tmp_ctx here, we should goto done instead.
> >
> > good catch, fixed.
> > > >            return EINVAL;
> > > >        }
> > > >        range.min = r->base_id;
> > > >        range.max = r->base_id + r->id_range_size -1;
> > > >        range_id = talloc_asprintf(tmp_ctx, "%s-%s", dom_sid_str, r->name);
> > > >        if (range_id == NULL) {
> > > >            DEBUG(SSSDBG_OP_FAILURE, ("talloc_asprintf failed.\n"));
> > > >            ret = ENOMEM;
> > > >            goto done;
> > > >        }
> > > >        err = sss_idmap_add_domain_ex(idmap_ctx->map, dom_name, dom_sid_str,
> > > >                                      &range, range_id, 0, true);
> > > >        if (err != IDMAP_SUCCESS && err != IDMAP_COLLISION) {
> > > >            DEBUG(SSSDBG_CRIT_FAILURE, ("Could not add range [%s] to ID map\n",
> > > >                                        range_id));
> > >
> > > If you're changing the patch, can you also reflow this line? It's past
> > > 80 characters and my editor highlights the line..
> >
> > fixed.
> >
> > Thank you for the review, new versions attached.
> >
> > bye,
> > Sumit
>
> ACK to these patches.

Pushed to master and sssd-1-11