From b9c028aa06dee38d6758fe303d8be17db81db0c9 Mon Sep 17 00:00:00 2001 From: Alvin Chang <1977968+alvin-chang@users.noreply.github.com> Date: Fri, 12 Jun 2026 21:07:24 +0100 Subject: [PATCH] fix(credential-store): mirror .encryption_key instead of deleting it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The keyring backend in resolve_key() was auto-deleting the .encryption_key fallback file when a key was found in the OS keyring. The function docstring explicitly stated the file should never be deleted ('it always serves as a durable fallback for environments where the keyring is ephemeral'), but the code contradicted the doc. This caused a TOCTOU bug: when a user switched backends (e.g. from GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=file to default keyring), the keyring would return a stale entry from before the file backend was used, then delete the .encryption_key file (which contained the actual current key). This made credentials.enc undecryptable, triggering a second cleanup that deleted the credentials themselves — silently wiping the user's authenticated session. The fix mirrors the keyring key to the .encryption_key file (keeping the file in sync with the keyring) instead of deleting it. Per Gemini code review, the mirror uses the existing save_key_file (atomic_write with 0o600 permissions + fsync) and ensure_key_dir (0o700 on parent) helpers to keep file permissions restrictive and the write race-free. A second optimization (also per Gemini) only invokes the fsync when the file content does not already match the keyring key, eliminating per-invocation disk wear on the common case. Error messages are sanitized via sanitize_for_terminal. This matches the function docstring's design intent and prevents the silent-wipe bug. Tests (29/29 pass): - Renamed 'cleans_up_legacy_file_*' to 'mirrors_legacy_file_*' to reflect the new behavior - Updated assertions: file now exists after the operation and contains the keyring key - New: 'keyring_backend_skips_write_when_file_already_has_correct_key' verifies the fsync is skipped when the file matches (via mtime check) - New: 'keyring_backend_writes_when_file_has_stale_key' verifies the file IS updated when the content doesn't match - Linux fallback tests already mirrored this behavior; macOS/Windows tests now match Reported-by: Alvin Chang (alvin@goodciso.org) Original-patch-author: Cole (AI closer agent, goodciso.org) Tested-by: Cole (Cole, 2026-06-12) Co-authored-by: Cole Co-authored-by: Alvin Chang --- .changeset/fix-credential-store-toctou.md | 5 + .../src/credential_store.rs | 144 +++++++++++++++--- 2 files changed, 130 insertions(+), 19 deletions(-) create mode 100644 .changeset/fix-credential-store-toctou.md diff --git a/.changeset/fix-credential-store-toctou.md b/.changeset/fix-credential-store-toctou.md new file mode 100644 index 00000000..8b2b42c7 --- /dev/null +++ b/.changeset/fix-credential-store-toctou.md @@ -0,0 +1,5 @@ +--- +"@googleworkspace/cli": patch +--- + +Fix silent auth wipe: the keyring backend in `resolve_key()` was auto-deleting the `.encryption_key` fallback file when a key was found in the OS keyring, contradicting the function's own docstring. The fix mirrors the keyring key to the file (via `save_key_file` + `ensure_key_dir` for atomic write with 0o600 permissions and 0o700 parent) instead of deleting it. Switching backends mid-session no longer wipes credentials. diff --git a/crates/google-workspace-cli/src/credential_store.rs b/crates/google-workspace-cli/src/credential_store.rs index ffc6db25..5a30e0e6 100644 --- a/crates/google-workspace-cli/src/credential_store.rs +++ b/crates/google-workspace-cli/src/credential_store.rs @@ -210,14 +210,32 @@ fn resolve_key( if decoded.len() == 32 { let mut arr = [0u8; 32]; arr.copy_from_slice(&decoded); - // Cleanup insecure file fallback if it still exists. - // TOCTOU race condition is a known limitation. - if let Err(e) = std::fs::remove_file(key_file) { - if e.kind() != std::io::ErrorKind::NotFound { + // Mirror the keyring's key to the .encryption_key file + // so the file stays a durable fallback for headless/CI envs. + // Per the function docstring, the file is never deleted. + // Uses save_key_file (atomic_write with 0o600 + fsync) and + // ensure_key_dir (0o700 on parent) to keep file permissions + // restrictive and the write race-free. + // Per Gemini code review, only write when the existing file + // content does not already match the keyring key — avoids + // unnecessary fsync + disk wear on every CLI invocation. + let needs_write = match read_key_file(key_file) { + Some(existing) => existing != arr, + None => true, + }; + if needs_write { + if let Err(e) = ensure_key_dir(key_file) { eprintln!( - "Warning: failed to remove legacy key file at '{}': {}", + "Warning: failed to set up key directory '{}': {}", key_file.display(), - e + sanitize_for_terminal(&e.to_string()) + ); + } + if let Err(e) = save_key_file(key_file, b64_key.trim()) { + eprintln!( + "Warning: failed to mirror keyring key to '{}': {}", + key_file.display(), + sanitize_for_terminal(&e.to_string()) ); } } @@ -243,12 +261,32 @@ fn resolve_key( sanitize_for_terminal(&e.to_string()) ); } - if let Err(e) = std::fs::remove_file(key_file) { - if e.kind() != std::io::ErrorKind::NotFound { + // Mirror the new key to the .encryption_key file as a durable fallback + // for headless/CI environments. Per the function docstring, the file + // is never deleted. Uses save_key_file (atomic_write with 0o600 + fsync) + // and ensure_key_dir (0o700 on parent) to keep file permissions + // restrictive and the write race-free. + // Per Gemini code review, only write when the existing file content does + // not already match — avoids unnecessary fsync + disk wear on every + // CLI invocation. (For a freshly generated key, the file should never + // match, so this is a defensive check that costs only one file read.) + let needs_write = match read_key_file(key_file) { + Some(existing) => existing != key, + None => true, + }; + if needs_write { + if let Err(e) = ensure_key_dir(key_file) { eprintln!( - "Warning: failed to remove legacy key file at '{}': {}", + "Warning: failed to set up key directory '{}': {}", key_file.display(), - e + sanitize_for_terminal(&e.to_string()) + ); + } + if let Err(e) = save_key_file(key_file, b64_key.trim()) { + eprintln!( + "Warning: failed to mirror keyring key to '{}': {}", + key_file.display(), + sanitize_for_terminal(&e.to_string()) ); } } @@ -581,31 +619,39 @@ mod tests { #[test] #[cfg(any(target_os = "macos", target_os = "windows"))] - fn keyring_backend_cleans_up_legacy_file_on_success() { + fn keyring_backend_mirrors_legacy_file_on_success() { use base64::{engine::general_purpose::STANDARD, Engine as _}; let dir = tempfile::tempdir().unwrap(); let key_file = dir.path().join(".encryption_key"); - // Create a legacy fallback file - std::fs::write(&key_file, STANDARD.encode([99u8; 32])).unwrap(); + // Create a legacy fallback file with a stale key + let stale_key = [99u8; 32]; + std::fs::write(&key_file, STANDARD.encode(stale_key)).unwrap(); assert!(key_file.exists()); - // Keyring has a valid key + // Keyring has a valid (different) key let expected = [7u8; 32]; + assert_ne!(stale_key, expected, "stale key must differ for this test"); let mock = MockKeyring::with_password(&STANDARD.encode(expected)); let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); assert_eq!(result, expected); assert!( - !key_file.exists(), - "Legacy file must be deleted upon successful keyring read" + key_file.exists(), + "Legacy file must NOT be deleted — it serves as a durable fallback" + ); + let synced = read_key_file(&key_file).unwrap(); + assert_eq!( + synced, expected, + "Legacy file must be updated to mirror the keyring key" ); } #[test] #[cfg(any(target_os = "macos", target_os = "windows"))] - fn keyring_backend_cleans_up_legacy_file_on_generation() { + fn keyring_backend_mirrors_legacy_file_on_generation() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; let dir = tempfile::tempdir().unwrap(); let key_file = dir.path().join(".encryption_key"); @@ -618,8 +664,13 @@ mod tests { assert_eq!(result.len(), 32); assert!( - !key_file.exists(), - "Legacy file must be deleted upon successful keyring generation" + key_file.exists(), + "Legacy file must NOT be deleted — it serves as a durable fallback" + ); + let synced = read_key_file(&key_file).unwrap(); + assert_eq!( + synced, result, + "Legacy file must be updated to mirror the newly-generated keyring key" ); } @@ -642,6 +693,61 @@ mod tests { // ---- Backend::Keyring tests (Linux fallback behavior) ---- + + #[test] + #[cfg(any(target_os = "macos", target_os = "windows"))] + fn keyring_backend_skips_write_when_file_already_has_correct_key() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + // Pre-populate the file with the SAME key the keyring will return + let expected = [42u8; 32]; + std::fs::write(&key_file, STANDARD.encode(expected)).unwrap(); + let original_mtime = std::fs::metadata(&key_file).unwrap().modified().unwrap(); + + // Keyring has the same key + let mock = MockKeyring::with_password(&STANDARD.encode(expected)); + + // Brief sleep to ensure mtime would change if the file were rewritten + std::thread::sleep(std::time::Duration::from_millis(10)); + + let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(result, expected); + + // File mtime should be unchanged (no write happened) + let new_mtime = std::fs::metadata(&key_file).unwrap().modified().unwrap(); + assert_eq!( + original_mtime, new_mtime, + "File should not be rewritten when its content already matches the keyring key" + ); + // File content should be unchanged + let synced = read_key_file(&key_file).unwrap(); + assert_eq!(synced, expected); + } + + #[test] + #[cfg(any(target_os = "macos", target_os = "windows"))] + fn keyring_backend_writes_when_file_has_stale_key() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + // Pre-populate the file with a DIFFERENT (stale) key + let stale_key = [99u8; 32]; + std::fs::write(&key_file, STANDARD.encode(stale_key)).unwrap(); + + // Keyring has a different valid key + let expected = [7u8; 32]; + assert_ne!(stale_key, expected); + let mock = MockKeyring::with_password(&STANDARD.encode(expected)); + + let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(result, expected); + // File should be updated to the new key + let synced = read_key_file(&key_file).unwrap(); + assert_eq!(synced, expected, "file should be updated when stale"); + } #[test] #[cfg(not(any(target_os = "macos", target_os = "windows")))] fn keyring_backend_creates_file_backup_when_missing() {