On Tue, 18 Mar 2014, Tomas Babej wrote:
On 03/18/2014 09:19 AM, Alexander Bokovoy wrote:
On Mon, 17 Mar 2014, Tomas Babej wrote:
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.
https://fedorahosted.org/freeipa/ticket/4137
Test coverage coming soon!
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.
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.
If you make a {NULL, NULL} entry as the last one, no need to pass array
size. But anyway...
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.
See comments below.
+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);
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.
+
+ /* 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;
in done: label you don't deal with 'ret' at all. Do you need it?
+ //goto done;
is it goto or not?
+ }
+
+ /* 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),
here just use 'base_dn'.
+ 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;
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.
+ }
+
+ 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;
+ }
same here
+
+ 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");
same here
+ goto done;
+ }
+
+ if (trusted_domain_entries == NULL || trusted_domain_entries[0] == NULL) {
+ LOG("No existing root domain entries.\n");
+ ret = 0;
+ goto done;
+ }
Here as well
+
+ /* 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;
+ }
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.
+
if (ret != 0) {
if (errmsg == NULL) {
errmsg = "Range Check error";
--
1.8.5.3
--
/ Alexander Bokovoy
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
