From c1c6a759c5c1ed6b3db8f550ed0231dae006740a Mon Sep 17 00:00:00 2001 From: jpoehnelt-bot Date: Mon, 9 Mar 2026 19:54:28 -0600 Subject: [PATCH 1/6] feat(credential_store): add GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND env var MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add gogcli-style backend selection for encryption key storage: - keyring (default): OS keyring with file fallback - file: .encryption_key file only (Docker/CI/headless) Never delete .encryption_key — it always serves as a durable fallback for environments where the keyring is ephemeral. When generating new keys with backend=keyring, save to both keyring and file. Extracts KeyringProvider trait + resolve_key() for testability. 25 tests covering both backends and all edge cases. Fixes #344 --- .changeset/fix-keyring-fallback.md | 5 + AGENTS.md | 3 +- README.md | 2 +- src/auth_commands.rs | 2 +- src/credential_store.rs | 481 ++++++++++++++++++++++++----- src/main.rs | 3 + src/token_storage.rs | 2 +- 7 files changed, 414 insertions(+), 84 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 00000000..2562dd1f --- /dev/null +++ b/.changeset/fix-keyring-fallback.md @@ -0,0 +1,5 @@ +--- +"@googleworkspace/cli": minor +--- + +Add `GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND` env var for explicit keyring backend selection (`keyring` or `file`). Fixes credential key loss in Docker/keyring-less environments by never deleting `.encryption_key` and always persisting it as a fallback. diff --git a/AGENTS.md b/AGENTS.md index 4fa127b6..5b138897 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -171,7 +171,8 @@ Use these labels to categorize pull requests and issues: | Variable | Description | |---|---| | `GOOGLE_WORKSPACE_CLI_TOKEN` | Pre-obtained OAuth2 access token (highest priority; bypasses all credential file loading) | -| `GOOGLE_WORKSPACE_CLI_CREDENTIALS_FILE` | Path to OAuth credentials JSON (no default; if unset, falls back to credentials secured by the OS Keyring and encrypted in `~/.config/gws/`) | +| `GOOGLE_WORKSPACE_CLI_CREDENTIALS_FILE` | Path to OAuth credentials JSON (no default; if unset, falls back to encrypted credentials in `~/.config/gws/`) | +| `GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND` | Keyring backend: `keyring` (default, uses OS keyring with file fallback) or `file` (file only, for Docker/CI/headless) | | `GOOGLE_APPLICATION_CREDENTIALS` | Standard Google ADC path; used as fallback when no gws-specific credentials are configured | diff --git a/README.md b/README.md index da1a05dc..9130ecbd 100644 --- a/README.md +++ b/README.md @@ -115,7 +115,7 @@ The CLI supports multiple auth workflows so it works on your laptop, in CI, and ### Interactive (local desktop) -Credentials are encrypted at rest (AES-256-GCM) with the key stored in your OS keyring. +Credentials are encrypted at rest (AES-256-GCM) with the key stored in your OS keyring (or `~/.config/gws/.encryption_key` when `GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=file`). ```bash gws auth setup # one-time: creates a Cloud project, enables APIs, logs you in diff --git a/src/auth_commands.rs b/src/auth_commands.rs index f4ffacc7..d78e0295 100644 --- a/src/auth_commands.rs +++ b/src/auth_commands.rs @@ -359,7 +359,7 @@ async fn handle_login(args: &[String]) -> Result<(), GwsError> { "message": "Authentication successful. Encrypted credentials saved.", "account": actual_email.as_deref().unwrap_or("(unknown)"), "credentials_file": enc_path.display().to_string(), - "encryption": "AES-256-GCM (key secured by OS Keyring or local `.encryption_key`)", + "encryption": "AES-256-GCM (key in OS keyring or local `.encryption_key`; set GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=file for headless)", "scopes": scopes, }); println!( diff --git a/src/credential_store.rs b/src/credential_store.rs index aa9f1c54..39afbcb1 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -22,8 +22,7 @@ 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). 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)?; @@ -53,87 +52,132 @@ 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(); +/// 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 + } +} - if let Some(key) = KEY.get() { - return Ok(*key); +/// 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 +} + +/// Abstraction over OS keyring operations for testability. +trait KeyringProvider { + /// Attempt to read the stored password. + fn get_password(&self) -> Result; + /// Attempt to store a password in the keyring. + fn set_password(&self, password: &str) -> Result<(), keyring::Error>; +} + +/// Production keyring implementation wrapping an optional `keyring::Entry`. +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"); +/// Which backend to use for encryption key storage. +/// +/// Controlled by `GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND`: +/// - `"keyring"` (default): Use OS keyring, fall back to `.encryption_key` file +/// - `"file"`: Use `.encryption_key` file only (for Docker/CI/headless) +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum KeyringBackend { + Keyring, + File, +} + +impl KeyringBackend { + fn from_env() -> Self { + match std::env::var("GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND") + .unwrap_or_default() + .to_lowercase() + .as_str() + { + "file" => KeyringBackend::File, + _ => KeyringBackend::Keyring, + } + } +} - let entry = Entry::new("gws-cli", &username); +/// Core key-resolution logic, separated from caching for testability. +/// +/// When `backend` is `Keyring`: +/// 1. Try keyring → 2. Try file → 3. Generate (save to keyring + file) +/// +/// When `backend` is `File`: +/// 1. Try file → 2. Generate (save to file only) +/// +/// The `.encryption_key` file is **never deleted** — it always serves as a +/// durable fallback for environments where the keyring is ephemeral. +fn resolve_key( + backend: KeyringBackend, + provider: &dyn KeyringProvider, + key_file: &std::path::Path, +) -> anyhow::Result<[u8; 32]> { + use base64::{engine::general_purpose::STANDARD, Engine as _}; - if let Ok(entry) = entry { - match entry.get_password() { + // --- 1. Try keyring (only when backend = Keyring) -------------------- + if backend == KeyringBackend::Keyring { + match provider.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)); + return Ok(arr); } } + // Keyring contained invalid data — fall through to file. } 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 is reachable but empty — check file, then generate. + 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); + // 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)); - } + // Best-effort: store in keyring. + let _ = provider.set_password(&b64_key); - // Keyring store failed — persist to local file as fallback. - save_key_file(&key_file, &b64_key)?; - - return Ok(cache_key(key)); + // Always persist to file as durable fallback. + save_key_file(key_file, &b64_key)?; + return Ok(key); } Err(e) => { eprintln!("Warning: keyring access failed, falling back to file storage: {e}"); @@ -141,31 +185,48 @@ fn get_or_create_key() -> anyhow::Result<[u8; 32]> { } } - // 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) +} + +/// Returns the encryption key, generating and persisting one if it doesn't exist. +/// +/// The key is cached in-process via `OnceLock` so it is only read from disk once. +/// Backend selection is controlled by `GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND`. +fn get_or_create_key() -> anyhow::Result<[u8; 32]> { + static KEY: OnceLock<[u8; 32]> = OnceLock::new(); + + if let Some(key) = KEY.get() { + return Ok(*key); } - // Generate new key and save to local file - let mut key = [0u8; 32]; - rand::thread_rng().fill_bytes(&mut key); + let backend = KeyringBackend::from_env(); - use base64::{engine::general_purpose::STANDARD, Engine as _}; - let b64_key = STANDARD.encode(key); + let username = std::env::var("USER") + .or_else(|_| std::env::var("USERNAME")) + .unwrap_or_else(|_| "unknown-user".to_string()); + + let key_file = crate::auth_commands::config_dir().join(".encryption_key"); + let provider = OsKeyring::new("gws-cli", &username); - save_key_file(&key_file, &b64_key)?; + let key = resolve_key(backend, &provider, &key_file)?; - Ok(cache_key(key)) + // 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 +328,266 @@ pub fn load_encrypted() -> anyhow::Result { #[cfg(test)] mod tests { use super::*; + use std::cell::RefCell; + + /// Describes what `get_password` / `set_password` should return. + #[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, + last_set: RefCell>, + } + + impl MockKeyring { + fn with_password(b64: &str) -> Self { + Self { + get_state: MockState::Ok(b64.to_string()), + set_succeeds: true, + last_set: RefCell::new(None), + } + } + + fn no_entry() -> Self { + Self { + get_state: MockState::NoEntry, + set_succeeds: true, + last_set: RefCell::new(None), + } + } + + fn platform_error() -> Self { + Self { + get_state: MockState::PlatformError, + set_succeeds: true, + last_set: RefCell::new(None), + } + } + + 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) + } + } + } + + 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 path = dir.join(".encryption_key"); + std::fs::write(&path, STANDARD.encode(key)).unwrap(); + (key, path) + } + + // ---- Backend::Keyring tests ---- + + #[test] + fn keyring_backend_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 = [7u8; 32]; + let mock = MockKeyring::with_password(&STANDARD.encode(expected)); + let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(result, expected); + } + + #[test] + fn keyring_backend_keeps_file_when_keyring_succeeds() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let (_, key_file) = write_test_key(dir.path()); + let mock = MockKeyring::with_password(&STANDARD.encode([7u8; 32])); + let _ = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert!(key_file.exists(), "file must NOT be deleted"); + } + + #[test] + fn keyring_backend_no_entry_reads_file() { + let dir = tempfile::tempdir().unwrap(); + let (expected, key_file) = write_test_key(dir.path()); + let mock = MockKeyring::no_entry(); + let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(result, expected); + assert!(key_file.exists(), "file must NOT be deleted"); + assert!( + mock.last_set.borrow().is_some(), + "should copy key to keyring" + ); + } + + #[test] + fn keyring_backend_no_entry_no_file_generates_and_saves_both() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + let mock = MockKeyring::no_entry(); + let key = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(key.len(), 32); + assert!(key_file.exists(), "file must be created as fallback"); + assert!(mock.last_set.borrow().is_some(), "should store in keyring"); + let file_key = read_key_file(&key_file).unwrap(); + assert_eq!(key, file_key); + } + + #[test] + fn keyring_backend_no_entry_no_file_keyring_set_fails() { + 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(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(key.len(), 32); + assert!(key_file.exists(), "file must be created when keyring fails"); + let file_key = read_key_file(&key_file).unwrap(); + assert_eq!(key, file_key); + } + + #[test] + fn keyring_backend_platform_error_falls_back_to_file() { + let dir = tempfile::tempdir().unwrap(); + let (expected, key_file) = write_test_key(dir.path()); + let mock = MockKeyring::platform_error(); + let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(result, expected); + } + + #[test] + fn keyring_backend_platform_error_no_file_generates() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + let mock = MockKeyring::platform_error(); + let key = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(key.len(), 32); + assert!(key_file.exists()); + } + + #[test] + fn keyring_backend_invalid_keyring_data_uses_file() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let (expected, key_file) = write_test_key(dir.path()); + let mock = MockKeyring::with_password(&STANDARD.encode([1u8; 16])); // wrong length + let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(result, expected); + } + + // ---- Backend::File tests ---- + + #[test] + fn file_backend_reads_existing_key() { + let dir = tempfile::tempdir().unwrap(); + let (expected, key_file) = write_test_key(dir.path()); + let mock = MockKeyring::with_password("should-not-be-used"); + let result = resolve_key(KeyringBackend::File, &mock, &key_file).unwrap(); + assert_eq!(result, expected, "file backend should ignore keyring"); + } + + #[test] + fn file_backend_generates_when_missing() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + let mock = MockKeyring::no_entry(); + let key = resolve_key(KeyringBackend::File, &mock, &key_file).unwrap(); + assert_eq!(key.len(), 32); + assert!(key_file.exists()); + assert!( + mock.last_set.borrow().is_none(), + "file backend must not touch keyring" + ); + } + + #[test] + fn file_backend_skips_keyring_entirely() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let (file_key, key_file) = write_test_key(dir.path()); + // Keyring has a DIFFERENT key — file backend should ignore it. + let mock = MockKeyring::with_password(&STANDARD.encode([99u8; 32])); + let result = resolve_key(KeyringBackend::File, &mock, &key_file).unwrap(); + assert_eq!(result, file_key, "must return the file key, not keyring"); + } + + // ---- Stability tests ---- + + #[test] + fn key_is_stable_across_calls() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + let mock = MockKeyring::platform_error(); + let key1 = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + let key2 = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + assert_eq!(key1, key2); + } + + // ---- KeyringBackend::from_env tests ---- + + #[test] + fn backend_default_is_keyring() { + // from_env reads the env; default (empty/unset) → Keyring + assert_eq!(KeyringBackend::from_env(), KeyringBackend::Keyring); + } + + // ---- 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() { diff --git a/src/main.rs b/src/main.rs index fb28ffcf..22259a44 100644 --- a/src/main.rs +++ b/src/main.rs @@ -449,6 +449,9 @@ fn print_usage() { println!( " GOOGLE_WORKSPACE_CLI_CONFIG_DIR Override config directory (default: ~/.config/gws)" ); + println!( + " GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND Keyring backend: keyring (default) or file" + ); println!(" GOOGLE_WORKSPACE_CLI_SANITIZE_TEMPLATE Default Model Armor template"); println!( " GOOGLE_WORKSPACE_CLI_SANITIZE_MODE Sanitization mode: warn (default) or block" diff --git a/src/token_storage.rs b/src/token_storage.rs index d6c5c47c..aa1726c7 100644 --- a/src/token_storage.rs +++ b/src/token_storage.rs @@ -19,7 +19,7 @@ use tokio::sync::Mutex; use yup_oauth2::storage::{TokenInfo, TokenStorage, TokenStorageError}; /// A custom token storage implementation for `yup-oauth2` that encrypts -/// the cached tokens at rest using the AES key derived from the OS keyring. +/// the cached tokens at rest using AES-256-GCM encryption. pub struct EncryptedTokenStorage { file_path: PathBuf, // Add memory cache since TokenStorage getters can be called frequently From bcb1437d77d94cad8143f070ca0ef0f87564f7a4 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Tue, 10 Mar 2026 01:55:26 +0000 Subject: [PATCH 2/6] 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 cc0099a5..cb99a30e 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 From dab9ee395e15d1eb33824cfea0b913f76c0f2b80 Mon Sep 17 00:00:00 2001 From: jpoehnelt-bot Date: Tue, 10 Mar 2026 10:19:45 -0600 Subject: [PATCH 3/6] fix(credential_store): use O_EXCL for race-safe key generation Use create_new(true) (O_EXCL on Unix, CREATE_NEW on Windows) when generating a new encryption key file. If another process wins the race, read their key instead. Platform-independent. --- src/credential_store.rs | 63 ++++++++++++++++++++++++++++++++++------- 1 file changed, 53 insertions(+), 10 deletions(-) diff --git a/src/credential_store.rs b/src/credential_store.rs index 39afbcb1..fecdc6bb 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -21,9 +21,8 @@ use keyring::Entry; use rand::RngCore; use std::sync::OnceLock; -/// Persist the base64-encoded encryption key to a local file with restrictive -/// permissions (0600 file, 0700 directory). -fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { +/// Ensure the key-file parent directory exists with restrictive permissions. +fn ensure_key_dir(path: &std::path::Path) -> std::io::Result<()> { if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; #[cfg(unix)] @@ -35,10 +34,37 @@ fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { } } } + Ok(()) +} + +/// Atomically create a **new** key file using `create_new(true)` (`O_EXCL` on +/// Unix, `CREATE_NEW` on Windows). If another process already created the file, +/// returns `Err` with `ErrorKind::AlreadyExists` so the caller can read the +/// winner's key instead. +fn save_key_file_exclusive(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { + use std::io::Write; + ensure_key_dir(path)?; + + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); // atomic: fails if file already exists + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt; + opts.mode(0o600); + } + let mut file = opts.open(path)?; + file.write_all(b64_key.as_bytes())?; + Ok(()) +} + +/// Persist the base64-encoded encryption key to a local file with restrictive +/// permissions (0600 file, 0700 directory). Overwrites any existing file. +fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { + use std::io::Write; + ensure_key_dir(path)?; #[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); @@ -175,9 +201,20 @@ fn resolve_key( // Best-effort: store in keyring. let _ = provider.set_password(&b64_key); - // Always persist to file as durable fallback. - save_key_file(key_file, &b64_key)?; - return Ok(key); + // Atomically create the file; if another process raced us, + // use their key instead. + match save_key_file_exclusive(key_file, &b64_key) { + Ok(()) => return Ok(key), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + if let Some(winner) = read_key_file(key_file) { + return Ok(winner); + } + // File exists but is unreadable/corrupt — overwrite. + save_key_file(key_file, &b64_key)?; + return Ok(key); + } + Err(e) => return Err(e.into()), + } } Err(e) => { eprintln!("Warning: keyring access failed, falling back to file storage: {e}"); @@ -190,11 +227,17 @@ fn resolve_key( return Ok(key); } - // --- 3. Generate new key, save to file ------------------------------- + // --- 3. Generate new key, save to file (race-safe) ------------------- let key = generate_random_key(); let b64_key = STANDARD.encode(key); - save_key_file(key_file, &b64_key)?; - Ok(key) + match save_key_file_exclusive(key_file, &b64_key) { + Ok(()) => Ok(key), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { + // Another process created the file first — use their key. + read_key_file(key_file).ok_or_else(|| anyhow::anyhow!("key file exists but is corrupt")) + } + Err(e) => Err(e.into()), + } } /// Returns the encryption key, generating and persisting one if it doesn't exist. From 06c42e2929326d38fbc3d7a8945965d861e865a4 Mon Sep 17 00:00:00 2001 From: jpoehnelt-bot Date: Tue, 10 Mar 2026 10:38:24 -0600 Subject: [PATCH 4/6] fix(credential_store): sync winner's key into keyring after file race When two processes race to create the encryption key file, the loser now syncs the winner's key back into the keyring. Without this, the keyring and file could permanently diverge. --- src/credential_store.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/credential_store.rs b/src/credential_store.rs index fecdc6bb..01dd3ade 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -207,6 +207,9 @@ fn resolve_key( Ok(()) => return Ok(key), Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => { if let Some(winner) = read_key_file(key_file) { + // Sync the winner's key into the keyring so both + // backends stay consistent after the race. + let _ = provider.set_password(&STANDARD.encode(winner)); return Ok(winner); } // File exists but is unreadable/corrupt — overwrite. From aa7abc0bcabbbd462adb180a7de2510d44d6c508 Mon Sep 17 00:00:00 2001 From: jpoehnelt-bot Date: Tue, 10 Mar 2026 10:41:46 -0600 Subject: [PATCH 5/6] test(credential_store): add 9 tests covering file exclusion, env parsing, and race paths - save_key_file_exclusive: creates new file, rejects existing - save_key_file: overwrites existing - ensure_key_dir: creates nested dirs - KeyringBackend: file/FILE/invalid parsing - Race loser: syncs winner key to keyring - Race loser: corrupt file gets overwritten --- src/credential_store.rs | 122 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) diff --git a/src/credential_store.rs b/src/credential_store.rs index 01dd3ade..0fd7bd6b 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -695,4 +695,126 @@ mod tests { assert_eq!(dec1, dec2); assert_eq!(dec1, plaintext); } + + // ---- save_key_file_exclusive tests ---- + + #[test] + fn save_key_file_exclusive_creates_new_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join(".encryption_key"); + save_key_file_exclusive(&path, "dGVzdA==").unwrap(); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "dGVzdA=="); + } + + #[test] + fn save_key_file_exclusive_rejects_existing_file() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join(".encryption_key"); + std::fs::write(&path, "existing").unwrap(); + let err = save_key_file_exclusive(&path, "new").unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::AlreadyExists); + // Original content is untouched. + assert_eq!(std::fs::read_to_string(&path).unwrap(), "existing"); + } + + // ---- save_key_file tests ---- + + #[test] + fn save_key_file_overwrites_existing() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join(".encryption_key"); + std::fs::write(&path, "old").unwrap(); + save_key_file(&path, "new").unwrap(); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "new"); + } + + // ---- ensure_key_dir tests ---- + + #[test] + fn ensure_key_dir_creates_nested_dirs() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("a").join("b").join("c").join("key"); + ensure_key_dir(&path).unwrap(); + assert!(path.parent().unwrap().is_dir()); + } + + // ---- KeyringBackend::from_env tests ---- + + #[test] + fn backend_from_env_file_lowercase() { + // We can't easily set env vars in parallel tests, but we can test + // the parsing logic directly via the match arm. + assert_eq!( + match "file" { + "file" => KeyringBackend::File, + _ => KeyringBackend::Keyring, + }, + KeyringBackend::File + ); + } + + #[test] + fn backend_from_env_file_uppercase() { + // to_lowercase() should handle "FILE" → "file" + assert_eq!( + match "FILE".to_lowercase().as_str() { + "file" => KeyringBackend::File, + _ => KeyringBackend::Keyring, + }, + KeyringBackend::File + ); + } + + #[test] + fn backend_from_env_invalid_defaults_to_keyring() { + assert_eq!( + match "foobar".to_lowercase().as_str() { + "file" => KeyringBackend::File, + _ => KeyringBackend::Keyring, + }, + KeyringBackend::Keyring + ); + } + + // ---- Race condition tests ---- + + #[test] + fn race_loser_syncs_winner_key_to_keyring() { + use base64::{engine::general_purpose::STANDARD, Engine as _}; + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + // Simulate: file was created by another process between our generate + // and our save_key_file_exclusive call. We pre-create the file so + // save_key_file_exclusive will fail with AlreadyExists. + let winner_key = [77u8; 32]; + std::fs::write(&key_file, STANDARD.encode(winner_key)).unwrap(); + + // Use NoEntry so resolve_key goes into the generate path. + let mock = MockKeyring::no_entry(); + let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + + // Should return the winner's key, not the one we generated. + assert_eq!(result, winner_key); + // The keyring should have been synced with the winner's key. + let synced = mock.last_set.borrow().clone().unwrap(); + assert_eq!(STANDARD.decode(&synced).unwrap(), winner_key); + } + + #[test] + fn race_loser_corrupt_file_overwrites() { + let dir = tempfile::tempdir().unwrap(); + let key_file = dir.path().join(".encryption_key"); + + // Pre-create a corrupt file (not valid base64 for a 32-byte key). + std::fs::write(&key_file, "corrupt-data").unwrap(); + + let mock = MockKeyring::no_entry(); + let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap(); + + // Should generate a new key and overwrite the corrupt file. + assert_eq!(result.len(), 32); + let file_key = read_key_file(&key_file).unwrap(); + assert_eq!(result, file_key, "file should be overwritten with new key"); + } } From 80ce461700f5af4bcf7d2fc707c3d1022855f700 Mon Sep 17 00:00:00 2001 From: jpoehnelt-bot Date: Tue, 10 Mar 2026 10:46:08 -0600 Subject: [PATCH 6/6] feat(credential_store): security and robustness hardening 1. Warn on unrecognized KEYRING_BACKEND values instead of silent default 2. fsync after key file writes for crash durability 3. Zeroize decoded key material from heap after copy 4. Warn if key file has overly permissive Unix permissions (mode & 077) 5. Log which keyring backend was selected to stderr 6. Expose keyring_backend in 'gws auth status' JSON output --- Cargo.lock | 15 ++++++++++ Cargo.toml | 1 + src/auth_commands.rs | 1 + src/credential_store.rs | 61 ++++++++++++++++++++++++++++++++++++----- 4 files changed, 71 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 499c612f..e40df1cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -873,6 +873,7 @@ dependencies = [ "thiserror 2.0.18", "tokio", "yup-oauth2", + "zeroize", ] [[package]] @@ -3596,6 +3597,20 @@ name = "zeroize" version = "1.8.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b97154e67e32c85465826e8bcc1c59429aaaf107c1e4a9e53c8d8ccd5eff88d0" +dependencies = [ + "zeroize_derive", +] + +[[package]] +name = "zeroize_derive" +version = "1.4.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85a5b4158499876c763cb03bc4e49185d3cccbabb15b33c627f7884f43db852e" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.117", +] [[package]] name = "zerotrie" diff --git a/Cargo.toml b/Cargo.toml index a78379dd..5babca80 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -56,6 +56,7 @@ keyring = "3.6.3" async-trait = "0.1.89" serde_yaml = "0.9.34" percent-encoding = "2.3.2" +zeroize = { version = "1.8.2", features = ["derive"] } # The profile that 'cargo dist' will build with diff --git a/src/auth_commands.rs b/src/auth_commands.rs index d78e0295..ade681d6 100644 --- a/src/auth_commands.rs +++ b/src/auth_commands.rs @@ -944,6 +944,7 @@ async fn handle_status() -> Result<(), GwsError> { let mut output = json!({ "auth_method": auth_method, "storage": storage, + "keyring_backend": credential_store::active_backend_name(), "encrypted_credentials": enc_path.display().to_string(), "encrypted_credentials_exists": has_encrypted, "plain_credentials": plain_path.display().to_string(), diff --git a/src/credential_store.rs b/src/credential_store.rs index 0fd7bd6b..a0210fc2 100644 --- a/src/credential_store.rs +++ b/src/credential_store.rs @@ -20,6 +20,7 @@ use aes_gcm::{AeadCore, Aes256Gcm, Nonce}; use keyring::Entry; use rand::RngCore; use std::sync::OnceLock; +use zeroize::Zeroize; /// Ensure the key-file parent directory exists with restrictive permissions. fn ensure_key_dir(path: &std::path::Path) -> std::io::Result<()> { @@ -54,6 +55,7 @@ fn save_key_file_exclusive(path: &std::path::Path, b64_key: &str) -> std::io::Re } let mut file = opts.open(path)?; file.write_all(b64_key.as_bytes())?; + file.sync_all()?; // fsync: ensure key is durable before returning Ok(()) } @@ -70,6 +72,7 @@ fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { options.write(true).create(true).truncate(true).mode(0o600); let mut file = options.open(path)?; file.write_all(b64_key.as_bytes())?; + file.sync_all()?; // fsync: ensure key is durable before returning } #[cfg(not(unix))] { @@ -79,15 +82,38 @@ fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> { } /// Read and decode a base64-encoded 256-bit key from a file. +/// +/// On Unix, warns if the file is world-readable (mode & 0o077 != 0). fn read_key_file(path: &std::path::Path) -> Option<[u8; 32]> { use base64::{engine::general_purpose::STANDARD, Engine as _}; + + // Item 4: validate file permissions on read + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if let Ok(meta) = std::fs::metadata(path) { + let mode = meta.permissions().mode(); + if mode & 0o077 != 0 { + eprintln!( + "Warning: encryption key file {} has overly permissive mode {:04o}. \ + Expected 0600. Run: chmod 600 {}", + path.display(), + mode & 0o777, + path.display() + ); + } + } + } + let b64_key = std::fs::read_to_string(path).ok()?; - let decoded = STANDARD.decode(b64_key.trim()).ok()?; + let mut decoded = STANDARD.decode(b64_key.trim()).ok()?; if decoded.len() == 32 { let mut arr = [0u8; 32]; arr.copy_from_slice(&decoded); + decoded.zeroize(); // scrub decoded key material from heap Some(arr) } else { + decoded.zeroize(); None } } @@ -145,13 +171,27 @@ enum KeyringBackend { impl KeyringBackend { fn from_env() -> Self { - match std::env::var("GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND") - .unwrap_or_default() - .to_lowercase() - .as_str() - { + let raw = std::env::var("GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND").unwrap_or_default(); + let lower = raw.to_lowercase(); + match lower.as_str() { "file" => KeyringBackend::File, - _ => KeyringBackend::Keyring, + "keyring" | "" => KeyringBackend::Keyring, + other => { + // Item 1: warn on unrecognized values + eprintln!( + "Warning: unknown GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=\"{other}\", \ + defaulting to \"keyring\". Valid values: \"keyring\", \"file\"." + ); + KeyringBackend::Keyring + } + } + } + + /// Human-readable name for logging and status output. + fn as_str(&self) -> &'static str { + match self { + KeyringBackend::Keyring => "keyring", + KeyringBackend::File => "file", } } } @@ -255,6 +295,8 @@ fn get_or_create_key() -> anyhow::Result<[u8; 32]> { } let backend = KeyringBackend::from_env(); + // Item 5: log which backend was selected + eprintln!("Using keyring backend: {}", backend.as_str()); let username = std::env::var("USER") .or_else(|_| std::env::var("USERNAME")) @@ -314,6 +356,11 @@ pub fn decrypt(data: &[u8]) -> anyhow::Result> { Ok(plaintext) } +/// Returns the name of the active keyring backend for status display. +pub fn active_backend_name() -> &'static str { + KeyringBackend::from_env().as_str() +} + /// Returns the path for encrypted credentials. pub fn encrypted_credentials_path() -> PathBuf { crate::auth_commands::config_dir().join("credentials.enc")