From abbf85a180f2be6074de7f0c1a04307d64edeba6 Mon Sep 17 00:00:00 2001 From: mikeciesla Date: Tue, 10 Mar 2026 23:01:48 +1100 Subject: [PATCH 1/2] fix(auth): verify keyring writes with round-trip read to prevent phantom key loss On macOS, keyring set_password() can return Ok without persisting. This adds get_password() verification after every set_password() and always saves the encryption key to file as a backup, so the key survives between runs even when the OS keyring silently fails. --- .changeset/fix-keyring-roundtrip-verify.md | 12 ++++++++ src/credential_store.rs | 35 ++++++++++++++-------- 2 files changed, 35 insertions(+), 12 deletions(-) create mode 100644 .changeset/fix-keyring-roundtrip-verify.md diff --git a/.changeset/fix-keyring-roundtrip-verify.md b/.changeset/fix-keyring-roundtrip-verify.md new file mode 100644 index 0000000..8fdb536 --- /dev/null +++ b/.changeset/fix-keyring-roundtrip-verify.md @@ -0,0 +1,12 @@ +--- +"@googleworkspace/cli": patch +--- + +fix(auth): verify keyring writes with round-trip read before trusting them + +On macOS, `keyring::Entry::set_password()` can return `Ok(())` without +actually persisting to Keychain (phantom write). This caused the encryption +key to be lost between runs, breaking all commands with "Decryption failed". + +The fix adds a `get_password()` verification after every `set_password()` and +always persists the key to the local file as a backup. diff --git a/src/credential_store.rs b/src/credential_store.rs index aa9f1c5..846c1d2 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -89,11 +89,9 @@ fn get_or_create_key() -> anyhow::Result<[u8; 32]> { if decoded.len() == 32 { let mut arr = [0u8; 32]; arr.copy_from_slice(&decoded); - // Keyring is authoritative — remove redundant file copy - // if it exists (migrates existing installs on upgrade). - if key_file.exists() { - let _ = std::fs::remove_file(&key_file); - } + // Keep the file as a backup — do not delete it. + // On some platforms (macOS) keyring reads may succeed + // now but fail after a binary-path or OS change. return Ok(cache_key(arr)); } } @@ -109,8 +107,13 @@ fn get_or_create_key() -> anyhow::Result<[u8; 32]> { let mut arr = [0u8; 32]; arr.copy_from_slice(&decoded); // Migrate file key into keyring; remove the - // file if the keyring store succeeds. - if entry.set_password(b64_key.trim()).is_ok() { + // file only if a round-trip read confirms the + // keyring actually persisted the value (macOS + // Keychain can return Ok without writing). + if entry.set_password(b64_key.trim()).is_ok() + && entry.get_password().ok().as_deref() + == Some(b64_key.trim()) + { let _ = std::fs::remove_file(&key_file); } return Ok(cache_key(arr)); @@ -124,13 +127,21 @@ fn get_or_create_key() -> anyhow::Result<[u8; 32]> { rand::thread_rng().fill_bytes(&mut key); let b64_key = STANDARD.encode(key); - // Try keyring first; only fall back to file storage - // if the keyring is unavailable. - if entry.set_password(&b64_key).is_ok() { - return Ok(cache_key(key)); + // Try keyring; verify with a round-trip read (macOS + // Keychain can return Ok from set_password without + // persisting). Always save to file as a backup. + let keyring_ok = entry.set_password(&b64_key).is_ok() + && entry.get_password().ok().as_deref() == Some(b64_key.as_str()); + + if !keyring_ok { + eprintln!( + "Warning: keyring write could not be verified, \ + falling back to file storage" + ); } - // Keyring store failed — persist to local file as fallback. + // Always persist to file so the key survives keyring + // failures, binary-path changes, or OS upgrades. save_key_file(&key_file, &b64_key)?; return Ok(cache_key(key)); From 804a61bf5d70cd752878a3fba0ac05c429e4b394 Mon Sep 17 00:00:00 2001 From: mikeciesla Date: Tue, 10 Mar 2026 23:42:08 +1100 Subject: [PATCH 2/2] fix(auth): harden keyring fallback with atomic writes and file retention - Stop deleting file backup in migration path (consistent with new philosophy) - Switch save_key_file to atomic_write (tmp + rename) for crash safety - Set 0600 permissions on key file after atomic write - Add doc comment explaining the security trade-off - Add tests for save_key_file: round-trip, permissions, overwrite, nested dirs --- src/credential_store.rs | 104 +++++++++++++++++++++++++++++++--------- 1 file changed, 82 insertions(+), 22 deletions(-) diff --git a/src/credential_store.rs b/src/credential_store.rs index 846c1d2..59a4b11 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -22,8 +22,13 @@ use rand::RngCore; use std::sync::OnceLock; /// Persist the base64-encoded encryption key to a local file with restrictive -/// permissions (0600 file, 0700 directory). Used only as a fallback when the OS -/// keyring is unavailable. +/// permissions (0600 file, 0700 directory). +/// +/// The file is always kept as a backup even when the OS keyring is available. +/// On macOS, Keychain writes can silently fail (phantom `Ok`) after binary-path +/// or OS changes, so the file ensures the key is never lost between runs. +/// The security trade-off is acceptable: the file is owner-readable only (0600) +/// and lives in a 0700 directory, comparable to how `~/.ssh/` keys are stored. fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; @@ -37,18 +42,16 @@ fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { } } + // Write atomically (tmp + rename) so a crash never leaves a + // partial key file. The rename is atomic on POSIX. + crate::fs_util::atomic_write(path, b64_key.as_bytes())?; + #[cfg(unix)] { - use std::io::Write; - use std::os::unix::fs::OpenOptionsExt; - let mut options = std::fs::OpenOptions::new(); - options.write(true).create(true).truncate(true).mode(0o600); - let mut file = options.open(path)?; - file.write_all(b64_key.as_bytes())?; - } - #[cfg(not(unix))] - { - std::fs::write(path, b64_key)?; + use std::os::unix::fs::PermissionsExt; + if let Err(e) = std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600)) { + eprintln!("Warning: failed to set secure permissions on key file: {e}"); + } } Ok(()) } @@ -106,16 +109,11 @@ fn get_or_create_key() -> anyhow::Result<[u8; 32]> { if decoded.len() == 32 { let mut arr = [0u8; 32]; arr.copy_from_slice(&decoded); - // Migrate file key into keyring; remove the - // file only if a round-trip read confirms the - // keyring actually persisted the value (macOS - // Keychain can return Ok without writing). - if entry.set_password(b64_key.trim()).is_ok() - && entry.get_password().ok().as_deref() - == Some(b64_key.trim()) - { - let _ = std::fs::remove_file(&key_file); - } + // Try to copy the key into the keyring for + // convenience, but always keep the file as the + // authoritative backup (the keyring can silently + // fail on macOS — see round-trip checks below). + let _ = entry.set_password(b64_key.trim()); return Ok(cache_key(arr)); } } @@ -339,4 +337,66 @@ mod tests { assert_eq!(dec1, dec2); assert_eq!(dec1, plaintext); } + + #[test] + fn save_key_file_round_trip() { + let dir = tempfile::tempdir().unwrap(); + let key_path = dir.path().join(".encryption_key"); + + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let mut key = [0u8; 32]; + rand::thread_rng().fill_bytes(&mut key); + let b64 = STANDARD.encode(key); + + save_key_file(&key_path, &b64).expect("save should succeed"); + + let read_back = std::fs::read_to_string(&key_path).unwrap(); + assert_eq!(read_back, b64); + + let decoded = STANDARD.decode(read_back.trim()).unwrap(); + assert_eq!(decoded.len(), 32); + assert_eq!(decoded, key); + } + + #[test] + fn save_key_file_sets_secure_permissions() { + let dir = tempfile::tempdir().unwrap(); + let key_path = dir.path().join(".encryption_key"); + save_key_file(&key_path, "dGVzdA==").expect("save should succeed"); + + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + let perms = std::fs::metadata(&key_path).unwrap().permissions(); + assert_eq!(perms.mode() & 0o777, 0o600, "key file should be 0600"); + + let dir_perms = std::fs::metadata(dir.path()).unwrap().permissions(); + assert_eq!(dir_perms.mode() & 0o777, 0o700, "key dir should be 0700"); + } + } + + #[test] + fn save_key_file_overwrites_existing() { + let dir = tempfile::tempdir().unwrap(); + let key_path = dir.path().join(".encryption_key"); + + save_key_file(&key_path, "first_key").expect("first save"); + save_key_file(&key_path, "second_key").expect("second save"); + + let contents = std::fs::read_to_string(&key_path).unwrap(); + assert_eq!(contents, "second_key"); + } + + #[test] + fn save_key_file_creates_parent_directories() { + let dir = tempfile::tempdir().unwrap(); + let key_path = dir + .path() + .join("nested") + .join("dir") + .join(".encryption_key"); + + save_key_file(&key_path, "test_key").expect("save with nested dirs"); + assert!(key_path.exists()); + } }