Hi,
this patch set aims to fix https://fedorahosted.org/sssd/ticket/2105 .
But since I had to touch the libsss_idmap code for this I found an issue
here as well, which is fixed by the first two patches.
bye,
Sumit
From 5e9daa3af6b1d5ef178ad8737e98908543aa66eb Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Wed, 9 Oct 2013 15:20:38 +0200
Subject: [PATCH 1/5] idmap: add internal function to free a domain struct
---
src/lib/idmap/sss_idmap.c | 19 +++++++++++++++----
1 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index c7ac0c7..45797f2 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -213,6 +213,20 @@ enum idmap_error_code sss_idmap_init(idmap_alloc_func
*alloc_func,
return IDMAP_SUCCESS;
}
+static void sss_idmap_free_domain(struct sss_idmap_ctx *ctx,
+ struct idmap_domain_info *dom)
+{
+ if (ctx == NULL || dom == NULL) {
+ return;
+ }
+
+ ctx->free_func(dom->range_id, ctx->alloc_pvt);
+ ctx->free_func(dom->range, ctx->alloc_pvt);
+ ctx->free_func(dom->name, ctx->alloc_pvt);
+ ctx->free_func(dom->sid, ctx->alloc_pvt);
+ ctx->free_func(dom, ctx->alloc_pvt);
+}
+
enum idmap_error_code sss_idmap_free(struct sss_idmap_ctx *ctx)
{
struct idmap_domain_info *dom;
@@ -224,10 +238,7 @@ enum idmap_error_code sss_idmap_free(struct sss_idmap_ctx
*ctx)
while (next) {
dom = next;
next = dom->next;
- ctx->free_func(dom->range, ctx->alloc_pvt);
- ctx->free_func(dom->name, ctx->alloc_pvt);
- ctx->free_func(dom->sid, ctx->alloc_pvt);
- ctx->free_func(dom, ctx->alloc_pvt);
+ sss_idmap_free_domain(ctx, dom);
}
ctx->free_func(ctx, ctx->alloc_pvt);
--
1.7.7.6
From e7263a273d6f0aa1d4ec83a9f9bcbd32d65e23c8 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Wed, 9 Oct 2013 15:21:48 +0200
Subject: [PATCH 2/5] idmap: fix a memory leak if a collision is detected
---
src/lib/idmap/sss_idmap.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 45797f2..89c55fc 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -432,24 +432,28 @@ enum idmap_error_code sss_idmap_add_domain_ex(struct
sss_idmap_ctx *ctx,
dom->name = idmap_strdup(ctx, domain_name);
if (dom->name == NULL) {
+ err = IDMAP_OUT_OF_MEMORY;
goto fail;
}
if (domain_sid != NULL) {
dom->sid = idmap_strdup(ctx, domain_sid);
if (dom->sid == NULL) {
+ err = IDMAP_OUT_OF_MEMORY;
goto fail;
}
}
dom->range = idmap_range_dup(ctx, range);
if (dom->range == NULL) {
+ err = IDMAP_OUT_OF_MEMORY;
goto fail;
}
if (range_id != NULL) {
dom->range_id = idmap_strdup(ctx, range_id);
if (dom->range_id == NULL) {
+ err = IDMAP_OUT_OF_MEMORY;
goto fail;
}
}
@@ -459,8 +463,7 @@ enum idmap_error_code sss_idmap_add_domain_ex(struct
sss_idmap_ctx *ctx,
err = dom_check_collision(ctx->idmap_domain_info, dom);
if (err != IDMAP_SUCCESS) {
- ctx->free_func(dom, ctx->alloc_pvt);
- return err;
+ goto fail;
}
dom->next = ctx->idmap_domain_info;
@@ -469,11 +472,9 @@ enum idmap_error_code sss_idmap_add_domain_ex(struct
sss_idmap_ctx *ctx,
return IDMAP_SUCCESS;
fail:
- ctx->free_func(dom->sid, ctx->alloc_pvt);
- ctx->free_func(dom->name, ctx->alloc_pvt);
- ctx->free_func(dom, ctx->alloc_pvt);
+ sss_idmap_free_domain(ctx, dom);
- return IDMAP_OUT_OF_MEMORY;
+ return err;
}
enum idmap_error_code sss_idmap_add_domain(struct sss_idmap_ctx *ctx,
--
1.7.7.6
From c2af969630611dc1fc91d86e1d6141746c2cc82b Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Wed, 9 Oct 2013 15:22:53 +0200
Subject: [PATCH 3/5] idmap: allow ranges with external mapping to overlap
If POSIX IDs are managed externally e.g. by AD it might be possible that
the IDs are centrally manages for the whole forest. Hence there might
not be a single ID range for each member domain in the forest but only a
single ID range for the whole forest. This means that we have to allow
collisions if ID ranges in this case.
Unit tests are added to make sure that the collisions are only allowed
for external mappings.
---
src/lib/idmap/sss_idmap.c | 12 +++++---
src/tests/sss_idmap-tests.c | 64 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 71 insertions(+), 5 deletions(-)
diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index 89c55fc..17bd577 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -357,11 +357,13 @@ static enum idmap_error_code dom_check_collision(
/* TODO: if both ranges have the same ID check if an update is
* needed. */
- /* check if ID ranges overlap */
- if ((new_dom->range->min >= dom->range->min
- && new_dom->range->min <= dom->range->max)
- || (new_dom->range->max >= dom->range->min
- && new_dom->range->max <= dom->range->max)) {
+ /* Check if ID ranges overlap.
+ * ID ranges with external mapping may overlap. */
+ if ((!new_dom->external_mapping && !dom->external_mapping)
+ && ((new_dom->range->min >= dom->range->min
+ && new_dom->range->min <= dom->range->max)
+ || (new_dom->range->max >= dom->range->min
+ && new_dom->range->max <= dom->range->max))) {
return IDMAP_COLLISION;
}
diff --git a/src/tests/sss_idmap-tests.c b/src/tests/sss_idmap-tests.c
index eb20413..65e6135 100644
--- a/src/tests/sss_idmap-tests.c
+++ b/src/tests/sss_idmap-tests.c
@@ -29,6 +29,9 @@
#define IDMAP_RANGE_MIN 1234
#define IDMAP_RANGE_MAX 9876
+#define IDMAP_RANGE_MIN2 11234
+#define IDMAP_RANGE_MAX2 19876
+
const char test_sid[] = "S-1-5-21-2127521184-1604012920-1887927527-72713";
uint8_t test_bin_sid[] = {0x01, 0x05, 0x00, 0x00, 0x00, 0x00, 0x00, 0x05, 0x15,
0x00, 0x00, 0x00, 0xA0, 0x65, 0xCF, 0x7E, 0x78, 0x4B,
@@ -142,6 +145,65 @@ START_TEST(idmap_test_add_domain)
}
END_TEST
+START_TEST(idmap_test_add_domain_collisions)
+{
+ enum idmap_error_code err;
+ struct sss_idmap_range range = {IDMAP_RANGE_MIN, IDMAP_RANGE_MAX};
+ struct sss_idmap_range range2 = {IDMAP_RANGE_MIN2, IDMAP_RANGE_MAX2};
+
+ err = sss_idmap_add_domain(idmap_ctx, "test.dom", "S-1-5-21-1-2-3",
&range);
+ fail_unless(err == IDMAP_SUCCESS, "sss_idmap_add_domain failed.");
+
+ err = sss_idmap_add_domain(idmap_ctx, "test.dom", "S-1-5-21-1-2-4",
+ &range2);
+ fail_unless(err == IDMAP_COLLISION,
+ "sss_idmap_add_domain added domain with the same name.");
+
+ err = sss_idmap_add_domain(idmap_ctx, "test.dom2", "S-1-5-21-1-2-3",
+ &range2);
+ fail_unless(err == IDMAP_COLLISION,
+ "sss_idmap_add_domain added domain with the same SID.");
+
+ err = sss_idmap_add_domain(idmap_ctx, "test.dom2", "S-1-5-21-1-2-4",
+ &range);
+ fail_unless(err == IDMAP_COLLISION,
+ "sss_idmap_add_domain added domain with the same range.");
+
+ err = sss_idmap_add_domain(idmap_ctx, "test.dom2", "S-1-5-21-1-2-4",
+ &range2);
+ fail_unless(err == IDMAP_SUCCESS,
+ "sss_idmap_add_domain failed to add second domain.");
+}
+END_TEST
+
+START_TEST(idmap_test_add_domain_collisions_ext_mapping)
+{
+ enum idmap_error_code err;
+ struct sss_idmap_range range = {IDMAP_RANGE_MIN, IDMAP_RANGE_MAX};
+ struct sss_idmap_range range2 = {IDMAP_RANGE_MIN2, IDMAP_RANGE_MAX2};
+
+ err = sss_idmap_add_domain_ex(idmap_ctx, "test.dom", "S-1-5-21-1-2-3",
+ &range, NULL, 0, true);
+ fail_unless(err == IDMAP_SUCCESS, "sss_idmap_add_domain failed.");
+
+ err = sss_idmap_add_domain_ex(idmap_ctx, "test.dom", "S-1-5-21-1-2-4",
+ &range2, NULL, 0, true);
+ fail_unless(err == IDMAP_COLLISION,
+ "sss_idmap_add_domain added domain with the same name.");
+
+ err = sss_idmap_add_domain_ex(idmap_ctx, "test.dom2", "S-1-5-21-1-2-3",
+ &range2, NULL, 0, true);
+ fail_unless(err == IDMAP_COLLISION,
+ "sss_idmap_add_domain added domain with the same SID.");
+
+ err = sss_idmap_add_domain_ex(idmap_ctx, "test.dom2", "S-1-5-21-1-2-4",
+ &range, NULL, 0, true);
+ fail_unless(err == IDMAP_SUCCESS,
+ "sss_idmap_add_domain failed to add second domain with " \
+ "external mapping and the same range.");
+}
+END_TEST
+
START_TEST(idmap_test_sid2uid)
{
enum idmap_error_code err;
@@ -510,6 +572,8 @@ Suite *idmap_test_suite (void)
idmap_ctx_teardown);
tcase_add_test(tc_dom, idmap_test_add_domain);
+ tcase_add_test(tc_dom, idmap_test_add_domain_collisions);
+ tcase_add_test(tc_dom, idmap_test_add_domain_collisions_ext_mapping);
suite_add_tcase(s, tc_dom);
--
1.7.7.6
From af4ca9eec65e4bae6ed399bf2136c7a8542f2e59 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Wed, 9 Oct 2013 15:30:29 +0200
Subject: [PATCH 4/5] sdap_idmap: add
sdap_idmap_get_configured_external_range()
---
src/providers/ldap/sdap_idmap.c | 49 ++++++++++++++++++++++++++++----------
1 files changed, 36 insertions(+), 13 deletions(-)
diff --git a/src/providers/ldap/sdap_idmap.c b/src/providers/ldap/sdap_idmap.c
index 667e377..18e1986 100644
--- a/src/providers/ldap/sdap_idmap.c
+++ b/src/providers/ldap/sdap_idmap.c
@@ -27,12 +27,13 @@
#include "util/util_sss_idmap.h"
static errno_t
-sdap_idmap_add_configured_external_range(struct sdap_idmap_ctx *idmap_ctx)
+sdap_idmap_get_configured_external_range(struct sdap_idmap_ctx *idmap_ctx,
+ struct sss_idmap_range *range)
{
int int_id;
- struct sss_idmap_range range;
struct sdap_id_ctx *id_ctx;
- enum idmap_error_code err;
+ uint32_t min;
+ uint32_t max;
if (idmap_ctx == NULL) {
return EINVAL;
@@ -45,32 +46,54 @@ sdap_idmap_add_configured_external_range(struct
sdap_idmap_ctx *idmap_ctx)
DEBUG(SSSDBG_CONF_SETTINGS, ("ldap_min_id must be greater than 0.\n"));
return EINVAL;
}
- range.min = int_id;
+ min = int_id;
int_id = dp_opt_get_int(id_ctx->opts->basic, SDAP_MAX_ID);
if (int_id < 0) {
- DEBUG(SSSDBG_CONF_SETTINGS, ("ldap_min_id must be greater than 0.\n"));
+ DEBUG(SSSDBG_CONF_SETTINGS, ("ldap_max_id must be greater than 0.\n"));
return EINVAL;
}
- range.max = int_id;
+ max = int_id;
- if ((range.min == 0 && range.max != 0)
- || (range.min != 0 && range.max == 0)) {
+ if ((min == 0 && max != 0) || (min != 0 && max == 0)) {
DEBUG(SSSDBG_CONF_SETTINGS, ("Both ldap_min_id and ldap_max_id " \
"either must be 0 (not set) " \
"or positive integers.\n"));
return EINVAL;
}
- if (range.min == 0 && range.max == 0) {
+ if (min == 0 && max == 0) {
/* ldap_min_id and ldap_max_id not set, using min_id and max_id */
- range.min = id_ctx->be->domain->id_min;
- range.max = id_ctx->be->domain->id_max;
- if (range.max == 0) {
- range.max = UINT32_MAX;
+ min = id_ctx->be->domain->id_min;
+ max = id_ctx->be->domain->id_max;
+ if (max == 0) {
+ max = UINT32_MAX;
}
}
+ range->min = min;
+ range->max =max;
+
+ return EOK;
+}
+
+static errno_t
+sdap_idmap_add_configured_external_range(struct sdap_idmap_ctx *idmap_ctx)
+{
+ int ret;
+ struct sss_idmap_range range;
+ struct sdap_id_ctx *id_ctx;
+ enum idmap_error_code err;
+
+ ret = sdap_idmap_get_configured_external_range(idmap_ctx, &range);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ ("sdap_idmap_get_configured_external_range failed.\n"));
+ return ret;
+ }
+
+ id_ctx = idmap_ctx->id_ctx;
+
err = sss_idmap_add_domain_ex(idmap_ctx->map, id_ctx->be->domain->name,
id_ctx->be->domain->domain_id, &range,
NULL, 0, true);
--
1.7.7.6
From 96387421488982ef67dd90e6eb7653c2c9cff04d Mon Sep 17 00:00:00 2001
From: Sumit Bose <sb...@redhat.com>
Date: Wed, 9 Oct 2013 15:31:33 +0200
Subject: [PATCH 5/5] sdap_idmap: properly handle ranges for external mappings
Currently we relied on the fact that external ID mapping is used as
default fallback in case of an error and did not properly add subdomains
with external ID mapping to the idmap library. If debugging is enabled
this leads to irritating debug messages for every user or group lookup.
With this patch this subdomains are added to the idmap library.
Fixes https://fedorahosted.org/sssd/ticket/2105
---
src/providers/ldap/sdap_idmap.c | 68 +++++++++++++++++++++++++--------------
1 files changed, 44 insertions(+), 24 deletions(-)
diff --git a/src/providers/ldap/sdap_idmap.c b/src/providers/ldap/sdap_idmap.c
index 18e1986..af69ee1 100644
--- a/src/providers/ldap/sdap_idmap.c
+++ b/src/providers/ldap/sdap_idmap.c
@@ -329,6 +329,7 @@ sdap_idmap_add_domain(struct sdap_idmap_ctx *idmap_ctx,
struct sss_idmap_range range;
enum idmap_error_code err;
id_t idmap_upper;
+ bool external_mapping = true;
ret = sss_idmap_ctx_get_upper(idmap_ctx->map, &idmap_upper);
if (ret != IDMAP_SUCCESS) {
@@ -338,28 +339,39 @@ sdap_idmap_add_domain(struct sdap_idmap_ctx *idmap_ctx,
goto done;
}
- ret = sss_idmap_calculate_range(idmap_ctx->map, dom_sid, &slice, &range);
- if (ret != IDMAP_SUCCESS) {
- DEBUG(SSSDBG_CRIT_FAILURE,
- ("Failed to calculate range for domain [%s]: [%d]\n", dom_name,
- ret));
- ret = EIO;
- goto done;
- }
- DEBUG(SSSDBG_TRACE_LIBS,
- ("Adding domain [%s] as slice [%"SPRIid"]\n", dom_sid, slice));
-
- if (range.max > idmap_upper) {
- /* This should never happen */
- DEBUG(SSSDBG_CRIT_FAILURE,
- ("BUG: Range maximum exceeds the global maximum: "
- "%d > %"SPRIid"\n", range.max, idmap_upper));
- ret = EINVAL;
- goto done;
+ if (dp_opt_get_bool(idmap_ctx->id_ctx->opts->basic, SDAP_ID_MAPPING)) {
+ external_mapping = false;
+ ret = sss_idmap_calculate_range(idmap_ctx->map, dom_sid, &slice,
&range);
+ if (ret != IDMAP_SUCCESS) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Failed to calculate range for domain [%s]: [%d]\n",
dom_name,
+ ret));
+ ret = EIO;
+ goto done;
+ }
+ DEBUG(SSSDBG_TRACE_LIBS,
+ ("Adding domain [%s] as slice [%"SPRIid"]\n", dom_sid, slice));
+
+ if (range.max > idmap_upper) {
+ /* This should never happen */
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("BUG: Range maximum exceeds the global maximum: "
+ "%u > %"SPRIid"\n", range.max, idmap_upper));
+ ret = EINVAL;
+ goto done;
+ }
+ } else {
+ ret = sdap_idmap_get_configured_external_range(idmap_ctx, &range);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE,
+ ("sdap_idmap_get_configured_external_range failed.\n"));
+ return ret;
+ }
}
/* Add this domain to the map */
- err = sss_idmap_add_domain(idmap_ctx->map, dom_name, dom_sid, &range);
+ err = sss_idmap_add_domain_ex(idmap_ctx->map, dom_name, dom_sid, &range,
+ NULL, 0, external_mapping);
if (err != IDMAP_SUCCESS) {
DEBUG(SSSDBG_CRIT_FAILURE,
("Could not add domain [%s] to the map: [%d]\n",
@@ -368,11 +380,19 @@ sdap_idmap_add_domain(struct sdap_idmap_ctx *idmap_ctx,
goto done;
}
- /* Add this domain to the SYSDB cache so it will survive reboot */
- ret = sysdb_idmap_store_mapping(idmap_ctx->id_ctx->be->domain->sysdb,
- idmap_ctx->id_ctx->be->domain,
- dom_name, dom_sid,
- slice);
+ /* If algorithmic mapping is used add this domain to the SYSDB cache so it
+ * will survive reboot */
+ if (!external_mapping) {
+ ret = sysdb_idmap_store_mapping(idmap_ctx->id_ctx->be->domain->sysdb,
+ idmap_ctx->id_ctx->be->domain,
+ dom_name, dom_sid,
+ slice);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_OP_FAILURE, ("sysdb_idmap_store_mapping failed.\n"));
+ goto done;
+ }
+ }
+
done:
return ret;
}
--
1.7.7.6
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
