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(a)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(a)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(a)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..