RocketChat / Rocket.Chat

The communications platform that puts data protection first.
https://rocket.chat/
Other
39.96k stars 10.3k forks source link

Disabled LDAP accounts can still login into Rocket Chat #4554

Closed doctorwho92 closed 1 year ago

doctorwho92 commented 7 years ago

Your Rocket.Chat version: (0.41.0)

Two issues in one:

  1. Disabling an user account in Active Directory does not prevent that user from being able to login to Rocket Chat;
  2. Disabling an user account in Active Directory does not terminate any existing Rocket Chat session for that user. Each authenticated session should perform periodic re-authentication, or the server should have an ldap listener and terminate the session(s) for any account which is no longer valid in the directory.
doctorwho92 commented 7 years ago

Would like to request that high priority be given to this issue, since it is a serious security issue.

Thanks in advance.

jamie-owen commented 7 years ago

@doctorwho92 Why did you close this? As you said, it's a pretty big problem for people using LDAP auth.

doctorwho92 commented 7 years ago

I didn't close it, or certainly not deliberately.

qk4l commented 7 years ago

Have anybody tried to disable LDAP > Login Fallback to prevent this security issue?

alexvranceanu commented 7 years ago

Tested using latest develop branch, still happening. Login Fallback will prevent disabled users from logging on, but it will not allow local users (such as admin) to login (as designed).

alexvranceanu commented 7 years ago

We're using this filter for LDAP logins, this allows subtree search and logins with username or email, for enabled users only: {"filter": "(&(objectClass=person)(|(userAccountControl=512)(userAccountControl=66048))(|(mail=#{username})(sAMAccountName=#{username})))", "scope": "sub", "userDN": "SERVICE_ACCOUNTS@your-company.com", "password": "HIDDEN"}

Details about userAccountControl values here: https://support.microsoft.com/en-us/kb/305144 and here: http://jackstromberg.com/2013/01/useraccountcontrol-attributeflag-values/

Perhaps this could be set as a default filter for a clean Rocket.Chat install? It will speed up adoption and ease the process of integrating with LDAP.

localguru commented 7 years ago

I can confirm the problem: a removed user from AD is still able to login as long as "LDAP/Login Fallback" is set to "True". If it's set to "False" the user can't login, but this disables the initial, local admin user too and one has to put another AD user to admin group the get admin access again:

db.users.update({username:'otheruser'}, {$set: {'roles' : [ "admin" ]}});

But what happens if one changes the LDAP configuration to something wrong and the LDAP connection is not working any more? In that case your are blocked completely.

LDAP_Login_Fallback workaround? db.rocketchat_settings.update({'_id':'LDAP_Login_Fallback'}, {$set: { 'value':true }})

If the LDAP/AD connection is fine and "Domain Search User" can log in, an user object that does not exist any more, to my mind this is not a case of LDAP_Login_Fallback.

Ciao!

gregs007 commented 7 years ago

I think doing what's suggested in #7235 should work for LDAP auth. Basically you would disable RC users that are disabled or missing in LDAP directory. This would be during regular LDAP database sync. I don't think regular re-auth is a good idea in the case of an unstable LDAP connection locking out users. Sync on the other hand, is more definitive--- If the user is disabled in the LDAP/AD database, it gets disabled in RC.

doctorwho92 commented 7 years ago

If your LDAP servers connectivity is unstable, you have bigger things to worry about than access to RocketChat. This is why people run multiple directory servers. We have two in every office, and additional ones in offsite datacenters.

If the RocketChat server's connectivity is down, the link to an LDAP server is kinda moot.

Live queries to an LDAP server is the way to go, in my opinion. We have no other services that cache authentication credentials except RocketChat. Clients frequently cache credentials, but that's a different story.

Alan.

On Jun 13, 2017, at 12:43 PM, gregs007 notifications@github.com wrote:

I think doing what's suggested in #7235 should work for LDAP auth. Basically you would disable RC users that are disabled or missing in LDAP directory. This would be during regular LDAP database sync. I don't think regular re-auth is a good idea in the case of an unstable LDAP connection locking out users. Sync on the other hand, is more definitive--- If the user is disabled in the LDAP/AD database, it gets disabled in RC.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

koen-serneels commented 7 years ago

Hi. As I see it, the fallback option is too overloaded. First, when LDAP is configured, the option allows to login using both LDAP and local accounts (that could have been created manually outside the LDAP user scope) but if disabled, login with a local account is no longer possible. Secondly, it automatically mirrors the LDAP accounts to the local account system upon login, hence creating the "fallback", including credentials.

In our case we really do not want rocket to mirror accounts from LDAP (and hence we do not want any sync). The LDAP remains the main authority for checking credentials and account status.The fact that rocket automatically creates the local account is highly undesirable and creates all kind of unwanted side effects. To name a few: it imposes an extra security risk as it is "secretly" storing credentials as well. It is also confusing users on their username, if the LDAP user is an email, rocket will allow to login the 2nd time by only using the email user (without the @) as this is the username that rocket used on the local account system. This way it appears that the user can either login with their email (which would authenticate against LDAP) but also by just typing the user (which would fail against LDAP but then succeed when falling back to the local account system).

As for the argument of "unstable LDAP connections" , like @doctorwho92 said, in that case you have other problems, but if you still need a user to login (to adjust the LDAP settings or whatever in case of problems) one can simply create a local account manually outside LDAP scope (by registration or by admin console) and grant it admin rights.

So besides an LDAP sync, we really need to split the fallback option into two; one for allowing local accounts to login (when an external source such as LDAP is specified) and one for disallowing account mirroring all together

localguru commented 7 years ago

see #6144 too

Shaverdoff commented 5 years ago

The bug or notgood feature still here 073 version of RC i disable user on ldap, user do not disconnect on client and i do not delete that user on RC users-table. if user no logout User-session in mobile client still exist and user can chat and enter to conversations

UPD1: If user account was disable, but in RC user do not deleted - user can login!

that option on filter search do not help.... (objectClass=person)(|(userAccountControl=512)(userAccountControl=66048))(|(mail=#{username})(sAMAccountName=#{username})))", "scope": "sub", "userDN": "rocket_svc@domain", "***": "HIDDEN" of couse maybe i use it on mistaken plase

UPD2: if user removed from RC users-table in admin panel, but user do not disable in ldap, user cant login.

iesit commented 5 years ago

+1... Also this is mentioned and discussed, but not quite the same issue in issue #9900

justalreadytaken commented 5 years ago

If you will change option Login Fallback to False Users will not able to login anymore if LDAP auth was failed in any reason.

iesit commented 5 years ago

Yes, however, then you cannot authenticate users using the local database, so you MUST have ALL users (including the administrator) assigned LDAP accounts. I think ideally you should allow external and internal authentication with some sort of "hybrid" system.

Shaverdoff commented 5 years ago

agree with the previous speaker.. is more than one software that use two backend auth, and have that option will be nice

scratttt commented 5 years ago

Hi all!

+1 with @iesit. Is it possible to know if someone is going to try to solve this?

Thanks!

kokotko1337 commented 5 years ago

is here something new? im using latest version but im scared to run off local admin acc and use only ldap admin accounts

mertv0e commented 4 years ago

any issues?

Shaverdoff commented 4 years ago

Issues?;) you mean progress?

-- Gerasim Shaverdov | Deputy CTO | Altarix Mobile: +7 937 070 66 84 | Skype/email: gerasim@altarix.ru Lenina av.25, Samara, Russia, 443068

22 янв. 2020 г., в 16:58, rnqlover notifications@github.com написал(а):

any issues?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

mertv0e commented 4 years ago

Issues?;) you mean progress? -- Gerasim Shaverdov | Deputy CTO | Altarix Mobile: +7 937 070 66 84 | Skype/email: gerasim@altarix.ru Lenina av.25, Samara, Russia, 443068 22 янв. 2020 г., в 16:58, rnqlover @.***> написал(а): any issues? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

Yep =) Blocked LDAP users still can auth in rocket chat

Madic- commented 4 years ago

Why was the issue closed? Is the problem fixed?

rangelbb commented 4 years ago

I have the same question! What do i need to do for fix this?

rodrigok commented 4 years ago

Our enterprise version 3.0.2 covers this feature now.

Madic- commented 4 years ago

Do you plan to port this essential security feature to the basic edition?

sandreddu commented 4 years ago

@rodrigok it could be argued that then the issue can not be closed, if it's only fixed in one version

TwizzyDizzy commented 4 years ago

Hi @rodrigok / @engelgabriel

I totally concur with the irritated replies above: you cannot simply provide a feature in the basic edition and then tell people to buy the enterprise edition when they want the feature to be implemented properly.

Breaking it down to this issue: there is no LDAP authentication "that still works, when the account in the directory is disabled". From a security perspective, LDAP authentication without failing authentication when the account does not exist (or has a different password than the one supplied) is simply a broken LDAP implementation.

So what you do is: have a broken feature in the open source version and tell people to unbreak that feature by buying enterprise.

That is not how open source works. I understand that Rocket.Chat needs to pay the bills and they should earn money as they please. Nothing against that. But please do so in a decent manner. When I reflect on the broader chat ecosystem, the one obvious reason I would choose Rocket.Chat over Mattermost is, that the LDAP implementation is free. Would this not have been the case, we would not have chosen Rocket.Chat, but Mattermost.

They, for example, make it very clear, that LDAP is considered an enterprise feature (which I agree with) and make it part of their commercial editions.

We're not the ones to tell you which road to choose, but we are the ones to tell you: be decent. Fully implement LDAP in the Open Source version or decide against it, but don't try to go down that half-hearted road where the open source version basically acts as "the first shot" to get you hooked on the enterprise version. I'd expect this practice would I be buying drugs at the train station but not when I'm using an Open Source software of an otherwise decent company.

Just my two cents... Thomas

Shaverdoff commented 4 years ago

i agree it!

Отправлено с iPhone

28 февр. 2020 г., в 19:41, Thomas Dalichow notifications@github.com написал(а):

 Hi @rodrigok

I totally concur with the irritated replies above: you cannot simply provide a feature in the basic edition and then tell people to buy the enterprise edition when they want the feature to be implemented properly.

Breaking it down to this issue: there is no LDAP authentication "that still works, when the account in the directory is disabled". From a security perspective, LDAP authentication without failing authentication when the account does not exist is simply a broken LDAP implementation.

So what you do is: have a broken feature in the open source version and tell people to unbreak that feature by buying enterprise.

That is not how open source works. I understand that Rocket.Chat needs to pay the bills and they should earn money as they please. Nothing against that. But please do so in a decent manner. When I reflect on the broader chat ecosystem, the one obvious reason I would choose Rocket.Chat over Mattermost is, that the LDAP implementation is free. Would this not have been the case, we would not have chosen Rocket.Chat, but Mattermost.

They, for example, make it very clear, that LDAP is considered an enterprise feature (which I agree with) and make it part of their commercial editions.

We're not the ones to tell you which road to choose, but we are the ones to tell you: be decent. Fully implement LDAP in the Open Source version or decide against it, but don't try to go down that half-hearted road where the open source version basically acts as "the first shot" to get you hooked on the enterprise version. I'd expect this practice would I be buying drugs at the trains station but not when I'm using an Open Source software of an otherwise decent company.

Just my two cents... Thomas

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

reetp commented 4 years ago

/rodrigok it could be argued that then the issue can not be closed, if it's only fixed in one version

It is broken in the 'released' version available here. This is a bug in the released version, and should therefore logically be re-opened.

As @TwizzyDizzy correctly states, either LDAP is in, or it is out.

But it should not be the 'Hokey Cokey' version with one foot in, and one foot out.

one obvious reason I would choose Rocket.Chat over Mattermost is, that the LDAP implementation is free. Would this not have been the case, we would not have chosen Rocket.Chat, but Mattermost.

Yup - I'm pretty sure you are far from alone and Rocket.Chat would pull it now if they could (hindsight is a wonderful thing) but can't for this very reason which will cost them a lot of users and bad publicity at a time when the market is extremely competitive and they are up against it.

Caught between a rock and a hard place.

That is not how open source works

Yup. There are a number of people at Rocket.Chat who still don't seem to understand it.

TwizzyDizzy commented 4 years ago

There are a number of people at Rocket.Chat who still don't seem to understand it.

I wouldn't go so far as to state it this way. That'd be a bit unfair. I simply think it's like you said:

Caught between a rock and a hard place.

The thing is: there needs to be a proper decision - both directions have implications (and pondering the pros/cons is not up to us) but trying to go half-way is most definately the worst choice here.

Cheers Thomas

reetp commented 4 years ago

There are a number of people at Rocket.Chat who still don't seem to understand it. I wouldn't go so far as to state it this way. That'd be a bit unfair. I simply think it's like you said:

YMMV.... ;-)

Caught between a rock and a hard place.

The thing is: there needs to be a proper decision - both directions have implications (and pondering the pros/cons is not up to us) but trying to go half-way is most definately the worst choice here.

I am thinking that as above, the fix to this bug is going to be 'upgrade to enterprise'

https://github.com/RocketChat/Rocket.Chat/pull/16801

"and otherwise have a valid Rocket.Chat Enterprise Edition subscription for the correct number of user seats. Subject to the foregoing sentence, you are free to modify this Software and publish patches to the Software"

So the fix will be in the Enterprise code that they will put in the repo, but you can only use or modify it if you have a licence.

I suspect this is a move to make LDAP enterprise only - which is particularly galling when quite a lot of additional 'enterprise' level code for this was donated as open source here:

https://github.com/RocketChat/Rocket.Chat/pull/14278

What WOULD be nice is a proper statement on their intentions instead of back door PRs etc

It's the old communication problem once again.

Shaverdoff commented 4 years ago

i suject wait while rodrigok get answer on it

rodrigok commented 4 years ago

We are just migrating the enterprise code from our closed repo to this repo under a different license so it will be easier for us to show everything we add to the EE version and make it easy for who wants to contribute to the EE features (we have customers that want to contribute as well).

So we think that things will be more clear with this move. We will not remove anything from the community edition and move to the enterprise edition, we never did that and we don't think it's fair with the community. We are following how GitLab did that and we think it's better to protect the code under a license rather than keep it closed and make it hard to receive contributions.

reetp commented 4 years ago

So we think that things will be more clear with this move.

Excellent - so all users will benefit from a fix to this bug then?

rodrigok commented 4 years ago

@reetp the disabling of removed users from LDAP is still an enterprise implementation. Not related with the repositories merge.

reetp commented 4 years ago

@reetp the disabling of removed users from LDAP is still an enterprise implementation. Not related with the repositories merge.

Ahhh. As expected.

I am thinking that as above, the fix to this bug is going to be 'upgrade to enterprise'

So as per my previous comments, and as I suspected, you are not going to fix this then? A simple yes or no answer to this will suffice, so people know where they are and your policy.

The Community version IS broken.

This is the Community repo (as far as I am aware).

This bug is against the Community version (this repo) and not the Enterprise version and should therefore be re-opened.

rodrigok commented 4 years ago

Yes, we are not moving this feature to the community version now. I can't say about the future.

It's not a bug since we never implemented such sync. We only import new users and sync data changes.

Users deleted from LDAP or with password changed can't login but they will continue to be logged on already existent sessions. It's still possible to disable them manually, it's not blocked to do such thing, you just need to add that item to your process. Big companies where this may happen frequently can use our Enterprise version.

Shaverdoff commented 4 years ago

So if i kill active session from mongo in disabled or changedpasswd ldap user it cant login? This you wanasay?

Отправлено с iPhone

13 марта 2020 г., в 01:37, Rodrigo Nascimento notifications@github.com написал(а):

 Yes, we are not moving this feature to the community version now. I can't say about the future.

It's not a bug since we never implemented such sync. We only import new users and sync data changes.

Users deleted from LDAP or with password changed can't login but they will continue to be logged on already existent sessions. It's still possible to disable them manually, it's not blocked to do such thing, you just need to add that item to your process. Big companies where this may happen frequently can use our Enterprise version.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

fbartels commented 4 years ago

Maybe there is just a misunderstanding going on here that is causing the (understandable) disappointment. @rodrigok wrote:

Users deleted from LDAP or with password changed can't login but they will continue to be logged on already existent sessions.

While the initial report was (and what I also was able to observe when I subscribed to this issue):

Disabling an user account in Active Directory does not prevent that user from being able to login to Rocket Chat;

So in other words: If the user was disabled in ldap or its password was changed the user will no longer be able to create new sessions going forward?

This would be a good thing and I think what most users in this thread are after.

(in all still not knowing when existing sessions do expire and therefore users will eventually be logged out)

engelgabriel commented 4 years ago

Rocketeers, after consideration, we will merge the code to fix these LDAP issues into the main repository. I am reopening a few of the LDAP issues to be resolved this week.

reetp commented 4 years ago

Rocketeers, after consideration, we will merge the code to fix these LDAP issues into the main repository. I am reopening a few of the LDAP issues to be resolved this week.

An extremely sensible move thank you.

Either LDAP is in, and works, or it isn't. Sitting on the fence doesn't make Rocket look very good.

If you really plan to remove features that people have become accustomed too, and been a 'selling' point (and I think that LDAP applies to a LOT of community users), then you have to plan that well in advance, and do it slowly.

I would imagine if you pulled LDAP for community now you'd lose a lot of people. They are all potential customers.

(IMHO a genuinely Open Source LDAP implementation is a really good way to attract users to Rocket and create opportunities to 'up sell' other products and services precisely because most other systems lock it behind a paywall)

Again, thank you.

winstonrojas commented 4 years ago

I thank you (rocket chat team) and encourage you to keep forward and make rocket chat a real community worldwide people project. Unfortunately many of us still rely on closed monopolistic technologies like Microsoft Windows workstations (i'm grateful with windows, in a way i have benefited a lot from it and i enjoy some of their products, but in a philosophical way i can not agree ) and therefore Active Directory almost like the natural method for administration.

Shaverdoff commented 4 years ago

Замудрил:))

--

Gerasim Shaverdov | Deputy CTO | Altarix

Mobile: +7 937 070 66 84 | Skype/email: gerasim@altarix.ru Lenina av.25, Samara, Russia, 443068

27 апр. 2020 г., в 02:38, winstonrojas notifications@github.com написал(а):

 I thank you (rocket chat team) and encourage you to keep forward and make rocket chat a real community worldwide people project. Unfortunately many of us still rely on closed monopolistic technologies like Microsoft Windows workstations (i'm grateful with windows, in a way i have benefited a lot from it and i enjoy some of their products, but in a philosophical way i can not agree ) and therefore Active Directory almost like the natural method for administration.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

moonwolf-github commented 3 years ago

Looks like in 3.5.3 it still doesn't work as it should.

Nikk-0 commented 3 years ago

Hi,

Any news about this ?

eengstrom commented 1 year ago

So, it's been another three years since @engelgabriel said:

Rocketeers, after consideration, we will merge the code to fix these LDAP issues into the main repository. I am reopening a few of the LDAP issues to be resolved this week.

To me, that implied the required features to enable proper LDAP sync and (I inferred) automatic user disabling during said sync. Perhaps I misunderstood, but if not, those features are still not working in the CE version. Both appear to be enterprise only features.

... notably, the following obviously useful features:

While I totally understand the financial need to break out some features into the enterprise version so that the company may generate revenue, I completely disagree that any security-related capabilities should not be automatic, and in the community edition, especially if the contributions made came from the community.

If not, and without the ability to automatically and periodically update user "disabled" status and auto-logout (newly) disabled users, RocketChat has decided that the security of users using the community edition version is not of their concern.

Moreover, having the bug/feature reports remain open implies that it's still a possibility, when in fact there is no intention to "fix", since it's already working, if one is using Enterprise mode. Someone should at least acknowledge the "enterprise only" decision and close the issue.

caskelly commented 1 year ago

Thanks for your comment,

You are correct that after Gabriel’s comment which you quoted we refactored LDAP code including addressing a number of open issues. At that time we made the updated functionality part of the enterprise offering. We will close the associated issues.

Regards, Chris Skelly

eengstrom commented 1 year ago

@caskelly - I appreciate the acknowledgement here (and #6174 and #7235), but I encourage you to read my entire post again. While I get the motivation for revenue, I would encourage you to consider any features that affect the security of deployments and not put those behind paywalls. In this case, there is an implicit expectation that an account disabled in some LDAP / AD system would be forbidden from logging into RC, but that's not true.

While I don't know the extent to which the code that implements the LDAP integration came from CE contributions, I completely believe that security related code should never be behind a paywall. Other stuff that makes life better? Sure. But don't endanger contributing CE users by putting features like the LDAP sync and auto-disable in the enterprise only version. That's just asking for people to be impacted negatively, which will reflect poorly on your product when incidents occur. Oh, and blaming the user? That's not an option.