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

ensure that objectguid or guid really return binary data before converting it #8317

Closed
fleish opened this issue Apr 23, 2014 · 34 comments
Closed

Comments

@fleish
Copy link

fleish commented Apr 23, 2014

I am unable to login to owncloud using certain ldap usernames that contain a period. I get:
{"app":"core","message":"Login failed: user 'some.username' , wrong password, IP:set log_authfailip=true in conf","level":2,"time":"2014-04-23T00:26:24+00:00"}

updating this to say it's not all ldap users with periods in their names. but a good number of them fail to login and do not even show up in the user list. however, when I look at the owncloud ldap configuration the user filter shows 61 users found which includes all of the users with periods in their names.

another update to say I figured out the cause of my issue. the problem users were not in the sqlite3 owncloud.db for oc_ldap_user_mapping & oc_group_user. not sure why these users were not added into there by owncloud automatically though so not closing this just yet. hopefully @blizzz or someone can shed some light.

@blizzz

@fleish fleish changed the title owncloud does not recognize ldap users with periods in their usernames owncloud does not recognize some ldap users with periods in their usernames Apr 23, 2014
@blizzz
Copy link
Contributor

blizzz commented Apr 23, 2014

do they have the configured display name attribute set?

@fleish
Copy link
Author

fleish commented Apr 23, 2014

assuming you mean cn in ldap, yes

@blizzz
Copy link
Contributor

blizzz commented Apr 23, 2014

Not necessarily, I mean the attribute defined in User Display Name Field: (http://doc.owncloud.org/server/6.0/admin_manual/configuration/auth_ldap.html#directory-settings)

@fleish
Copy link
Author

fleish commented Apr 23, 2014

Yes, that's the one we have set. All users have a cn defined in ldap

@blizzz
Copy link
Contributor

blizzz commented Apr 23, 2014

Interesting. Anything in the ownCloud log file? What is your user filter? You said you use an sqlite database – is this for evaluation only? For couple more users we strongly recommend to use MySQL or Postgres.

@fleish
Copy link
Author

fleish commented Apr 23, 2014

the only thing in the log that looks interesting/related was this: {"app":"PHP","message":"ldap_search(): Partial search results returned: Sizelimit exceeded at /var
/www/owncloud/apps/user_ldap/lib/ldap.php#141","level":3,"time":"2014-04-23T01:44:50+00:00"}

This started out as for evaluation, but quickly became popular so we have more users trying to use it now. Is there a procedure for converting form sqlite to MySQL that won't break all our existing users and their files?

EDIT: user filter is inetOrgPerson

@blizzz
Copy link
Contributor

blizzz commented Apr 24, 2014

A command line tool for migration is in the working: #6457

Could you find out whether the issues exists with the upcoming 6.0.3? It's an RC yet so not recommended for production usage. http://mailman.owncloud.org/pipermail/testpilots/2014-April/000132.html

@PVince81
Copy link
Contributor

PVince81 commented Dec 5, 2014

Are you still seeing this in OC 7.0.3 ?

@fleish
Copy link
Author

fleish commented Dec 9, 2014

Since discovering the cause of this issue being the missing entries in the sqlite database, I have taken to inserting rows manually when new users are created in LDAP. I'm not yet running 7.0.3, but would want to know if the expected behavior in that release is the same? Once I upgrade (and afterwards convert from sqlite to mysql), will I still be required to insert rows manually into my OC database when new users are added to LDAP?

@blizzz
Copy link
Contributor

blizzz commented Mar 23, 2015

@fleish did you upgrate to 7 and swtich to MySQL?

@fleish
Copy link
Author

fleish commented Mar 24, 2015

@blizz I did upgrade using a funky path, details here: #12801 (comment)

I assume I still have to manually add new LDAP users to the mysql database which is what I've been doing. If that's correct & expected we can probably close this.

@blizzz
Copy link
Contributor

blizzz commented Mar 24, 2015

Manually adding rows to the database is never expected, actually.

@blizzz
Copy link
Contributor

blizzz commented Jun 16, 2015

@fleish does it work better with mysql now?

@blizzz blizzz added this to the 8.2-next milestone Jun 16, 2015
@fleish
Copy link
Author

fleish commented Jun 16, 2015

@blizzz I haven't had any complaints since moving to mysql. we do still have to manually insert a row into the db for each new LDAP user. It would be nice if OC automatically checked the LDAP server for new users periodically IMO, but we're used to doing it now so it's not a big burden.

@blizzz
Copy link
Contributor

blizzz commented Jun 17, 2015

This is what is supposed to do. Just yesterday I tried again with both periods and commas in the DN and it worked fine. However I also have another report who has troubles.

Do run it against OpenLDAP, AD or another server?

@blizzz
Copy link
Contributor

blizzz commented Jun 17, 2015

Also, could you paste your LDAP configuration?

@blizz
Copy link

blizz commented Jun 17, 2015

Please remove me from the CC line.
Brett

 On Wednesday, June 17, 2015 5:34 AM, blizzz <[email protected]> wrote:

Also, could you paste your LDAP configuration? —
Reply to this email directly or view it on GitHub.

@fleish
Copy link
Author

fleish commented Jun 17, 2015

@blizzz are you saying OC is supposed to sync with LDAP or are you referring to periods in usernames? for clarity, the periods in usernames issue hasn't been observed in a long time now. I think it was probably user error being masked as something else.

We are using OpenLDAP. Configuration below:
+------------------------------+---------------------------------------------+
| Configuration | |
+------------------------------+---------------------------------------------+
| hasMemberOfFilterSupport | |
| hasPagedResultSupport | |
| homeFolderNamingRule | |
| lastJpegPhotoLookup | 0 |
| ldapAgentName | |
| ldapAgentPassword | *** |
| ldapAttributesForGroupSearch | |
| ldapAttributesForUserSearch | |
| ldapBackupHost | |
| ldapBackupPort | |
| ldapBase | dc=domain,dc=org |
| ldapBaseGroups | dc=domain,dc=org |
| ldapBaseUsers | dc=domain,dc=org |
| ldapCacheTTL | 5 |
| ldapConfigurationActive | 1 |
| ldapEmailAttribute | |
| ldapExperiencedAdmin | 0 |
| ldapExpertUUIDGroupAttr | |
| ldapExpertUUIDUserAttr | UUID |
| ldapExpertUsernameAttr | |
| ldapGroupDisplayName | cn |
| ldapGroupFilter | (|(cn=owncloud)) |
| ldapGroupFilterGroups | |
| ldapGroupFilterMode | 0 |
| ldapGroupFilterObjectclass | |
| ldapGroupMemberAssocAttr | memberUid |
| ldapHost | ldaps://ldap.domain.org/ |
| ldapIgnoreNamingRules | |
| ldapLoginFilter | (&(|(objectclass=inetOrgPerson))(uid=%uid)) |
| ldapLoginFilterAttributes | |
| ldapLoginFilterEmail | 0 |
| ldapLoginFilterMode | 1 |
| ldapLoginFilterUsername | 1 |
| ldapNestedGroups | 0 |
| ldapNoCase | 0 |
| ldapOverrideMainServer | 0 |
| ldapPagingSize | 500 |
| ldapPort | 636 |
| ldapQuotaAttribute | |
| ldapQuotaDefault | |
| ldapTLS | 0 |
| ldapUserDisplayName | cn |
| ldapUserFilter | (|(objectclass=inetOrgPerson)) |
| ldapUserFilterGroups | |
| ldapUserFilterMode | 1 |
| ldapUserFilterObjectclass | inetOrgPerson |
| ldapUuidGroupAttribute | auto |
| ldapUuidUserAttribute | auto |
| turnOffCertCheck | 0 |
+------------------------------+---------------------------------------------+

@blizz I don't see where you were tagged here for cc, nor do I see a way for me to remove you. You should be able to unsubscribe from notifications by visiting the github thread directly here: #8317

@blizz
Copy link

blizz commented Jun 17, 2015

Am on the CC line.  Do I need to log into github to unsubscribe?  Please know that I have never subscribed to any of these hundreds of messages that are often filling up my inbox and notifying my cell.
Brett

fleishToowncloud/coreCCblizzToday at 10:37 AM@blizzz are you saying OC is supposed to sync with LDAP or are you referring to periods in usernames? for clarity, the periods in usernames issue hasn't been observed in a long time now. I think it was probably user error being masked as something else.We are using OpenLDAP. Configuration below:
+------------------------------+---------------------------------------------+

 On Wednesday, June 17, 2015 10:37 AM, fleish <[email protected]> wrote:

@blizzz are you saying OC is supposed to sync with LDAP or are you referring to periods in usernames? for clarity, the periods in usernames issue hasn't been observed in a long time now. I think it was probably user error being masked as something else.We are using OpenLDAP. Configuration below:
+------------------------------+---------------------------------------------+
| Configuration | |
+------------------------------+---------------------------------------------+
| hasMemberOfFilterSupport | |
| hasPagedResultSupport | |
| homeFolderNamingRule | |
| lastJpegPhotoLookup | 0 |
| ldapAgentName | |
| ldapAgentPassword | *** |
| ldapAttributesForGroupSearch | |
| ldapAttributesForUserSearch | |
| ldapBackupHost | |
| ldapBackupPort | |
| ldapBase | dc=domain,dc=org |
| ldapBaseGroups | dc=domain,dc=org |
| ldapBaseUsers | dc=domain,dc=org |
| ldapCacheTTL | 5 |
| ldapConfigurationActive | 1 |
| ldapEmailAttribute | |
| ldapExperiencedAdmin | 0 |
| ldapExpertUUIDGroupAttr | |
| ldapExpertUUIDUserAttr | UUID |
| ldapExpertUsernameAttr | |
| ldapGroupDisplayName | cn |
| ldapGroupFilter | (|(cn=owncloud)) |
| ldapGroupFilterGroups | |
| ldapGroupFilterMode | 0 |
| ldapGroupFilterObjectclass | |
| ldapGroupMemberAssocAttr | memberUid |
| ldapHost | ldaps://ldap.domain.org/ |
| ldapIgnoreNamingRules | |
| ldapLoginFilter | (&(|(objectclass=inetOrgPerson))(uid=%uid)) |
| ldapLoginFilterAttributes | |
| ldapLoginFilterEmail | 0 |
| ldapLoginFilterMode | 1 |
| ldapLoginFilterUsername | 1 |
| ldapNestedGroups | 0 |
| ldapNoCase | 0 |
| ldapOverrideMainServer | 0 |
| ldapPagingSize | 500 |
| ldapPort | 636 |
| ldapQuotaAttribute | |
| ldapQuotaDefault | |
| ldapTLS | 0 |
| ldapUserDisplayName | cn |
| ldapUserFilter | (|(objectclass=inetOrgPerson)) |
| ldapUserFilterGroups | |
| ldapUserFilterMode | 1 |
| ldapUserFilterObjectclass | inetOrgPerson |
| ldapUuidGroupAttribute | auto |
| ldapUuidUserAttribute | auto |
| turnOffCertCheck | 0 |
+------------------------------+---------------------------------------------+@blizz I don't see where you were tagged here for cc, nor do I see a way for me to remove you. You should be able to unsubscribe from notifications by visiting the github thread directly here: #8317
Reply to this email directly or view it on GitHub.

@LukasReschke
Copy link
Member

@blizz

2015-06-17_20-06-09

@blizzz
Copy link
Contributor

blizzz commented Jun 18, 2015

@fleish

are you saying OC is supposed to sync with LDAP or are you referring to periods in usernames?

ownCloud is getting it's users live form LDAP, unless cache is in effect (10min per default, but yours is set to 5sec. So it's working pretty much online for you). Known users are mapped in the database table. A constraint is that users to be fetched must have the display name attribute set, but we ensured it is in your case.

About periods in usernames I don't know, but they should not be an issue anyway.

In your configuration i've seen this:

| ldapExpertUUIDUserAttr | UUID |

That is most likely the issue here. UUID itself is not an attribute, thus ownCloud is not able to read it and will not map the users. When you inserted the user manually into the DB table, what did you set for directory_uuid?

This value is checked against the UUID attribute (either set or auto-detected) in case a DN changes to be able to find the user again.

@blizzz blizzz added Type:Bug and removed Type:Bug labels Jun 18, 2015
@fleish
Copy link
Author

fleish commented Jun 18, 2015

@blizzz we modified our directory to have an attribute named UUID for each user for OC and other things that require it. when we insert a user into the table, we provide the user's UUID from our LDAP like so:
INSERT INTO oc_ldap_user_mapping VALUES('uid=userfirst.userlast,dc=domain,dc=org','49bda228-6cbf-4f12-a987-49b9ed08f6df','49bda228-6cbf-4f12-a987-49b9ed08f6df');

@blizzz
Copy link
Contributor

blizzz commented Jun 19, 2015

Does the attribute contain string or binary data?

@fleish
Copy link
Author

fleish commented Jun 19, 2015

String. And now that I'm looking closer at it, the attribute isn't UUID, it's GUID.

Going in the way-back part of my brain, I think GUID might have been setup for caldav initially and then OC piggy-backed onto it. I'm now thinking it was not necessary for the latter (and maybe even the former) since LDAP auto-generates "entryUUID" for every record anyway. We manually add GUID to each user we create which now seems like extra work over just re-using the entryUUID and telling apps to look for that instead of GUID.

Anyway, for OC in particular it seems like this should be set to GUID instead of UUID in our current setup. This also explains why some of our initial users UUID in OC does not match their GUID in ldap. These users are actually using their entryUUID instead of GUID value. So on second though, perhaps we cannot switch this value to GUID as it might break the old users who are setup like this.

@blizzz
Copy link
Contributor

blizzz commented Jun 19, 2015

That is the answer to that riddle.

If you would decide to switch from invalid UUID to GUID, old users are not affected from the change, as long as you don't clear the mappings (which you should not do in production for exactly this reason). In case a DN might change for a user, you would run into an issue right now as well. So changing it to GUID should be safe and help with new users. ⚠️ EXCEPT ⚠️ GUID is used by AD (i think) in a binary format (definitely) and we would attempt to convert it. Likely you get something useless out of it, in the worst case even same results for different users.

So don't do it right now (unless you have something else that is not called GUID or objectGUID, or just openLDAPs own entryuuid which would be used by default if you empty the setting).

@blizzz
Copy link
Contributor

blizzz commented Jun 19, 2015

Task on ownCloud is now to figure out whether it's possible to distinguish whether we get binary or string data from LDAP (I assume so) and to build in a check to avoid such happenings.

@blizzz blizzz changed the title owncloud does not recognize some ldap users with periods in their usernames ensure that objectguid or guid really return binary data before converting it Jun 19, 2015
@fleish
Copy link
Author

fleish commented Jun 22, 2015

@blizzz Doh! I just confirmed by removing UUID and new user logins have entries auto-created in DB with internal LDAP entryUUID. I suspect when I implemented GUID for our calendarserver I thought it would be good to have a single GUID across all services and mistakenly changed Owncloud to UUID instead of GUID.

And for sure something weird happens if set to GUID, I got a non-matching (and much longer) UUID inserted into the database along with some constraint violations logged (pasted below).

{"app":"hook","message":"error while running hook (OCA\Encryption\Hooks::login): An exception occurred while executing 'INSERT INTO oc_preferences (userid, appid, configkey, configvalue) VALUES (?, ?, ?, ?)' with params ["65646434-3664-3161-2D63-3334392D346232372D613339322D663862353164653437653361", "files_encryption", "migration_status", "0"]:\n\nSQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '65646434-3664-3161-2D63-3334392D346232372D613339322D663862353164' for key 'PRIMARY'","level":3,"time":"2015-06-22T19:52:25+00:00"}

{"app":"index","message":"Doctrine\DBAL\DBALException: An exception occurred while executing 'INSERT INTO oc_preferences (userid, appid, configkey, configvalue) VALUES (?, ?, ?, ?)' with params ["65646434-3664-3161-2D63-3334392D346232372D613339322D663862353164653437653361", "files_encryption", "migration_status", "0"]:\n\nSQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '65646434-3664-3161-2D63-3334392D346232372D613339322D663862353164' for key 'PRIMARY'","level":4,"time":"2015-06-22T19:52:25+00:00"}

{"app":"index","message":"Doctrine\DBAL\DBALException: An exception occurred while executing 'INSERT INTO oc_preferences (userid, appid, configkey, configvalue) VALUES (?, ?, ?, ?)' with params ["65646434-3664-3161-2D63-3334392D346232372D613339322D663862353164653437653361", "files_encryption", "migration_status", "0"]:\n\nSQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '65646434-3664-3161-2D63-3334392D346232372D613339322D663862353164' for key 'PRIMARY'","level":4,"time":"2015-06-22T19:52:27+00:00"}

@blizzz
Copy link
Contributor

blizzz commented Jun 22, 2015

And for sure something weird happens if set to GUID, I got a non-matching (and much longer) UUID inserted into the database along with some constraint violations logged (pasted below).

Because we assume that a binary value was retrieved and convert it. "Works" with string data apparently as well.

@PVince81
Copy link
Contributor

@fleish are you still seeing this in 8.1 ?

@blizzz what's the status ?

@blizzz
Copy link
Contributor

blizzz commented Sep 22, 2015

Moving to 9.0 since it's feature/enhancement

@blizzz blizzz modified the milestones: 9.0-next, 8.2-current Sep 22, 2015
@fleish
Copy link
Author

fleish commented Nov 9, 2015

@PVince81 I'm not yet on 8.1, but I can't imagine any behavior has changed unless the code/logic around the GUID/UUID value has changed. We'll probably be forced to continue setting this manually and creating users in the DB by hand until it is addressed in a future release since changing the GUID for all of our users so we could go back to using UUID seems like it'd have the potential to be a major issue.

@ghost ghost modified the milestones: 9.1-next, 9.0-current Feb 17, 2016
@PVince81 PVince81 modified the milestones: 9.1-current, 9.2-next Jun 14, 2016
@PVince81
Copy link
Contributor

@jvillafanez your thoughts ?

@PVince81
Copy link
Contributor

PVince81 commented Apr 6, 2017

likely obsolete as the code has changed

if the problem still exists in 9.1.4 or 10.0 beta, feel free to reopen

@PVince81 PVince81 closed this as completed Apr 6, 2017
@lock
Copy link

lock bot commented Aug 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants