[Freeipa-devel] [PATCH 0158] Extend ipa-range-check DS plugin to handle range types
Tomas Babej
tbabej at redhat.com
Tue Mar 18 15:06:38 UTC 2014
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.
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.
>
>
>
>>
>>
>> --
>> Tomas Babej
>> Associate Software Engeneer | Red Hat | Identity Management
>> RHCE | Brno Site | IRC: tbabej | freeipa.org
>>
>>
>
>>> From 0d038fb71f02fab5320e4843be80feb34c5c3303 Mon Sep 17 00:00:00 2001
>> From: Tomas Babej <tbabej at redhat.com>
>> Date: Wed, 5 Mar 2014 12:28:18 +0100
>> Subject: [PATCH] Extend ipa-range-check DS plugin to handle range types
>>
>> 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
>> ---
>> .../ipa-range-check/ipa_range_check.c | 322
>> ++++++++++++++++++---
>> 1 file changed, 284 insertions(+), 38 deletions(-)
>>
>> diff --git
>> a/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
>> b/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
>> index
>> 391e2259b6eced31fed399c927cfe44c1d3e237e..2360e42025a0146a43c3237914c8165d35714a68
>> 100644
>> --- a/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
>> +++ b/daemons/ipa-slapi-plugins/ipa-range-check/ipa_range_check.c
>> @@ -47,10 +47,17 @@
>> #define IPA_CN "cn"
>> #define IPA_BASE_ID "ipaBaseID"
>> #define IPA_ID_RANGE_SIZE "ipaIDRangeSize"
>> +#define IPA_RANGE_TYPE "ipaRangeType"
>> #define IPA_BASE_RID "ipaBaseRID"
>> #define IPA_SECONDARY_BASE_RID "ipaSecondaryBaseRID"
>> #define IPA_DOMAIN_ID "ipaNTTrustedDomainSID"
>> #define RANGES_FILTER "objectclass=ipaIDRange"
>> +#define DOMAIN_ID_FILTER "ipaNTTrustedDomainSID=*"
>> +
>> +#define AD_TRUST_RANGE_TYPE "ipa-ad-trust"
>> +#define AD_TRUST_POSIX_RANGE_TYPE "ipa-ad-trust-posix"
>> +#define LOCAL_RANGE_TYPE "ipa-local"
>> +
>>
>> #define IPA_PLUGIN_NAME "ipa-range-check"
>> #define IPA_RANGE_CHECK_FEATURE_DESC "IPA ID range check plugin"
>> @@ -72,13 +79,225 @@ struct ipa_range_check_ctx {
>> struct range_info {
>> char *name;
>> char *domain_id;
>> + char *forest_root_id;
>> + char *id_range_type;
>> uint32_t base_id;
>> uint32_t id_range_size;
>> uint32_t base_rid;
>> uint32_t secondary_base_rid;
>> };
>>
>> -static int slapi_entry_to_range_info(struct slapi_entry *entry,
>> +struct domain_info {
>> + char *domain_id;
>> + char *forest_root_id;
>> + struct domain_info *next;
>> +};
>> +
>> +
>> +static struct domain_info* map_domain_to_root(struct domain_info *head,
>> + struct slapi_entry
>> *domain,
>> + struct slapi_entry
>> *root_domain){
>> +
>> + struct domain_info* new_head = NULL;
>> + new_head = (struct domain_info*) malloc(sizeof(struct
>> domain_info));
>> + if (new_head == NULL) {
>> + return NULL;
>> + }
>> +
>> + new_head->domain_id = slapi_entry_attr_get_charptr(domain,
>> + IPA_DOMAIN_ID);
>> + new_head->forest_root_id =
>> slapi_entry_attr_get_charptr(root_domain,
>> +
>> IPA_DOMAIN_ID);
>> + new_head->next = head;
>> +
>> + return new_head;
>> +}
>> +
>> +/* Searches for the domain_info struct with the specified domain_id
>> + * in the linked list. Returns the forest root domain's ID, or NULL for
>> + * local ranges. */
>> +
>> +static char* get_forest_root_id(struct domain_info *head, char*
>> domain_id) {
>> +
>> + /* For local ranges there is no forest root domain,
>> + * so consider only ranges with domain_id set */
>> + if (domain_id != NULL) {
>> + while(head) {
>> + if (strcasecmp(head->domain_id, domain_id) == 0) {
>> + return head->forest_root_id;
>> + }
>> + head = head->next;
>> + }
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +
>> +/*
>> + * This function builds a mapping from domain ID to forest
>> + * root domain ID.
>> + */
>> +
>> +static struct domain_info* build_domain_to_forest_root_map(struct
>> domain_info *head,
>> + struct
>> ipa_range_check_ctx *ctx){
>> +
>> + Slapi_PBlock *root_domain_search_pb = NULL;
>> + Slapi_PBlock *child_domain_search_pb = NULL;
>> + Slapi_Entry **root_domain_entries = NULL;
>> + Slapi_Entry **child_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);
>> +
>> + /* Allocate a new parameter block */
>> + root_domain_search_pb = slapi_pblock_new();
>> + if (root_domain_search_pb == NULL) {
>> + LOG_FATAL("Failed to create new pblock.\n");
>> + ret = LDAP_OPERATIONS_ERROR;
>> + //goto done;
>> + }
>> +
>> + /* Search for all the root domains, note the LDAP_SCOPE_ONELEVEL */
>> + slapi_search_internal_set_pb(root_domain_search_pb,
>> + slapi_sdn_get_dn(base_dn),
>> + LDAP_SCOPE_ONELEVEL, DOMAIN_ID_FILTER,
>> + NULL, 0, NULL, NULL,
>> ctx->plugin_id, 0);
>> +
>> + ret = slapi_search_internal_pb(root_domain_search_pb);
>> + if (ret != 0) {
>> + LOG_FATAL("Starting internal search failed.\n");
>> + goto done;
>> + }
>> +
>> + ret = slapi_pblock_get(root_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;
>> + }
>> +
>> + ret = slapi_pblock_get(root_domain_search_pb,
>> SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES,
>> + &root_domain_entries);
>> +
>> + if (ret != 0) {
>> + LOG_FATAL("Failed to read searched root domain entries.\n");
>> + goto done;
>> + }
>> +
>> + if (root_domain_entries == NULL || root_domain_entries[0] ==
>> NULL) {
>> + LOG("No existing root domain entries.\n");
>> + ret = 0;
>> + goto done;
>> + }
>> +
>> + /* now we iterate over root domains */
>> + for (int i = 0; root_domain_entries[i] != NULL; i++) {
>> + /* map root domain to itself */
>> + head = map_domain_to_root(head,
>> + root_domain_entries[i],
>> + root_domain_entries[i]);
>> + if (head == NULL) {
>> + LOG_FATAL("Error when allocating new domain_info struct.");
>> + goto child_done;
>> + }
>> +
>> + /* let's reuse the variables */
>> + child_domain_search_pb = NULL;
>> + child_domain_entries = NULL;
>> +
>> + /* allocate a new parameter block */
>> + child_domain_search_pb = slapi_pblock_new();
>> + if (child_domain_search_pb == NULL) {
>> + LOG_FATAL("Failed to create new pblock.\n");
>> + ret = LDAP_OPERATIONS_ERROR;
>> + goto child_done;
>> + }
>> +
>> + /* search for all the child domains, note the
>> LDAP_SCOPE_ONELEVEL */
>> + slapi_search_internal_set_pb(child_domain_search_pb,
>> +
>> slapi_entry_get_dn_const(root_domain_entries[i]),
>> + LDAP_SCOPE_ONELEVEL,
>> DOMAIN_ID_FILTER,
>> + NULL, 0, NULL, NULL,
>> ctx->plugin_id, 0);
>> +
>> + ret = slapi_search_internal_pb(child_domain_search_pb);
>> + if (ret != 0) {
>> + LOG_FATAL("Starting internal search failed.\n");
>> + goto child_done;
>> + }
>> +
>> + ret = slapi_pblock_get(child_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 child_done;
>> + }
>> +
>> + ret = slapi_pblock_get(child_domain_search_pb,
>> SLAPI_PLUGIN_INTOP_SEARCH_ENTRIES,
>> + &child_domain_entries);
>> +
>> + if (ret != 0) {
>> + LOG_FATAL("Failed to read searched child domain
>> entries.\n");
>> + goto child_done;
>> + }
>> +
>> + if (child_domain_entries == NULL || child_domain_entries[0]
>> == NULL) {
>> + LOG("No existing child domain entries.\n");
>> + ret = 0;
>> + goto child_done;
>> + }
>> +
>> + /* Now we iterate over child domains */
>> + for (int j = 0; child_domain_entries[j] != NULL; j++) {
>> +
>> + /* map each child domain to the root domain */
>> + head = map_domain_to_root(head,
>> + child_domain_entries[j],
>> + root_domain_entries[i]);
>> +
>> + }
>> +
>> + child_done:
>> + slapi_free_search_results_internal(child_domain_search_pb);
>> + slapi_pblock_destroy(child_domain_search_pb);
>> + }
>> +
>> +done:
>> + slapi_free_search_results_internal(root_domain_search_pb);
>> + slapi_pblock_destroy(root_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 +316,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 +383,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 +479,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 +569,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 +616,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 +645,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 +670,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;
>> + }
>> +
>> if (ret != 0) {
>> if (errmsg == NULL) {
>> errmsg = "Range Check error";
>> --
>> 1.8.5.3
>>
>
>> _______________________________________________
>> Freeipa-devel mailing list
>> Freeipa-devel at redhat.com
>> https://www.redhat.com/mailman/listinfo/freeipa-devel
>
>
--
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0159-2-Extend-ipa-range-check-DS-plugin-to-handle-range-typ.patch
Type: text/x-patch
Size: 16091 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140318/599fd033/attachment.bin>
More information about the Freeipa-devel
mailing list