kegsay commented 3 years ago

Spec: https://matrix.org/docs/spec/client_server/r0.6.1#sso-client-login Sytests:

    × Interactive authentication types include SSO
    × Can perform interactive authentication with SSO
    × The user must be consistent through an interactive authentication session with SSO
    × The operation must be consistent through an interactive authentication session
    × Can login with 3pid and password using m.login.password
    × login types include SSO
    × /login/cas/redirect redirects if the old m.login.cas login type is listed
    × Can login with new user via CAS

anandv96 commented 3 years ago

I'm working on this issue with @rohitmohan96 . Can you assign it to us?

duritong commented 3 years ago

Will CAS the only supported SSO mechanism or will you also add SAML? How will one be able todo usermapping?

olanod commented 2 years ago

PR was closed, nobody that could work on this? 😞

tommie commented 2 years ago

I'm forking @anandv96's PR and continuing. Since it included a solution to #403, and we need that first, I'll start by splitting and trying to get hat closed first.

gregistech commented 2 years ago

Is this still being worked on? This is the only thing holding me back from switching to Dandrite.

tommie commented 2 years ago

No. I'm waiting for a review of PR #2014. So far, no response.

ynerant commented 2 years ago

Hi! I am also very interested in this feature, this is the main reason I still using Synapse for a personal project. Do you know if 1) the PR #2014 is going to be merged soon, and 2) if that means that other login types will be supported once the PR got merged? Thanks!

kegsay commented 2 years ago

I am aiming to get #2014 merged soon yes, we're basically happy with it at this point. That is a pre-requisite for allowing SSO flows.

compgeniuses commented 2 years ago

I am curious About one thing, Why is this still in Opn state, if it has been done and merged, or was it only components of its implementation that were implemented.

If an issue was implemented, and merged, or Done, should it not be marked as closed state, so that other people could know its done.

Or maybe Add new tags like.

Partially-Done Being-implemented etcetra

tommie commented 2 years ago

It's not done. This was only a part of it. Using tags sounds like a good idea.

The next step is rebasing and creating the next PR from https://github.com/tommie/dendrite/commits/loginsso. @kegsay also asked me for some post-merge cleanups in #2014, so that also needs to get done.

compgeniuses commented 2 years ago

ok sure

scatterd commented 2 years ago

@tommie can you use some help with this? I built dendrite docker images in order to run my own instance. SSO is a feature I'd very much appreciate. I'm a newbie here on github and with Go as well, so I'm not entirely sure I can be very helpful. However, I do have a ready-to-go test environment and could at least help testing the code by integrating some SSO provider.

But I'm open for other tasks as well should you come up with a better idea.

tommie commented 2 years ago

The sad truth is I only use Dendrite for family, so the lack of SSO was solved by hard-coding users and passwords. :) So my intrinsic motivation disappeared after the initial work.

Thanks for the offer to help out. You can play around with https://github.com/tommie/dendrite/commits/loginsso and see what's missing there. PR #2014 was a base of that branch, but it contains the actual OAuth-logic.

Since there are multiple people who want this, I really should take the time to get the ball rolling on a new PR. I'll have a look tomorrow to assess how much merge conflict there is in the branch. (Also: the spec may have moved since I last looked at it, but that's probably a secondary concern. I don't remember if Sytests cover this functionality well.)

scatterd commented 2 years ago

The sad truth is I only use Dendrite for family, so the lack of SSO was solved by hard-coding users and passwords. :) So my intrinsic motivation disappeared after the initial work.

That's something I'm familiar with ;) Btw I too intend to use it for friends and family only at the moment.

Thanks for the offer to help out. You can play around with https://github.com/tommie/dendrite/commits/loginsso and see what's missing there. PR https://github.com/matrix-org/dendrite/pull/2014 was a base of that branch, but it contains the actual OAuth-logic.

Sure, I will take a look. Do not expect anything coming from this but I'll see what I can do.

Since there are multiple people who want this, I really should take the time to get the ball rolling on a new PR. I'll have a look tomorrow to assess how much merge conflict there is in the branch. (Also: the spec may have moved since I last looked at it, but that's probably a secondary concern. I don't remember if Sytests cover this functionality well.)

I really appreciate that, thanks!

tommie commented 2 years ago

Rebased my branch on main: https://github.com/tommie/dendrite/commit/c9ad7206c83baabf7a63cf1cf37295e865d53cae

The only change was that accountDB seems to not be leaking into clientapi/ nowadays. Using exposed UserAPI functions instead.

Looking through the code, it looks like account registration is missing. Synapse performs registration implicitly on SSO login, so it's a matter of creating an account, device and ID association if the SSO ID isn't recognized: https://github.com/matrix-org/synapse/blob/a00462dd9927558532b030593f8914ade53b7214/synapse/handlers/sso.py#L372

The code currently uses 3PID to store this association, but Synapse actually has a separate table for the (auth provider, 3PID) -> MXID mapping. I wonder if that matters: could the same email address be used for different accounts at different auth providers? Anyway, it also seems Synapse allows the user to not associate an email 3PID with the account, so perhaps this should be kept separate for that reason. Since the current code doesn't ensure the email received from IdP is verified, this needs to change. BTW, I'm not sure using email is the right approach for this assocation. Using the "sub" attribute is more stable: https://openid.net/specs/openid-connect-core-1_0.html#UserInfo

A complication is that Synapse also provides a way for the user to confirm what the localpart should be. See https://github.com/matrix-org/synapse/blob/a00462dd9927558532b030593f8914ade53b7214/synapse/handlers/sso.py#L569, https://github.com/matrix-org/synapse/blob/a00462dd9927558532b030593f8914ade53b7214/synapse/handlers/sso.py#L816 and https://github.com/matrix-org/synapse/blob/003cc6910af177fec86ae7f43683d146975c7f4b/synapse/res/templates/sso_auth_account_details.html

Essentially, SSO has its own registration sub-flow, separate from /register, and it will require a fair bit of more code to support: https://github.com/matrix-org/synapse/blob/a00462dd9927558532b030593f8914ade53b7214/synapse/handlers/sso.py#L542

I added a TODO-point for now: https://github.com/tommie/dendrite/blob/loginsso/clientapi/routing/sso.go#L178

Another thing is that only GitHub is implemented as an auth provider. That's probably good as a start. A smaller thing that's missing is support for OpenID Connect discovery URLs. It's perhaps more of a "nice to have" to make configuration/extension simpler.

Test coverage of SSO in SyTest is minimal. It only checks that m.login.sso is announced: https://github.com/matrix-org/sytest/blob/develop/tests/12login/02cas.pl

There are tests called "SSO", but they're really CAS tests: https://github.com/matrix-org/sytest/blob/develop/tests/10apidoc/13ui-auth.pl

It would be nice to have Sytests for this, including a fake Oauth2 server, to ensure it works. But my Perl isn't good enough for it to be worth it for me to write, I think.

SyTest requires this change: https://github.com/tommie/sytest/commit/dbf9bb1316fc6ce9d65435587676d7c54bcb4d28

Summary It's certainly not done yet. :)

tommie commented 2 years ago

Implemented separate SSO association storage and non-interactive account registration.

scatterd commented 2 years ago

Awesome! Thanks for taking the time to work on this :)

While I can't answer your questions concerning the implementation I instead tried to run your code. This config snippet gets dendrite up and running.

client_api:
  login:
    sso:
      enabled: true
      providers:
        - type: github
          id: github
          name: github.com
          oidc:
            client_id: abc123
            client_secret: abc123

But at this point I'm missing the redirectUrl. As far as I can tell the config key doesn't exist yet. Please correct me if I'm wrong.

tommie commented 2 years ago

The redirect URL is indeed internal. Synapse places it under /_synapse/client/oidc/callback, but I just placed it next to the speced redirect endpoint: https://github.com/tommie/dendrite/blob/loginsso/clientapi/routing/routing.go#L566

So that would be /_matrix/client/v3/login/sso/callback to register with GitHub.

What we send to the OIDC provider is that: https://github.com/tommie/dendrite/blob/loginsso/clientapi/routing/sso.go#L81

The callback will in turn redirect to the redirect_url provided by the client: https://github.com/tommie/dendrite/blob/loginsso/clientapi/routing/sso.go#L48

tommie commented 2 years ago

Looks like the client secret isn't sent when requesting the access token. I'm reworking the OIDC/OAuth2 bits. The code states GitHub is an OpenID Connect provider, which it isn't. It only supports OAuth2.

tommie commented 2 years ago

Made some fixes.

I remembered that I saw Complement being a Go replacement for SyTest, so I've been hacking on that:

Created an HTTP mock server.
Converted the trivial has-SSO-flow from SyTest.
Added a full SSO OIDC integration test.
Fixed things that were broken.

There's now a non-trivial chance logging in with Google might work, so I created a draft PR: https://github.com/matrix-org/dendrite/pull/2492

Edit I'm not adding a test for GitHub since there are hard-coded URLs, and I don't feel like making the URLs configurable just for testing that provider.

scatterd commented 2 years ago

Alright, I've done some tests with Google login.

I tried Element desktop but it wouldn't even query the available login schemes and I couldn't find a quick way to hardcode the URL. So I gave up on that one for the moment.

Then I switched to my mobile and tested with syphon. That didn't work either. But the app is currently in open alpha and from what I see it's the client at fault. I might open an issue over there later on.

Now it got interesting with element.io Google login get's advertised on the login screen and the client seems to do what it should. Here's an excerpt from my reverse proxy log.

GET /_matrix/client/r0/login
GET /_matrix/client/r0/login/sso/redirect/google?redirectUrl=https://app.element.io...

Then after logging in with Google comes the callback:

GET /_matrix/client/v3/login/sso/callback?provider=google&state=...

And this request doesn't contain the oidc_nonce cookie. So everything I get is

{"errcode":"M_MISSING_ARGUMENT","error":"no nonce cookie: http: named cookie not present"}

It's due to sameOrigin being set to strict. But that's where I'm stuck because the domain set in the cookie and the request's target domain are the same. If I set sameOrigin to none I do not comply with Google's validation rules anymore. At this point I have to admit that I haven't read the OAuth/OIDC specification yet. Guess it's time to do that now. It might make things clearer for me.

Anyways, if anyone visiting this issue is interested in doing it's own test here is the config I used:

client_api:
  login:
    sso:
      enabled: true
      callback_url: https://example.com/_matrix/client/v3/login/sso/callback
      providers:
        - id: google
          type: oidc
          name: google.com
          oidc:
            client_id: *******************.apps.googleusercontent.com
            client_secret: *******************
            discovery_url: https://accounts.google.com/.well-known/openid-configuration

tommie commented 2 years ago

Thanks for the testing!

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite

We have a navigation event or a redirect after a navigation event sending us to the callback, depending on provider.

Following the link in https://github.com/httpwg/http-extensions/issues/2104:

https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-10#section-5.2 says

A request is "same-site" if the following criteria are true:

The request is not the result of a cross-site redirect. That is, the origin of every url in the request's url list is same-site with the request's current url's origin.

So if the end of OAuth2 is a POST to IdP, with a redirect to Dendrite, that's not same-site.

This is what Synapse sets:

Max-Age=3600; Path=/_synapse/client/oidc; HttpOnly; Secure; SameSite=None

In fact, it also sets a oidc_session_no_samesite cookie without the SameSite attribute and Secure. I'm assuming that's some transition workaround. Confirmed here: https://github.com/matrix-org/synapse/blob/6ff99e3bea481790782c252c5433e9a88f65c4b0/synapse/handlers/oidc.py#L55

I'm hoping we don't need that workaround. I'll get that fixed.

scatterd commented 2 years ago

I see, that's why it's not sameSite. That does make sense.

Your latest commit fixed the nonce cookie problem for me. Thanks for that! Now I get the error message again saying

failed to retrieve OIDC access token
[...] because it doesn't comply with Google's OAuth 2.0 policy [...]

I'm here right now.

https://developers.google.com/identity/protocols/oauth2/web-server says client_id and redirect_uri are required auth parameters. Shouldn't they be part of the query?

query="map[authuser:[0] code:[...some string...] prompt:[none] provider:[google] scope:[email profile https://www.googleapis.com/auth/userinfo.profile https://www.googleapis.com/auth/userinfo.email openid] state:[...same as nonce...]]"

I'm not sure, might as well be mistaken. It would be nice if Google gave me a hint more useful.

tommie commented 2 years ago

The redirect_uri, client_id and others are sent in the POST body: https://github.com/tommie/dendrite/blob/loginsso/clientapi/auth/sso/oauth2.go#L119

More specifically https://developers.google.com/identity/protocols/oauth2/web-server#exchange-authorization-code

(I don't really understand why they require the redirect_uri there, since it's never returned.)

BTW, it's actually the OIDC we're using: https://developers.google.com/identity/protocols/oauth2/openid-connect#exchangecode

This is what my uncommitted test case sends:

client_id=aclientid&client_secret=aclientsecret&code=acode&grant_type=authorization_code&redirect_uri=%2F_matrix%2Fclient%2Fr0%2Flogin%2Fsso%2Fcallback%3Fprovider%3Dgoogle

It seems to be a URI with only a path, which is because req.URL doesn't contain the host: https://github.com/tommie/dendrite/blob/loginsso/clientapi/routing/sso.go#L181

Will fix. :)

tommie commented 2 years ago

Maybe we should move the testing discussion to https://github.com/matrix-org/dendrite/pull/2492 to keep the noise down on this FR.

tbrandirali commented 1 year ago

What the state of this feature? It's been pretty quiet for a while here...

genofire commented 1 year ago

2492 was closed :(

compgeniuses commented 1 year ago

so basically completed.

tommie commented 1 year ago

#2492 was killed, and external contributions have been shelved:

We recently updated our contributing guidelines. This PR is being closed because it isn't a feature we want to maintain going forwards. If you need this feature, it is possible to have a sidecar process handle registration, which upon success registers an account on Dendrite.

When we have more bandwidth as a team, we would be very interested in supporting this natively.