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

CAS validation issue with synapse #2639

Closed
tyxieblub opened this issue Nov 4, 2017 · 11 comments
Closed

CAS validation issue with synapse #2639

tyxieblub opened this issue Nov 4, 2017 · 11 comments

Comments

@tyxieblub
Copy link

Hello,

Description

It seems there is a problem with how CAS validation is handled in login.py#L420.

Steps to reproduce

  • Install and configure CAS 5.1.x (I didn't try with earlier ones)
  • Install matrix-synapse
  • Configure matrix-synapse to use CAS to connect with service_url: https://matrix.domain
  • Try to login!

You'll see it doesn't work and if I'm not mistaken it is because in login.py#L397 we ask for a ticket with a service in the form of https://matrix.domain/_matrix/client/api/v1/login/cas/ticket?redirectUrl=***.
But when we validate the ticket, we only send cas_service_url which lacks the /_matrix/client/api/v1/login/cas/ticket?redirectUrl=*** part so CAS fails the validation saying the ticket was used for another service than the one it was created for.

I don't know if earlier versions were more lenient on validation, but I got it to work by replacing line 420 by this line:

"service": self.cas_service_url + request.uri[:request.uri.find("&ticket")]

But I'm sure there's a better way to do it...

I believe #2404 may be related to this problem.

Version information

  • Homeserver: local homeserver

If not matrix.org:

  • Version: matrix-synapse 0.24.1-1
  • Install method: Debian repository
  • Platform: VM on Debian9
@johappel
Copy link

same issue

@mijutu
Copy link

mijutu commented Mar 10, 2019

The problem still exists in matrix-synapse-py3 0.99.2+stretch1

matrix-synapse-0.99.2-CAS-fix.diff.gz

The service URL given to CAS is not supposed to change as long as synapse configuration stays the same, so we can have CasRedirectServlet store the url somewhere and later get it back in CasTicketServlet

@quorak
Copy link

quorak commented Mar 13, 2019

This ticket is s old. I came across the same problem. So how are others using cas. It seems impossible to me, or can there be a less strict validation on the cas side?

we should implement the patch though! thanks @mijutu

@mikifus
Copy link

mikifus commented Mar 22, 2019

I had the same problem. I can confirm the patch works.

ghost pushed a commit to eyetime-international-ltd/synapse that referenced this issue Apr 11, 2019
@ghost ghost mentioned this issue Apr 11, 2019
3 tasks
@SeaLife
Copy link

SeaLife commented Oct 24, 2019

// EDIT:

After we implemented JSON in our CAS Server, it seems to be working BUT we had to disable the service url validation in our case to get this working. So this Bug is still there.

Why is no one fixing it or applying the provided workaround/fix?

@mijutu
Copy link

mijutu commented Nov 7, 2019

The old patch didn't apply, so I made a new one.

matrix-synapse-1.5.1-CAS-fix.diff.gz

@richvdh
Copy link
Member

richvdh commented Nov 7, 2019

so somebody opened a pull request with @mijutu's patch at #5044; however I had questions about it which were never answered so we had to close the PR.

@mijutu: perhaps you could open a PR so that we can work on landing your changes?

@mijutu
Copy link

mijutu commented Dec 9, 2019

We had a pull request and there richvdh commented that the solution I used was wrong. I think he is correct. If two CAS logins happen at the same time, then users might end up at a wrong url.

Probability of two simultaneous CAS logins is small because CAS is used only at the initial login. Browsers remember access tokens and CAS is rarely used.

Homeserver owners might be able to use my patch for months or years without having issues, but if it's wrong, then it's wrong and shouldn't be merged.

@mijutu
Copy link

mijutu commented Dec 31, 2019

matrix-synapse-1.7.2-CAS-fix.diff.gz

Here is a better fix for the problem. CasTicketServlet generates the service parameter from synapse config and redirectUrl parameter of the request.

@richvdh
Copy link
Member

richvdh commented Jan 2, 2020

@mijutu please could you open a PR with your patch so that we can read and discuss it?

@Naugrimm Naugrimm mentioned this issue Jan 5, 2020
4 tasks
Naugrimm added a commit to Naugrimm/synapse that referenced this issue Mar 11, 2020
@richvdh
Copy link
Member

richvdh commented Mar 24, 2020

fixed by #6634. Thanks @Naugrimm!

@richvdh richvdh closed this as completed Mar 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants