Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for login with OIDC. #859

Closed
pixlwave opened this issue Jul 19, 2022 · 8 comments
Closed

Add support for login with OIDC. #859

pixlwave opened this issue Jul 19, 2022 · 8 comments

Comments

@pixlwave
Copy link
Member

Is your feature request related to a problem? Please describe.
The authentication issuer is now available from the client since #820 was merged. The next step would be to add some form of OIDC service to the SDK and allow library users to perform a login with OIDC.

Describe the solution you'd like
As I see it the solution would be something along the following lines:

  • Add a login_oidc method to Client that is similar to login_sso but that uses some kind of service to do the following:
    1. Creates an OIDC service based on the client's authentication_issuer URL.
    2. Gets the configuration from the issuer and registers a new client on the server (this is persisted for use in the future).
    3. Uses a callback to inform the library user of the URL to perform web authentication through.
    4. The web authentication will complete with an OIDC auth response.
    5. This response can then be exchanged with the issuer to get an expiring access token and a refresh token.
  • Perform a /whoami with the access token to get the user ID and device ID to create a new session.
    • It would be nice to be able to get these back via the auth issuer rather than the homeserver.
  • Finish the login with a restore_login call.
  • Observe when the access token needs to be refreshed and use the service to get a new one.

Presumably the refresh token would need to be made available to library users alongside the access token for storage in the keychain etc (assuming this is how all apps do that part).

There's some sample code of what this dance looks like in the openidconnect crate docs, although presumably we can re-use whatever library MAS adopted.

Describe alternatives you've considered
We could do all this in the apps themselves via options such as AppAuth iOS/Android/JS, and simply add a login_access_token method to Client that performs the /whoami and restores with a session. However the requirement for access token expiry doesn't make this a great fit given that the app would also be responsible for refreshing the access token with the authentication issuer from time to time.

Additional context
https://github.com/sandhose/matrix-doc/blob/msc/sandhose/oauth2-profile/proposals/2964-oauth2-profile.md

@zecakeh
Copy link
Collaborator

zecakeh commented Jul 19, 2022

I looked in the code lately and it seems MAS implements almost everything itself, but mostly the server-side of things, of course.

It would probably nice to see with them if there are crates we can reuse. Using a crate that already works with oidc might be simpler for us though.

Edit: Looked into it more, we might only need the oauth2-types crate.

@zecakeh
Copy link
Collaborator

zecakeh commented Jul 20, 2022

Trying to use the oauth2-types crate results in a dependency version conflict over zeroize because of x25519-dalek's strict version pinning. There are already PRs to fix this, who knows when they'll be merged and released.

Error output:

error: failed to select a version for `zeroize`.
    ... required by package `elliptic-curve v0.12.2`
    ... which satisfies dependency `elliptic-curve = "^0.12"` of package `ecdsa v0.14.3`
    ... which satisfies dependency `ecdsa = "^0.14.3"` of package `mas-jose v0.1.0 (https://github.com/matrix-org/matrix-authentication-service?rev=fa1f714#fa1f7145)`
    ... which satisfies git dependency `mas-jose` of package `oauth2-types v0.1.0 (https://github.com/matrix-org/matrix-authentication-service?rev=fa1f714#fa1f7145)`
    ... which satisfies git dependency `oauth2-types` of package `matrix-sdk v0.5.0 (~/matrix-rust-sdk/crates/matrix-sdk)`
    ... which satisfies path dependency `matrix-sdk` of package `matrix-sdk-appservice v0.1.0 (~/matrix-rust-sdk/crates/matrix-sdk-appservice)`
versions that meet the requirements `^1.5` are: 1.5.6, 1.5.5, 1.5.4, 1.5.3

all possible versions conflict with previously selected packages.

  previously selected package `zeroize v1.3.0`
    ... which satisfies dependency `zeroize = "=1.3"` of package `x25519-dalek v1.2.0`
    ... which satisfies dependency `x25519-dalek = "^1.2.0"` of package `vodozemac v0.2.0 (https://github.com/matrix-org/vodozemac/?rev=2404f83f7d3a3779c1f518e4d949f7da9677c3dd#2404f83f)`
    ... which satisfies git dependency `vodozemac` of package `matrix-sdk-crypto v0.5.0 (~/matrix-rust-sdk/crates/matrix-sdk-crypto)`
    ... which satisfies path dependency `matrix-sdk-crypto` of package `benchmarks v1.0.0 (~/matrix-rust-sdk/benchmarks)`

failed to select a version for `zeroize` which could resolve this conflict

@jplatte
Copy link
Collaborator

jplatte commented Jul 20, 2022

Yay, again. I previously moved canonical-json stuff around in Ruma so the SDK wouldn't have to use ruma-signatures, which triggers the same error. (but that change made sense either way, the SDK shouldn't pull in all the event signing code)

@zecakeh
Copy link
Collaborator

zecakeh commented Jul 21, 2022

I made some progress, and right now I have a types incompatibility issue between MAS and openidconnect regarding the client registration response: MAS doesn't send all the fields that openidconnect expects.

In particular, openidconnect expects that the server sends back the client metadata, which is kind of mentionned in the OIDC Dynamic Client Registration spec, but it doesn't seem to be a MUST. The only place a MUST is mentioned for metadata is if a field's value was changed by the server.

So I'm thinking I'll try to use the oauth2-types crate after all, I'm not a fan of the openidconnect crate anyway.

@zecakeh
Copy link
Collaborator

zecakeh commented Jul 24, 2022

I've created matrix-org/matrix-authentication-service#313 to discuss improvements to oauth2-types for clients.

@zecakeh
Copy link
Collaborator

zecakeh commented Jul 27, 2022

So the good news is I have a working prototype (I could probably open a draft PR with it) that successfully logs in with MAS, and supports other OpenID Connect interactions. However it doesn't yet "finish" login by requesting the user ID from the homeserver. I also need to figure out how much data is needed from the users of the SDK, and when to request it.

One thing that would simplify this work is already implementing refresh tokens in the SDK.

The current implementation is quite involved, meaning that the requests are built manually, and some types are a pain to construct for users, but that should be improved upstream in the MAS crates ideally.

I realized that since only a subset of OpenID Connect is allowed by the MSCs, it doesn't make sense to expose the full oauth2-types to users of the SDK. For example, since only the code flow is allowed, we don't need to make available fields or variants that apply to other flows. It might be interesting to have a "wrapper" crate around the oauth2-types to simplify them, but it probably doesn't make sense to have it in the MAS repo, because the server still needs to support other flows to be spec compliant. I'd see it more in Ruma or the SDK itself.

@zecakeh
Copy link
Collaborator

zecakeh commented Aug 11, 2022

It's been decided to work on a client library as part of MAS, since it'll also need to interact with other OIDC servers.

There's a WIP PR at: matrix-org/matrix-authentication-service#347.

I could use it to make a PoC PR in the SDK, if someone is interested, although the API will likely change.

@pixlwave
Copy link
Member Author

Closing this one as completed by #1019 and subsequently #2412.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants