[Freeipa-devel] [PATCH 0158] Extend ipa-range-check DS plugin to handle range types
Tomas Babej
tbabej at redhat.com
Thu Mar 27 12:11:15 UTC 2014
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
>>
>
>
--
Tomas Babej
Associate Software Engineer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-tbabej-0158-3-Extend-ipa-range-check-DS-plugin-to-handle-range-typ.patch
Type: text/x-patch
Size: 16798 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20140327/42828b87/attachment.bin>
More information about the Freeipa-devel
mailing list