Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove search limit for aci group evaluation #1038

Closed
389-ds-bot opened this issue Sep 12, 2020 · 23 comments
Closed

remove search limit for aci group evaluation #1038

389-ds-bot opened this issue Sep 12, 2020 · 23 comments
Labels
closed: fixed Migration flag - Issue
Milestone

Comments

@389-ds-bot
Copy link

Cloned from Pagure issue: https://pagure.io/389-ds-base/issue/47703


This is the more general solution for ticket47702. If groups are used in acis they should always be fully evaluated.

@389-ds-bot 389-ds-bot added the closed: fixed Migration flag - Issue label Sep 12, 2020
@389-ds-bot 389-ds-bot added this to the 1.2.11.33 milestone Sep 12, 2020
@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2015-06-19 21:37:09

Strange, ticket 47702 apparently does not exist - not sure how that is possible. Anyway, continuing investigation.

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2015-08-07 19:31:02

William wrote:

Why was the search limit added initially into the aci plugin? By deleting this
code, is there some other edge case that it may cause?

Reply:

It was old code, and it was probably there to keep aci evaluation from taking too long. However, acis should always be fully evaluated.

William wrote:

However, this test is clearly broken, as it still works even without your patch applied.

Reply:

Are you binding as Directory Manager? The "Directory Manager" is not processed by acls. You should binding as a "regular user" from the database if you want to test this fix.

Verification Steps:

[1] Add 10000 users (with passwords)
[2] Add these users to a single group
[3] Add an aci that allows anyone from that group to have all rights to the database.
[4] Bind as the last user listed in the group, and try and add a new entry
[5] If should succeed with the fix, and not without it.

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2015-08-07 19:48:00

My test sets the sizelimit low (set to N), creates N+1 users, then re-binds with a user and sees if the aci works.

I think the issue is that I'm not testing as the last user in the group. I'll rework the test.

I've attached the work in progress to the ticket.

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2015-08-07 19:52:30

example test
ticket47703_test.py

@389-ds-bot
Copy link
Author

Comment from firstyear (@Firstyear) at 2015-08-07 19:55:45

Still can't produce this error. I have attached a tweaked version of the test that attempts to bind every possible user to try and fail the aci. I've now also tested with:

MAX_USERS = 10000
SIZELIMIT = '2000'

Is there some specifics around the aci needed to cause the failure or is:

(targetattr ="uniqueMember")(targetfilter ="(cn=target)")(version 3.0;acl "Test ACI";allow (write)(groupdn = "ldap:///cn=testgroup,dc=example,dc=com");)

Followed by a test user in testgroup, writing to cn=target sufficient?

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2015-08-08 00:11:46

Yeah I'm not sure how to reproduce this. I've cc'ed Ludwig who worked on this initially and discovered the problem.

Ludwig, how do you reproduce this problem? Seems to work fine without any changes to the acl code.

Thanks,
Mark

@389-ds-bot
Copy link
Author

Comment from nhosoi (@nhosoi) at 2015-08-13 03:10:51

The patch 0001-Ticket-47703-remove-search-limit-for-aci-group-evalu.patch​ looks good to me.

I'm confused at this note:

This is the more general solution for ticket47702.

Was 47702 removed? If I try to open the ticket, I get:
Ticket 47702 does not exist.

If "47702" is a typo of some other ticket, was the original issue already fixed by that and that's why we could not reproduce the problem???

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2015-08-13 03:24:58

Replying to [comment:12 nhosoi]:

The patch 0001-Ticket-47703-remove-search-limit-for-aci-group-evalu.patch​ looks good to me.

I'm confused at this note:

This is the more general solution for ticket47702.

Was 47702 removed? If I try to open the ticket, I get:
Ticket 47702 does not exist.

I know and I can't find it either. Ludwig said it was deleted somehow. There was a bugzilla about this on the IPA side, but I can't find it.

If "47702" is a typo of some other ticket, was the original issue already fixed by that and that's why we could not reproduce the problem???

@389-ds-bot
Copy link
Author

Comment from nhosoi (@nhosoi) at 2015-08-25 04:36:52

Hi Mark, Hi Ludwig,

Do you think the patch 0001-Ticket-47703-remove-search-limit-for-aci-group-evalu.patch should not be pushed until the bug is successfully reproduced?

If so, can we push this bug to 1.3.5?

Thanks,
--noriko

@389-ds-bot
Copy link
Author

Comment from lkrispen (@elkris) at 2015-08-25 18:46:46

I will look into it and try to reproduce

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2015-08-25 21:47:16

Ludwig told me that this only applies to search operations, not "modify" operations. I had previously only tested delete operations. I will be testing searches later today...

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2015-08-26 20:26:53

Replying to [comment:16 mreynolds389]:

Ludwig told me that this only applies to search operations, not "modify" operations. I had previously only tested delete operations. I will be testing searches later today...

I still can not reprocuce the problem using searches(or modifies). Here is exactly what I did:

  • Set the size limit to 100
  • Added a group wih 50k members
  • Created a single aci that allows anyone from that group full access
  • Bound as the first member of the group and was able to search for a user
  • Bound as the last member of the group and was able to search for a user

Ludwig can you look into this and see if you can figure out how to reproduce the issue?

@389-ds-bot
Copy link
Author

Comment from nhosoi (@nhosoi) at 2016-05-13 00:18:00

Per triage, push the target milestone to 1.3.6.

@389-ds-bot
Copy link
Author

Comment from nhosoi (@nhosoi) at 2016-10-14 00:55:50

There was an error case reported in which sizelimit was set to 1 by some application.
I think we should push Ludwig's patch to the master and 1.3.5.

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2016-10-14 02:55:36

Replying to [comment:20 nhosoi]:

There was an error case reported in which sizelimit was set to 1 by some application.
I think we should push Ludwig's patch to the master and 1.3.5.

Its my patch, but I couldn't reproduce the issue. So I can't verify if the fix did anything. This simply needs to be revisited to see if there is an issue.

@389-ds-bot
Copy link
Author

Comment from nhosoi (@nhosoi) at 2016-10-15 02:28:12

Note: a customer is hoping to have this fix backported to 1.2.11 if it solves their problem.

@389-ds-bot
Copy link
Author

Comment from nhosoi (@nhosoi) at 2016-10-19 00:10:56

Input from Hiroko-san.

On 10/13/2016 10:35 AM, Hiroko Miura wrote:

Anyway, I could reproduce the similar issue when client specify sizelimit=1.

Here is my sample ldif/operation log/ access &errors logs.

http://mocha.nrt.redhat.com/dav-readonly/temp/hmiura/case01707132/example.ldif

http://mocha.nrt.redhat.com/dav-readonly/temp/hmiura/case01707132/operation.txt

http://mocha.nrt.redhat.com/dav-readonly/temp/hmiura/case01707132/access

http://mocha.nrt.redhat.com/dav-readonly/temp/hmiura/case01707132/errors

This happens just after restarting server.
It does not happen once client perform search without sizelimit.
I'm not sure if I could reproduce the customer's issue exactly.

But anyway, if client specify too small sizelimit in search operation
like this test case, search result is not correct.
Therefore, I think it would be better to set
aclpb_max_member_sizelimit to more safe value
like SLAPD_DEFAULT_LOOKTHROUGHLIMIT in acl_init_aclpb().

@389-ds-bot
Copy link
Author

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2016-10-20 01:54:20

1e1f6fe..3151648 master -> master
commit 3151648
Author: Mark Reynolds mreynolds389@redhat.com
Date: Wed Oct 19 15:50:15 2016 -0400

@389-ds-bot
Copy link
Author

Comment from nhosoi (@nhosoi) at 2016-10-20 08:01:24

Set the target milestone to 1.2.11.x.

Mark, could it be possible to backport to the branch?

1.3.5 is likely to have the fix, too.

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2016-10-20 14:14:23

9982033..99a34b4 389-ds-base-1.3.5 -> 389-ds-base-1.3.5
commit 99a34b4
Author: Mark Reynolds mreynolds389@redhat.com
Date: Wed Oct 19 15:50:15 2016 -0400

48ed4c8..68cc036 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 68cc036

c55e708..3fd372e 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 3fd372e

@389-ds-bot
Copy link
Author

Comment from mreynolds (@mreynolds389) at 2017-02-11 22:50:57

Metadata Update from @mreynolds389:

  • Issue assigned to mreynolds389
  • Issue set to the milestone: 1.2.11.33

@389-ds-bot
Copy link
Author

Comment from gparente (@germanparente) at 2017-06-14 17:54:20

Hi,

this ticket has been commit in 1.3.5 branch but it's not delivered into RHEL7.3 from what I have checked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed: fixed Migration flag - Issue
Projects
None yet
Development

No branches or pull requests

1 participant