[SSSD] [PATCHES] IPA: add trusted domains with missing idrange

Jakub Hrozek jhrozek at redhat.com
Fri Oct 25 09:21:28 UTC 2013


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 <sbose at redhat.com>
> 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 <sbose at redhat.com>
> 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 <sbose at redhat.com>
> 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 <sbose at redhat.com>
> 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;
> +        }
> +    }



More information about the sssd-devel mailing list