[WIP] Implement m.login.token and SSO login initial changes #1374

anandv96 · 2020-09-01T10:52:24Z

Made some changes to implement CAS SSO support

made config changes for cas
GET /login returns the m.login.sso flow if the config is enabled
- login types include SSO test passes if changes are made to the sytest dendrite config
redirect user to the CAS server upon hitting /login/sso/redirect
validate token from CAS server
create user if required
(pending) redirect client to the redirectUrl with a login token to complete the login
(pending) change sytest whitelist

Clarifications needed

The spec mentions that a client should hit a User interactive flow endpoint to get a list of flows and stages (https://matrix.org/docs/spec/client_server/latest#id186).
- What endpoint needs to be hit? Not clear in the spec.
- A few sytests are dependent on this
To actually authenticate the user, the spec says that the client should use an m.login.token.
- Has this been implemented already? Couldn't find many references in the codebase
- What kind of token is used here? How can I create this token?

Signed-off-by: Anand Vasudevan [email protected]

Pull Request Checklist

I have added any new tests that need to pass to testfile as specified in docs/sytest.md
Pull request includes a sign off

kegsay · 2020-09-01T13:44:54Z

The spec mentions that a client should hit a User interactive flow endpoint to get a list of flows and stages - What endpoint needs to be hit?

In this case the /register endpoint. The term "user interactive auth" is used to describe the set of JSON responses returned from endpoints which implement it for authentication, so anything which requires special auth (registration, login, deleting devices, changing passwords) can use "user interactive auth" to provide a standardised way to authenticate these operations.

To actually authenticate the user, the spec says that the client should use an m.login.token.

We do not have token auth yet - see #403

anandv96 · 2020-09-02T03:26:19Z

To actually authenticate the user, the spec says that the client should use an m.login.token.

We do not have token auth yet - see #403

#429 for m.login.token was closed without without merging, I'm not sure why though.

Should I work on #403 now before finishing #1297 ? Can #429 be modified and used?

kegsay · 2020-09-02T09:04:18Z

Yes work on #403 first I think. The problem with #429 is that it used macaroons rather than just a random string like we do for many other things. You may be able to take bits of it and use it successfully though.

anandv96 · 2020-09-02T14:29:39Z

The problem with #429 is that it used macaroons rather than just a random string like we do for many other things

Looks like the spec recommends that we use macaroons. https://matrix.org/docs/spec/client_server/latest#id575

They also say that we can implement access tokens in any way we want. https://matrix.org/docs/spec/client_server/latest#id180

Should I go ahead and use random string tokens for the login token?

neilalexander · 2020-09-04T14:19:28Z

Yes, I would use random strings for now.

Database level changes not made

kegsay

Looking roughly correct, would be good to get sytests passing.

kegsay · 2020-09-17T16:05:19Z

clientapi/routing/login.go

+		if err != nil {
+			// TODO: is this appropriate?
+			return util.JSONResponse{
+				Code: http.StatusMethodNotAllowed,


The method is fine, it's just they didn't give an HTTP body.

clientapi/routing/login.go

kegsay · 2020-09-17T16:08:20Z

clientapi/routing/login.go

-		if authErr != nil {
-			return *authErr
+
+		loginType := jsonBody["type"].(string)


Won't this panic if the request body has no type key?

Yes it did. I hadn't checked for that earlier

kegsay · 2020-09-17T16:09:37Z

clientapi/routing/login.go

+	// the login is successful, delete the login token before returning the access token to the client
+	if authResult.Code == http.StatusOK {
+		if err := auth.DeleteLoginToken(r.(*auth.LoginTokenRequest).Token); err != nil {
+			// TODO: what to do here?


Just log it and continue. This is just to cleanup.

kegsay · 2020-09-17T16:11:58Z

clientapi/routing/sso.go

+	if !cfg.CAS.Enabled || cfg.CAS.Server == "" {
+		return util.JSONResponse{
+			Code: http.StatusMethodNotAllowed,
+			JSON: jsonerror.NotFound("Bad method"),


There's nothing wrong with the HTTP method here, the code is wrong. Give a 501 Not Implemented or something.

You're right. I changed this and a few other http code returns as well to something I thought were more appropriate

kegsay · 2020-09-17T16:12:35Z

clientapi/routing/sso.go

+	}
+
+	// Try to parse the SSO URL configured to a url.URL type
+	ssoURL, err := url.Parse(cfg.CAS.Server)


You could do this on startup rather than at request time.

kegsay · 2020-09-17T16:15:48Z

clientapi/routing/sso.go

+	redirectURL, err := url.Parse(redirectURLStr)
+	if err != nil {
+		return util.JSONResponse{
+			Code: http.StatusInternalServerError,


This isn't a server error if we fail to parse the URL, it's the client who supplies the URL. Perhaps a 400 Bad Request would be more appropriate.

kegsay · 2020-09-17T16:18:16Z

clientapi/routing/sso.go

+	cfg *config.ClientAPI,
+) util.JSONResponse {
+	// form the ticket validation URL from the config
+	ssoURL, err := url.Parse(cfg.CAS.Server + cfg.CAS.ValidateEndpoint)


This can be done at startup.

clientapi/routing/sso.go

anandv96 · 2020-09-21T12:30:57Z

Made another commit with some of the changes from the comments.

I need some help with making database changes in clientapi/auth/login_token.go L179 onwards
https://github.com/matrix-org/dendrite/pull/1374/files#diff-1f4ba2edfc578ed53786cf30d3148eedR179

anandv96 · 2020-09-21T13:06:17Z

I changed lib/SyTest/Homeserver/Dendrite.pm in sytest for the test login types include SSO to include the config values for SSO in the dendrite.yaml. Should I wait until all the other tests pass before I make the change in the sytest repo?

Changed with comments from Kegsay and Half-Shot - changed some return error codes - moved sso url creation &validation to startup time - added test to sytest whitelist

clokep · 2020-09-25T12:08:17Z

clientapi/routing/sso.go

+	// the service url that we send to CAS is homeserver.com/_matrix/client/r0/login/sso/redirect?redirectUrl=xyz
+	ssoQueries.Set("service", req.RequestURI)


I suspect it doesn't matter too much, but it could be clearer to redirect to a different endpoint instead of using /login/sso/redirect as both the redirect endpoint and the endpoint that resolves the ticket. I'm not sure if this is common in CAS implementations or not. Synapse uses a separate endpoint, but it is also under the /_matrix prefix, which it should not be.

Yeah, I thought about this too and in the older version of the spec (r0.4.0) there are 2 endpoints specified for CAS auth (/login/cas/redirect and /login/cas/ticket). But the newer spec (r0.6.1) doesn't mention the separate /ticket endpoint, so I thought I should just go with the single endpoint.

Splitting it up into 2 endpoints can be easily done.

About it not being in the /_matrix prefix, I don't know enough about common practices in other CAS implementations to comment. The spec does say /_matrix/client/r0/login/sso/redirect though.

Yeah, I thought about this too and in the older version of the spec (r0.4.0) there are 2 endpoints specified for CAS auth (/login/cas/redirect and /login/cas/ticket). But the newer spec (r0.6.1) doesn't mention the separate /ticket endpoint, so I thought I should just go with the single endpoint.

I believe the second endpoint is meant to be implementation specific!

Splitting it up into 2 endpoints can be easily done.

I'd let @kegsay or @neilalexander decide whether this should be done or not. 😄

About it not being in the /_matrix prefix, I don't know enough about common practices in other CAS implementations to comment. The spec does say /_matrix/client/r0/login/sso/redirect though.

Sorry for not being clear, I meant the ticket endpoint. The redirect endpoint must be /_matrix/client/r0/login/sso/redirect as you noted.

I'd let @kegsay or @neilalexander decide whether this should be done or not

Yeah I agree 😅

I meant the ticket endpoint

The ticket endpoint is /_matrix/client/r0/login/cas/ticket according to the old spec, that could explain why synapse has it under /_matrix as well.

2 endpoints please, both under /_matrix will do. If we don't put it under /_matrix then we need to mess around with routers more, and the only reason why we don't want it there is "just in case" the spec suddenly uses that endpoint for something, which feels unlikely.

kegsay

There's a bunch of sytests here #1297 - some of them should be passing by now right? Please update sytest-whitelist.

kegsay · 2020-10-06T10:04:03Z

clientapi/auth/login_token.go

+
+	// check whether the token has a valid time.
+	// TODO: should this 5 second window be configurable?
+	if time.Now().Unix()-token.CreationTime > 5 {


5 seconds seems very short, perhaps 1 minute?

I only choose 5 seconds because the spec suggested it: https://matrix.org/docs/spec/client_server/r0.6.1#handling-the-authentication-endpoint

Oh wow I see. In which case please just link to this on this line to justify why 5s.

5 seconds sounds pretty short to me as well, if you have a connection or DNS failure that could easily time out. It seems that Synapse uses 2 minutes, although I was unable to find any reference to why that was chosen: https://github.com/matrix-org/synapse/blob/78d5f91de1a9baf4dbb0a794cb49a799f29f7a38/synapse/handlers/auth.py#L1323-L1325

In that case, should I make this time configurable by the admin (with a default time of 5 seconds)?

kegsay · 2020-10-06T10:04:41Z

clientapi/auth/login_token.go

+
+	// check whether the UserID is malformed
+	if !strings.Contains(token.UserID, "@") {
+		// TODO: should we reveal details about the error with the token or give vague responses instead?


Revealing details is fine for now.

kegsay · 2020-10-06T10:10:04Z

clientapi/auth/login_token.go

+		// TODO: should we reveal details about the error with the token or give vague responses instead?
+		return "", errors.New("Invalid UserID")
+	}
+	if _, err := userutil.ParseUsernameParam(token.UserID, serverName); err != nil {


Please add some prose here to explain why this check alone is insufficient (it doesn't check there is an @).

kegsay · 2020-10-06T10:11:35Z

clientapi/routing/login.go

-		typePassword := auth.LoginTypePassword{
-			GetAccountByPassword: accountDB.GetAccountByPassword,
-			Config:               cfg,
+		// TODO: is the the right way to read the body and re-add it?


kegsay · 2020-10-06T10:15:17Z

clientapi/routing/sso.go

+	// the service url that we send to CAS is homeserver.com/_matrix/client/r0/login/sso/redirect?redirectUrl=xyz
+	ssoQueries.Set("service", req.RequestURI)


2 endpoints please, both under /_matrix will do. If we don't put it under /_matrix then we need to mess around with routers more, and the only reason why we don't want it there is "just in case" the spec suddenly uses that endpoint for something, which feels unlikely.

kegsay · 2020-10-06T10:19:56Z

clientapi/routing/sso.go

+	ssoURL string,
+) (string, error) {
+	// make the call to the sso server to validate
+	response, err := http.Get(ssoURL)


Please use an HTTP client here, as the default http.Get has no timeout.

kegsay · 2020-10-06T10:20:23Z

clientapi/routing/sso.go

+	}
+
+	// extract the response from the sso server
+	data, err := ioutil.ReadAll(response.Body)


Don't forget to defer response.Body.Close()

kegsay · 2020-10-06T10:21:35Z

clientapi/routing/sso.go

+	}
+	token, err := auth.GenerateLoginToken(account.UserID)
+	if err != nil || token == "" {
+		return util.JSONResponse{


Please do not drop err to the floor, log it.

kegsay · 2020-12-21T14:38:57Z

This PR will be closed on 23/12/2020 without further input.

anandv96 · 2020-12-22T16:22:56Z

This PR will be closed on 23/12/2020 without further input.

Sorry. I got busy with other work, I will start working on this again this week (have time off at the end of the year)

kegsay · 2021-01-05T10:38:13Z

Closing this for now. Please feel free to re-open/ping when you get back to this.

tommie · 2021-09-25T06:24:43Z

@anandv96 Can I take your code and fork this PR?

anandv96 · 2021-09-26T09:30:15Z

@anandv96 Can I take your code and fork this PR?

Yes of course! There isn't too much left to do IIRC

tommie · 2021-09-26T09:47:51Z

Yes of course! There isn't too much left to do IIRC

Thanks! Yeah, and the fact that you've already had review rounds is a strong motivator to not start over. :)

@anandv96

This is forked from @anandv96's matrix-org#1374. Closes matrix-org#1297.

@anandv96

This is forked from @anandv96's matrix-org#1374. Closes matrix-org#1297.

@anandv96

This is forked from @anandv96's matrix-org#1374. Closes matrix-org#1297.

@anandv96

This is forked from @anandv96's matrix-org#1374. Closes matrix-org#1297.

LoginToken and SSO Login initial changes

9dc798c

Database level changes not made

anandv96 force-pushed the master branch from 663e8c8 to 9dc798c Compare September 14, 2020 15:42

anandv96 changed the title ~~[WIP] Implement SSO login initial changes~~ [WIP] Implement m.login.token and SSO login initial changes Sep 14, 2020

kegsay reviewed Sep 17, 2020

View reviewed changes

Half-Shot reviewed Sep 19, 2020

View reviewed changes

clientapi/routing/sso.go Outdated Show resolved Hide resolved

anandv96 requested a review from kegsay September 21, 2020 13:08

anandv96 force-pushed the master branch from 6d5a446 to a13aea1 Compare September 21, 2020 13:20

Updated sso implementation

ef21bed

Changed with comments from Kegsay and Half-Shot - changed some return error codes - moved sso url creation &validation to startup time - added test to sytest whitelist

anandv96 force-pushed the master branch from a13aea1 to ef21bed Compare September 21, 2020 13:25

clokep reviewed Sep 25, 2020

View reviewed changes

kegsay suggested changes Oct 6, 2020

View reviewed changes

kegsay added the stale This issue or PR is at risk of being closed without further feedback label Dec 17, 2020

kegsay closed this Jan 5, 2021

tommie mentioned this pull request Sep 26, 2021

Login: Implement Token-based authentication #403

Closed

3 tasks

tommie mentioned this pull request Sep 26, 2021

Support for m.login.token #2014

Merged

2 tasks

tommie added a commit to tommie/dendrite that referenced this pull request Sep 27, 2021

Support for m.login.sso.

f2bccd7

This is forked from @anandv96's matrix-org#1374. Closes matrix-org#1297.

tommie added a commit to tommie/dendrite that referenced this pull request May 23, 2022

Support for m.login.sso.

4a4e7d6

This is forked from @anandv96's matrix-org#1374. Closes matrix-org#1297.

tommie added a commit to tommie/dendrite that referenced this pull request May 23, 2022

Support for m.login.sso.

43989aa

This is forked from @anandv96's matrix-org#1374. Closes matrix-org#1297.

samip5 pushed a commit to samip5/dendrite that referenced this pull request Jan 6, 2023

Support for m.login.sso.

6880886

This is forked from @anandv96's matrix-org#1374. Closes matrix-org#1297.

matrixbot mentioned this pull request Oct 31, 2024

[WIP] Implement m.login.token and SSO login initial changes element-hq/dendrite#1374

Closed

		// the service url that we send to CAS is homeserver.com/_matrix/client/r0/login/sso/redirect?redirectUrl=xyz
		ssoQueries.Set("service", req.RequestURI)

[WIP] Implement m.login.token and SSO login initial changes #1374

[WIP] Implement m.login.token and SSO login initial changes #1374

Conversation

anandv96 commented Sep 1, 2020

Pull Request Checklist

kegsay commented Sep 1, 2020

anandv96 commented Sep 2, 2020

kegsay commented Sep 2, 2020

anandv96 commented Sep 2, 2020

neilalexander commented Sep 4, 2020

kegsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandv96 commented Sep 21, 2020

anandv96 commented Sep 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kegsay commented Dec 21, 2020

anandv96 commented Dec 22, 2020

kegsay commented Jan 5, 2021

tommie commented Sep 25, 2021

anandv96 commented Sep 26, 2021

tommie commented Sep 26, 2021

anandv96 commented Sep 21, 2020 •

edited

Loading