From b53a3a66db31c157ec7a14bab8017b6929cf5ace Mon Sep 17 00:00:00 2001 From: jpoehnelt-bot Date: Mon, 9 Mar 2026 19:41:56 -0600 Subject: [PATCH 1/2] fix(credential_store): preserve .encryption_key file for keyring-less environments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fix for #344 introduced automatic deletion of .encryption_key when the OS keyring succeeds. In Docker/ephemeral-keyring environments, this permanently loses the encryption key on container restart. Changes: - Never delete .encryption_key — it persists as a safe fallback - Always save new keys to file, even when keyring set_password succeeds - Extract resolve_key() with KeyringProvider trait for testability - Add 13 new unit tests covering all keyring/file scenarios Fixes #344 (regression) --- .changeset/fix-keyring-fallback.md | 5 + src/credential_store.rs | 471 +++++++++++++++++++++++------ 2 files changed, 387 insertions(+), 89 deletions(-) create mode 100644 .changeset/fix-keyring-fallback.md diff --git a/.changeset/fix-keyring-fallback.md b/.changeset/fix-keyring-fallback.md new file mode 100644 index 0000000..564c3db --- /dev/null +++ b/.changeset/fix-keyring-fallback.md @@ -0,0 +1,5 @@ +--- +"@googleworkspace/cli": patch +--- + +Fix credential key loss in Docker/keyring-less environments by no longer deleting `.encryption_key` on keyring success diff --git a/src/credential_store.rs b/src/credential_store.rs index aa9f1c5..db605b7 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -53,119 +53,155 @@ fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { Ok(()) } -/// Returns the encryption key derived from the OS keyring, or falls back to a local file. -/// Generates a random 256-bit key and stores it securely if it doesn't exist. -fn get_or_create_key() -> anyhow::Result<[u8; 32]> { - static KEY: OnceLock<[u8; 32]> = OnceLock::new(); +/// Abstraction over OS keyring operations for testability. +trait KeyringProvider { + /// Attempt to read the stored password. Returns `Err(NoEntry)` if + /// no entry exists, or another `keyring::Error` on platform failure. + fn get_password(&self) -> Result; + /// Attempt to store a password in the keyring. + fn set_password(&self, password: &str) -> Result<(), keyring::Error>; +} - if let Some(key) = KEY.get() { - return Ok(*key); +/// Production keyring implementation wrapping an optional `keyring::Entry`. +/// `None` means `Entry::new` itself failed (no backend available). +struct OsKeyring(Option); + +impl OsKeyring { + fn new(service: &str, user: &str) -> Self { + Self(Entry::new(service, user).ok()) } +} - let cache_key = |candidate: [u8; 32]| -> [u8; 32] { - if KEY.set(candidate).is_ok() { - candidate - } else { - // If set() fails, another thread already initialized the key. .get() is - // guaranteed to return Some at this point. - *KEY.get() - .expect("key must be initialized if OnceLock::set() failed") +impl KeyringProvider for OsKeyring { + fn get_password(&self) -> Result { + match &self.0 { + Some(entry) => entry.get_password(), + None => Err(keyring::Error::NoEntry), } - }; + } - let username = std::env::var("USER") - .or_else(|_| std::env::var("USERNAME")) - .unwrap_or_else(|_| "unknown-user".to_string()); + fn set_password(&self, password: &str) -> Result<(), keyring::Error> { + match &self.0 { + Some(entry) => entry.set_password(password), + None => Err(keyring::Error::NoEntry), + } + } +} - let key_file = crate::auth_commands::config_dir().join(".encryption_key"); +/// Core key-resolution logic, separated from caching for testability. +/// +/// Priority order: +/// 1. Keyring entry (authoritative when available) +/// 2. Local `.encryption_key` file (persistent fallback) +/// 3. Generate a new random 256-bit key +/// +/// The file is **never deleted** — it serves as a safe fallback for +/// environments where the keyring is ephemeral (e.g. Docker containers). +fn resolve_key( + provider: &dyn KeyringProvider, + key_file: &std::path::Path, +) -> anyhow::Result<[u8; 32]> { + use base64::{engine::general_purpose::STANDARD, Engine as _}; - let entry = Entry::new("gws-cli", &username); - - if let Ok(entry) = entry { - match entry.get_password() { - Ok(b64_key) => { - use base64::{engine::general_purpose::STANDARD, Engine as _}; - if let Ok(decoded) = STANDARD.decode(&b64_key) { - 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); - } - return Ok(cache_key(arr)); - } + // --- 1. Try keyring ------------------------------------------------- + match provider.get_password() { + Ok(b64_key) => { + if let Ok(decoded) = STANDARD.decode(&b64_key) { + if decoded.len() == 32 { + let mut arr = [0u8; 32]; + arr.copy_from_slice(&decoded); + return Ok(arr); } } - Err(keyring::Error::NoEntry) => { - use base64::{engine::general_purpose::STANDARD, Engine as _}; - - // If keyring is empty, prefer a persisted local key first. - if key_file.exists() { - if let Ok(b64_key) = std::fs::read_to_string(&key_file) { - if let Ok(decoded) = STANDARD.decode(b64_key.trim()) { - if decoded.len() == 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() { - let _ = std::fs::remove_file(&key_file); - } - return Ok(cache_key(arr)); - } - } - } - } + // Keyring contained invalid data — fall through to file/generate. + } + Err(keyring::Error::NoEntry) => { + // Keyring is reachable but empty — check file, then generate. + + // 1a. Prefer an existing file key. + if let Some(key) = read_key_file(key_file) { + // Best-effort: copy file key into keyring for future runs. + let _ = provider.set_password(&STANDARD.encode(key)); + return Ok(key); + } - // Generate a new random 256-bit key. - let mut key = [0u8; 32]; - rand::thread_rng().fill_bytes(&mut key); - let b64_key = STANDARD.encode(key); + // 1b. Generate a new key. + let key = generate_random_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 first. + let _ = provider.set_password(&b64_key); - // Keyring store failed — persist to local file as fallback. - save_key_file(&key_file, &b64_key)?; + // Always persist to file as a durable fallback. This ensures + // the key survives keyring loss (e.g. Docker container restart). + save_key_file(key_file, &b64_key)?; - return Ok(cache_key(key)); - } - Err(e) => { - eprintln!("Warning: keyring access failed, falling back to file storage: {e}"); - } + return Ok(key); + } + Err(e) => { + eprintln!("Warning: keyring access failed, falling back to file storage: {e}"); } } - // Fallback: Local file `.encryption_key` + // --- 2. File fallback ------------------------------------------------ + if let Some(key) = read_key_file(key_file) { + return Ok(key); + } - if key_file.exists() { - if let Ok(b64_key) = std::fs::read_to_string(&key_file) { - use base64::{engine::general_purpose::STANDARD, Engine as _}; - if let Ok(decoded) = STANDARD.decode(b64_key.trim()) { - if decoded.len() == 32 { - let mut arr = [0u8; 32]; - arr.copy_from_slice(&decoded); - return Ok(cache_key(arr)); - } - } - } + // --- 3. Generate new key, save to file ------------------------------- + let key = generate_random_key(); + let b64_key = STANDARD.encode(key); + save_key_file(key_file, &b64_key)?; + Ok(key) +} + +/// Read and decode a base64-encoded 256-bit key from a file. +fn read_key_file(path: &std::path::Path) -> Option<[u8; 32]> { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let b64_key = std::fs::read_to_string(path).ok()?; + let decoded = STANDARD.decode(b64_key.trim()).ok()?; + if decoded.len() == 32 { + let mut arr = [0u8; 32]; + arr.copy_from_slice(&decoded); + Some(arr) + } else { + None } +} - // Generate new key and save to local file +/// Generate a random 256-bit key. +fn generate_random_key() -> [u8; 32] { let mut key = [0u8; 32]; rand::thread_rng().fill_bytes(&mut key); + key +} - use base64::{engine::general_purpose::STANDARD, Engine as _}; - let b64_key = STANDARD.encode(key); +/// Returns the encryption key derived from the OS keyring, or falls back to a local file. +/// Generates a random 256-bit key and stores it securely if it doesn't exist. +fn get_or_create_key() -> anyhow::Result<[u8; 32]> { + static KEY: OnceLock<[u8; 32]> = OnceLock::new(); - save_key_file(&key_file, &b64_key)?; + if let Some(key) = KEY.get() { + return Ok(*key); + } + + let username = std::env::var("USER") + .or_else(|_| std::env::var("USERNAME")) + .unwrap_or_else(|_| "unknown-user".to_string()); - Ok(cache_key(key)) + let key_file = crate::auth_commands::config_dir().join(".encryption_key"); + let provider = OsKeyring::new("gws-cli", &username); + + let key = resolve_key(&provider, &key_file)?; + + // Cache for subsequent calls within this process. + if KEY.set(key).is_ok() { + Ok(key) + } else { + Ok(*KEY + .get() + .expect("key must be initialized if OnceLock::set() failed")) + } } /// Encrypts plaintext bytes using AES-256-GCM with a machine-derived key. @@ -267,6 +303,263 @@ pub fn load_encrypted() -> anyhow::Result { #[cfg(test)] mod tests { use super::*; + use std::cell::RefCell; + + /// Describes what `get_password` / `set_password` should return, + /// without storing `keyring::Error` (which is not `Clone`). + #[derive(Clone)] + enum MockState { + Ok(String), + NoEntry, + PlatformError, + } + + /// Mock keyring for testing `resolve_key()` without OS dependencies. + struct MockKeyring { + get_state: MockState, + set_succeeds: bool, + /// Tracks the last value passed to `set_password`. + last_set: RefCell>, + } + + impl MockKeyring { + /// Keyring that returns the given password on `get_password`. + fn with_password(b64: &str) -> Self { + Self { + get_state: MockState::Ok(b64.to_string()), + set_succeeds: true, + last_set: RefCell::new(None), + } + } + + /// Keyring that returns `NoEntry` on `get_password`. + fn no_entry() -> Self { + Self { + get_state: MockState::NoEntry, + set_succeeds: true, + last_set: RefCell::new(None), + } + } + + /// Keyring that returns a platform error on `get_password`. + fn platform_error() -> Self { + Self { + get_state: MockState::PlatformError, + set_succeeds: true, + last_set: RefCell::new(None), + } + } + + /// Configure `set_password` to fail. + fn with_set_failure(mut self) -> Self { + self.set_succeeds = false; + self + } + } + + impl KeyringProvider for MockKeyring { + fn get_password(&self) -> Result { + match &self.get_state { + MockState::Ok(s) => Ok(s.clone()), + MockState::NoEntry => Err(keyring::Error::NoEntry), + MockState::PlatformError => { + Err(keyring::Error::PlatformFailure("mock: no backend".into())) + } + } + } + + fn set_password(&self, password: &str) -> Result<(), keyring::Error> { + *self.last_set.borrow_mut() = Some(password.to_string()); + if self.set_succeeds { + Ok(()) + } else { + Err(keyring::Error::NoEntry) + } + } + } + + /// Helper: write a known key to a temp file, return (dir, path, key_bytes). + fn write_test_key(dir: &std::path::Path) -> ([u8; 32], std::path::PathBuf) { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let key = [42u8; 32]; + let b64 = STANDARD.encode(key); + let path = dir.join(".encryption_key"); + std::fs::write(&path, &b64).unwrap(); + (key, path) + } + + // ---- resolve_key tests ---- + + #[test] + fn keyring_ok_returns_keyring_key() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + let expected_key = [7u8; 32]; + let mock = MockKeyring::with_password(&STANDARD.encode(expected_key)); + + let result = resolve_key(&mock, &key_file).unwrap(); + assert_eq!(result, expected_key); + } + + #[test] + fn keyring_ok_file_exists_keeps_file() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let (_, key_file) = write_test_key(dir.path()); + + let keyring_key = [7u8; 32]; + let mock = MockKeyring::with_password(&STANDARD.encode(keyring_key)); + + let result = resolve_key(&mock, &key_file).unwrap(); + assert_eq!(result, keyring_key, "keyring key should be authoritative"); + assert!(key_file.exists(), "file must NOT be deleted"); + } + + #[test] + fn no_entry_file_exists_returns_file_key_and_keeps_file() { + let dir = tempfile::tempdir().unwrap(); + let (expected_key, key_file) = write_test_key(dir.path()); + + let mock = MockKeyring::no_entry(); + let result = resolve_key(&mock, &key_file).unwrap(); + + assert_eq!(result, expected_key, "should return the file key"); + assert!( + key_file.exists(), + "file must NOT be deleted after migration" + ); + assert!( + mock.last_set.borrow().is_some(), + "should attempt to copy key into keyring" + ); + } + + #[test] + fn no_entry_no_file_keyring_set_succeeds_still_creates_file() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + let mock = MockKeyring::no_entry(); + let key = resolve_key(&mock, &key_file).unwrap(); + + assert_eq!(key.len(), 32); + assert!( + key_file.exists(), + "file must be created as durable fallback even when keyring succeeds" + ); + // The file should contain the same key. + let file_key = read_key_file(&key_file).unwrap(); + assert_eq!(key, file_key); + } + + #[test] + fn no_entry_no_file_keyring_set_fails_creates_file() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + let mock = MockKeyring::no_entry().with_set_failure(); + let key = resolve_key(&mock, &key_file).unwrap(); + + assert_eq!(key.len(), 32); + assert!(key_file.exists(), "file must be created as fallback"); + let file_key = read_key_file(&key_file).unwrap(); + assert_eq!(key, file_key); + } + + #[test] + fn keyring_platform_error_file_exists_returns_file_key() { + let dir = tempfile::tempdir().unwrap(); + let (expected_key, key_file) = write_test_key(dir.path()); + + let mock = MockKeyring::platform_error(); + let result = resolve_key(&mock, &key_file).unwrap(); + + assert_eq!(result, expected_key); + assert!(key_file.exists()); + } + + #[test] + fn keyring_platform_error_no_file_generates_and_saves() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + let mock = MockKeyring::platform_error(); + let key = resolve_key(&mock, &key_file).unwrap(); + + assert_eq!(key.len(), 32); + assert!(key_file.exists(), "file must be created"); + let file_key = read_key_file(&key_file).unwrap(); + assert_eq!(key, file_key); + } + + #[test] + fn resolve_key_is_stable_across_calls() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + // First call: no keyring, no file → generates key. + let mock = MockKeyring::platform_error(); + let key1 = resolve_key(&mock, &key_file).unwrap(); + + // Second call: same file exists → returns same key. + let key2 = resolve_key(&mock, &key_file).unwrap(); + assert_eq!(key1, key2, "must return the same key on subsequent calls"); + } + + #[test] + fn keyring_invalid_data_falls_through_to_file() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let (expected_key, key_file) = write_test_key(dir.path()); + + // Keyring returns invalid (wrong length) data. + let mock = MockKeyring::with_password(&STANDARD.encode([1u8; 16])); + let result = resolve_key(&mock, &key_file).unwrap(); + + assert_eq!( + result, expected_key, + "should fall through to file when keyring data is invalid" + ); + } + + // ---- read_key_file tests ---- + + #[test] + fn read_key_file_valid() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("key"); + let key = [99u8; 32]; + std::fs::write(&path, STANDARD.encode(key)).unwrap(); + assert_eq!(read_key_file(&path), Some(key)); + } + + #[test] + fn read_key_file_missing() { + let dir = tempfile::tempdir().unwrap(); + assert_eq!(read_key_file(&dir.path().join("nonexistent")), None); + } + + #[test] + fn read_key_file_wrong_length() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("key"); + std::fs::write(&path, STANDARD.encode([1u8; 16])).unwrap(); + assert_eq!(read_key_file(&path), None); + } + + #[test] + fn read_key_file_invalid_base64() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("key"); + std::fs::write(&path, "not-valid-base64!!!").unwrap(); + assert_eq!(read_key_file(&path), None); + } + + // ---- Existing encrypt/decrypt tests ---- #[test] fn get_or_create_key_is_deterministic() { From 5e6070a8fc5dc9f108be6d99bd9541da4b787283 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 10 Mar 2026 01:42:52 +0000 Subject: [PATCH 2/2] chore: regenerate skills [skip ci] --- skills/gws-gmail-forward/SKILL.md | 1 + 1 file changed, 1 insertion(+) diff --git a/skills/gws-gmail-forward/SKILL.md b/skills/gws-gmail-forward/SKILL.md index cc0099a..cb99a30 100644 --- a/skills/gws-gmail-forward/SKILL.md +++ b/skills/gws-gmail-forward/SKILL.md @@ -44,6 +44,7 @@ gws gmail +forward --message-id 18f1a2b3c4d --to dave@example.com --cc eve@examp ## Tips - Includes the original message with sender, date, subject, and recipients. +- Sends the forward as a new message rather than forcing it into the original thread. ## See Also