#50137 DSLdapObject _create() doesn't work correctly for users with limited permissions
Closed: wontfix 2 years ago by mreynolds. Opened 3 years ago by vashirov.

Issue Description

DSLdapObject _create() method checks if the entry exists before creating it:
https://pagure.io/389-ds-base/blob/master/f/src/lib389/lib389/_mapped_object.py#_728

        try:
            self._instance.search_ext_s(dn, ldap.SCOPE_BASE, self._object_filter, attrsonly=1, serverctrls=self._server_controls, clientctrls=self._client_controls)
            exists = True
        except ldap.NO_SUCH_OBJECT:
            pass

But it does the search using the current bind, which can return different result errors depending on the permissions. For example, under DM if entry doesn't exist, result will be err=32, but under non-privileged user results will be err=0 (to prevent information disclosure?).

Package Version and Platform

389-ds-base-1.4.0.20-1.fc29.x86_64

Steps to reproduce

See attached reproducer
reproducer.py


I'm not quite sure I understand the problem you have in this scenario. It sounds like everything is working as intended but maybe I'm missing something? Is the issue that the non-priv user with err=0 then still tries to create the object and fails later in the script?

Metadata Update from @firstyear:
- Custom field component adjusted to None
- Custom field origin adjusted to None
- Custom field reviewstatus adjusted to None
- Custom field type adjusted to None
- Custom field version adjusted to None

3 years ago

I'm not quite sure I understand the problem you have in this scenario.

Have you tried the reproducer?

It sounds like everything is working as intended but maybe I'm missing something?

The logic is flawed for users with different permissions. For non-privleged users we hide the existence of the entry in case it is not found. And based on that script decides if it exist or not. And for non-privelged users the result will be always "exist".

Is the issue that the non-priv user with err=0 then still tries to create the object and fails later in the script?

No, it doesn't try to create it, because it thinks that user is already there, because ldapsearch returned success (err=0) in try block, when under DM it would return error (err=32, already exist).
Instead of relying on catching error in try block perhaps we should count number of entries returned? In this case if the entry doesn't exist it will be always 0 (since the search is done with basedn of the entry), and in case it is exist, it will be 1.

I can't try any reproducers as some of my patches are still in review to make installs work on my machine ... :( I've been doing some weird rebase-hacks to even submit the ones I have done .... :(

So does this mean we hide the existance of the entry, so create thinks "it does not exist" and then tries to create anyway?

Or that it reports there are no conflicting entries, then we try to create and it errors then?

So does this mean we hide the existance of the entry, so create thinks "it does not exist" and then tries to create anyway?

It think it exists already and doesn't proceed to create.

Or that it reports there are no conflicting entries, then we try to create and it errors then?

I linked the problematic code in the description. Here's with some more explanations:

     try:
            self._instance.search_ext_s(dn, ldap.SCOPE_BASE, self._object_filter, attrsonly=1, serverctrls=self._server_controls, clientctrls=self._client_controls)

Under DM we get err=32:

[root@server-f29 ds]# ldapsearch -D "cn=Directory Manager" -w password -h localhost -p 38901 -b "cn=user2,ou=People,dc=example,dc=com"
# extended LDIF
#
# LDAPv3
# base <cn=user2,ou=People,dc=example,dc=com> with scope subtree
# filter: (objectclass=*)
# requesting: ALL
#

# search result
search: 2
result: 32 No such object
matchedDN: ou=People,dc=example,dc=com

# numResponses: 1

But under non-priveleged user (that has an aci allowing to create entries):

[root@server-f29 ds]# ldapsearch -D "cn=user1,ou=People,dc=example,dc=com" -w password -h localhost -p 38901 -b "cn=user2,ou=People,dc=example,dc=com"
# extended LDIF
#
# LDAPv3
# base <cn=user2,ou=People,dc=example,dc=com> with scope subtree
# filter: (objectclass=*)
# requesting: ALL
#

# search result
search: 2
result: 0 Success

# numResponses: 1

So it succeeds and ldap.NO_SUCH_OBJECT is not caught:

            exists = True
        except ldap.NO_SUCH_OBJECT:
            pass

And because ensure is False by default and was not passed:

        elif exists and not ensure:
            # raise "already exists."
            raise ldap.ALREADY_EXISTS("Entry %s already exists" % dn)

HTH

I have a different view on this, when reading this I just realized that "create" tries to be smart and behave differently if the entry exists or not. I do not like this for different reasons:

if in a test I want to create an entry I want a convenient wrapper around add_s but nothing more, doing an additional search already adds another operation and changes the load profile.
if I do an add I want the result of the add, not the interpreted result of a search, if I try to add an entry which already exists I get an return code to handle, same if I do not have the permissions to add.

so in my opinion, the search part in create should not be there (and will not cause problems)

Metadata Update from @mreynolds:
- Issue set to the milestone: 1.4.0

3 years ago

@lkrispen I think that the ensure() function relies on the exist check, but I'll draft a patch that addresses this today

Metadata Update from @mreynolds:
- Issue close_status updated to: fixed
- Issue status updated to: Closed (was: Open)

2 years ago

389-ds-base is moving from Pagure to Github. This means that new issues and pull requests
will be accepted only in 389-ds-base's github repository.

This issue has been cloned to Github and is available here:
- https://github.com/389ds/389-ds-base/issues/3196

If you want to receive further updates on the issue, please navigate to the github issue
and click on subscribe button.

Thank you for understanding. We apologize for all inconvenience.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: fixed)

2 years ago

Login to comment on this ticket.

Metadata
Attachments 1
Attached 3 years ago View Comment