diff --git a/docs/microsoft-account.md b/docs/microsoft-account.md index 9be71a56d7..6d2174e6b4 100644 --- a/docs/microsoft-account.md +++ b/docs/microsoft-account.md @@ -146,7 +146,9 @@ one is selected. "Sign out" deletes the account's secrets from the keychain. of its own, so launcher and CLI processes running at once lose nothing. The lock is never held while the keychain is asked (it can wait on an unlock prompt): a file entry the keychain took is removed only if it still holds what was read before, so another process's newer write stays and - is read first. Once the keychain refuses or doesn't answer (a dismissed prompt), the run stops + is read first. Writes to the keychain hold `account-secrets.keychain.lock` instead, with what they + write read under it, so a hand-over never puts back a token another process renewed or signed out + meanwhile; only keychain writes wait on a prompt, as they would on the keychain itself. Once the keychain refuses or doesn't answer (a dismissed prompt), the run stops asking it and keeps new sign-ins in the file, until it finds that another process handed the keychain a secret it put there. Every start hands the keychain what the file kept, one secret at a time up to the first it refuses; the rest @@ -155,7 +157,7 @@ one is selected. "Sign out" deletes the account's secrets from the keychain. only its line and column, never what it holds. Only sign-ins go there: the Tailscale access token (Internet servers) administers the player's whole tailnet, so without a keychain it stays in memory for the run (`account::keychain`). MCP's `account_list` says `secrets_file: true` while - sign-ins sit in the file. + the file holds a sign-in (sign-out marks alone, or a file that can't be read, don't count). - No keychain on macOS or Windows, or `HORIZON_NO_KEYCHAIN` (the launcher's own tests): secrets stay in memory for the run, and the launcher says "No keychain available: you are signed in until the launcher closes". diff --git a/launcher/src/account.rs b/launcher/src/account.rs index 6d64fc8a97..15ace6f865 100644 --- a/launcher/src/account.rs +++ b/launcher/src/account.rs @@ -613,8 +613,13 @@ impl FileStore { format!("{}: {e}", self.path.display()) } - /// Runs `f` holding the lock file (`account-secrets.lock`): other processes wait. + /// Runs `f` holding the file's lock (`account-secrets.lock`): other processes wait. fn locked(&self, f: impl FnOnce() -> Result) -> Result { + self.holding("lock", f) + } + + /// Runs `f` holding the lock file `account-secrets.`, made if need be. + fn holding(&self, name: &str, f: impl FnOnce() -> Result) -> Result { if let Some(dir) = self.path.parent() { std::fs::create_dir_all(dir).map_err(|e| self.shown(e))?; } @@ -622,13 +627,19 @@ impl FileStore { .create(true) .truncate(false) .write(true) - .open(self.path.with_extension("lock")) + .open(self.path.with_extension(name)) .map_err(|e| self.shown(e))?; lock.lock().map_err(|e| self.shown(e))?; // Closing the file releases the lock, after `f` whatever it answers. f() } + /// Whether the file holds a sign-in: not only sign-out marks, and readable. + fn holds_secrets(&self) -> bool { + self.entries() + .is_ok_and(|secrets| secrets.values().any(|secret| secret != SIGNED_OUT)) + } + fn read(&self) -> Result, String> { match std::fs::read(&self.path) { // Only where the file breaks: serde's own message can quote what the file holds. @@ -1110,8 +1121,11 @@ const KEYCHAIN_AWAY: &str = /// is the newer one), writes go to the keychain and to the file only when it refuses. /// /// The file's lock is never held across a keychain call, which can wait on an unlock prompt: other -/// processes keep reading and writing the file meanwhile. A file entry is only removed if it still -/// holds what was read before the keychain call, so another process's newer write stays. And once the +/// processes keep reading and writing the file meanwhile. Every write to the keychain holds a lock of +/// its own instead (`account-secrets.keychain.lock`), with what it writes read under it, so a hand-over +/// never puts back a secret another process renewed or signed out since; only keychain writes wait +/// on a prompt, as they would on the keychain itself. A file entry is only removed if it still holds +/// what was read before the keychain call, so another process's newer write stays. And once the /// keychain refuses or doesn't answer, this run stops asking it (no prompt after a dismissed /// prompt), until another process hands it a secret this run parked in the file. pub struct Layered { @@ -1124,7 +1138,8 @@ pub struct Layered { impl Layered { /// Starts by handing the keychain what the file kept while it was away, one secret at a time, - /// up to the first it refuses; sign-outs it missed are done there too. + /// up to the first it refuses; sign-outs it missed are done there too. Each is read again under + /// the keychain's lock: one another process renewed or signed out meanwhile isn't handed over. pub fn new(keychain: Box, file: FileStore) -> Self { let layered = Self { keychain, @@ -1132,21 +1147,29 @@ impl Layered { away: Default::default(), parked: Default::default(), }; - let mut moved = Vec::new(); - for (key, secret) in layered.file.entries().unwrap_or_default() { - let handed = layered.ask(|keychain| match secret.as_str() { - SIGNED_OUT => keychain.delete(&key), - _ => keychain.set(&key, &secret), + for key in layered.file.entries().unwrap_or_default().into_keys() { + let handed = layered.keychain_writes(|| { + let Some(secret) = layered.file.entry(&key)? else { + return Ok(()); + }; + layered.ask(|keychain| match secret.as_str() { + SIGNED_OUT => keychain.delete(&key), + _ => keychain.set(&key, &secret), + })?; + layered.file.take_unchanged(&[(key.clone(), secret)]) }); if handed.is_err() { break; } - moved.push((key, secret)); } - let _ = layered.file.take_unchanged(&moved); layered } + /// Runs `f`, a write to the keychain and what it reads first, holding the keychain's lock. + fn keychain_writes(&self, f: impl FnOnce() -> Result) -> Result { + self.file.holding("keychain.lock", f) + } + fn is_away(&self) -> bool { self.away.load(std::sync::atomic::Ordering::Relaxed) } @@ -1198,19 +1221,29 @@ impl SecretStore for Layered { self.ask(|keychain| keychain.get(key)) } fn set(&self, key: &str, secret: &str) -> Result<(), String> { - let before = self.file.entry(key)?; - match self.ask(|keychain| keychain.set(key, secret)) { - Ok(()) => self.unpark(key, before), - Err(_) => self.park(key, secret), + if self.is_away() { + return self.park(key, secret); } + self.keychain_writes(|| { + let before = self.file.entry(key)?; + match self.ask(|keychain| keychain.set(key, secret)) { + Ok(()) => self.unpark(key, before), + Err(_) => self.park(key, secret), + } + }) } fn delete(&self, key: &str) -> Result<(), String> { - let before = self.file.entry(key)?; - match self.ask(|keychain| keychain.delete(key)) { - Ok(()) => self.unpark(key, before), - // The keychain may hold a copy: the mark hides it until it can go. - Err(_) => self.park(key, SIGNED_OUT), + // The keychain may hold a copy: the mark hides it until it can go. + if self.is_away() { + return self.park(key, SIGNED_OUT); } + self.keychain_writes(|| { + let before = self.file.entry(key)?; + match self.ask(|keychain| keychain.delete(key)) { + Ok(()) => self.unpark(key, before), + Err(_) => self.park(key, SIGNED_OUT), + } + }) } fn persistent(&self) -> bool { true @@ -1282,11 +1315,12 @@ pub fn keychain() -> &'static dyn SecretStore { store() } -/// Whether sign-ins sit in the secrets file now, the keychain having refused them (#5095). +/// Whether sign-ins sit in the secrets file now, the keychain having refused them (#5095): sign-out +/// marks alone, or a file that can't be read, don't count. pub fn in_secrets_file() -> bool { cfg!(any(target_os = "linux", target_os = "freebsd")) && store().persistent() - && FileStore::launcher().path.exists() + && FileStore::launcher().holds_secrets() } // ----- Accounts ------------------------------------------------------------------------------------ diff --git a/launcher/src/account/tests.rs b/launcher/src/account/tests.rs index f5dc5ef1bf..938798b4d9 100644 --- a/launcher/src/account/tests.rs +++ b/launcher/src/account/tests.rs @@ -1380,6 +1380,80 @@ fn a_keychain_waiting_on_a_prompt_never_holds_the_file_up() { remove_secrets_dir(&path); } +/// A start handing the file's A to the keychain, held up mid-way by an unlock prompt, while a +/// launcher already running does `meanwhile` to A. The running one waits for the hand-over instead +/// of racing it, and goes on once the prompt is answered. +fn hand_over_racing( + name: &str, + meanwhile: impl FnOnce(&Layered) + Send + 'static, +) -> (Arc, PathBuf) { + let path = secrets_file(name); + let keychain = Arc::new(Keychain::default()); + // Started before the file had anything: it asked the keychain nothing. + let running = launcher_with(&keychain, &path); + // A run the keychain missed kept A in the file. + FileStore::new(path.clone()).set("A", "old-a").unwrap(); + let prompt = keychain.prompt.lock().unwrap(); + let asked = keychain.asked(); + let starting = { + let (keychain, path) = (keychain.clone(), path.clone()); + std::thread::spawn(move || drop(launcher_with(&keychain, &path))) + }; + while keychain.asked() == asked { + std::thread::sleep(Duration::from_millis(1)); + } + let racing = std::thread::spawn(move || meanwhile(&running)); + std::thread::sleep(Duration::from_millis(100)); + assert_eq!( + keychain.asked(), + asked + 1, + "the running launcher reached the keychain during the hand-over" + ); + drop(prompt); + starting.join().unwrap(); + racing.join().unwrap(); + (keychain, path) +} + +#[test] +fn a_hand_over_never_brings_back_an_account_signed_out_meanwhile() { + let (keychain, path) = hand_over_racing("race-sign-out", |running| { + running.delete("A").unwrap(); + }); + assert_eq!(keychain.kept.get("A").unwrap(), None); + assert_eq!(launcher_with(&keychain, &path).get("A").unwrap(), None); + assert!(!path.exists()); + remove_secrets_dir(&path); +} + +#[test] +fn a_hand_over_never_writes_over_a_token_renewed_meanwhile() { + let (keychain, path) = hand_over_racing("race-renewal", |running| { + running.set("A", "renewed-a").unwrap(); + }); + assert_eq!(keychain.kept.get("A").unwrap(), Some("renewed-a".into())); + assert_eq!( + launcher_with(&keychain, &path).get("A").unwrap(), + Some("renewed-a".into()) + ); + assert!(!path.exists()); + remove_secrets_dir(&path); +} + +#[test] +fn the_secrets_file_counts_only_sign_ins() { + let path = secrets_file("counts"); + let file = FileStore::new(path.clone()); + assert!(!file.holds_secrets()); + file.put("A", SIGNED_OUT).unwrap(); + assert!(!file.holds_secrets(), "a sign-out mark alone"); + file.put("B", "secret-b").unwrap(); + assert!(file.holds_secrets()); + std::fs::write(&path, "{\"B\":\"secret-b").unwrap(); + assert!(!file.holds_secrets(), "a file that can't be read"); + remove_secrets_dir(&path); +} + #[test] fn a_dismissed_prompt_is_asked_once_a_run() { let path = secrets_file("dismissed"); diff --git a/launcher/src/mcp.rs b/launcher/src/mcp.rs index 41c2c49254..05db2ebd6d 100644 --- a/launcher/src/mcp.rs +++ b/launcher/src/mcp.rs @@ -200,7 +200,7 @@ impl Session { "plays_as": accounts.selected().map(|a| a.name.clone()), "sign_in_configured": account::client_id().is_some(), // Sign-ins outlast the launcher: in the keychain, or in the secrets file - // where it refused them (`secrets_file`). + // where it refused them (`secrets_file`: the file holds a sign-in). "keychain": account::store().persistent(), "secrets_file": account::in_secrets_file(), // Play needs an account that owns the game (the owner gate).