<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
A slightly new version, properly adds new attributes of the
range_info struct to the free_range_info method.<br>
<br>
Should be applied on top of my 161 patch.<br>
<br>
<div class="moz-cite-prefix">On 03/27/2014 01:11 PM, Tomas Babej
wrote:<br>
</div>
<blockquote cite="mid:53341563.9050204@redhat.com" type="cite">
<pre wrap="">The updated version handles the ret variable properly. It also makes
sure the memory is properly freed.
On 03/18/2014 04:45 PM, Alexander Bokovoy wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On Tue, 18 Mar 2014, Tomas Babej wrote:
</pre>
<blockquote type="cite">
<pre wrap="">
On 03/18/2014 09:19 AM, Alexander Bokovoy wrote:
</pre>
<blockquote type="cite">
<pre wrap="">On Mon, 17 Mar 2014, Tomas Babej wrote:
</pre>
<blockquote type="cite">
<pre wrap="">Hi,
The ipa-range-check plugin used to determine the range type depending
on the value of the attributes such as RID or secondary RID base. This
approached caused variety of issues since the portfolio of ID range
types expanded.
The patch makes sure the following rules are implemented:
* No ID range pair can overlap on base ranges, with exception
of two ipa-ad-trust-posix ranges belonging to the same forest
* For any ID range pair of ranges belonging to the same domain:
* Both ID ranges must be of the same type
* For ranges of ipa-ad-trust type or ipa-local type:
* Primary RID ranges can not overlap
* For ranges of ipa-local type:
* Primary and secondary RID ranges can not overlap
* Secondary RID ranges cannot overlap
For the implementation part, the plugin was extended with a domain ID
to forest root domain ID mapping derivation capabilities.
<a class="moz-txt-link-freetext" href="https://fedorahosted.org/freeipa/ticket/4137">https://fedorahosted.org/freeipa/ticket/4137</a>
Test coverage coming soon!
</pre>
</blockquote>
<pre wrap="">
I don't really like that you are using a list here. The list is built
and then destroyed in preop callback every time we process the range
object, so it is one-off operation. Now, when you fetch trust domain
objects to build the list, why can't you use an array directly?
Given that all you need the list for is to map domain to a trust (if
any) and trust name is the name of the root level domain, you can
simply
make an array of a struct (name, root) where root is a and index to the
same array or -1 if this is the root domain itself.
</pre>
</blockquote>
<pre wrap="">
I don't see much benefit of using array over linked list in this case.
In both cases, we would need to build the data container, and it would
be one-off operation (once for the ADD/MOD operation).
Additionaly, using linked list, I can only pass around the pointer to
the head of the list, instead of passing around the array and it's
size.
</pre>
</blockquote>
<pre wrap="">If you make a {NULL, NULL} entry as the last one, no need to pass array
size. But anyway...
</pre>
<blockquote type="cite">
<pre wrap="">I reworked the part of the patch that fetches the data from the LDAP as
we discussed. Instead of performing multiple LDAP searches, we do a
single extra search per operation.
</pre>
</blockquote>
<pre wrap="">See comments below.
</pre>
<blockquote type="cite">
<pre wrap="">+static struct domain_info* build_domain_to_forest_root_map(struct
domain_info *head,
+ struct
ipa_range_check_ctx *ctx){
+
+ Slapi_PBlock *trusted_domain_search_pb = NULL;
+ Slapi_Entry **trusted_domain_entries = NULL;
+ Slapi_DN *base_dn = NULL;
+ Slapi_RDN *rdn = NULL;
+
+ int search_result;
+ int ret;
+
+ /* Set the base DN for the search to cn=ad, cn=trusts, $SUFFIX */
+ base_dn = slapi_sdn_new_dn_byref(ctx->base_dn);
+ if (base_dn == NULL) {
+ LOG_FATAL("Failed to convert base DN.\n");
+ ret = LDAP_OPERATIONS_ERROR;
+ goto done;
+ }
+
+ rdn = slapi_rdn_new();
+ if (rdn == NULL) {
+ LOG_FATAL("Failed to obtain RDN struct.\n");
+ ret = LDAP_OPERATIONS_ERROR;
+ goto done;
+ }
+
+ slapi_rdn_add(rdn, "cn", "trusts");
+ slapi_sdn_add_rdn(base_dn, rdn);
+ slapi_rdn_done(rdn);
+
+ slapi_rdn_add(rdn, "cn", "ad");
+ slapi_sdn_add_rdn(base_dn, rdn);
+ slapi_rdn_done(rdn);
</pre>
</blockquote>
<pre wrap="">given that slapi_search_internal_set_pb() wants 'const char *base', why
not use asprintf() instead?
Here is what we do in ipa-kdb:
ret = asprintf(&base, "cn=ad,cn=trusts,%s", ipactx->base);
if (ret == -1) {
ret = ENOMEM;
goto done;
}
That's enough, IMHO. 389-ds internally will anyway create new sdn
explicitly from a string you've passed.
</pre>
<blockquote type="cite">
<pre wrap="">+
+ /* Allocate a new parameter block */
+ trusted_domain_search_pb = slapi_pblock_new();
+ if (trusted_domain_search_pb == NULL) {
+ LOG_FATAL("Failed to create new pblock.\n");
+ ret = LDAP_OPERATIONS_ERROR;
</pre>
</blockquote>
<pre wrap="">in done: label you don't deal with 'ret' at all. Do you need it?
</pre>
<blockquote type="cite">
<pre wrap="">+ //goto done;
</pre>
</blockquote>
<pre wrap="">is it goto or not?
</pre>
<blockquote type="cite">
<pre wrap="">+ }
+
+ /* Search for all the root domains, note the LDAP_SCOPE_ONELEVEL */
+ slapi_search_internal_set_pb(trusted_domain_search_pb,
+ slapi_sdn_get_dn(base_dn),
</pre>
</blockquote>
<pre wrap="">here just use 'base_dn'.
</pre>
<blockquote type="cite">
<pre wrap="">+ LDAP_SCOPE_SUBTREE, DOMAIN_ID_FILTER,
+ NULL, 0, NULL, NULL,
ctx->plugin_id, 0);
+
+ ret = slapi_search_internal_pb(trusted_domain_search_pb);
+ if (ret != 0) {
+ LOG_FATAL("Starting internal search failed.\n");
+ goto done;
</pre>
</blockquote>
<pre wrap="">make sure you are consistent with 'ret' -- either handling it or not,
and if overriding with LDAP_OPERATIONS_ERROR, make sure it is overridden
everywhere.
</pre>
<blockquote type="cite">
<pre wrap="">+ }
+
+ ret = slapi_pblock_get(trusted_domain_search_pb,
SLAPI_PLUGIN_INTOP_RESULT, &search_result);
+ if (ret != 0 || search_result != LDAP_SUCCESS) {
+ LOG_FATAL("Internal search failed.\n");
+ ret = LDAP_OPERATIONS_ERROR;
+ goto done;
+ }
</pre>
</blockquote>
<pre wrap="">same here
</pre>
<blockquote type="cite">
<pre wrap="">+
+ ret = slapi_pblock_get(trusted_domain_search_pb,
SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES,
+ &trusted_domain_entries);
+
+ if (ret != 0) {
+ LOG_FATAL("Failed to read searched root domain entries.\n");
</pre>
</blockquote>
<pre wrap="">same here
</pre>
<blockquote type="cite">
<pre wrap="">+ goto done;
+ }
+
+ if (trusted_domain_entries == NULL || trusted_domain_entries[0]
== NULL) {
+ LOG("No existing root domain entries.\n");
+ ret = 0;
+ goto done;
+ }
</pre>
</blockquote>
<pre wrap="">Here as well
</pre>
<blockquote type="cite">
<pre wrap="">+
+ /* now we iterate the domains and determine which of them are
root domains */
+ for (int i = 0; trusted_domain_entries[i] != NULL; i++) {
+
+ ret = slapi_sdn_isparent(base_dn,
+
slapi_entry_get_sdn(trusted_domain_entries[i]));
+
+ /* trusted domain is root domain */
+ if (ret == 1) {
+ map_domain_to_root(head,
+ trusted_domain_entries[i],
+ trusted_domain_entries[i]);
+ }
+ else {
+ /* we need to search for the root domain */
+ for (int j = 0; trusted_domain_entries[j] != NULL; j++) {
+ ret = slapi_sdn_isparent(
+
slapi_entry_get_sdn(trusted_domain_entries[j]),
+
slapi_entry_get_sdn(trusted_domain_entries[i]));
+
+ /* match, set the jth domain as the root domain for
the ith */
+ if (ret == 1) {
+ map_domain_to_root(head,
+ trusted_domain_entries[i],
+ trusted_domain_entries[j]);
+ break;
+ }
+ }
+ }
+ }
+
+done:
+ slapi_free_search_results_internal(trusted_domain_search_pb);
+ slapi_pblock_destroy(trusted_domain_search_pb);
+ slapi_rdn_free(&rdn);
+
+ return head;
+
+}
+
+static int slapi_entry_to_range_info(struct domain_info
*domain_info_head,
+ struct slapi_entry *entry,
struct range_info **_range)
{
int ret;
@@ -97,6 +273,9 @@ static int slapi_entry_to_range_info(struct
slapi_entry *entry,
}
range->domain_id = slapi_entry_attr_get_charptr(entry,
IPA_DOMAIN_ID);
+ range->id_range_type = slapi_entry_attr_get_charptr(entry,
IPA_RANGE_TYPE);
+ range->forest_root_id = get_forest_root_id(domain_info_head,
+ range->domain_id);
ul_val = slapi_entry_attr_get_ulong(entry, IPA_BASE_ID);
if (ul_val == 0 || ul_val >= UINT32_MAX) {
@@ -161,58 +340,67 @@ static bool intervals_overlap(uint32_t x,
uint32_t base, uint32_t x_size, uint32
* | | / \ |
* new range: base rid sec_rid
**/
-static int ranges_overlap(struct range_info *r1, struct range_info *r2)
+static int check_ranges(struct range_info *r1, struct range_info *r2)
{
+ /* Do not check overlaps of range with the range itself */
if (r1->name != NULL && r2->name != NULL &&
strcasecmp(r1->name, r2->name) == 0) {
return 0;
}
- /* check if base range overlaps with existing base range */
- if (intervals_overlap(r1->base_id, r2->base_id,
- r1->id_range_size, r2->id_range_size)){
- return 1;
+ /* Check if base range overlaps with existing base range.
+ * Exception: ipa-ad-trust-posix ranges from the same forest */
+ if (!(strcasecmp(r1->id_range_type, AD_TRUST_POSIX_RANGE_TYPE) &&
+ strcasecmp(r2->id_range_type, AD_TRUST_POSIX_RANGE_TYPE) &&
+ r1->forest_root_id != NULL && r2->forest_root_id !=NULL &&
+ strcasecmp(r1->forest_root_id, r2->forest_root_id) == 0)) {
+
+ if (intervals_overlap(r1->base_id, r2->base_id,
+ r1->id_range_size, r2->id_range_size)){
+ return 1;
+ }
+
}
- /* if both base_rid and secondary_base_rid = 0, the rid range is
not set */
- bool rid_ranges_set = (r1->base_rid != 0 ||
r1->secondary_base_rid != 0) &&
- (r2->base_rid != 0 ||
r2->secondary_base_rid != 0);
-
- /**
- * ipaNTTrustedDomainSID is not set for local ranges, use it to
- * determine the type of the range **/
- bool local_ranges = r1->domain_id == NULL && r2->domain_id == NULL;
-
+ /* Following checks apply for the ranges from the same domain */
bool ranges_from_same_domain =
(r1->domain_id == NULL && r2->domain_id == NULL) ||
(r1->domain_id != NULL && r2->domain_id != NULL &&
strcasecmp(r1->domain_id, r2->domain_id) == 0);
- /**
- * in case rid range is not set or ranges belong to different
domains
- * we can skip rid range tests as they are irrelevant **/
- if (rid_ranges_set && ranges_from_same_domain){
-
- /* check if rid range overlaps with existing rid range */
- if (intervals_overlap(r1->base_rid, r2->base_rid,
- r1->id_range_size, r2->id_range_size))
- return 2;
-
- /**
- * The following 3 checks are relevant only if both ranges
are local.
- * Check if secondary rid range overlaps with existing
secondary rid
- * range. **/
- if (local_ranges){
+ if (ranges_from_same_domain) {
+
+ /* Ranges from the same domain must have the same type */
+ if (strcasecmp(r1->id_range_type, r2->id_range_type) != 0) {
+ return 6;
+ }
+
+ /* For ipa-local or ipa-ad-trust range types primary RID
ranges should
+ * not overlap */
+ if (strcasecmp(r1->id_range_type, AD_TRUST_RANGE_TYPE) == 0 ||
+ strcasecmp(r1->id_range_type, LOCAL_RANGE_TYPE) == 0) {
+
+ /* Check if rid range overlaps with existing rid range */
+ if (intervals_overlap(r1->base_rid, r2->base_rid,
+ r1->id_range_size, r2->id_range_size))
+ return 2;
+ }
+
+ /* The following 3 checks are relevant only if both ranges
are local. */
+ if (strcasecmp(r1->id_range_type, LOCAL_RANGE_TYPE) == 0){
+
+ /* Check if secondary RID range overlaps with existing
secondary or
+ * primary RID range. */
if (intervals_overlap(r1->secondary_base_rid,
r2->secondary_base_rid, r1->id_range_size,
r2->id_range_size))
return 3;
- /* check if rid range overlaps with existing secondary
rid range */
+ /* Check if RID range overlaps with existing secondary
RID range */
if (intervals_overlap(r1->base_rid, r2->secondary_base_rid,
r1->id_range_size, r2->id_range_size))
return 4;
- /* check if secondary rid range overlaps with existing
rid range */
+ /* Check if secondary RID range overlaps with existing
RID range */
if (intervals_overlap(r1->secondary_base_rid, r2->base_rid,
r1->id_range_size, r2->id_range_size))
return 5;
@@ -248,9 +436,10 @@ static int ipa_range_check_pre_op(Slapi_PBlock
*pb, int modtype)
int search_result;
Slapi_Entry **search_entries = NULL;
size_t c;
- int no_overlap = 0;
+ int ranges_valid = 0;
const char *check_attr;
char *errmsg = NULL;
+ struct domain_info *domain_info_head = NULL;
ret = slapi_pblock_get(pb, SLAPI_IS_REPLICATED_OPERATION,
&is_repl_op);
if (ret != 0) {
@@ -337,7 +526,10 @@ static int ipa_range_check_pre_op(Slapi_PBlock
*pb, int modtype)
goto done;
}
- ret = slapi_entry_to_range_info(entry, &new_range);
+ /* build a linked list of domain_info structs */
+ domain_info_head =
build_domain_to_forest_root_map(domain_info_head, ctx);
+
+ ret = slapi_entry_to_range_info(domain_info_head, entry,
&new_range);
if (ret != 0) {
LOG_FATAL("Failed to convert LDAP entry to range struct.\n");
goto done;
@@ -381,19 +573,20 @@ static int ipa_range_check_pre_op(Slapi_PBlock
*pb, int modtype)
}
for (c = 0; search_entries[c] != NULL; c++) {
- ret = slapi_entry_to_range_info(search_entries[c], &old_range);
+ ret = slapi_entry_to_range_info(domain_info_head,
search_entries[c],
+ &old_range);
if (ret != 0) {
LOG_FATAL("Failed to convert LDAP entry to range
struct.\n");
goto done;
}
- no_overlap = ranges_overlap(new_range, old_range);
+ ranges_valid = check_ranges(new_range, old_range);
free(old_range);
old_range = NULL;
- if (no_overlap != 0) {
+ if (ranges_valid != 0) {
ret = LDAP_CONSTRAINT_VIOLATION;
- switch (no_overlap){
+ switch (ranges_valid){
case 1:
errmsg = "New base range overlaps with existing base
range.";
break;
@@ -409,6 +602,8 @@ static int ipa_range_check_pre_op(Slapi_PBlock
*pb, int modtype)
case 5:
errmsg = "New secondary rid range overlaps with
existing primary rid range.";
break;
+ case 6:
+ errmsg = "New ID range has invalid type. All ranges
in the same domain must be of the same type.";
default:
errmsg = "New range overlaps with existing one.";
break;
@@ -432,6 +627,14 @@ done:
slapi_entry_free(entry);
}
+ /* Remove the domain info linked list from memory */
+ struct domain_info *next;
+ while(domain_info_head) {
+ next = domain_info_head->next;
+ free(domain_info_head);
+ domain_info_head = next;
+ }
</pre>
</blockquote>
<pre wrap="">Who would clean up all the strings in each record?
I think we also have the issue in the original code with freeing
range objects. A mere free(new_range) is not enough.
</pre>
<blockquote type="cite">
<pre wrap="">+
if (ret != 0) {
if (errmsg == NULL) {
errmsg = "Range Check error";
--
1.8.5.3
</pre>
</blockquote>
<pre wrap="">
</pre>
</blockquote>
<pre wrap="">
</pre>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
<br>
<pre wrap="">_______________________________________________
Freeipa-devel mailing list
<a class="moz-txt-link-abbreviated" href="mailto:Freeipa-devel@redhat.com">Freeipa-devel@redhat.com</a>
<a class="moz-txt-link-freetext" href="https://www.redhat.com/mailman/listinfo/freeipa-devel">https://www.redhat.com/mailman/listinfo/freeipa-devel</a></pre>
</blockquote>
<br>
<pre class="moz-signature" cols="72">--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org </pre>
</body>
</html>