Add support for SSO login #174

dezeroku · 2024-05-13T23:05:11Z

Resolves #118

Implementation is mostly ported from the official CLI, with minor changes to utilise having predefined sso_id in the config file.

Tested with Google based SSO.

Auth flow

Can be summarised with:

Generate two random values, state and ssoCodeVerifier
hash and B64 escape the ssoCodeVerifier
spin up a local callback server (I assume port range is important here because of the allowed redirect_uris on Bitwarden end)
open a webbrowser initializing the SSO flow using the previously generated values
obtain the authorization_code via the callback server
replace the code for tokens by calling the /identity/token endpoint (same as for passwords, but different params)

Quirks

2FA

Due to the way 2FA is implemented in rbw (try to authenticate without 2FA, fallback to it as needed based on the response from server) SSO users with 2FA configured have to go through the "webbrowser flow" twice: once to get the response about the missing 2FA, second time to send 2FA alongside the SSO obtained code.
Bitwarden CLI reuses the authorization_code obtained in first iteration in such cases, but I feel this approach would require bigger overhaul in rbw.

Password

SSO flow doesn't use the master password in its auth, so theoretically we don't have to ask for it during login.
Of course it's still required for the unlock operation.

I kept the master password prompt in SSO flow to be in sync with the password flow, which automatically calls unlock after login.

Notes

Usual disclaimer: I don't really write Rust, feel free to suggest more idiomatic code wherever you see fit.
Especially the error handling feels like it could use some improvements

dezeroku · 2024-05-13T23:25:32Z

src/api.rs

+                let (sso_code, sso_code_verifier, callback_url) =
+                    self.obtain_sso_code(sso_id).await?;
+
+                connect_req = ConnectPasswordReq {


I'd like to separate it into ConnectPasswordReq and ConnectSSOReq, both "inheriting" fields from ConnectCommonReq, not sure if (or rather how) same effect can be achieved with composition.

something like https://serde.rs/attr-flatten.html would probably be what you want here

Renamed to ConnectTokenReq so it corresponds better to the /connect/token that it's meant for and added subtypes for specific auth methods/strategies.

doy

the overall structure looks good, but i had some feedback on a few of the implementation details

doy · 2024-05-21T04:01:32Z

src/config.rs


+    #[must_use]
+    pub fn ui_url(&self) -> String {
+        // TODO: default to either vault.bitwarden.com or vault.bitwarden.eu based on the base_url?


yeah, i think ideally we would follow the same pattern here that is used in identity_url and notifications_url

doy · 2024-05-21T04:05:08Z

src/api.rs

+        };
+
+        let callback_server =
+            async { start_sso_callback_server(&listener, state.as_str()) };


this isn't going to work like you want - an async block that doesn't await anything isn't actually running asynchronously, it's just going to block the thread when it runs, which isn't what we want. you'll want to use async networking calls (from tokio, or a library which uses it) to make this work.

doy · 2024-05-21T04:07:24Z

src/api.rs

+        ) {
+            // TODO: probably it'd be better to display the URL in the console if the automatic
+            // open operation fails, instead of failing the whole process? E.g. docker container
+            // case


yeah, i'd go farther here and say that we should explicitly support both options here (via a command line argument to rbw login or similar)

doy · 2024-05-21T04:10:32Z

src/api.rs

+        .1;
+
+    let states_match = received_state.split("_identifier=").next().unwrap()
+        == state.split("_identifier=").next().unwrap();


why are we splitting state here? isn't it just a purely random value?

Hmm, tbh I ported it as-is from the official implementation, it might be that it's used in more contexts there?

As indeed, this split doesn't make sense, state is purely random and is always alphanumeric, so we won't get any collisions with _identitifer=. I'll remove it

doy · 2024-05-21T04:14:44Z

src/api.rs

+fn start_sso_callback_server(
+    listener: &TcpListener,
+    state: &str,
+) -> Result<String> {


in general in this function, i'm not particularly comfortable with hand-parsing and writing raw http here. we already pull in enough libraries that we can do this in a less fragile way (i do kind of agree that we don't necessarily need a full blown web server here, but at least using hyper or http or something like that to parse and generate the http would be a lot less fragile)

I've refactored the whole thing to use axum, the worst change that it introduced is using channels for throwing the data around, instead of just the function return values (these now only handle HTTP stuff). A matter of preference I guess

I've tried to go with http and tokio networking calls, but still ended up with hand-crafting HTTP responses (maybe there is some nice http.Response -> bytes crate I wasn't able to find 😅 ) . Also reading from streams in async manner seemed to require more careful handling.
I've also played with hyper directly, but it was giving me strange issues (and still required channels for moving the data around).

With axum we also get query parameters parsing for free.

doy · 2024-05-21T04:15:56Z

src/api.rs

+                let (sso_code, sso_code_verifier, callback_url) =
+                    self.obtain_sso_code(sso_id).await?;
+
+                connect_req = ConnectPasswordReq {


something like https://serde.rs/attr-flatten.html would probably be what you want here

"desktop" client_id does not seem to work with refresh token obtained via SSO

dezeroku commented May 13, 2024

View reviewed changes

doy reviewed May 21, 2024

View reviewed changes

dezeroku added 6 commits June 19, 2024 03:43

Add support for SSO login

b06535a

apply clippy suggestions

74d096b

replace client_id values with cli for refreshing the token

6b3bfb9

"desktop" client_id does not seem to work with refresh token obtained via SSO

Separate ConnectToken structs per auth method

79d5925

Refactor SSO callback server to use axum

b13bad7

Cleaner error handling

f2e893e

dezeroku force-pushed the sso-auth branch from 83d2354 to f2e893e Compare June 19, 2024 01:50

Merge branch 'main' into sso-auth

20f6974

doy merged commit cb232b8 into doy:main Jun 19, 2024

Add support for SSO login #174

Add support for SSO login #174

Uh oh!

Conversation

dezeroku commented May 13, 2024

Auth flow

Quirks

2FA

Password

Notes

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!

doy left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants