nextcloud / server

☁️ Nextcloud server, a safe home for all your data
https://nextcloud.com
GNU Affero General Public License v3.0
27.38k stars 4.07k forks source link

SAML SSO with LDAP Backend userExists quite messy #16173

Closed mrvanes closed 4 years ago

mrvanes commented 5 years ago

I successully configured user_saml to authenticate against an IdP and wanted to prevent login if user does not exist in our LDAP, which can be achieved with the "Only allow authentication if an account exists on some other backend. (e.g. LDAP)" setting (invoking userExists on the LDAP backend). This is where the problems start.

I configured the LDAP backend to correctly find my users in the "Users" and "Login Attributes" tab of the LDAP settings. Monitoring the slapd.log of my LDAP server I see correct searches for "(&(objectClass=inetOrgPerson)(uid=student1))" when I test the access for student1 in the Login Attributes test box.

On succesfull SAML authentication however, I get an error saying that "Auto provisioning not allowed and user student1 does not exist" in the logs. It turns out the LDAP backend now queries the LDAP for "(&(objectClass=inetOrgPerson)(cn=*)(cn=student1*))". That is not correct and not what I configured for User Login?

It turns out, that the Advanced settings/Directory Settings points "User Display Name Field" to cn, which inexplicably triggers this behaviour (why would the Display Name hold the user ID?). If I then add "User Search Attributes" value uid, the query changes to "(&(objectClass=inetOrgPerson)(cn=*)(uid=student1*))". This is still wrong as I'm not interrested in the cn being set and searching for the uid using a wildcard seems completely wrong because I DON'T want to match any other user sharing the same student1 prefix, only an exact match should suffice?

Then, I'm still unable to login, because in the "Expert" settings I need to set the "Internal Username Attribute" field to uid, because for some reason in this setup ldap_user_mapping doesn't work? I can see the mapping table is correctly setup using the entryUUID of the user, but login won't correctly match it unless I tell user_ldap to use uid as "Internal Username Attribute".

Then, I'm still unable to login, because I need to set "UUID Attribute for Users" to uid and "UUID Attribute for Groups" to cn. If I now clear the LDAP user mapping, I can successfully login using the SAML auth plugin, while enabling the condition of having a valid user in LDAP.

I would say this setup could use a little love, to say the least.

Nevertheless, I'm happy I was able to create a working setup, so there is a work-around. It's just not very intuitive to setup.

Nextcloud version: 16.0.1.1 SSO & SAML authentication: 2.3.1 LDAP user and group backend: 1.6.0 SAML setup: native SP, using urn:mace:dir:attribute-def:uid as the attribute containing the user identifier. Both "Only allow authentication if an account exists on some other backend. (e.g. LDAP)" and "Allow the use of multiple user back-ends (e.g. LDAP)" enabled.

I known the user_saml plugin is a separate project, but since the "bugs" arrise from the userExists implementation in the LDAP backend I thought the issue should be filed here.

aignerat commented 5 years ago

I'd recommend to use sAMAccountname as username, if you switch the group of a user, you can get unwanted duplicated users like: user_1234. Depends on your organisation but I had this problem some times. Did you look up the saml_response if the mapping was correct? On most SSO-Implementations the mapping takes longer than any other task, especially because adfs-versions differ a lot.

mrvanes commented 5 years ago

The LDAP server is an OpenLDAP instance and the problem has nothing to do with user mapping. That works like a charm. The problem is there is unnecessary duplication of ldap configuration in the context of authentication or authorization backend and incorrect naming of these parameters.

aignerat commented 5 years ago

Ok, now I understood what you mean, thanks for clarifying. You think the code can be cleaner and there are unnecesary auth-requests from user_ldap. You have the latest app, maybe you can look up 1.7.0 for NC17 and make recommendations or even create a pullrequest. I know of problems that can trigger internal server-errors with ldap-remnants. The getdisplayname-function seems to trigger some problems with the userlist. I sadly couldn't figure out how to solve the problem in the code, atm I have to delete remnants that trigger errors.

mrvanes commented 5 years ago

Background requirement: We need users to exist in LDAP for a SAML authentication to succeed.

Now, the problem is, the LDAP plugin has 6 tabs for LDAP configuration. 4 of those (Server | Users | Login Attributes | Groups) are used to authenticate users when LDAP is used stand-alone. When LDAP is used in conjunction with SAML (to check if the user has a valid SAML uid) I would expect the USER configuration to help me find (but NOT authenticate) the requested SAML uid. This is not the case. I need to tweak Advanced and Expert settings to make that happen, even if I can find the requested SAML uid in the USER config.

The names Advanced and Expert are misleading in the sense that they suggest more finegrained configation of what is configured in the four prior tabs Server | Users | Login Attributes | Groups, but that isn't true. The Advanced and Expert tabs configuration seems used (completely disjunct from Login) for SAML uid confirmation.

That is quite unexpected and confusing, and thus the broader message in my issue. Apart from that, the Advanced and Expert settings need to be tweaked more than necessary because of strange default assumptions.

My point is: as far as I understand the configurtation, for me everything could have been done using Server | Users | Login Attributes | Groups for both authentication and SAML authorization flow, but for some reason an extra layer of complication was introduced, hence the subject of my issue "quite messy".

aignerat commented 5 years ago

ok, so you mean the UX could be improved. From my point of view you're partly right, but the extratab advanced holds information about quota, TTL, chunking etc. that's not needed for user_saml/SSO. Only the expert-tab - internal Username is needed for SSO, correct me if I'm mistaken. I think you're right in some points, the structure could be better, but it's far from awful.

mrvanes commented 5 years ago

The Avanced tab "User Display Name Field" is used to create the user lookup query (&(objectClass=inetOrgPerson)(cn=*)(cn=student1*)) which is wrong. In the Login Attributes tab I have set LDAP query to (&(objectclass=inetOrgPerson)(uid=%uid)), which should suffice. It's unexpected the "User Display Name Field" (cn) is then used to check existence of the uid in the LDAP backend? Also, the wildcard behind student1* introduces a security risk when two users with the same prefix exist. This query would allow student1 to login when only student12 exists in LDAP. That's more than a UX problem imho?

aignerat commented 5 years ago

yeah you're right, the wildcard is definitly no UX-problem but a security risk.

kesselb commented 4 years ago

cc @nextcloud/ldap

blizzz commented 4 years ago

With SAML authentication does not go via LDAP. The IdP takes care of it and only submits us a bunch of attributes, including a user id.

When you choose that a user must exist in another backend, Nextcloud needs to know the user.

If a user logs in for the first time, they might not be known in Nextcloud yet. Thus a search query is run against LDAP, to ensure a valid login will succeed. Auth as the user against LDAP will not and can not happen.

mrvanes commented 4 years ago

@blizzz I know how SAML works. I was just pointing out the configuration mess that is necessary in the LDAP back-end to make this work in conjunction with the SAML plugin. Apart from that, the bug I mention is real and should be fixed as soon as possible.

aignerat commented 4 years ago

@mrvanes one more question to be clear: If the user student1 doesn't exist in LDAP, there should be no way to log in as this user. Do you want to point out a vulnerability to bring up LDAP-Queries for user-lookup? At the moment I'm not totally sure if I understand your problem correctly. And don't forget to use SAMAccountname or userPrinicipalName in the expert-tab, that way you can't get duplicated users when a deactivated user is moved to another group and reactivated afterwards.

mrvanes commented 4 years ago

What I'm saying is that the identifier filter is too lax to be 100% safe. The authorization LDAP does not necessarily need to be the authentication LDAP, they may be cascaded and have periodic synchronisation e.g. Checking if uid=student1 is allowed to log in, while uid=student12 exists and student1 doesn't should never succeed. The wildcard should be removed from the lookup.

aignerat commented 4 years ago

Yeah it doesn't need to be, but if you don't do it that way it's insecure anyway to my mind. If you got no LDAP-Backend in your organisation you have to think about other ways to restrict the user-permissions. What i get from your comments, is that you plan to bring all users that should be eligable for login to the LDAP-Query, I don't think that this is good practice. Maybe line up your usecase and possible security problems you have with that scenario to get a better help, I'm not sure if this works, but maybe you can use \' to prevent the wildcard as a workaround.

blizzz commented 4 years ago

What I'm saying is that the identifier* filter is too lax to be 100% safe.

Safe of what?

mrvanes commented 4 years ago

What I'm saying is that the identifier* filter is too lax to be 100% safe.

Safe of what?

Safe of letting someone access what shouldn't be accessed? The scenario is that SAML is only authenticating the user but LDAP is the authorization source. In that case it is imperative that the user lookup exactly matches the SAML uid attribute, not a wildcard match.

blizzz commented 4 years ago

What I'm saying is that the identifier* filter is too lax to be 100% safe.

Safe of what?

Safe of letting someone access what shouldn't be accessed?

Doesn't has a say in it.

The scenario is that SAML is only authenticating the user but LDAP is the authorization source. In that case it is imperative that the user lookup exactly matches the SAML uid attribute, not a wildcard match.

It does. The wildcard search is only to ensure potential users are known to Nextcloud.

mrvanes commented 4 years ago

The scenario is that SAML is only authenticating the user but LDAP is the authorization source. In that case it is imperative that the user lookup exactly matches the SAML uid attribute, not a wildcard match.

It does. The wildcard search is only to ensure potential users are known to Nextcloud.

I may completely misunderstand the value of the added LDAP check on top of SAML, but my assumption is that the SAML plugin and the LDAP check are logically AND'ed to allow a user to access the nextcloud service. How does knowing that a user student12 exists (as a result of the wildcard search) help to prevent user student1 from logging in?

blizzz commented 4 years ago

I may completely misunderstand the value of the added LDAP check on top of SAML, but my assumption is that the SAML plugin and the LDAP check are logically AND'ed to allow a user to access the nextcloud service.

Correct so far

How does knowing that a user student12 exists (as a result of the wildcard search) help to prevent user student1 from logging in?

Student12 is irrelevant.

  1. We get a user id from SAML IdP. Say student1
  2. We need to ensure the incoming user id exists on one of the connected backends like LDAP
    1. We check whether such a user id exists within our backends
    2. If not, we execute a search against the backends (including LDAP) for the user id we got from the IdP. After all, it might be just a fresh user we are not aware of. Searches against the backends typically include wildcards and might go over more than one attribute.
    3. If a new user record was found, a mapping is created locally and the Nextcloud user id assigned. It may create user records for student1, student154 or student1464379
    4. We check again whether student1 is known
  3. If student1 is known login is granted

To be precise "known" is not enough, but it must be a legit user depending on the constraints set up in the LDAP configuration. These checks happen as well.

Now you can argue 2.ii can be optimized (and it can), for know this is a way that works backend agnostic. Yes, we may want to extend the user backend layer to provide a means to find a user specifically and allow backends to offer configuration for this in future. For now it works, so it is not an urgent field. If someone wants to work on it, I can give guidance.

mrvanes commented 4 years ago

Thank you for clarifying that. As long as we can rest assured that non-existent LDAP id's can't access resources even if the SAML auth succeeded, we're ok! I was just alarmed by the wildcard check as you can see.

blizzz commented 4 years ago

As long as we can rest assured that non-existent LDAP id's can't access resources even if the SAML auth succeeded, we're ok!

We can. We also enforce case sensitivity of user ids btw (which an LDAP search wouldn't necessarily also without the wildcard), someone stumbled over it not too long ago ;)

aignerat commented 4 years ago

Now I fully understood the problem.

No LDAP-comparison for SAML-Request -> no user found in LDAP -> new user (you still need to get a positive SAML-Request)

For Data: There is no real "everyone"-group, but if you have a cronjob, an adjustment for new users or a user that sets group it could lead to unwanted access. If you can assure that doesn't happen accidently you are safe.

For Apps: Similar, possible security risk if you use open circles

The risk is very low, but it's not 0. Depends on your IDP if it's only possible to login with vpn/internal or even external.

blizzz commented 4 years ago

No LDAP-comparison for SAML-Request -> no user found in LDAP -> new user (you still need to get a positive SAML-Request)

Depends on you config. If you disable auto-provisioning, there well be no new user and no login.

aignerat commented 4 years ago

Correct, thx for clarifying this. Just wanted to point out what possible security issues could exist as there is not much documentation on sso/saml-plugin.

mrvanes commented 4 years ago

Which still leaves the original "issue" that the configuration of the LDAP requirement was quite daunting to the non initiated. Like I said: for LDAP to be a fully functional backstop I could have gotten away with just configuring it as the primary authentication plugin (tell LDAP how to find a unique user, based on the primary attribute) and use that as the requirement, but I had to go to "advanced/expert" setting to reconfigure the same to reach my goal? See my original rant on top for more details.

blizzz commented 4 years ago

Yes, it is complicated, alone because every LDAP is different, and we can guess some things, but not everything. If you like to use something that works out of the box, take the UCS app. There, everything is preconfigured (but of course requires you to have everything within Univention Corporate Server).

aignerat commented 4 years ago

even every ADFS-version (Microsoft-iDP) has differences that can even require minor adjustments in the user_saml-app https://rephlex.de/blog/2018/04/05/how-to-connect-nextcloud-to-active-directory-using-ad-fs-without-losing-your-mind/ maybe this guide can help you, it's the most complete with insight that I could find up to now.

mrvanes commented 4 years ago

Thx for trying to help me out but again, neither SAML, nor LDAP is my problem. I try to convey in a friendly manner that it's counter intuitive to have a 100% working LDAP login configuration (find the user, bind using pwd) does not seem to do the job when configured as SAML backstop (find the user, stop if not found). You have to see the similarities in the "find the user" part? But I needed to duplicate settings in the back-alleys of the configuration to make it do the same thing it already did very well for login?

aignerat commented 4 years ago

I ran into an interesting problem, in advanced tab/connection settings you have the option configuration active, after that you see backup, if you turn off the "configuration active" you might assume the backup-server isn't active, but in fact the whole LDAP-Configuration is inactive. To my mind this checkbox is absolutely at a misleading area. Still the tabs are called advanced/expert, so you should exactly know what you do, when you change something in there. You're right, the internal username in expert-tab that's needed should be at least pointed out somewhere.

blizzz commented 4 years ago

you can configure the IdP to also return the UUID of the LDAP entry (as it would work with default settings). This is also being used in the wild.

aignerat commented 4 years ago

uuid can make problems when you reactivate a user that has been moved to another group, the user is duplicated then, that's why userPrincipalName and SAMAccountname seem to be a better choice.

blizzz commented 4 years ago

UUIDs (not DNs) are stable, unless you delete or newly create a user. Or it might happen when migrating servers. Those others actually change occasionally (typical scenario: name change after marriage) and then you have a mismatch, because user ids in Nextcloud are immutable.

rtheys commented 3 years ago

Hi,

We're in the process of adding SAML authentication to our existing nextcloud which currently has LDAP authentication.

Since we never explicitly configured the attribute to use for the internal username it's currently based on the entryUUID of the user in LDAP.

We want SAML authentication to only allow if the user is known in another database (LDAP). To get this to work with the entryUUID as the internal username, we return a SAML attribute that contains the entryUUID from LDAP and configure this attribute in the nextcloud SAML settings. This way, existing users can log in using SAML as they are already in the database with that internal username.

If a user is not (yet) in the database, nextcloud performs an LDAP search for the specified user. Since the username is the entryUUID, we've added the entryUUID field to the list of fields to search for users. As already mentioned in this ticket, this results in an LDAP search for the configured fields with a * appended to make it a substring search.

However the entryUUID field in LDAP does not allow substring searches, so it returns no results, and the user is not found. If the LDAP search would search for the username without the * appended to it, the LDAP search would work and the user would be found.

For example:

ldapsearch -x entryUUID=f9af84f1-b87a-4266-864f-904e5fbe4a40 entryUUID => This returns the entry with this ID

ldapsearch -x entryUUID=f9af84f1-b87a-4266-864f-904e5fbe4a40* entryUUID => This does not return any entries.

Unless I'm missing something it does not seem to be possible to get this to work if the defaults are used for the LDAP settings (use the entryUUID as the internal username)?

Regards, Rik

uka-support commented 2 years ago

Just to add another datapoint. I just struggled with a similar issue (user not provisioned) even though I got a working LDAP backend against Active Directory (using sAMAccountname as UID, that is "LDAP/AD username" in LDAP config and (samaccountmame=%uid) in LDAP query).

I worked around the issue by telling user_saml to user sAMAccountname, but passing objectGUID as that attribute from ADFS. Pretty counter-intuitive but it works (and hopefully keeps working).