[Freeipa-devel] [PATCH 0158] Extend ipa-range-check DS plugin to handle range types
Alexander Bokovoy
abokovoy at redhat.com
Tue Mar 18 15:45:30 UTC 2014
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
More information about the Freeipa-devel
mailing list