[Freeipa-devel] [PATCH 0158] Extend ipa-range-check DS plugin to handle range types
Tomas Babej
tbabej at redhat.com
Tue Apr 1 08:08:00 UTC 2014
A slightly new version, properly adds new attributes of the range_info
struct to the free_range_info method.
Should be applied on top of my 161 patch.
On 03/27/2014 01:11 PM, Tomas Babej wrote:
> 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:
>> 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
>>>
>>
>
>
> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140401/9303d596/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0158-4-Extend-ipa-range-check-DS-plugin-to-handle-range-typ.patch
Type: text/x-patch
Size: 17113 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140401/9303d596/attachment.bin>
More information about the Freeipa-devel
mailing list