Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Access tokens are not invalidated when credentials are invalidated via an external auth provider #4158

Open
richvdh opened this issue Nov 7, 2018 · 32 comments
Labels
Security T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. z-p2 (Deprecated Label)

Comments

@richvdh
Copy link
Member

richvdh commented Nov 7, 2018

If synapse is configured to interface with an external auth provider (such as LDAP), there is no mechanism for that external system to feed back to synapse if a user's credentials have been locked/revoked/etc. Synapse's access_tokens stay valid forever.

A related question: if a user's password is changed in the external system, should we require all matrix clients to log in again, as we would with a local password change? If so, how should this be implemented?

@neilisfragile neilisfragile added the z-p2 (Deprecated Label) label Nov 9, 2018
@BloodyIron
Copy link

Any idea when this might get looked into? :)

@BloodyIron
Copy link

This is actually a blocker for me. Is there any possibility of a milestone for this one? :(

@neilisfragile
Copy link
Contributor

Hi @BloodyIron fixing this properly is going to need some thought and is probably related to matrix-org/matrix-spec-proposals#1467

As of the right now, the main focus of the team is to complete our r0 release of the S2S API, you can track progress here https://github.com/orgs/matrix-org/projects/2 though is still a lot outstanding.

This means that I can't in good faith give you a time frame for addressing this issue. Things that would help bump it up the to do list would be commercial sponsorship of the work, or lots of people also needing it fixed.

@BloodyIron
Copy link

Hi @neilisfragile well, the ticket you link to is kinda similar, but I wouldn't fully agree about it being completely related.

As for S2S API, I'm not familiar with what that is and I can't really discern that from the trelo board you linked me to.

As for this particular topic, I've been thinking about it more. My understanding is that those behind riot.im and matrix.org rolled out the modular.im service, with the intent of it being a B2B kind of service. If the intent is indeed to provide the service to businesses, I guarantee you the majority of them are going to ask how it will integrate with their existing Authentication system (probably will be Active Directory, failing that LDAP).

If those people are unable to lock users out, or force people to log back in with a password change, that will be a hard block for setting that up. Think about it, if a staff member is fired, and you can't prevent them from being able to use riot.im on their phone by locking them out, that's a big deal.

I can understand that other things are likely going to be hammered out before this. But I really do think the perception of this issue is being undervalued in terms of a security and business perspective. I may be asking for it now, but I guarantee you there will be plenty of clients that will NEED this before they can have confidence in riot.im/matrix.org.

So, not sure if you're involved with flagging tickets, or if that's someone else, but I really do think this warrants a p1.

@neilisfragile
Copy link
Contributor

I think we are agreeing with each other here

As you suggest there is commercial value in the feature and Modular is an obvious use case hence. "Things that would help bump it up the to do list would be commercial sponsorship of the work ..."

The feature is obviously valuable, but open spec work to back r0 takes precedence.

If I give it P1 it means we are working on it pretty much right now, it goes on our immediate to do at the expense of something else, this is why it is P2.

@BloodyIron
Copy link

@neilisfragile alright, well do you anticipate this could be explored in the next say 1-2ish months? I'm not exactly 100% on how the priorities (numbering) work for this team. :)

But I am glad that you see the value I'm trying to express! :D

@neilisfragile
Copy link
Contributor

Given the volume of r0 work, doing this in next 1-2 months is not realistic. Really, it will take commercial sponsorship to get this done anytime soon.

@BloodyIron
Copy link

Any word on this? I'm quite certain entities such as the Government of France will likely care about something like this. What's your take @ara4n ?

@madmatt
Copy link

madmatt commented Apr 13, 2019

A related question: if a user's password is changed in the external system, should we require all matrix clients to log in again, as we would with a local password change? If so, how should this be implemented?

As far as I know this isn't possible. LDAP works by binding to an LDAP controller (either binding as an admin and then authenticating with user-supplied details, or binding directly with the user's credentials). So whenever you get the user's raw username/password, you can check that they still work, but unless you keep those details decryptable in a server-side session or similar, you can't re-check that their login is still valid and that their password hasn't changed in the meantime.

Either way, if you're using LDAP or SAML to authenticate, you generally end up creating a Synapse-specific session, and as long as that session remains valid a user can remain logged in. If the user is removed from the source in the meantime, you won't see that until the user next attempts to login.

Maybe I'm just misunderstand what you're asking for though, if so please let me know. I'm not familiar at all with Matrix or Synapse, so might be totally missing the point - sorry if that's the case!

@BloodyIron
Copy link

@madmatt IMO binding with a trusted account is the way to go, as it can check the account status against the domain (AD/LDAP) and the domain can return "locked/disabled/good" or whatever, without having to "pass the hash" for the user, in the account status check.

I'm not sure if this check should reside in the identity server or matrix itself, however what I see is that at some point matrix needs to be aware of these account states, so that it can enforce an appropriate action. (tell user account is locked, or disabled, or whatever)

If we look at the recent token invalidation action that matrix.org and riot.im took, that's a pretty dang good example of a way this can be functionally achieved, except more finely tuned to the per-case basis (UX adjustment based on state).

This is commonplace for any system working against LDAP/AD, as this is a required function in corporate/secured space. Otherwise you couldn't prevent users who are already logged in from doing nasty stuff (say when you fire them).

Alternatively this could involve kerberos, but not 100% sure if that's required or not, as I don't believe all LDAP ecosystems use kerberos, while AD does.

@richvdh
Copy link
Member Author

richvdh commented Jul 12, 2019

#5660 adds a feature by which you can make access tokens expire after a defined period.

When an access token expires, clients will make you log in again to confirm your credentials. Those clients which support soft-logout (eg riot-web, as of current develop) will preserve your E2E history, but those which do not (all other clients, at the moment) will lose your E2E history.

So obviously this needs soft-logout support from more clients to be usable, but I'm calling this resolved from the server side.

@richvdh richvdh closed this as completed Jul 12, 2019
@BloodyIron
Copy link

Time expiration only covers one facet of token expiration. I don't see that covering disabled/locked scenarios. Please re-open.

@anoadragon453
Copy link
Member

I don't think #5660 solves this either. It allows server admins to configure a period which access tokens expire, but even if you set it to ~1hr which would force fired employees to be logged out and thus can't participate anymore, it's not a very user-friendly to have to login every hour.

In hope of a generalised solution, I propose adding a new callback to login providers (such as matrix-synapse-ldap3 that checks if a user still exists on the service. We can tell if a user has come in through a login provider by seeing if they have a device but don't exist in the users table (and aren't an appservice user).

With this user list we could run a periodic job asking each login provider if this user still exists. If none reply that they do, we soft-logout the user.

My worries with this system are that login provider may not be able to check if a user exists without having the user's password, which we obviously don't want to store in plain-text.

Additionally, as login providers switch over to this new system, a sysadmin could be running with a login provider that does support the callback, and one that does not. If the one that does not is in charge of that user, but can't respond that they are still valid, that user will receive a soft logout.

This is all based on checking through Synapse for about an hour, so excuse if any information is incorrect.

@BloodyIron
Copy link

Can we have this re-opened please? Even @ara4n recognises this hasn't been addressed yet.

@turt2live turt2live reopened this Sep 29, 2019
@BloodyIron
Copy link

Thanks! @turt2live :D

@richvdh
Copy link
Member Author

richvdh commented Sep 30, 2019

With this user list we could run a periodic job asking each login provider if this user still exists.

That wouldn't cover the case where the user exists but their credentials have been changed, hence requiring a re-login.

In the general case, we can't go to an external auth provider to check if a user's credentials are still valid, without having those credentials to hand. We can do it for some situations with LDAP, but not at all for things like SAML.

In other words: it's not as easy as it sounds.

Even @ara4n recognises this hasn't been addressed yet

This is presumably refering to this reddit thread.

@anoadragon453
Copy link
Member

@BloodyIron I know you've mentioned Nextcloud having a solution to this with LDAP user cleanup, but do you know if this extends to other auth providers or just LDAP?

@BloodyIron
Copy link

@BloodyIron I know you've mentioned Nextcloud having a solution to this with LDAP user cleanup, but do you know if this extends to other auth providers or just LDAP?

I haven't tried it myself just yet, but I would expect it to be appropriate for AD too, however I'm not sure if there's AD-centric stuff it's missing that would be helpful for the riot/matrix ecosystem. Perhaps reach out to NC devs?

@BloodyIron
Copy link

BloodyIron commented Sep 30, 2019

With this user list we could run a periodic job asking each login provider if this user still exists.

That wouldn't cover the case where the user exists but their credentials have been changed, hence requiring a re-login.

In the general case, we can't go to an external auth provider to check if a user's credentials are still valid, without having those credentials to hand. We can do it for some situations with LDAP, but not at all for things like SAML.

In other words: it's not as easy as it sounds.

Even @ara4n recognises this hasn't been addressed yet

This is presumably refering to this reddit thread.

Considering @ara4n is literally directly responding to me... uh... yes. ;)

Also, there are secure ways to have one-way caches of user creds, without giving up sensitive info. Not just with kerberos tickets (although that would be nice). This is how federation and even Linux systems can let users log on when the AD is not available (for a limited time).

@richvdh
Copy link
Member Author

richvdh commented Sep 30, 2019

@BloodyIron I know you've mentioned Nextcloud having a solution to this with LDAP user cleanup, but do you know if this extends to other auth providers or just LDAP?

I haven't tried it myself just yet, but I would expect it to be appropriate for AD too

doesn't AD expose an LDAP interface? The question was about other authentication protocols such as SAML.

This is presumably refering to this reddit thread.

Considering @ara4n is literally directly responding to me... uh... yes. ;)

I don't think @ara4n was aware of #5660 being done when he gave that answer; certainly I'm not aware of any immediate intention to do any further development in this area.

I understand that it's a blocker for your use-case, and I'm sorry that we're not in a position to address your needs; however we can't prioritise everything and this is one we're having to park for now. Of course we're always happy to consider well-written pull-requests that add useful functionality.

@BloodyIron
Copy link

@richvdh

  1. I don't know SAML well enough to comment on AD SAML stuff. In my experience, AD does offer LDAP DN interfacing by default. I myself plan to use Samba 4 AD DCs, so unsure if I can do SAML with that or not, but plan to primarily interface other things via LDAP DN stuff.
  2. I cannot speak for what @ara4n is or is not aware of. However, I have raised this as a legitimate security concern at the beginning of this year and have been told by multiple developers in (your?) team that they agree this has legitimacy.

I recommend you re-familiarise yourself with why it is security-critical to invalidate auth tokens when a user is disabled/locked/password changed in an LDAP/AD environment, because it sounds like you do not understand the security-critical facets here. The majority of orgs in the world use AD/LDAP for authentication, and if we want to convince more of them to use matrix/riot, they will need this before they consider implementing it.

I may have a use-case, but so does the majority of the world (should they want to implement matrix/riot).

@richvdh richvdh removed their assignment Feb 26, 2020
@BloodyIron
Copy link

So any word on this issue? It's been like two years now since I originally raised the point to the devs (before this issue existed).

@BloodyIron
Copy link

I'd like to point out this issue represents a pretty tangible security risk to any AD/LDAP integrated environment. Having the LDAP/AD account be disabled/locked/password reset, and the relevant riot account unaffected, means that security cannot reasonably be enforced. Kind of frustrating this is being kicked along like a can for so long. :/

@localguru
Copy link
Contributor

@BloodyIron To my mind this is not a "fault" of a SP, but a common SSO problem, not Synapse specific. What about a script which checks by API of your IdP if accounts have been disabled or looked. If so, do look them here too.

@BloodyIron
Copy link

This isn't SSO, this is direct authentication interfacing, I'm not talking about SAML or other federated authentication. When account is disabled, password reset, account is locked, and other earlier described scenarios, the account in Matrix MUST reflect that immediately or it can be abused. As in, push/automatic, not pull/periodic, as any delay can lead to abuse/data exfiltration in secure environments. This is how proper AD/LDAP integration works in the greater IT industry. So this is a provable flaw, and others in the Matrix staff agree.

@localguru
Copy link
Contributor

@BloodyIron The beginning of this discussion by @richvdh was: "If synapse is configured to interface with an external auth provider (such as LDAP), there is no mechanism for that external system to feed back to synapse if a user's credentials have been locked/revoked/etc. Synapse's access_tokens stay valid forever."

In the end it doesn't matter whether you log on to Matrix via SSO or LDAP or any other external auth. For example, how should an AD know which services authenticate against its LDAP interface and give feedback to all these services if the AD account is locked/deleted/expired? This is not the task of an auth provider. I see the problem with the access token too, yes. You said that this is possible with AD/LDAP. Do you have more information about this? I authenticate against SAML, but the accounts are in AD.

@BloodyIron
Copy link

An auth provider IS in fact supposed to know if an account is locked, disabled, password reset, otherwise those states CANNOT BE ENFORCED. I welcome you to discuss this with Matrix staff, whom are already aware of this situation and are mitigating it.

@vsatmydynipnet
Copy link

Upvoting this one because things can go terrible wrong if user is deactivatated, deleted or whatever in LDAP and the session are still active. IMHO there are 2 possibilities:

  • user is temporary disabled -> Delete all tokens and log them out. They hopefully have done a key backup.
  • user is deleted, fully deactivated -> deactivate user on matrix, get them out of all rooms

@BloodyIron
Copy link

I've heard word that this might already be addressed, but not 100% sure. @anoadragon453 would you be able to comment on yay/nay/etc please? :)

@anoadragon453
Copy link
Member

This is still an issue, though could possibly be addressed by:

  1. Handling hooks sent from identity providers when a user is deactivated - this depends on the SSO protocol.
  2. For those protocols which do not offer such hooks (i.e LDAP), one may be able to build a custom account validity module built off the back of Add a module type for account validity #9884.

The current workaround today is to set the session_lifetime option in Synapse, which will limit how long a user's session can last before they will be soft-logged out (encryption keys will be kept) and need to authenticate with the identity provider again before they can continue using their account.

@MuhChy
Copy link

MuhChy commented Jun 26, 2023

Is this still not that important ? I would like to see this fixed :P

@BloodyIron
Copy link

Is this issue still going on???? more than 5 years later?!?!?!?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Security T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. z-p2 (Deprecated Label)
Projects
None yet
Development

No branches or pull requests