New login flow: Use OAuth2 JWT as external identifier #12830

aytchell · 2022-05-21T17:04:03Z

Pull Request Checklist

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
- Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
Pull request includes a sign off
Code style is correct
(run the linters)

This new login flow is an improved version of org.matrix.login.jwt The later * required the OICD provider's public key to be written to the homeserver.yml * accepted only one possible issuer of JWT tokens * has its own section in homeserver.yml and completely ignores 'oidc_providers' * Used the 'sub' from the JWT as localpart of the user's address * required the JWT to be sent in the body of the login request * Created a new user if it wasn't found - no configuration possible The new login flow (called org.matrix.login_oidc_jwt) * uses section 'oidc_providers' from homeserver.yml * accepts all configured providers as (possible) issuer of JWT tokens * uses the configured 'jwks_uri' to fetch the provider's public key * fetches the JWT either from the body and if there's none it checks the 'Authentication' http header * extracts iss/sub from JWT and searches the 'external_ids' of the user database. If such an entry is found the user is authorized; otherwise the user is rejected

New config parameters in section 'oidc_providers': * sso_jwt_enabled (boolean) - allows an admin to disable the new login flow for a given oidc_provider while at the same time keep the standard sso login flow for the oidc_provider allowed * standalone_jwt_audience (string) - if the new JWT login flow is allowed for an oidc_provider then this parameter can be used to enforce a specific audience claim to be contained in the JWT (otherwise the login attempt will be rejected)

Signed-off-by: Hannes Lerchl <[email protected]>

aytchell · 2022-05-21T18:42:49Z

[Edited by @richvdh for clearer formatting]

This new login flow is an improved version of org.matrix.login.jwt

The latter

required the OICD provider's public key to be written to the homeserver.yml
accepted only one possible issuer of JWT tokens
has its own section in homeserver.yml and completely ignores oidc_providers
Used the sub from the JWT as localpart of the user's address
required the JWT to be sent in the body of the login request
Created a new user if it wasn't found - no configuration possible

The new login flow (called org.matrix.login.sso_jwt)

uses section oidc_providers from homeserver.yml
accepts all configured providers as issuer of JWT tokens; might be disabled on a case by case basis
uses the configured jwks_uri to fetch the provider's public key
fetches the JWT either from the body and if there's none it checks the Authentication http header
extracts iss/sub from JWT and searches the external_ids of the user database. If such an entry is found the user is authorized; otherwise the user is rejected
This is especially useful if an administrator (or user) wants to chose the localpart but doesn't have control over the oidc provider

* generated sample_config * used black to format code * fixed tyoe hint findings of mypy

richvdh

Thanks very much for this contribution: I've not reviewed the code in detail, but it looks quite comprehensive.

I'm a bit confused about how this works though.

It's not really clear to me what benefit this offers over the existing JWT support (see https://matrix-org.github.io/synapse/latest/jwt.html). In particular, you appear to be skipping the OIDC flow (or at least, Synapse has no involvement with OIDC), so the use of oidc_providers seems rather arbitrary. I may well be missing something here though.

I'll also mention, for completeness, that there are efforts underway to replace authentication in Matrix with standard OAuth2 mechanisms (see matrix-org/matrix-spec-proposals#2964). Part of that work will see most of Synapse's authentication code (including the current JWT support and OidcHandler) ripped out, and replaced with an external authentication service (https://github.com/matrix-org/matrix-authentication-service).

richvdh · 2022-05-24T11:17:37Z

docs/sso_jwt.md

+[JSON Web Tokens](https://en.wikipedia.org/wiki/JSON_Web_Token)
+as external identifiers.
+The general mechanics is similar to other standard
+[authentication types](https://spec.matrix.org/v1.2/client-server-api/#authentication-types).


This link refers to "user-interactive authentication", which is not used by /_matrix/client/v3/login.

A more relevant link is https://spec.matrix.org/v1.2/client-server-api/#login, which documents the authentication types currently supported by /login.

richvdh · 2022-05-24T13:38:24Z

It's not really clear to me what benefit this offers over the existing JWT support

Oh, I've just seen your comment at #12830 (comment) - sorry, I should have read that first. But honestly, the list you provide there sound like things that should be fixed in the existing JWT support, rather than completely reinventing it:

required the OICD provider's public key to be written to the homeserver.yml

Ok, so let's add support for loading the keys from a URL to jwt_config...

accepted only one possible issuer of JWT tokens

... and support for multiple issuers

has its own section in homeserver.yml and completely ignores oidc_providers

This sounds like a feature, not a bug? Why should they be the same thing?

Used the sub from the JWT as localpart of the user's address

Yup, it would be nice to add a mapping mechanism to jwt_config

required the JWT to be sent in the body of the login request

Given it's a custom API, I don't see why this is a problem?

Created a new user if it wasn't found - no configuration possible

Also something we can add to jwt_config.

clokep · 2022-05-24T13:44:10Z

#9493 seems to implement some of these features for the current JWT flow.

aytchell · 2022-05-25T11:17:02Z

Thanks for your answers and thoughts

I'd like to reorder your responses so "the bigger part" is combined at the end

has its own section in homeserver.yml and completely ignores oidc_providers

This sounds like a feature, not a bug? Why should they be the same thing?

Because both sections basically configure "the same thing". If an admin wants to allow users to log in via a redirect-based OIDC flow as well as through JWT acquired from another app, then there'll be two similar config section which imo violate the DRY principle. On the other hand my overview of the system and its history is very limited.

required the JWT to be sent in the body of the login request

Given it's a custom API, I don't see why this is a problem?

The Authorization header is the "usual place" for a JWT according to OAuth2 and http client libs will place it there. This makes things easier for the caller.

Now these answers all seem to me to point towards org.matrix.login.jwt:

required the OICD provider's public key to be written to the homeserver.yml

Ok, so let's add support for loading the keys from a URL to jwt_config...

accepted only one possible issuer of JWT tokens

... and support for multiple issuers

created a new user if it wasn't found - no configuration possible

Also something we can add to jwt_config.

used the sub from the JWT as localpart of the user's address

Yup, it would be nice to add a mapping mechanism to jwt_config

The behaviour of my proposed mechanism is different from the one of org.matrix.login.jwt and I didn't want to "crash" existing installations. But what you say sounds to me like I should've added these things to ...login.jwt. Is that correct?

If so: how important is it to 100% keep the current behaviour intact or at least available?

richvdh · 2022-05-25T17:23:11Z

Because both sections basically configure "the same thing". If an admin wants to allow users to log in via a redirect-based OIDC flow as well as through JWT acquired from another app,

Right, it wasn't obvious that you wanted to allow users to log in via either OIDC or JWT (and use the same providers for both).

That seems like a bit of a niche requirement to me, to be honest. Could you expand on the usecase for it?

then there'll be two similar config section which imo violate the DRY principle.

Yeah, the problems here are actually worse than DRY, since Synapse will need to know that a user that arrives at /login directly with a JWT token is the same as a user that comes via OIDC.

I guess having an allow_standalone_jwt setting in oidc_providers is maybe not the worst situation.

The Authorization header is the "usual place" for a JWT according to OAuth2 and http client libs will place it there.

Kinda, but it seems to me that model assumes that you're going to use the JWT for the duration of your interaction with the system. In this case we're just using it for a single /login call, to exchange the JWT for a synapse's access token. It's not obvious to me that's really the intention of the Authorization: Bearer header.

That said, I don't see a huge issue with extending org.matrix.login.jwt to allow the token to be passed in an Authorization header as an alternative to passing it in the body.

The behaviour of my proposed mechanism is different from the one of org.matrix.login.jwt and I didn't want to "crash" existing installations. But what you say sounds to me like I should've added these things to ...login.jwt. Is that correct?

If so: how important is it to 100% keep the current behaviour intact or at least available?

The existing functionality is used, so we should avoid disrupting it more than absolutely necessary. But I think it should be possible to support both mechanisms via org.matrix.login.jwt ? Perhaps Synapse could fall back to the jwt_config if the issuer doesn't match any of the oidc providers?

richvdh · 2022-05-25T16:50:56Z

docs/sample_config.yaml

@@ -1986,6 +1986,20 @@ saml2_config:
 #       match a pre-existing account instead of failing. This could be used if
 #       switching from password logins to OIDC. Defaults to false.
 #
+#   sso_jwt_enabled: by using the login flow org.matrix.login.sso_jwt it


shouldn't this rather be called something like allow_standalone_jwt?

richvdh · 2022-05-25T16:53:13Z

docs/sample_config.yaml

+#   standalone_jwt_audience: if sso_jwt_enabled is 'true' (or not set)
+#       then we accept JWT token acquired via another application. In this case
+#       there's an additional check: the audience claim given in the token
+#       must contain this entry.
+#
+#       If there is no audience configured then this check is skipped.


why does this only apply in the standalone case? Is there not a usecase for restricting by audience in the OIDC case?

Also: I note that this is a list of acceptable audiences for jwt_config. We should probably have the same here, for conistency.

richvdh · 2022-05-25T17:06:34Z

docs/sample_config.yaml

+#       provider) via another application. If this should be disallowed
+#       _for this oidc_provider_ then set this value to 'false'.
+#
+#       If not set this defaults to 'true'.


should it not default to false? it seems like that would be safer.

also: if it is set, then there needs to be a JWKS for this provider, and some oidc providers do not have a jwks_uri. If allow_standalone_jwt is enabled for an oidc provider without a jwks_uri, we should detect that and report an error.

aytchell · 2022-05-31T05:54:28Z

Hi there,

I did some prototyping and I think I'm going to re-implement this PR (based on the then latest develop) by extending the org.matrix.login.jwt login flow (while keeping the current behavior as default).

Regarding your comments

the functionality of sso_jwt_enabled will then be handled by jwt_config.enable (which defaults to false)
standalone_jwt_audience will be handled via jwt_config.audiences(which already is a list of audiences)

I'm planning to

extract (most of) the code in synapse.rest.client.LoginServlet._do_jwt_login() to a new handler class which will then be located in synapse/handlers/jwt.py
keep the current mechanism with secret and algorithm (if and only if both are configured)
while additionally providing a mechanism via OIDC meta-data discovery
as well as with a manually configurable jwks_uri
for the sake of simplicity stick with only one possible issuer

aytchell · 2022-05-31T05:55:09Z

Also I have a (different) question: just today I stumbled upon the lines

synapse/rest/client/login.py line 426: payload = jwt.decode( and
synapse/handlers/oidc.py line 698 + 708: claims = jwt.decode(

and became aware that while login.py uses PyJWT, oidc.py uses authlib.

Would it be OK, if I drop PyJWT and use authlib for jwt login, too?

richvdh · 2022-05-31T10:34:10Z

Would it be OK, if I drop PyJWT and use authlib for jwt login, too?

Yes, rationalising this sounds like it would be great, thank you! Suggest making a separate PR for it though, rather than mixing it in with other functionality changes.

clokep · 2022-05-31T12:05:30Z

Might I also suggest that if the JWT logic becomes complicated, it could make sense to split some of it out into a separate handler (instead of embedding it all into the login REST servlet).

clokep · 2023-08-24T14:30:06Z

This was partially replaced by #13011. I suspect we should close this and if there are other bits that are still useful then please open new PRs @aytchell!

aytchell added 3 commits May 21, 2022 19:01

Added documentation for the new login flow

a107999

Signed-off-by: Hannes Lerchl <[email protected]>

aytchell requested a review from a team as a code owner May 21, 2022 17:04

Added changelog entry

92d04fd

Signed-off-by: Hannes Lerchl <[email protected]>

aytchell added 2 commits May 21, 2022 22:56

Fixed two failing unit tests

0963b65

Fixed failing PR tests

1778292

* generated sample_config * used black to format code * fixed tyoe hint findings of mypy

richvdh suggested changes May 24, 2022

View reviewed changes

richvdh reviewed May 26, 2022

View reviewed changes

aytchell mentioned this pull request Jun 9, 2022

Replace pyjwt with authlib in org.matrix.login.jwt #13011

Merged

4 tasks

clokep closed this Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New login flow: Use OAuth2 JWT as external identifier #12830

New login flow: Use OAuth2 JWT as external identifier #12830

aytchell commented May 21, 2022 •

edited

Loading

aytchell commented May 21, 2022 •

edited by richvdh

Loading

richvdh left a comment

richvdh May 24, 2022

richvdh commented May 24, 2022

clokep commented May 24, 2022

aytchell commented May 25, 2022

richvdh commented May 25, 2022

richvdh May 25, 2022

richvdh May 25, 2022

richvdh May 25, 2022

richvdh May 25, 2022

aytchell commented May 31, 2022 •

edited

Loading

aytchell commented May 31, 2022 •

edited

Loading

richvdh commented May 31, 2022

clokep commented May 31, 2022

clokep commented Aug 24, 2023

New login flow: Use OAuth2 JWT as external identifier #12830

New login flow: Use OAuth2 JWT as external identifier #12830

Conversation

aytchell commented May 21, 2022 • edited Loading

Pull Request Checklist

aytchell commented May 21, 2022 • edited by richvdh Loading

richvdh left a comment

Choose a reason for hiding this comment

richvdh May 24, 2022

Choose a reason for hiding this comment

richvdh commented May 24, 2022

clokep commented May 24, 2022

aytchell commented May 25, 2022

richvdh commented May 25, 2022

richvdh May 25, 2022

Choose a reason for hiding this comment

richvdh May 25, 2022

Choose a reason for hiding this comment

richvdh May 25, 2022

Choose a reason for hiding this comment

richvdh May 25, 2022

Choose a reason for hiding this comment

aytchell commented May 31, 2022 • edited Loading

aytchell commented May 31, 2022 • edited Loading

richvdh commented May 31, 2022

clokep commented May 31, 2022

clokep commented Aug 24, 2023

aytchell commented May 21, 2022 •

edited

Loading

aytchell commented May 21, 2022 •

edited by richvdh

Loading

aytchell commented May 31, 2022 •

edited

Loading

aytchell commented May 31, 2022 •

edited

Loading