[SSSD] [PATCH] IPA: check ranges for collisions before saving them

Jakub Hrozek jhrozek at redhat.com
Mon Feb 24 18:12:00 UTC 2014


On Thu, Feb 20, 2014 at 10:34:44AM +0100, Sumit Bose wrote:
> On Wed, Feb 19, 2014 at 04:23:55PM +0100, Jakub Hrozek wrote:
> > On Wed, Feb 12, 2014 at 03:43:51PM +0100, Sumit Bose wrote:
> > > Hi,
> > > 
> > > currently we trust the idrange data we get from the IPA server. But it
> > > turned out that some checks are missing on the server
> > > (https://fedorahosted.org/freeipa/ticket/4137) so SSSD should check the
> > > input before saving the data to the cache.
> > > 
> > > The first two patches make some code which already exists available for
> > > other callers. The third contains the actual functionality.
> > > 
> > > bye,
> > > Sumit
> > 
> > Hi, I rebased your patches on top of master, can you check if they look
> > OK to you and resend if they are fine? I'll continue testing with my
> > version in the meantime.
> 
> sorry I forgot to send a version for master myself. The patches are
> exactly the same as in my master tree.
> 
> Thanks
> 
> bye,
> Sumit

> From 2def1c79630be290ec53a5c17871238b3fb388bb Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Mon, 3 Feb 2014 13:30:35 +0100
> Subject: [PATCH 1/3] IDMAP: add sss_idmap_check_collision(_ex)

ACK

> From 4b6b398e56da8453141a18882e39cfdca86d8941 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 7 Feb 2014 15:54:30 +0100
> Subject: [PATCH 2/3] IPA: refactor idmap code and add test

See two comments inline:

> @@ -187,72 +245,36 @@ errno_t ipa_idmap_find_new_domain(struct sdap_idmap_ctx *idmap_ctx,
>      }
>  
>      for (c = 0; c < range_count; c++) {
> -        r = range_list[c];
> -
> -        if (r->range_type == NULL) {
> -            /* Older IPA servers might not have the range_type attribute, but
> -             * only support local ranges and trusts with algorithmic mapping. */
> -
> -            if (r->trusted_dom_sid == NULL && r->secondary_base_rid != 0) {
> -                /* local IPA domain */
> -                rid = 0;
> -                external_mapping = true;
> -                name = idmap_ctx->id_ctx->be->domain->name;
> -                sid = NULL;
> -            } else if (r->trusted_dom_sid != NULL
> -                    && r->secondary_base_rid == 0) {
> -                /* trusted domain */
> -                rid = r->base_rid;
> -                external_mapping = false;
> -                name = r->trusted_dom_sid;
> -                sid = r->trusted_dom_sid;
> -            } else {
> -                DEBUG(SSSDBG_MINOR_FAILURE, "Cannot determine range type, " \
> -                                             "skipping id ange [%s].\n",
> -                                             r->name);
> -                continue;
> -            }
> -        } else {
> -            if (strcmp(r->range_type, IPA_RANGE_LOCAL) == 0) {
> -                rid = 0;
> -                external_mapping = true;
> -                name = idmap_ctx->id_ctx->be->domain->name;
> -                sid = NULL;
> -            } else if (strcmp(r->range_type, IPA_RANGE_AD_TRUST_POSIX) == 0) {
> -                rid = 0;
> -                external_mapping = true;
> -                name = r->trusted_dom_sid;
> -                sid = r->trusted_dom_sid;
> -            } else if (strcmp(r->range_type, IPA_RANGE_AD_TRUST) == 0) {
> -                rid = r->base_rid;
> -                external_mapping = false;
> -                name = r->trusted_dom_sid;
> -                sid = r->trusted_dom_sid;
> -            } else {
> -                DEBUG(SSSDBG_MINOR_FAILURE, "Range type [%s] not supported, " \
> -                                             "skipping id range [%s].\n",
> -                                             r->range_type, r->name);
> -                continue;
> -            }
> +        ret = get_idmap_data_from_range(range_list[c],
> +                                        idmap_ctx->id_ctx->be->domain->name,
> +                                        &name, &sid, &rid, &range,
> +                                        &external_mapping);
> +        if (ret != EOK) {
> +            DEBUG(SSSDBG_OP_FAILURE, "get_idmap_data_from_range failed for " \
> +                                     "id range [%s], skipping.\n",
> +                                     range_list[c]->name);
> +            continue;
>          }
>  
> -        range.min = r->base_id;
> -        range.max = r->base_id + r->id_range_size -1;
>          err = sss_idmap_add_domain_ex(idmap_ctx->map, name, sid, &range,
> -                                      r->name, rid, external_mapping);
> -        if (err != IDMAP_SUCCESS && err != IDMAP_COLLISION) {
> +                                      range_list[c]->name, rid,
> +                                      external_mapping);
> +        if (err != IDMAP_SUCCESS
> +                && allow_collisions && err != IDMAP_COLLISION) {

I don't think this condition is right, if allow_collisions is false,
then it never matches.

>              DEBUG(SSSDBG_CRIT_FAILURE, "Could not add range [%s] to ID map\n",
> -                                        r->name);
> +                                       range_list[c]->name);
>              ret = EIO;
>              goto done;
>          }
>      }

[snip]

> diff --git a/src/tests/cmocka/test_ipa_idmap.c b/src/tests/cmocka/test_ipa_idmap.c

[snip]

> +int main(int argc, const char *argv[])
> +{
> +    poptContext pc;
> +    int opt;
> +    struct poptOption long_options[] = {
> +        POPT_AUTOHELP
> +        SSSD_DEBUG_OPTS
> +        POPT_TABLEEND
> +    };
> +
> +    const UnitTest tests[] = {
> +        //unit_test_setup_teardown(test_add_domain,
> +        //                         test_sss_idmap_setup, test_sss_idmap_teardown),

It seems that no further patch includes test_add_domain either, did you
want to remove the commented out code?

> +        unit_test(test_get_idmap_data_from_range),
> +    };
> +
> +    /* Set debug level to invalid value so we can deside if -d 0 was used. */
> +    debug_level = SSSDBG_INVALID;
> +
> +    pc = poptGetContext(argv[0], argc, argv, long_options, 0);
> +    while((opt = poptGetNextOpt(pc)) != -1) {
> +        switch(opt) {
> +        default:
> +            fprintf(stderr, "\nInvalid option %s: %s\n\n",
> +                    poptBadOption(pc, 0), poptStrerror(opt));
> +            poptPrintUsage(pc, stderr, 0);
> +            return 1;
> +        }
> +    }
> +    poptFreeContext(pc);
> +
> +    DEBUG_INIT(debug_level);
> +
> +    tests_set_cwd();
> +
> +    return run_tests(tests);
> +}
> -- 
> 1.8.5.3
> 

> From 07ca39a31c5abde685481ef6fe993a0eb7a7f9b3 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Fri, 7 Feb 2014 18:17:09 +0100
> Subject: [PATCH 3/3] IPA: check ranges for collisions before saving them

ACK

When you resend the patches, can you also include the version for 1.11 ?
I think the initial submission would be easy to reuse..



More information about the sssd-devel mailing list