experimental: Expose an OpenID Connect API #1019

zecakeh · 2022-09-08T18:06:32Z

This is a WIP implementation of OpenID Connect authentication using matrix-org/matrix-authentication-service#347.

I tried to write a thorough documentation in the oidc module on how to use it/what it supports.

This has been successfully tested against the synapse-oidc.lab.element.dev server from the OIDC Playground using the example CLI in examples/oidc-cli (logging and syncing). There is one quirk though, automatic refresh of token doesn't work because we don't seem to get the proper error type from Synapse.

It is currently not compatible with the synapse-auth0-oidc.lab.element.dev because of a metadata deserialization issue, and hasn't been tested with the 2 others because they don't have open registration according to the table.

Related to #859

zecakeh · 2022-09-08T18:13:07Z

@pixlwave I haven't worked on the FFI bindings, will you handle that yourself?

pixlwave · 2022-09-11T10:16:03Z

@zecakeh Yep, that’s totally fine by me, I’ll open another PR for the bindings when this gets merged 👍

Thanks for doing all the legwork on this 🙌

sandhose

I left a bunch of comments, but it looks good overall

A few things that I feel like I'm missing:

how does the refresh token logic work? Can we make it transparent for the user?
how transparent is it for an SDK user that it is an OIDC session?
why is there both a builder-pattern struct to build an authorization request and one as a function with a bunch of parameters?

Cargo.toml

sandhose · 2023-06-06T12:48:57Z

crates/matrix-sdk/src/oidc.rs

+//! [`AuthenticateError::InsufficientScope`]: ruma::api::client::error::AuthenticateError
+
+use chrono::Utc;
+pub use mas_oidc_client::*;


Is that reexport right?

I reduced it a bit but it's still not ideal, but it avoids to check which types need to be re-exported if we want users not to have to add the crate manually, like we do with Ruma.

The complicated part is that some types use a lot of other types (Verified)ProviderMetadata and (Verified)ClientMetadata among others, so we would have to hunt all the needed types.

It is however doable, if this is really not wanted.

crates/matrix-sdk/src/oidc.rs

sandhose · 2023-06-06T12:52:10Z

crates/matrix-sdk/src/oidc.rs

+    pub(crate) fn new(client: Client) -> Self {
+        Self { client }
+    }


This feels a bit weird to me. Why would we need to carry the Matrix API client? Wouldn't it be enough to configure the OIDC client with issuer + HttpService, and have a method to get one from the Matrix API Client?

OK, I guess it's for consistency with other 'modules' like that?

We need to keep the OIDC data in memory, and until now users only had to keep the Client around so it felt like a simpler API to keep the data in memory inside the Client.

Well, the Timeline API already deviates from this. I'm honestly not too happy with the ever-growing nature of Client. That said if it would be easy to misuse the API otherwise (which I think is not the case for the timeline), let's keep it as-is.

If we want to make refreshing tokens transparent, we also need the Client to be able to use this API. Of course we could also make the client keep an Arc<OidcInner> and give an Oidc { inner: Arc<OidcInner>} or something like that.

crates/matrix-sdk/src/oidc.rs

sandhose · 2023-06-06T14:29:32Z

crates/matrix-sdk/src/client/mod.rs

+    pub(crate) oidc_data: OnceCell<OidcData>,
+    /// The data needed to validate an OpenID Connect authorization request.
+    #[cfg(feature = "experimental-oidc")]
+    pub(crate) oidc_validation_data: Mutex<HashMap<String, AuthorizationValidationData>>,


It's probably OK for now, but it feels like it should be stored somewhere else, so that it can be serialized/stored and restored when it goes back to the client. For our use case it's probably fine, the Rust SDK will probably live through the whole flow

In OIDC clients for the browser, this kind of data is stored in the localstorage, and then the state parameter is there to support potentially multiple in-flight authorization, and restore associated parameters (in our case: homeserver, issuer, client registration infos)

crates/matrix-sdk/src/oidc.rs

jplatte · 2023-07-31T08:10:25Z

We no longer depend on zeroize <1.4 (#2339)! 🎉

As the next step for this PR, can you please rebase?

zecakeh · 2023-08-04T13:12:42Z

The force push was to make a rough rebase on top of #2377. The resulting code in c27440c does not work.

Commit 5853ac3 brings all the new changes.

Most of the TODOs that are left need changes in some of the MAS crates.

codecov · 2023-08-04T14:38:12Z

Codecov Report

Patch coverage: 3.01% and project coverage change: -0.93% ⚠️

Comparison is base (4643bae) 77.97% compared to head (6c58a49) 77.05%.
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1019      +/-   ##
==========================================
- Coverage   77.97%   77.05%   -0.93%     
==========================================
  Files         177      181       +4     
  Lines       18896    19130     +234     
==========================================
+ Hits        14734    14740       +6     
- Misses       4162     4390     +228

Files Changed	Coverage Δ
crates/matrix-sdk/src/error.rs	`63.15% <ø> (ø)`
crates/matrix-sdk/src/http_client/mod.rs	`75.00% <0.00%> (-8.34%)`	⬇️
crates/matrix-sdk/src/lib.rs	`100.00% <ø> (ø)`
crates/matrix-sdk/src/oidc/auth_code_builder.rs	`0.00% <0.00%> (ø)`
crates/matrix-sdk/src/oidc/data_serde.rs	`0.00% <0.00%> (ø)`
crates/matrix-sdk/src/oidc/end_session_builder.rs	`0.00% <0.00%> (ø)`
crates/matrix-sdk/src/oidc/mod.rs	`0.00% <0.00%> (ø)`
crates/matrix-sdk/src/client/mod.rs	`80.50% <12.50%> (-1.70%)`	⬇️
crates/matrix-sdk/src/authentication.rs	`40.00% <38.46%> (-20.00%)`	⬇️
crates/matrix-sdk/src/matrix_auth/mod.rs	`88.07% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…nnect Co-authored-by: Doug <[email protected]> Signed-off-by: Kévin Commaille <[email protected]>

…enID Connect

Signed-off-by: Kévin Commaille <[email protected]>

zecakeh · 2023-08-13T11:49:17Z

All the TODOs were handled so it should be good for review again.

crates/matrix-sdk/Cargo.toml

crates/matrix-sdk/src/oidc/data_serde.rs

jplatte · 2023-08-15T13:06:17Z

crates/matrix-sdk/src/oidc/auth_code_builder.rs

+    /// Set the requested Authentication Context Class Reference values.
+    ///
+    /// This is only necessary with specific providers.
+    pub fn acr_values(mut self, acr_values: HashSet<String>) -> Self {


If it's "Authentication Context Class Reference", why is the abbreviation acr instead of accr?

I don't know, but that's the abbreviation used by OIDC. Also this is the exact name of the field this is supposed to fill.

examples/oidc_cli/src/main.rs

Signed-off-by: Kévin Commaille <[email protected]>

zecakeh marked this pull request as draft September 8, 2022 18:06

stefanceriu mentioned this pull request Dec 22, 2022

Enable OIDC support element-hq/element-x-ios#261

Closed

ulyssa mentioned this pull request Mar 21, 2023

Signing in through one.ems.host using OpenID Connect ulyssa/iamb#61

Open

zecakeh force-pushed the oidc-mas branch from 46c9a7d to 6480880 Compare March 22, 2023 10:12

zecakeh force-pushed the oidc-mas branch from 87abaf8 to e1d5fd8 Compare April 27, 2023 15:52

pixlwave mentioned this pull request May 10, 2023

Remove AppAuth and prepare for Rust OIDC. element-hq/element-x-ios#870

Merged

manuroe requested a review from jplatte June 2, 2023 12:36

sandhose reviewed Jun 6, 2023

View reviewed changes

manuroe mentioned this pull request Jul 12, 2023

Element X Current Features Status element-hq/element-x-ios#1225

Open

manuroe added the Z-Schedule label Jul 26, 2023

zecakeh mentioned this pull request Aug 3, 2023

sdk: Don't derive (De)serialize for AuthSession #2378

Merged

zecakeh force-pushed the oidc-mas branch from e1d5fd8 to 7bb9d8f Compare August 4, 2023 13:09

zecakeh force-pushed the oidc-mas branch 2 times, most recently from 1a953db to f4ec8ec Compare August 4, 2023 13:47

zecakeh changed the title ~~PoC OIDC~~ experimental: Expose an OpenID Connect API Aug 4, 2023

zecakeh marked this pull request as ready for review August 4, 2023 13:58

zecakeh requested a review from a team as a code owner August 4, 2023 13:58

zecakeh force-pushed the oidc-mas branch 2 times, most recently from 97071c1 to 5853ac3 Compare August 4, 2023 14:10

bnjbvr removed the request for review from a team August 10, 2023 12:56

manuroe added the X-Needs-Rust label Aug 11, 2023

zecakeh and others added 4 commits August 13, 2023 11:44

feat(sdk): Add experimental support for authenticating with OpenID Co…

fa2b8d3

…nnect Co-authored-by: Doug <[email protected]> Signed-off-by: Kévin Commaille <[email protected]>

fixup! feat(sdk): Add experimental support for authenticating with Op…

cc874ab

…enID Connect

Upgrade mas-oidc-client

192f8a8

Signed-off-by: Kévin Commaille <[email protected]>

Add support for RP-Initiated Logout

4581982

Signed-off-by: Kévin Commaille <[email protected]>

zecakeh added 3 commits August 13, 2023 13:28

Add support for all auth code parameters

ed88c4c

Signed-off-by: Kévin Commaille <[email protected]>

Add support for Pushed Authorization Request

9d837f1

Signed-off-by: Kévin Commaille <[email protected]>

Remove outdated todo

d765658

Signed-off-by: Kévin Commaille <[email protected]>

zecakeh force-pushed the oidc-mas branch from 5853ac3 to d765658 Compare August 13, 2023 11:47

zecakeh requested a review from sandhose August 13, 2023 11:49

jplatte reviewed Aug 15, 2023

View reviewed changes

examples/oidc_cli/src/main.rs Outdated Show resolved Hide resolved

Handle review feedback

6c58a49

Signed-off-by: Kévin Commaille <[email protected]>

jplatte approved these changes Aug 16, 2023

View reviewed changes

jplatte merged commit 0dac508 into matrix-org:main Aug 16, 2023

pixlwave mentioned this pull request Aug 16, 2023

feat(bindings): OIDC in AuthenticationService. #2412

Merged

3 tasks

pixlwave mentioned this pull request Aug 24, 2023

Add support for login with OIDC. #859

Closed

zecakeh deleted the oidc-mas branch August 27, 2023 07:29

experimental: Expose an OpenID Connect API #1019

experimental: Expose an OpenID Connect API #1019

Uh oh!

Conversation

zecakeh commented Sep 8, 2022

Uh oh!

zecakeh commented Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pixlwave commented Sep 11, 2022

Uh oh!

sandhose left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zecakeh Jun 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jplatte commented Jul 31, 2023

Uh oh!

zecakeh commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zecakeh commented Aug 13, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

zecakeh commented Sep 8, 2022 •

edited

Loading

zecakeh Jun 13, 2023 •

edited

Loading

zecakeh commented Aug 4, 2023 •

edited

Loading

codecov bot commented Aug 4, 2023 •

edited

Loading