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(a)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(a)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(a)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(a)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;
+ }
+ }