Login method m.login.sso supporting GitHub and OpenID Connect

[tommie]

This is a first happy case. There's nowhere for the user to select a localpart, which makes it a bit useless in production. If the identity provider doesn't provide a preferred_username in the userinfo endpoint, we fall back to the normal "highest numeric localpart" as used in registration. An overview of HTML pages that need to be served by Dendrite to cover Synapse usecases is in config/sso.py.

A further improvement is to add a configuration option to limit who can use SSO. E.g. tying it to a custom domain using email and email_verified. After a cursory glance, I don't think Synapse has this ability, so it's a low priority.

Tested with tommie/complement@e08054c

Clarifications from comments

• This PR only implements /login, not /register. Supporting user-interactive authentication requires serving HTML pages, a big (or potentially contentious) task. A previous PR implemented m.login.token as a prerequisite for this PR; supporting SSO is a multi-PR endeavour.
• The configuration follows the code structure, and is not very user-friendly. The configuration format will evolve. It should have good defaults and make the simple cases terse.

TODOs from comments

• Merge oauth2 and oidc config parameters when using oidc. Infer type from the existence of either.
• Implement default_provider.
• Don't validate claims_supported, since it's speced as a partial list, and completely pointless.
• Don't require the path to be non-empty in client-provided URLs.
• Investigate if /register (or all) requests need to return some Unimplemented error code while user-interactive is unsupported. Cinny seems to never stop trying to register.

Signed-off-by: Tommie Gannert <[email protected]>

[scatterd]

So we are one step further now (or a couple) thanks to your last commit. The error I'm receiving now is

Failed to process callback      error="no \"sub\" in user info response body"

https://github.com/tommie/dendrite/blob/loginsso/clientapi/auth/sso/oauth2.go#L168

I took a look at the code but couldn't figure out if something needs to be fixed.

We should however have the right tokeninfo endpoint, right? https://openidconnect.googleapis.com/v1/userinfo
Because that's what is advertised in the openid discovery URL I have configured.

[tommie]

Hmm. I could see two issues:

1. it doesn't check the HTTP status, so this might actually be an error JSON returned from Google.
2. the Authorization header should probably be Bearer, not token.

Fixed those.

Status: I'm working on getting the Complement code in shape for a PR. Once this draft works for Google, I'll add some unit tests before it's ready for review. I won't have much time this weekend, though.

[scatterd]

Fixed those.

👍

I won't have much time this weekend, though.

Take your time, we're not in a hurry ;)
You've made so much more progress than I could've ever imagined anyways!

---

Okay I got pretty good news :)
Google Login works 🎉

The localpart is the sub which makes sense as it's truly unique. You mentioned there's no way to select it. I took a look at how matrix.org does it. There's this little web app at _synapse/client/pick_username/account_details which does exactly what the name says.

Anyways, that's another story. Great work tommie, kudos to you :)

[scatterd]

This is probably just a side note, as subs for localpart are not here to stay. But they're too big apparently.

userAPI.QueryNumericLocalpart failed          error="pq: value \"<myGoogleSub>\" is out of range for type integer"
´´´

[tommie]

Yay, milestone!

Okay, so if sub is being used for localpart, that means preferred_username isn't available in userinfo, which is expected.

I'd expect other OIDC providers to work as well, then.

The TODO for doing the account_details page is https://github.com/matrix-org/dendrite/pull/2492/files#diff-4acd21b365654e4ccdf129cb74d5bf44055715c0ae6cb89f5e8396615a119a44R217

But I think we should get the current code through review first. Less code is easier to review...

[tommie]

userAPI.QueryNumericLocalpart failed

That's strange. It looks like that's a function to generate a next higher numeric ID: https://github.com/matrix-org/dendrite/blob/main/userapi/storage/postgres/accounts_table.go#L67

const selectNewNumericLocalpartSQL = "" +
        "SELECT COALESCE(MAX(localpart::integer), 0) FROM account_accounts WHERE localpart ~ '^[0-9]*$'"

But combined with this comment: https://github.com/matrix-org/dendrite/blob/main/clientapi/routing/register.go#L555

	// Don't allow numeric usernames less than MAX_INT64.

Makes me think this is a Dendrite bug. It either shouldn't allow all-number localparts, or the MAX query should quietly ignore numbers that are too large. The code is used for guest accounts and when there's no requested localpart in registration.

[samip5]

It seems to show SSO buttons if registration is disabled, and that ought to work just like normal login, never calling /register.

The issue happens after completing the SSO flow, aka after I have returned from my IdP to Cinny.

[tommie]

In that case. Cinny sets the redirect URL to be the reg/login page: https://github.com/cinnyapp/cinny/blob/dev/src/client/action/auth.js#L18

On redirect, we end up with a loginToken: https://github.com/cinnyapp/cinny/blob/dev/src/app/templates/auth/Auth.jsx#L538

This causes Cinny to run perform m.login.token and then redirect to itself but with query params removed.

The app is supposed to instantiate Auth only if not authed: https://github.com/cinnyapp/cinny/blob/eef2d451b7a1332fa41d360aaa6dfbc29072beb3/src/client/state/auth.js#L7

The access token is set here: https://github.com/cinnyapp/cinny/blob/dev/src/client/action/auth.js#L52

If the access token is missing, we return to Auth useEffect. The loginToken state is never cleared on success, perhaps a redirect loop if access_token is missing? How that could be missing without client.login() failing, I don't understand. The m.login.token code looks fine to me.

[cknost]

This fork don't work with openid connect, because clientapi/auth/oauth2.go still use OAuth2, not oidc and the config validation prevents setting of oAuth2.

[tommie]

This fork don't work with openid connect, because clientapi/auth/oauth2.go still use OAuth2, not oidc and the config validation prevents setting of oAuth2.

Sorry, I don't understand what's wrong. What configuration is rejected in validation?

[samip5]

This fork don't work with openid connect, because clientapi/auth/oauth2.go still use OAuth2, not oidc and the config validation prevents setting of oAuth2.

I'm using OpenID Connect perfectly fine. :)

[cknost]

This fork don't work with openid connect, because clientapi/auth/oauth2.go still use OAuth2, not oidc and the config validation prevents setting of oAuth2.

Sorry, I don't understand what's wrong. What configuration is rejected in validation?

Clientid and client secret don't work.

[samip5]

Oh, I take that back.

The fields seem to be empty for client_id despite them being in the right place in config which happened after the config structure was changed.

[tommie]

Thanks for the clarification.

Yepp, I forgot to update the reading code when rearranging the config structure. Updated and merged from main.

[mklampfer]

Android matrix clients use a custom redirect url (element uses element://connect and fluffychat uses fluffychat.im://login) which cause an error on the server when trying to login using OIDC.

We found out that dendrite tries to parse the redirect url using url.parse, which fails on these custom urls. After commenting out the corresponding code in clientapi/routing/sso.go, lines 61 through 71, the error did not appear and login worked.

[tommie]

Android matrix clients use a custom redirect url (element uses element://connect and fluffychat uses fluffychat.im://login) which cause an error on the server when trying to login using OIDC.

Thanks for the report. If I play with those two URLs, it seems they should pass: https://go.dev/play/p/MqHeaM1yt9v

Is it failing with URL parse failure, or the second if-body?

Please also make sure you use the latest revision. The original didn't allow Path to be empty.

[samip5]

After pulling the new changes over (specifically the Docker changed made upstream), I'm not able to create a new container so not able to test unfortunately. :(

[mklampfer]

Is it failing with URL parse failure, or the second if-body?

It is actually failing in the secord if-body because ru.Path is empty.

[kegsay]

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.

[mpldr]

I think quite a number of people would consider this to be useful. What's your stance on a PAM-y approach where other authentication methods can be "plugged in" by providing a binary that acts as an interface between dendrite and whatever other service? I think that would be the easiest™ approach to allow OpenID support without massively increasing the burden on you to actually maintain it.

[tommie]

What's your stance on a PAM-y approach where other authentication methods can be "plugged in" by providing a binary that acts as an interface between dendrite and whatever other service?

Hmm, that's an interesting path forward. If Dendrite just implemented "trusted" authentication, we could write proxies that do all forms of authentication. The proxy just passes on an Authenticated-User header with the already verified user ID/device ID/session ID.

[genofire]

Something like https://dexidp.io/ which transfer oidc to LDAP, saml and so on?

[mpldr]

In a way… after all Matrix has some rather specific requirements which might not be available for all possible backends. So I think some (i.e. probably a lot) architecture work will still be necessary.