[Freeipa-devel] [PATCH 0058] Make get_entries not ignore its size_limit argument

Standa Laznicka slaznick at redhat.com
Mon Nov 21 16:08:26 UTC 2016 

On 10/10/2016 08:47 AM, Standa Laznicka wrote:
> On 10/10/2016 07:53 AM, Jan Cholasta wrote:
>> On 7.10.2016 12:23, Standa Laznicka wrote:
>>> On 10/07/2016 08:31 AM, Jan Cholasta wrote:
>>>> On 17.8.2016 13:47, Stanislav Laznicka wrote:
>>>>> On 08/11/2016 02:59 PM, Stanislav Laznicka wrote:
>>>>>> On 08/11/2016 07:49 AM, Jan Cholasta wrote:
>>>>>>> On 2.8.2016 13:47, Stanislav Laznicka wrote:
>>>>>>>> On 07/19/2016 09:20 AM, Jan Cholasta wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 14.7.2016 14:36, Stanislav Laznicka wrote:
>>>>>>>>>> Hello,
>>>>>>>>>>
>>>>>>>>>> This patch fixes https://fedorahosted.org/freeipa/ticket/5640.
>>>>>>>>>>
>>>>>>>>>> With not so much experience with the framework, it raises 
>>>>>>>>>> question
>>>>>>>>>> in my
>>>>>>>>>> head whether ipaldap.get_entries is used properly throughout the
>>>>>>>>>> system
>>>>>>>>>> - does it always assume that it gets ALL the requested 
>>>>>>>>>> entries or
>>>>>>>>>> just a
>>>>>>>>>> few of those as configured by the 'ipaSearchRecordsLimit'
>>>>>>>>>> attribute of
>>>>>>>>>> ipaConfig.etc which it actually gets?
>>>>>>>>>
>>>>>>>>> That depends. If you call get_entries() on the ldap2 plugin
>>>>>>>>> (which is
>>>>>>>>> usually the case in the framework), then ipaSearchRecordsLimit is
>>>>>>>>> used. If you call it on some arbitrary LDAPClient instance, the
>>>>>>>>> hardcoded default (= unlimited) is used.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> One spot that I know the get_entries method was definitely 
>>>>>>>>>> not used
>>>>>>>>>> properly before this patch is in the
>>>>>>>>>> baseldap.LDAPObject.get_memberindirect() method:
>>>>>>>>>>
>>>>>>>>>>  692             result = self.backend.get_entries(
>>>>>>>>>>  693                 self.api.env.basedn,
>>>>>>>>>>  694                 filter=filter,
>>>>>>>>>>  695                 attrs_list=['member'],
>>>>>>>>>>  696                 size_limit=-1, # paged search will get
>>>>>>>>>> everything
>>>>>>>>>> anyway
>>>>>>>>>>  697                 paged_search=True)
>>>>>>>>>>
>>>>>>>>>> which to me seems kind of important if the environment 
>>>>>>>>>> size_limit
>>>>>>>>>> is not
>>>>>>>>>> set properly :) The patch does not fix the non-propagation of 
>>>>>>>>>> the
>>>>>>>>>> paged_search, though.
>>>>>>>>>
>>>>>>>>> Why do you think size_limit is not used properly here?
>>>>>>>> AFAIU it is desired that the search is unlimited. However, due 
>>>>>>>> to the
>>>>>>>> fact that neither size_limit nor paged_search are passed from
>>>>>>>> ldap2.get_entries() to ldap2.find_entries() (methods inherited 
>>>>>>>> from
>>>>>>>> LDAPClient), only the number of records specified by
>>>>>>>> ipaSearchRecordsLimit is returned. That could eventually cause
>>>>>>>> problems
>>>>>>>> should ipaSearchRecordsLimit be set to a low value as in the 
>>>>>>>> ticket.
>>>>>>>
>>>>>>> I see. This is *not* intentional, the **kwargs of get_entries()
>>>>>>> should be passed to find_entries(). This definitely needs to be 
>>>>>>> fixed.
>>>>>>>
>>>>>>>>>
>>>>>>>>> Anyway, this ticket is not really easily fixable without more
>>>>>>>>> profound
>>>>>>>>> changes. Often, multiple LDAP searches are done during command
>>>>>>>>> execution. What do you do with the size limit then? Do you 
>>>>>>>>> pass the
>>>>>>>>> same size limit to all the searches? Do you subtract the 
>>>>>>>>> result size
>>>>>>>>> from the size limit after each search? Do you do something 
>>>>>>>>> else with
>>>>>>>>> it? ... The answer is that it depends on the purpose of each
>>>>>>>>> individual LDAP search (like in get_memberindirect() above, we
>>>>>>>>> have to
>>>>>>>>> do unlimited search, otherwise the resulting entry would be
>>>>>>>>> incomplete), and fixing this accross the whole framework is a
>>>>>>>>> non-trivial task.
>>>>>>>>>
>>>>>>>> I do realize that the proposed fix for the permission plugin is 
>>>>>>>> not
>>>>>>>> perfect, it would probably be better to subtract the number of
>>>>>>>> currently
>>>>>>>> loaded records from the sizelimit, although in the end the 
>>>>>>>> number of
>>>>>>>> returned values will not be higher than the given size_limit.
>>>>>>>> However,
>>>>>>>> it seems reasonable that if get_entries is passed a size limit, it
>>>>>>>> should apply it over current ipaSearchRecordsLimit rather than
>>>>>>>> ignoring
>>>>>>>> it. Then, any use of get_entries could be fixed accordingly if
>>>>>>>> someone
>>>>>>>> sees fit.
>>>>>>>>
>>>>>>>
>>>>>>> Right. Anyway, this is a different issue than above, so please put
>>>>>>> this into a separate commit.
>>>>>>>
>>>>>> Please see the attached patches, then.
>>>>>>
>>>>> Self-NACK, with Honza's help I found there was a mistake in the 
>>>>> code. I
>>>>> also found an off-by-one bug which I hope I could stick to the 
>>>>> other two
>>>>> patches (attaching only the modified and new patches).
>>>>
>>>> Works for me, but this bit in patch 0064 looks suspicious to me:
>>>>
>>>> +                        if max_entries > 0 and len(entries) ==
>>>> max_entries:
>>>>
>>>> Shouldn't it rather be:
>>>>
>>>> +                        if max_entries > 0 and len(entries) >=
>>>> max_entries:
>>>>
>>>> ?
>>>>
>>> The length of entries list should not exceed max_entries if size_limit
>>> is properly implemented. Therefore the list you get from execute()
>>> should not have more then max_entries entries. You shouldn't also be
>>> able to append a legacy entry to entries list if this check is the 
>>> first
>>> thing you do.
>>
>> That's a lot of shoulds :-) I would expect at least an assert 
>> statement to make sure everything is right.
>>
>>>
>>> That being said, >= could be used but then some popping of entries from
>>> entries list would be necessary. But it's perhaps safer to do, although
>>> if there's a bug, it won't be that obvious :)
>>
>> OK, nevermind then, just add the assert please, to make bugs *very* 
>> obvious.
>>
> An assert seems like a very good idea, attached is an asserting patch.
>
>
>
Attached is the patch rebased on the current master. Renumbered it a bit 
as previous numbers could've been confusing so I also omitted the 
revision number.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20161121/e218c40b/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0065-Make-get_entries-not-ignore-its-limit-arguments.patch
Type: text/x-patch
Size: 1711 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20161121/e218c40b/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0066-fix-permission_find-fail-on-low-search-size-limit.patch
Type: text/x-patch
Size: 1595 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20161121/e218c40b/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-slaznick-0067-permission-find-fix-a-sizelimit-off-by-one-bug.patch
Type: text/x-patch
Size: 2645 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20161121/e218c40b/attachment-0002.bin>

More information about the Freeipa-devel mailing list