feat(windows): switch to the new auth flow per #2823 (#3147)

Also refactored to extract an auth state machine. The auth logic
previously was scattered throughout the GUI module, which would make it
hard to audit. Because of the refactoring I was able to add some simple
unit tests.

rust/connlib/shared/src/control.rs

@@ -34,7 +34,7 @@ pub type Reference = String;
 // TODO: Refactor this PhoenixChannel to use the top-level phoenix-channel crate instead.
 // See https://github.com/firezone/firezone/issues/2158
 pub struct SecureUrl {
-    inner: Url,
+    pub inner: Url,
 }
 impl SecureUrl {
     pub fn from_url(url: Url) -> Self {

rust/windows-client/src-tauri/Cargo.toml

@@ -18,6 +18,7 @@ clap = { version = "4.4", features = ["derive",  "env"] }
 connlib-client-shared = { workspace = true }
 connlib-shared = { workspace = true }
 firezone-cli-utils = { workspace = true }
+hex = "0.4.3"
 git-version = "0.3.9"
 # Same crate Hickory uses
 hostname = "0.3.1"
@@ -28,6 +29,7 @@ ring = "0.17"
 secrecy = { workspace = true }
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
+subtle = "2.5.0"
 thiserror = { version = "1.0", default-features = false }
 tokio = { version = "1.33.0", features = ["time"] }
 tracing = { workspace = true }
@@ -37,6 +39,7 @@ url = { version = "2.5.0", features = ["serde"] }
 uuid = { version = "1.5.0", features = ["v4"] }
 tracing-panic = "0.1.1"
 zip = { version = "0.6.6", features = ["deflate", "time"], default-features = false }
+rand = "0.8.5"
 windows-implement = "0.52.0"
 
 # These dependencies are locked behind `cfg(windows)` because they either can't compile at all on Linux, or they need native dependencies like glib that are difficult to get. Try not to add more here.

rust/windows-client/src-tauri/src/client.rs

@@ -3,6 +3,7 @@ use clap::Parser;
 use cli::CliCommands as Cmd;
 use std::{os::windows::process::CommandExt, process::Command};
 
+mod auth;
 mod cli;
 mod debug_commands;
 mod deep_link;

rust/windows-client/src-tauri/src/client/auth.rs

@@ -0,0 +1,354 @@
+//! Fulfills <https://github.com/firezone/firezone/issues/2823>
+
+use connlib_shared::control::SecureUrl;
+use rand::{thread_rng, RngCore};
+use secrecy::{ExposeSecret, Secret, SecretString};
+use subtle::ConstantTimeEq;
+use url::Url;
+
+const NONCE_LENGTH: usize = 32;
+
+#[derive(thiserror::Error, Debug)]
+pub(crate) enum Error {
+    #[error(transparent)]
+    Keyring(#[from] keyring::Error),
+    #[error("No in-flight request")]
+    NoInflightRequest,
+    #[error("State in server response doesn't match state in client request")]
+    StatesDontMatch,
+}
+
+type Result<T> = std::result::Result<T, Error>;
+
+pub(crate) struct Auth {
+    /// Key for secure keyrings, e.g. "dev.firezone.client/token" for releases
+    /// and something else for automated tests of the auth module.
+    keyring_key: &'static str,
+    state: State,
+}
+
+pub(crate) enum State {
+    SignedOut,
+    // TODO: Need a way to time out this state if the server never signs us in
+    NeedResponse(Request),
+    SignedIn(Session),
+}
+
+pub(crate) struct Request {
+    nonce: SecretString,
+    state: SecretString,
+}
+
+impl Request {
+    pub fn to_url(&self, auth_base_url: &Url) -> Secret<SecureUrl> {
+        let mut url = auth_base_url.clone();
+        url.query_pairs_mut()
+            .append_pair("as", "client")
+            .append_pair("nonce", self.nonce.expose_secret())
+            .append_pair("state", self.state.expose_secret());
+        Secret::from(SecureUrl::from_url(url))
+    }
+}
+
+pub(crate) struct Response {
+    pub actor_name: String,
+    pub fragment: SecretString,
+    pub state: SecretString,
+}
+
+pub(crate) struct Session {
+    pub actor_name: String,
+}
+
+impl Auth {
+    /// Creates a new Auth struct using the "dev.firezone.client/token" keyring key. If the token is stored on disk, the struct is automatically signed in.
+    ///
+    /// Performs I/O.
+    pub fn new() -> Result<Self> {
+        Self::new_with_key("dev.firezone.client/token")
+    }
+
+    /// Creates a new Auth struct with a custom keyring key for testing.
+    fn new_with_key(keyring_key: &'static str) -> Result<Self> {
+        let mut this = Self {
+            keyring_key,
+            state: State::SignedOut,
+        };
+
+        if this.get_token_from_disk()?.is_some() {
+            this.state = State::SignedIn(Session {
+                // TODO: Save and reload actor name to/from disk
+                actor_name: "TODO".to_string(),
+            });
+            tracing::debug!("Reloaded token");
+        }
+
+        Ok(this)
+    }
+
+    pub fn session(&self) -> Option<&Session> {
+        match &self.state {
+            State::SignedIn(x) => Some(x),
+            _ => None,
+        }
+    }
+
+    /// Mark the session as signed out, or cancel an ongoing sign-in flow
+    ///
+    /// Performs I/O.
+    pub fn sign_out(&mut self) -> Result<()> {
+        // TODO: After we store the actor name on disk, clear the actor name here too.
+        match self.keyring_entry()?.delete_password() {
+            Ok(_) | Err(keyring::Error::NoEntry) => {}
+            Err(e) => Err(e)?,
+        }
+        self.state = State::SignedOut;
+        Ok(())
+    }
+
+    /// Start a new sign-in flow, replacing any ongoing flow
+    ///
+    /// Returns parameters used to make a URL for the web browser to open
+    /// May return Ok(None) if we're already signed in
+    pub fn start_sign_in(&mut self) -> Result<Option<&Request>> {
+        self.sign_out()?;
+        self.state = State::NeedResponse(Request {
+            nonce: generate_nonce(),
+            state: generate_nonce(),
+        });
+        Ok(Some(self.ongoing_request()?))
+    }
+
+    /// Complete an ongoing sign-in flow using parameters from a deep link
+    ///
+    /// Returns a valid token.
+    /// Performs I/O.
+    ///
+    /// Errors if we don't have any ongoing flow, or if the response is invalid
+    pub fn handle_response(&mut self, resp: Response) -> Result<SecretString> {
+        let req = self.ongoing_request()?;
+
+        if !secure_equality(&resp.state, &req.state) {
+            self.sign_out()?;
+            return Err(Error::StatesDontMatch);
+        }
+
+        let token = format!(
+            "{}{}",
+            req.nonce.expose_secret(),
+            resp.fragment.expose_secret()
+        );
+        self.keyring_entry()?.set_password(&token)?;
+        self.state = State::SignedIn(Session {
+            actor_name: resp.actor_name,
+        });
+        Ok(SecretString::from(token))
+    }
+
+    /// Returns the token if we are signed in
+    ///
+    /// This may always make syscalls, but it should be fast enough for normal use.
+    pub fn token(&self) -> Result<Option<SecretString>> {
+        match self.state {
+            State::SignedIn(_) => {}
+            _ => return Ok(None),
+        }
+
+        self.get_token_from_disk()
+    }
+
+    /// Retrieves the token from disk regardless of in-memory state
+    ///
+    /// Performs I/O
+    fn get_token_from_disk(&self) -> Result<Option<SecretString>> {
+        Ok(match self.keyring_entry()?.get_password() {
+            Err(keyring::Error::NoEntry) => None,
+            Ok(token) => Some(SecretString::from(token)),
+            Err(e) => Err(e)?,
+        })
+    }
+
+    /// Returns an Entry into the OS' credential manager
+    ///
+    /// Anything you do in there is technically blocking I/O.
+    fn keyring_entry(&self) -> Result<keyring::Entry> {
+        Ok(keyring::Entry::new_with_target(self.keyring_key, "", "")?)
+    }
+
+    fn ongoing_request(&self) -> Result<&Request> {
+        match &self.state {
+            State::NeedResponse(x) => Ok(x),
+            _ => Err(Error::NoInflightRequest),
+        }
+    }
+}
+
+/// Generates a random nonce using a CSPRNG, then returns it as hexadecimal
+fn generate_nonce() -> SecretString {
+    let mut buf = [0u8; NONCE_LENGTH];
+    // rand's thread-local RNG is said to be cryptographically secure here: https://docs.rs/rand/latest/rand/rngs/struct.ThreadRng.html
+    thread_rng().fill_bytes(&mut buf);
+
+    // Make sure it's not somehow all still zeroes.
+    assert_ne!(buf, [0u8; NONCE_LENGTH]);
+    hex::encode(buf).into()
+}
+
+/// Checks if two byte strings are equal in constant-time.
+/// May not be constant-time if the lengths differ:
+/// <https://docs.rs/subtle/2.5.0/subtle/trait.ConstantTimeEq.html#impl-ConstantTimeEq-for-%5BT%5D>
+fn secure_equality(a: &SecretString, b: &SecretString) -> bool {
+    let a = a.expose_secret().as_bytes();
+    let b = b.expose_secret().as_bytes();
+    a.ct_eq(b).into()
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn bogus_secret(x: &str) -> SecretString {
+        SecretString::new(x.into())
+    }
+
+    #[test]
+    fn utils() {
+        // This doesn't test for constant-time properties, it just makes sure the function
+        // gives the right result
+        let f = |a: &str, b: &str| secure_equality(&bogus_secret(a), &bogus_secret(b));
+
+        assert!(f("1234", "1234"));
+        assert!(!f("1234", "123"));
+        assert!(!f("1234", "1235"));
+
+        let hex_string = generate_nonce();
+        let hex_string = hex_string.expose_secret();
+        assert_eq!(hex_string.len(), NONCE_LENGTH * 2);
+        dbg!(hex_string);
+
+        let auth_base_url = Url::parse("https://app.firez.one").unwrap();
+        let req = Request {
+            nonce: bogus_secret("some_nonce"),
+            state: bogus_secret("some_state"),
+        };
+        assert_eq!(
+            req.to_url(&auth_base_url).expose_secret().inner,
+            Url::parse("https://app.firez.one?as=client&nonce=some_nonce&state=some_state")
+                .unwrap()
+        );
+    }
+
+    #[test]
+    fn happy_path() {
+        // Start the program
+        let mut state =
+            Auth::new_with_key("dev.firezone.client/test_DMRCZ67A_happy_path/token").unwrap();
+
+        // Delete any token on disk from previous test runs
+        state.sign_out().unwrap();
+        assert!(state.token().unwrap().is_none());
+
+        // User clicks "Sign In", build a fake server response
+        let req = state.start_sign_in().unwrap().unwrap();
+
+        let resp = Response {
+            actor_name: "Jane Doe".into(),
+            fragment: bogus_secret("fragment"),
+            state: req.state.clone(),
+        };
+        assert!(state.token().unwrap().is_none());
+
+        // Handle deep link from the server, now we are signed in and have a token
+        state.handle_response(resp).unwrap();
+        assert!(state.token().unwrap().is_some());
+
+        // Accidentally sign in again, this can happen if the user holds the systray menu open while a sign in is succeeding.
+        // For now, we treat that like signing out and back in immediately, so it wipes the old token.
+        // TODO: That sounds wrong.
+        assert!(state.start_sign_in().unwrap().is_some());
+        assert!(state.token().unwrap().is_none());
+
+        // Sign out again, now the token is gone
+        state.sign_out().unwrap();
+        assert!(state.token().unwrap().is_none());
+    }
+
+    #[test]
+    fn no_inflight_request() {
+        // Start the program
+        let mut state =
+            Auth::new_with_key("dev.firezone.client/test_DMRCZ67A_invalid_response/token").unwrap();
+
+        // Delete any token on disk from previous test runs
+        state.sign_out().unwrap();
+        assert!(state.token().unwrap().is_none());
+
+        // If we get a deep link with no in-flight request, it's invalid
+        let r = state.handle_response(Response {
+            actor_name: "Jane Doe".into(),
+            fragment: bogus_secret("fragment"),
+            state: bogus_secret("state"),
+        });
+
+        match r {
+            Err(Error::NoInflightRequest) => {}
+            _ => panic!("Expected NoInflightRequest error"),
+        }
+
+        // Clean up the test token
+        state.sign_out().unwrap();
+    }
+
+    #[test]
+    fn states_dont_match() {
+        // Start the program
+        let mut state =
+            Auth::new_with_key("dev.firezone.client/test_DMRCZ67A_states_dont_match/token")
+                .unwrap();
+
+        // Delete any token on disk from previous test runs
+        state.sign_out().unwrap();
+        assert!(state.token().unwrap().is_none());
+
+        // User clicks "Sign In", build a fake server response
+        state.start_sign_in().unwrap();
+        let resp = Response {
+            actor_name: "Jane Doe".into(),
+            fragment: bogus_secret("fragment"),
+            state: SecretString::from(
+                "bogus state from a replay attack or browser mis-click".to_string(),
+            ),
+        };
+        assert!(state.token().unwrap().is_none());
+
+        // Handle deep link from the server, we should get an error
+        let r = state.handle_response(resp);
+        match r {
+            Err(Error::StatesDontMatch) => {}
+            _ => panic!("Expected StatesDontMatch error"),
+        }
+        assert!(state.token().unwrap().is_none());
+    }
+
+    #[test]
+    fn test_keyring() -> anyhow::Result<()> {
+        // I used this test to find that `service` is not used - We have to namespace on our own.
+
+        let name_1 = "dev.firezone.client/test_1/token";
+        let name_2 = "dev.firezone.client/test_2/token";
+
+        keyring::Entry::new_with_target(name_1, "", "")?.set_password("test_password_1")?;
+
+        keyring::Entry::new_with_target(name_2, "", "")?.set_password("test_password_2")?;
+
+        let actual = keyring::Entry::new_with_target(name_1, "", "")?.get_password()?;
+        let expected = "test_password_1";
+
+        assert_eq!(actual, expected);
+
+        keyring::Entry::new_with_target(name_1, "", "")?.delete_password()?;
+        keyring::Entry::new_with_target(name_2, "", "")?.delete_password()?;
+
+        Ok(())
+    }
+}

rust/windows-client/src-tauri/src/client/deep_link.rs

@@ -1,6 +1,7 @@
 //! A module for registering, catching, and parsing deep links that are sent over to the app's already-running instance
 //! Based on reading some of the Windows code from <https://github.com/FabianLars/tauri-plugin-deep-link>, which is licensed "MIT OR Apache-2.0"
 
+use crate::client::auth::Response as AuthResponse;
 use secrecy::SecretString;
 use std::{ffi::c_void, io, path::Path};
 use tokio::{io::AsyncReadExt, io::AsyncWriteExt, net::windows::named_pipe};
@@ -34,15 +35,9 @@ pub enum Error {
     WindowsRegistry(io::Error),
 }
 
-pub(crate) struct AuthCallback {
-    pub actor_name: String,
-    pub token: SecretString,
-    pub _identifier: SecretString,
-}
-
-pub(crate) fn parse_auth_callback(url: &url::Url) -> Option<AuthCallback> {
+pub(crate) fn parse_auth_callback(url: &url::Url) -> Option<AuthResponse> {
     match url.host() {
-        Some(url::Host::Domain("handle_client_auth_callback")) => {}
+        Some(url::Host::Domain("handle_client_sign_in_callback")) => {}
         _ => return None,
     }
     if url.path() != "/" {
@@ -50,8 +45,8 @@ pub(crate) fn parse_auth_callback(url: &url::Url) -> Option<AuthCallback> {
     }
 
     let mut actor_name = None;
-    let mut token = None;
-    let mut identifier = None;
+    let mut fragment = None;
+    let mut state = None;
 
     for (key, value) in url.query_pairs() {
         match key.as_ref() {
@@ -62,28 +57,28 @@ pub(crate) fn parse_auth_callback(url: &url::Url) -> Option<AuthCallback> {
                 }
                 actor_name = Some(value.to_string());
             }
-            "client_auth_token" => {
-                if token.is_some() {
-                    // client_auth_token must appear exactly once
+            "fragment" => {
+                if fragment.is_some() {
+                    // must appear exactly once
                     return None;
                 }
-                token = Some(SecretString::new(value.to_string()));
+                fragment = Some(SecretString::new(value.to_string()));
             }
-            "identity_provider_identifier" => {
-                if identifier.is_some() {
-                    // identity_provider_identifier must appear exactly once
+            "state" => {
+                if state.is_some() {
+                    // must appear exactly once
                     return None;
                 }
-                identifier = Some(SecretString::new(value.to_string()));
+                state = Some(SecretString::new(value.to_string()));
             }
             _ => {}
         }
     }
 
-    Some(AuthCallback {
+    Some(AuthResponse {
         actor_name: actor_name?,
-        token: token?,
-        _identifier: identifier?,
+        fragment: fragment?,
+        state: state?,
     })
 }
 
@@ -235,15 +230,16 @@ mod tests {
 
     #[test]
     fn parse_auth_callback() -> Result<()> {
-        let input = "firezone://handle_client_auth_callback/?actor_name=Reactor+Scram&client_auth_token=a_very_secret_string&identity_provider_identifier=12345";
+        let input = "firezone://handle_client_sign_in_callback/?actor_name=Reactor+Scram&fragment=a_very_secret_string&state=a_less_secret_string&identity_provider_identifier=12345";
         let input = url::Url::parse(input)?;
         dbg!(&input);
         let actual = super::parse_auth_callback(&input).unwrap();
 
         assert_eq!(actual.actor_name, "Reactor Scram");
-        assert_eq!(actual.token.expose_secret(), "a_very_secret_string");
+        assert_eq!(actual.fragment.expose_secret(), "a_very_secret_string");
+        assert_eq!(actual.state.expose_secret(), "a_less_secret_string");
 
-        let input = "firezone://not_handle_client_auth_callback/?actor_name=Reactor+Scram&client_auth_token=a_very_secret_string&identity_provider_identifier=12345";
+        let input = "firezone://not_handle_client_sign_in_callback/?actor_name=Reactor+Scram&fragment=a_very_secret_string&state=a_less_secret_string&identity_provider_identifier=12345";
         let actual = super::parse_auth_callback(&url::Url::parse(input)?);
         assert!(actual.is_none());
 

rust/windows-client/src-tauri/src/client/gui.rs

@@ -16,10 +16,7 @@ use secrecy::{ExposeSecret, SecretString};
 use std::{net::IpAddr, path::PathBuf, str::FromStr, sync::Arc, time::Duration};
 use system_tray_menu::Event as TrayMenuEvent;
 use tauri::{Manager, SystemTray, SystemTrayEvent};
-use tokio::{
-    sync::{mpsc, oneshot, Notify},
-    task::spawn_blocking,
-};
+use tokio::sync::{mpsc, oneshot, Notify};
 use ControllerRequest as Req;
 
 mod system_tray_menu;
@@ -224,16 +221,6 @@ pub(crate) enum ControllerRequest {
     SignOut,
 }
 
-// TODO: Should these be keyed to the Google ID or email or something?
-// The callback returns a human-readable name but those aren't good keys.
-fn keyring_entry() -> Result<keyring::Entry> {
-    Ok(keyring::Entry::new_with_target(
-        "dev.firezone.client/token",
-        "",
-        "",
-    )?)
-}
-
 #[derive(Clone)]
 struct CallbackHandler {
     logger: file_logger::Handle,
@@ -298,8 +285,10 @@ struct Controller {
     /// Debugging-only settings like API URL, auth URL, log filter
     advanced_settings: AdvancedSettings,
     app: tauri::AppHandle,
+    // Sign-in state
+    auth: client::auth::Auth,
     ctlr_tx: CtlrTx,
-    /// Session for the currently signed-in user, if there is one
+    /// connlib session for the currently signed-in user, if there is one
     session: Option<Session>,
     /// The UUIDv4 device ID persisted to disk
     /// Sent verbatim to Session::connect
@@ -312,19 +301,10 @@ struct Controller {
 
 /// Everything related to a signed-in user session
 struct Session {
-    auth_info: AuthInfo,
     callback_handler: CallbackHandler,
     connlib: connlib_client_shared::Session<CallbackHandler>,
 }
 
-/// Auth info that's persisted to disk if a session outlives an app instance
-struct AuthInfo {
-    /// User name, e.g. "John Doe", from the sign-in deep link
-    actor_name: String,
-    /// Secret token to authenticate with the portal
-    token: SecretString,
-}
-
 impl Controller {
     async fn new(
         app: tauri::AppHandle,
@@ -338,6 +318,7 @@ impl Controller {
         let mut this = Self {
             advanced_settings,
             app,
+            auth: client::auth::Auth::new()?,
             ctlr_tx,
             session: None,
             device_id,
@@ -346,32 +327,9 @@ impl Controller {
             notify_controller,
         };
 
-        tracing::trace!("re-loading token");
-        // spawn_blocking because accessing the keyring is I/O
-        if let Some(auth_info) = spawn_blocking(|| {
-            let entry = keyring_entry()?;
-            match entry.get_password() {
-                Ok(token) => {
-                    let token = SecretString::new(token);
-                    tracing::debug!("re-loaded token from Windows credential manager");
-                    let auth_info = AuthInfo {
-                        // TODO: Reload actor name from disk here
-                        actor_name: "TODO".to_string(),
-                        token,
-                    };
-                    Ok(Some(auth_info))
-                }
-                Err(keyring::Error::NoEntry) => {
-                    tracing::debug!("no token in Windows credential manager");
-                    Ok(None)
-                }
-                Err(e) => Err(anyhow::Error::from(e)),
-            }
-        })
-        .await??
-        {
+        if let Some(token) = this.auth.token()? {
             // Connect immediately if we reloaded the token
-            if let Err(e) = this.start_session(auth_info) {
+            if let Err(e) = this.start_session(token) {
                 tracing::error!("couldn't restart session on app start: {e:#?}");
             }
         }
@@ -380,7 +338,8 @@ impl Controller {
     }
 
     // TODO: Figure out how re-starting sessions automatically will work
-    fn start_session(&mut self, auth_info: AuthInfo) -> Result<()> {
+    /// Pre-req: the auth module must be signed in
+    fn start_session(&mut self, token: SecretString) -> Result<()> {
         if self.session.is_some() {
             bail!("can't start session, we're already in a session");
         }
@@ -394,7 +353,7 @@ impl Controller {
 
         let connlib = connlib_client_shared::Session::connect(
             self.advanced_settings.api_url.clone(),
-            auth_info.token.clone(),
+            token,
             self.device_id.clone(),
             None, // TODO: Send device name here (windows computer name)
             None,
@@ -403,7 +362,6 @@ impl Controller {
         )?;
 
         self.session = Some(Session {
-            auth_info,
             callback_handler,
             connlib,
         });
@@ -430,28 +388,13 @@ impl Controller {
     }
 
     async fn handle_deep_link(&mut self, url: &url::Url) -> Result<()> {
-        let Some(auth) = client::deep_link::parse_auth_callback(url) else {
+        let Some(auth_response) = client::deep_link::parse_auth_callback(url) else {
             // TODO: `bail` is redundant here, just do `.context("")?;` since it's `anyhow`
             bail!("couldn't parse scheme request");
         };
 
-        let token = auth.token.clone();
-        // spawn_blocking because keyring access is I/O
-        if let Err(e) = spawn_blocking(move || {
-            let entry = keyring_entry()?;
-            entry.set_password(token.expose_secret())?;
-            Ok::<_, anyhow::Error>(())
-        })
-        .await?
-        {
-            tracing::error!("couldn't save token to keyring: {e:#?}");
-        }
-
-        let auth_info = AuthInfo {
-            actor_name: auth.actor_name,
-            token: auth.token,
-        };
-        if let Err(e) = self.start_session(auth_info) {
+        let token = self.auth.handle_response(auth_response)?;
+        if let Err(e) = self.start_session(token) {
             // TODO: Replace `bail` with `context` here too
             bail!("couldn't start session: {e:#?}");
         }
@@ -464,12 +407,11 @@ impl Controller {
             return Ok(());
         };
         let resources = session.callback_handler.resources.load();
-        // TODO: Save the user name between runs of the app
-        let actor_name = self
-            .session
-            .as_ref()
-            .map(|x| x.auth_info.actor_name.as_str())
-            .unwrap_or("TODO");
+        let actor_name = &self
+            .auth
+            .session()
+            .ok_or_else(|| anyhow!("connlib is signed in but auth module isn't"))?
+            .actor_name;
         self.app
             .tray_handle()
             .set_menu(system_tray_menu::signed_in(actor_name, &resources))?;
@@ -524,16 +466,17 @@ async fn run_controller(
                         tracing::error!("couldn't handle deep link: {e:#?}");
                     }
                     Req::SignIn => {
-                        // TODO: Put the platform and local server callback in here
-                        tauri::api::shell::open(
-                            &app.shell_scope(),
-                            &controller.advanced_settings.auth_base_url,
-                            None,
-                        )?;
+                        if let Some(req) = controller.auth.start_sign_in()? {
+                            let url = req.to_url(&controller.advanced_settings.auth_base_url);
+                            tauri::api::shell::open(
+                                &app.shell_scope(),
+                                &url.expose_secret().inner,
+                                None,
+                            )?;
+                        }
                     }
                     Req::SignOut => {
-                        // TODO: After we store the actor name on disk, clear the actor name here too.
-                        keyring_entry()?.delete_password()?;
+                        controller.auth.sign_out()?;
                         if let Some(mut session) = controller.session.take() {
                             tracing::debug!("disconnecting connlib");
                             session.connlib.disconnect(None);
@@ -563,28 +506,3 @@ async fn run_controller(
     tracing::debug!("GUI controller task exiting cleanly");
     Ok(())
 }
-
-#[cfg(test)]
-mod tests {
-    #[test]
-    fn test_keyring() -> anyhow::Result<()> {
-        // I used this test to find that `service` is not used - We have to namespace on our own.
-
-        let name_1 = "dev.firezone.client/test_1/token";
-        let name_2 = "dev.firezone.client/test_2/token";
-
-        keyring::Entry::new_with_target(name_1, "", "")?.set_password("test_password_1")?;
-
-        keyring::Entry::new_with_target(name_2, "", "")?.set_password("test_password_2")?;
-
-        let actual = keyring::Entry::new_with_target(name_1, "", "")?.get_password()?;
-        let expected = "test_password_1";
-
-        assert_eq!(actual, expected);
-
-        keyring::Entry::new_with_target(name_1, "", "")?.delete_password()?;
-        keyring::Entry::new_with_target(name_2, "", "")?.delete_password()?;
-
-        Ok(())
-    }
-}