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 https://github.com/tommie/complement/commit/e08054c34b918c150bf893c6ef1989f327e77229

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.

:+1:

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 :tada:

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.

[tommie]

• Filed a bug.
• Switched to using said numeric localpart generation instead of falling back to sub when preferred_username is missing.
• Breaking Pushed a configuration revamp. It moves the client_id and client_secret to an oauth2 subkey instead of oidc. You only need those two and brand: google for Google now. The rest is inferred.

[scatterd]

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

I agree on that.

Filed a bug.

Not just a side note then... :)

So meanwhile I continued testing. Made an auth0 account and successfully created an account in dendrite and logged in as well. Your assumption is therefore likely to be correct as this additional provider works too.

[tommie]

That's great news. Thanks!

[gedw99]

Would like to test this with Envoy. @tommie maybe have a setup for this.

[tommie]

@gedw99 No, sorry, I've only tested this with Complement.

[genofire]

Any updates?

[samip5]

Hmm, after trying to test this with Authentik (goauthentik.io), it seems to expect iss claim to be supported but I'm not sure why is that required?

Issue on Authentik about it: https://github.com/goauthentik/authentik/issues/3702

[tommie]

Thanks for testing!

Hmm, after trying to test this with Authentik (goauthentik.io), it seems to expect iss claim to be supported but I'm not sure why is that required?

Issue on Authentik about it: goauthentik/authentik#3702

I'm using it as the identifier for the service for OpenID Connect: https://github.com/matrix-org/dendrite/pull/2492/files#diff-43dbf8e7b26e9a9335430d8b9722da373a8027f28257923b2b5472f291a9bff9R171

Thanks for filing the issue with Authentik. Based on that repro, it seems it's a question of what Discovery returns in Authentik: https://openid.net/specs/openid-connect-discovery-1_0.html

The entire claims_supported is only marked recommended in the spec:

JSON array containing a list of the Claim Names of the Claims that the OpenID Provider MAY be able to supply values for. Note that for privacy or other reasons, this might not be an exhaustive list.

If claims_supported is present but not complete, it seems pointless to have it in the spec at all. However, the code is just a sanity check to fail a provider as early as possible, so the whole check can be removed if it's a problem.

[tommie]

I had a look through the code, and I don't think iss from user info or ID token is actually used anywhere, so this check should just be removed since it's causing issues with spec-compliant (but arguably non-cooperative) implementations. (It's probably just a left-over from when a version where I used the issuer URI as the provider identifier.)

[samip5]

I had a look through the code, and I don't think iss from user info or ID token is actually used anywhere, so this check should just be removed since it's causing issues with spec-compliant (but arguably non-cooperative) implementations. (It's probably just a left-over from when a version where I used the issuer URI as the provider identifier.)

I see that the check itself could say it as an warning if those don't exist but not generate an error of it as it's only recommended to have it, which may or may not be supported at all by the IdP.

[tommie]

I had a look through the code, and I don't think iss from user info or ID token is actually used anywhere, so this check should just be removed since it's causing issues with spec-compliant (but arguably non-cooperative) implementations. (It's probably just a left-over from when a version where I used the issuer URI as the provider identifier.)

I see that the check itself could say it as an warning if those don't exist but not generate an error of it as it's only recommended to have it, which may or may not be supported at all by the IdP.

To be fair, if they had completely left out claims_supported, this check would have passed. The code handles the "recommendation" aspect, but doesn't handle that it may be partial.

Now that I've read the spec more carefully, I think claims_supported is completely useless. It should be named nonstandard_claims_supported and only be used for unusual claims the IdP wishes to promote that the client couldn't expect the IdP to support. In light of that, I think the check should be removed completely. The warning would not be actionable to the Dendrite user.

[samip5]

I had a look through the code, and I don't think iss from user info or ID token is actually used anywhere, so this check should just be removed since it's causing issues with spec-compliant (but arguably non-cooperative) implementations. (It's probably just a left-over from when a version where I used the issuer URI as the provider identifier.)

I see that the check itself could say it as an warning if those don't exist but not generate an error of it as it's only recommended to have it, which may or may not be supported at all by the IdP.

To be fair, if they had completely left out claims_supported, this check would have passed. The code handles the "recommendation" aspect, but doesn't handle that it may be partial.

Now that I've read the spec more carefully, I think claims_supported is completely useless. It should be named nonstandard_claims_supported and only be used for unusual claims the IdP wishes to promote that the client couldn't expect the IdP to support. In light of that, I think the check should be removed completely. The warning would not be actionable to the Dendrite user.

That does seem troublesome, if supporting that whole thing is recommended but not at all required. I would also vote on removing it completely then if it's not really useful but rather can result in issues like mine where it's partial.

I don't know if this is intuitive config syntax for the user btw. I had first thought that the id and secret would also be under oidc.

client_api:
  login:
   sso:
    enabled: true
    providers:
     - id: authentik
       name: Authentik
       type: oidc
       oauth2:
         client_id: a6ae6d5e71260fa262cab88c7dc80d2014e22515
         client_secret: <snip>
       oidc:
         discovery_url: https://id.<snip>.fi/application/o/dendrite/.well-known/openid-configuration

[tommie]

Certainly a good point about the config, and I'll leave it to the Matrix team to decide on this.

The reason it's split is that OIDC is built on top of OAuth 2.0. Client ID and secret are defined for all OAuth 2.0 implementations (including GitHub). Discovery URI is an OIDC concept, and has no equivalent under OAuth 2.0.

I left it this way because it is the highest normal form of the config data, useful when reasoning about code structure. It would be more user-friendly to denormalize it and even infer type: oidc from which subkey exists:

     - id: authentik
       name: Authentik
       oidc:
         client_id: a6ae6d5e71260fa262cab88c7dc80d2014e22515
         client_secret: <snip>
         discovery_url: https://id.<snip>.fi/application/o/dendrite/.well-known/openid-configuration
     - id: github
       oauth2:
         client_id: ...
         client_secret: <snip>

I was hoping someone would speak up, so thanks for that. :)

(BTW, FTR, authentik is probably a bad ID in general. It should rather be id.<snip>.fi.)

[samip5]

I have tested this some more, and found out that Hydrogen and Flyffy will result in M_INVALID_ARGUMENT_VALUE due to redirect url. Element and Cinny does work though.

Hydrogen results in this url: https://hs/_matrix/client/r0/login/sso/redirect?redirectUrl=https://hydrogen.element.io and Flyffy uses https://hs/_matrix/client/r0/login/sso/redirect?redirectUrl=im.fluffychat://login

[tommie]

I have tested this some more,

Much appreciated.

Hydrogen results in this url: https://hs/_matrix/client/r0/login/sso/redirect?redirectUrl=https://hydrogen.element.io and Flyffy uses https://hs/_matrix/client/r0/login/sso/redirect?redirectUrl=im.fluffychat://login

I think this is because path is empty, failing this check: https://github.com/matrix-org/dendrite/pull/2492/files#diff-4acd21b365654e4ccdf129cb74d5bf44055715c0ae6cb89f5e8396615a119a44R66

For the non-HTTP URL, an empty path should definitely be accepted, and for the HTTP URL it doesn't matter, so the path check should be removed.

[samip5]

I have tested this some more,

Much appreciated.

Hydrogen results in this url: https://hs/_matrix/client/r0/login/sso/redirect?redirectUrl=https://hydrogen.element.io and Flyffy uses https://hs/_matrix/client/r0/login/sso/redirect?redirectUrl=im.fluffychat://login

I think this is because path is empty, failing this check: https://github.com/matrix-org/dendrite/pull/2492/files#diff-4acd21b365654e4ccdf129cb74d5bf44055715c0ae6cb89f5e8396615a119a44R66

For the non-HTTP URL, an empty path should definitely be accepted, and for the HTTP URL it doesn't matter, so the path check should be removed.

I compiled my own version without the path check, and it will just return an empty array with Hydrogen? It also logged this:

time="2022-09-30T18:29:31.320055971Z" level=error msg="Failed to get SSO authorization URL" error="unknown identity provider: " req.id=CMDCjR64OSvp req.method=GET req.path=/_matrix/client/r0/login/sso/redirect

[tommie]

Looks like Hydrogen implemented SSO a year ago: https://github.com/vector-im/hydrogen-web/pull/453

error: unknown identity provider: 

Probably from https://github.com/matrix-org/dendrite/pull/2492/files#diff-f090ff919cb4374ac16416becee1fb972e3252d341ab28d35a0a18a316f08084R70

Looks like providerID is empty when it gets there. I think this is because Hydrogen tries to use the SSO redirect with "default provider", but it seems I forgot to implement that.

This config option: https://github.com/matrix-org/dendrite/pull/2492/files#diff-84c40d4190c75903230c4c91a376c4c828031f5cd1a82968871a2496ce7e1e22R136

is supposed to be used instead of the empty string here: https://github.com/matrix-org/dendrite/pull/2492/files#diff-61c19b3366c81a067529fca47a1d0e7ae56d7bcde4e1bb75ba88137795a12a9eR621

or perhaps it's better to set it as a fallback at https://github.com/matrix-org/dendrite/pull/2492/files#diff-4acd21b365654e4ccdf129cb74d5bf44055715c0ae6cb89f5e8396615a119a44R46

Edit and if default_provider is empty, I suggest defaulting to the first provider.

[samip5]

Should I make some change PRs to your branch or are you going to make those regarding default_provider for example?

[tommie]

Since @neilalexander has picked this up now, I wasn't planning on making further changes unless asked to.

[neilalexander]

Since @neilalexander has picked this up now, I wasn't planning on making further changes unless asked to.

Please feel free, as this is still fairly low on my list at the moment. I'm hoping to dedicate some time to it next week.

[samip5]

It seems that if registration is disabled, except for shared secret, the SSO tries to create an account but seem to fail silently in the background? At least with Cinny that is.

It seems stuck:

[tommie]

I'm fairly sure what you see means that SSO created an account. Deleting the login token is the last thing to happen, and only happens on success. I don't what's going on with Cinny, but I'm guessing the client is not getting what it's expecting and retrying. So I don't think the new code in Dendrite is broken; just incomplete.

/register isn't part of the "login" flow, and it's not implemented in this PR, IIRC (see the note about missing ability to choose username). TBH, there are at least three reasons I didn't do this yet:

1. I want to get some code into master before spending more time,
2. Dendrite (like Synapse) needs to support serving HTML pages (the "fallback login") and
3. I didn't fully grok the spec when it comes to SSO account registration flow. It seems like registration should happen without requiring /register (what I inferred from Synapse code), but it can also do SSO "user-interactive" as part of /register. It could be that the intention is that implicitly created accounts are for "enterprise situations" and explicit registration is for public HS using 3P OIDC IdPs. I'm proud of that stretch of acronyms in one sentence.

• https://spec.matrix.org/v1.4/client-server-api/#client-login-via-sso (this PR)
• https://spec.matrix.org/v1.4/client-server-api/#sso-during-user-interactive-authentication (future PR)

You're probably also right (IIRC) that as long as you have SSO enabled, there are no restrictions on /login creating new accounts (because that's how private enterprise servers would want it.) Noted above about e.g. locking this down by email domain. This is part of the same configuration design question we discussed earlier, which I wanted to decide after we had some actual code.

Please let me know if you figure out why Cinny doesn't stop. Perhaps we should return an appropriate Unimplemented error code somewhere in /register.

[tommie]

@samip5 I've summarized our discussion so far in a TODO list in the PR description. I should be able to make some progress before the weekend.

[tommie]

Please feel free, as this is still fairly low on my list at the moment. I'm hoping to dedicate some time to it next week.

@neilalexander What is the general status of Dendrite work? What's the plan, roadmap, priority, etc.?