Implement REST password provider support #3193

raphaelbadawi · 2024-09-10T16:47:15Z

For now, compat login in MAS only supports local (DB) password authentication. But Synapse allows to configure a password_rest_provider in homeserver.yaml, to be able to authentify against services implementing the Identity Service API, such as MA1SD.

For this PR, I made it possible to configure a rest_auth_provider in the config.yaml, in the passwords block:

rest_auth_provider:
  url: "XXX" # the service endpoint implementing the Identity Service API
  version: "vX" # the implemented API version

If this configuration is filled, whenever a compat login occurs:

password is checked against the provider, not against the database
if the provider authenticates a user (so the user actually exists in the identity provider) but the user doesn't exist yet in MAS/Synapse, the user is created
whenever a device ID is present in the request body, it is reused, as per the matrix API spec

If this configuration is not filled, we fallback to the previous behaviour.

The same flow is applied when authenticating from the UI and when authenticating from the login API.

Signed-off-by: Raphaël Badawi raphael.badawi@ceo-vision.com

CLAassistant · 2024-09-10T16:47:21Z

All committers have signed the CLA.

…est password provider support

sandhose

Thanks a lot for this! I've quickly skimmed through it, so this is only a partial review for now.

I like the idea of pluggable password providers, but I would like to understand the use case, compared to using a lightweight IDP like Dex? This is what we use for LDAP deployments for instance.

I would also like to point out that we're considering disabling the legacy m.login.password API, at least by default; what is your use case exactly for keeping it?

sandhose · 2024-10-02T14:38:47Z

crates/handlers/src/views/login.rs

+            let result = password_manager
+                .hash(&mut rng, password.to_owned().into_bytes().into())
+                .await;
+            let (version, hashed_password) = match result {
+                Ok((version, hashed_password)) => (version, hashed_password),
+                Err(_err) => return Err(FormError::Internal),
+            };
+            repo.user_password()
+                .upsert(&mut rng, clock, &user, version, hashed_password)
+                .await
+                .map_err(|_err| FormError::Internal)?;


I'm a little confused on the usefulness of getting that persisted in the database still? Why would we want that, if we're talking to an external service to get verify the passwords?

sandhose · 2024-10-02T14:40:11Z

crates/handlers/src/views/login.rs

+    let user_password = repo
+        .user_password()
+        .active(&user)
+        .await
+        .map_err(|_| FormError::InvalidCredentials)?
+        .ok_or(FormError::InvalidCredentials)?;


Oh, is that why? To get an authentication session going on?
I feel like this should get recorded another way, maybe a new table recording authentications from an external password verification service?

Yes exactly I wanted to have an actual session going on after the call to the login API is made, so subsequent whoami calls and things like that actually work.

If there is a risk to mess things up with having every compat sessions on the same table, we can store REST password provided sessions in another table, but if it's not that risky it may be a bit overkill. In Synapse if you have REST password enabled all login requests go to the REST password provider, and it's the same with this implementation: if REST password provider is configured every compat login goes to it.

This is why it seemed logical to just use the same table, since when this config is enabled a compat session is unambiguously always a REST password provider compat session.

sandhose · 2024-10-02T14:41:18Z

crates/handlers/src/passwords.rs

@@ -35,6 +36,9 @@ struct InnerPasswordManager {

    /// A map of "old" hashers used only for verification
    other_hashers: HashMap<SchemeVersion, Hasher>,
+
+    /// The REST authentication provider URL, if any
+    rest_auth_provider: Option<RestAuthProviderConfig>,
 }

 impl PasswordManager {


It feels like the password verification logic should be somewhere here instead of in the compat module

You mean the whole call to the password provider (authenticate_via_rest_api)? This could be moved in the handlers crate, of course, but then we should also move user_login_with_password there, no?

sandhose · 2024-10-02T14:42:56Z

crates/handlers/Cargo.toml

@@ -34,6 +34,7 @@ tower-http.workspace = true
 axum.workspace = true
 axum-macros = "0.4.1"
 axum-extra.workspace = true
+reqwest = { version = "0.12", default-features = false, features = ["json", "rustls-tls"] }


So pulling reqwest might be something we want to do in the whole project. Right now we have a bit of a manual HTTP client based on hyper with a lot of metrics/tracing capabilities, and that use the right CA certificates & co.

I'll try to do a PR to move to reqwest overall, because it feels a lot simpler, but I would like to avoid having a mix of both reqwest and the current http client

Yes it's true such change should actually be in its own PR and be global. So for now I should use the current hyper implementation (I must admit, I didn't manage to make it work in my first attempts, but I will try again ;-) ).

sandhose · 2024-10-02T14:44:07Z

crates/config/src/sections/passwords.rs

+#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
+pub struct RestAuthProviderConfig {
+    /// The base URL where the identity service is implemented
+    pub url: String,


How does that work in terms of authentication, etc? It would be useful to include links here to specifications for the API of such services, if there is any

REST password providers are a kind of Synapse modules. What they do is calling an identity provider on login (e. g. MA1SD), which itself implements a Matrix Identity Service API.

It takes over all API login calls (e. g. all authentications using the login API are authenticated against the identity provider), and creates an account in the Synapse database when an authentication is successful for a user which doesn't exist yet in the database.

sandhose · 2024-10-02T14:44:30Z

crates/handlers/src/compat/login.rs

This feels like the wrong place to have all this logic

If authentication handlers are moved to ./crates/handlers/src/passwords.rs then those types will logically follow along.

sandhose · 2024-10-02T14:45:35Z

crates/handlers/src/compat/login.rs

+    #[serde(default, skip_serializing_if = "Option::is_none")]
+    device_id: Option<String>,


I'm really hesitant from adding support for this. The server doesn't have to respect this field. What's your use case?

I would like this feature to be split in another PR if you really need it so that we can discuss it separately

When login we pass a device_id unique to each user so we have a unique session per user coming from our web platform (otherwise each login to the platform would stack as a new session in Element and ask to be checked again which would be a mess).

Since it's actually in the Matrix specification, I felt like it was actually a missing piece from the compat crate. But yes, maybe it should sit in its own PR.

sandhose · 2024-10-02T14:47:15Z

crates/handlers/src/compat/login.rs

+    info!(
+        "Session and tokens saved successfully for user: {}",
+        user.username
+    );


Happy to add logging like that, but maybe at the debug level?

raphaelbadawi · 2024-10-02T15:48:22Z

Thanks a lot for this! I've quickly skimmed through it, so this is only a partial review for now.

I like the idea of pluggable password providers, but I would like to understand the use case, compared to using a lightweight IDP like Dex? This is what we use for LDAP deployments for instance.

I would also like to point out that we're considering disabling the legacy m.login.password API, at least by default; what is your use case exactly for keeping it?

Thanks for this first review which promises some discussions and clarifications to come. I will answer to every thread by the end of next week since I'm on vacation right now, but just to clarify the main purpose of all this, it is to have a replacement for the REST password provider for Synapse. We use a REST password provider to authenticate against a MA1SD instance (which implements the identity service API) which itself checks credentials against our own API.

Our current workflow is that a user can login to Element using our web platform credentials, with a simple Matrix login API call, then if MA1SD says the credentials are valid Synapse would actually create the user if it doesn't exist yet in its database and authenticate the user so we have a valid authenticated session. When login we pass a device_id unique to each user so we have a unique session per user coming from our web platform (otherwise each login to the platform would stack as a new session in Element and ask to be checked again which would be a mess).

I will check and answer every threads above at the end of next week, thanks a lot :-)

implement rest password provider support

52aecc4

Merge branch 'main' into rbadawi/rest-password-provider-support

7b03e3e

raphaelbadawi mentioned this pull request Sep 10, 2024

Compat login : ability to use a REST provider as a password provider #3168

Open

raphaelbadawi and others added 14 commits September 10, 2024 17:44

commit missing query data for rest password provider support

93a4638

Merge branch 'main' into rbadawi/rest-password-provider-support

bdf6859

replicate instantly on synapse on login registration occurring with r…

ef90a35

…est password provider support

amend rust style

0bb7c52

remove native-tls dependency

93fc3f1

normalize schema update

b2b1d6d

pass all unit tests

b174586

Merge branch 'main' into rbadawi/rest-password-provider-support

1c8db8b

Merge branch 'main' into rbadawi/rest-password-provider-support

42604b1

fix linter errors

81d04b2

Merge branch 'main' into rbadawi/rest-password-provider-support

f536e96

Merge branch 'main' into rbadawi/rest-password-provider-support

fe19a27

Merge branch 'main' into rbadawi/rest-password-provider-support

dded23b

fix fmt errors

5226f80

sandhose reviewed Oct 2, 2024

View reviewed changes

raphaelbadawi added 3 commits October 14, 2024 18:05

Merge branch 'main' into rbadawi/rest-password-provider-support

7dafef0

Merge branch 'main' into rbadawi/rest-password-provider-support

6fe85f1

Merge branch 'main' into rbadawi/rest-password-provider-support

8f8f871

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement REST password provider support #3193

Implement REST password provider support #3193

raphaelbadawi commented Sep 10, 2024 •

edited

Loading

CLAassistant commented Sep 10, 2024 •

edited

Loading

sandhose left a comment

sandhose Oct 2, 2024

sandhose Oct 2, 2024

raphaelbadawi Oct 11, 2024 •

edited

Loading

sandhose Oct 2, 2024

raphaelbadawi Oct 11, 2024 •

edited

Loading

sandhose Oct 2, 2024

raphaelbadawi Oct 11, 2024

sandhose Oct 2, 2024

raphaelbadawi Oct 11, 2024 •

edited

Loading

sandhose Oct 2, 2024

raphaelbadawi Oct 22, 2024

sandhose Oct 2, 2024

raphaelbadawi Oct 22, 2024 •

edited

Loading

sandhose Oct 2, 2024

raphaelbadawi commented Oct 2, 2024

		#[serde(default, skip_serializing_if = "Option::is_none")]
		device_id: Option<String>,

Implement REST password provider support #3193

Are you sure you want to change the base?

Implement REST password provider support #3193

Conversation

raphaelbadawi commented Sep 10, 2024 • edited Loading

CLAassistant commented Sep 10, 2024 • edited Loading

sandhose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelbadawi Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelbadawi Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelbadawi Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelbadawi Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raphaelbadawi commented Oct 2, 2024

raphaelbadawi commented Sep 10, 2024 •

edited

Loading

CLAassistant commented Sep 10, 2024 •

edited

Loading

raphaelbadawi Oct 11, 2024 •

edited

Loading

raphaelbadawi Oct 11, 2024 •

edited

Loading

raphaelbadawi Oct 11, 2024 •

edited

Loading

raphaelbadawi Oct 22, 2024 •

edited

Loading