Skip to content

feat(auth): use strict OS keychain integration on macOS and Windows #631

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jpoehnelt merged 8 commits into from
Mar 26, 2026
Merged

feat(auth): use strict OS keychain integration on macOS and Windows #631

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
791794d
fix(auth): implement platform-aware keyring storage
jpoehnelt-bot Mar 26, 2026
b8ecd29
chore: regenerate skills [skip ci]
googleworkspace-bot Mar 26, 2026
bebe089
fix(auth): address PR comments on error handling and TOCTOU vulnerabi…
jpoehnelt-bot Mar 26, 2026
63fb7f8
fix(test): isolate config directory to prevent race conditions during…
jpoehnelt-bot Mar 26, 2026
654ae46
refactor(auth): deduplicate key generation logic in credential store
jpoehnelt-bot Mar 26, 2026
af67473
fix(test): execute credential tests serially to prevent environment p…
jpoehnelt-bot Mar 26, 2026
3a33c93
fix(auth): sanitize os keyring error strings and handle missing file …
jpoehnelt-bot Mar 26, 2026
65b766a
chore: regenerate skills [skip ci]
googleworkspace-bot Mar 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/platform-aware-keyring.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@googleworkspace/cli": patch
---

feat(auth): use strict OS keychain integration on macOS and Windows

Closes #623. The CLI no longer writes a fallback `.encryption_key` text file on macOS and Windows when securely storing credentials. Instead, it strictly uses the native OS keychain (Keychain Access on macOS, Credential Manager on Windows). If an old `.encryption_key` file is found during a successful keychain login, it will be automatically deleted for security.
Linux deployments continue to use a seamless file-based fallback by default to ensure maximum compatibility with headless continuous integration (CI) runners, Docker containers, and SSH environments without desktop DBUS services.
3 changes: 3 additions & 0 deletions crates/google-workspace-cli/src/auth.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,6 +762,9 @@ mod tests {
let dir = tempfile::tempdir().unwrap();
let enc_path = dir.path().join("credentials.enc");

// Isolate global config dir to prevent races with other tests
std::env::set_var("GOOGLE_WORKSPACE_CLI_CONFIG_DIR", dir.path());

// Encrypt and write
let encrypted = crate::credential_store::encrypt(json.as_bytes()).unwrap();
std::fs::write(&enc_path, &encrypted).unwrap();
Expand Down
224 changes: 174 additions & 50 deletions crates/google-workspace-cli/src/credential_store.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ fn save_key_file_exclusive(path: &std::path::Path, b64_key: &str) -> std::io::Re

/// Persist the base64-encoded encryption key to a local file with restrictive
/// permissions (0600 file, 0700 directory). Overwrites any existing file.
/// Persist the base64-encoded encryption key to a local file with restrictive
/// permissions. Uses atomic_write to prevent TOCTOU/symlink race conditions.
/// Uses atomic_write to prevent TOCTOU/symlink race conditions.
#[allow(dead_code)]
fn save_key_file(path: &std::path::Path, b64_key: &str) -> std::io::Result<()> {
crate::fs_util::atomic_write(path, b64_key.as_bytes())
}
Expand Down Expand Up @@ -202,64 +202,106 @@ fn resolve_key(

// --- 1. Try keyring (only when backend = Keyring) --------------------
if backend == KeyringBackend::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);
// Ensure file backup stays in sync with keyring so
// credentials survive keyring loss (e.g. after OS
// upgrades, container restarts, daemon changes).
if let Err(err) = save_key_file(key_file, &b64_key) {
eprintln!(
"Warning: failed to sync keyring backup file at '{}': {err}",
key_file.display()
);
#[cfg(any(target_os = "macos", target_os = "windows"))]
{
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);
// Cleanup insecure file fallback if it still exists
let _ = std::fs::remove_file(key_file);
Comment thread
jpoehnelt marked this conversation as resolved.
Outdated
return Ok(arr);
}
return Ok(arr);
}
}
// Keyring contained invalid data — fall through to file.
}
Err(keyring::Error::NoEntry) => {
// 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));
Err(keyring::Error::NoEntry) => {
let key = generate_random_key();
let b64_key = STANDARD.encode(key);
if let Err(e) = provider.set_password(&b64_key) {
anyhow::bail!("Failed to initialize OS keyring: {e}");
}
let _ = std::fs::remove_file(key_file);
return Ok(key);
}
Err(e) => {
anyhow::bail!("OS keyring failed: {e}. Set GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=file to use file storage.");
Comment thread
jpoehnelt marked this conversation as resolved.
Outdated
}
}
// If keyring data was invalid length/base64, generate new
let key = generate_random_key();
let b64_key = STANDARD.encode(key);
if let Err(e) = provider.set_password(&b64_key) {
anyhow::bail!("Failed to set key in OS keyring after invalid data: {e}");
}
let _ = std::fs::remove_file(key_file);
Comment thread
jpoehnelt marked this conversation as resolved.
Outdated
return Ok(key);
Comment thread
jpoehnelt marked this conversation as resolved.
}
Comment thread
jpoehnelt marked this conversation as resolved.

// Generate a new key.
let key = generate_random_key();
let b64_key = STANDARD.encode(key);

// Best-effort: store in keyring.
let _ = provider.set_password(&b64_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) {
// 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);
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
{
// On Linux, keyring uses a mock store by default without C DBus dependencies,
// so we continue to use the file fallback for reliability.
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);
// Ensure file backup stays in sync with keyring so
// credentials survive keyring loss (e.g. after OS
// upgrades, container restarts, daemon changes).
if let Err(err) = save_key_file(key_file, &b64_key) {
eprintln!(
"Warning: failed to sync keyring backup file at '{}': {err}",
key_file.display()
);
}
return Ok(arr);
}
// File exists but is unreadable/corrupt — overwrite.
save_key_file(key_file, &b64_key)?;
}
// Keyring contained invalid data — fall through to file.
}
Err(keyring::Error::NoEntry) => {
// 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);
}
Err(e) => return Err(e.into()),

// Generate a new key.
let key = generate_random_key();
let b64_key = STANDARD.encode(key);

// Best-effort: store in keyring.
let _ = provider.set_password(&b64_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) {
// 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.
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: {}",
sanitize_for_terminal(&e.to_string())
);
}
}
Err(e) => {
eprintln!(
"Warning: keyring access failed, falling back to file storage: {}",
sanitize_for_terminal(&e.to_string())
);
}
}
}
Expand Down Expand Up @@ -293,7 +335,11 @@ fn get_or_create_key() -> anyhow::Result<[u8; 32]> {
return Ok(*key);
}

#[cfg(not(test))]
let backend = KeyringBackend::from_env();
#[cfg(test)]
let backend = KeyringBackend::File; // Force file to avoid native keychain prompts during test execution

// Item 5: log which backend was selected
eprintln!("Using keyring backend: {}", backend.as_str());

Expand Down Expand Up @@ -407,6 +453,8 @@ pub fn load_encrypted() -> anyhow::Result<String> {

#[cfg(test)]
mod tests {
#![allow(dead_code)]

use super::*;
use std::cell::RefCell;

Expand Down Expand Up @@ -513,7 +561,73 @@ mod tests {
assert_eq!(result, expected);
}

// ---- Backend::Keyring tests (macOS/Windows specific behavior) ----

#[test]
#[cfg(any(target_os = "macos", target_os = "windows"))]
fn keyring_backend_cleans_up_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();
assert!(key_file.exists());

// Keyring has a valid 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);
assert!(
!key_file.exists(),
"Legacy file must be deleted upon successful keyring read"
);
}

#[test]
#[cfg(any(target_os = "macos", target_os = "windows"))]
fn keyring_backend_cleans_up_legacy_file_on_generation() {
let dir = tempfile::tempdir().unwrap();
let key_file = dir.path().join(".encryption_key");

std::fs::write(&key_file, "legacy-data").unwrap();
assert!(key_file.exists());

let mock = MockKeyring::no_entry();

let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file).unwrap();

assert_eq!(result.len(), 32);
assert!(
!key_file.exists(),
"Legacy file must be deleted upon successful keyring generation"
);
}

#[test]
#[cfg(any(target_os = "macos", target_os = "windows"))]
fn keyring_backend_returns_error_on_platform_failure() {
let dir = tempfile::tempdir().unwrap();
let key_file = dir.path().join(".encryption_key");

let mock = MockKeyring::platform_error();

let result = resolve_key(KeyringBackend::Keyring, &mock, &key_file);

assert!(result.is_err());
assert!(result
.unwrap_err()
.to_string()
.contains("OS keyring failed"));
}

// ---- Backend::Keyring tests (Linux fallback behavior) ----

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn keyring_backend_creates_file_backup_when_missing() {
use base64::{engine::general_purpose::STANDARD, Engine as _};
let dir = tempfile::tempdir().unwrap();
Expand All @@ -535,6 +649,7 @@ mod tests {
}

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn keyring_backend_syncs_file_when_keyring_differs() {
use base64::{engine::general_purpose::STANDARD, Engine as _};
let dir = tempfile::tempdir().unwrap();
Expand All @@ -554,6 +669,7 @@ mod tests {
}

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn keyring_backend_no_entry_reads_file() {
let dir = tempfile::tempdir().unwrap();
let (expected, key_file) = write_test_key(dir.path());
Expand All @@ -568,6 +684,7 @@ mod tests {
}

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn keyring_backend_no_entry_no_file_generates_and_saves_both() {
let dir = tempfile::tempdir().unwrap();
let key_file = dir.path().join(".encryption_key");
Expand All @@ -581,6 +698,7 @@ mod tests {
}

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn keyring_backend_no_entry_no_file_keyring_set_fails() {
let dir = tempfile::tempdir().unwrap();
let key_file = dir.path().join(".encryption_key");
Expand All @@ -593,6 +711,7 @@ mod tests {
}

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn keyring_backend_platform_error_falls_back_to_file() {
let dir = tempfile::tempdir().unwrap();
let (expected, key_file) = write_test_key(dir.path());
Expand All @@ -602,6 +721,7 @@ mod tests {
}

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn keyring_backend_platform_error_no_file_generates() {
let dir = tempfile::tempdir().unwrap();
let key_file = dir.path().join(".encryption_key");
Expand All @@ -612,6 +732,7 @@ mod tests {
}

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn keyring_backend_invalid_keyring_data_uses_file() {
use base64::{engine::general_purpose::STANDARD, Engine as _};
let dir = tempfile::tempdir().unwrap();
Expand Down Expand Up @@ -660,6 +781,7 @@ mod tests {
// ---- Stability tests ----

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn key_is_stable_across_calls() {
let dir = tempfile::tempdir().unwrap();
let key_file = dir.path().join(".encryption_key");
Expand Down Expand Up @@ -858,6 +980,7 @@ mod tests {
// ---- Race condition tests ----

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn race_loser_syncs_winner_key_to_keyring() {
use base64::{engine::general_purpose::STANDARD, Engine as _};
let dir = tempfile::tempdir().unwrap();
Expand All @@ -881,6 +1004,7 @@ mod tests {
}

#[test]
#[cfg(not(any(target_os = "macos", target_os = "windows")))]
fn race_loser_corrupt_file_overwrites() {
let dir = tempfile::tempdir().unwrap();
let key_file = dir.path().join(".encryption_key");
Expand Down
2 changes: 2 additions & 0 deletions docs/skills.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Core Google Workspace API skills.
| [gws-events](../skills/gws-events/SKILL.md) | Subscribe to Google Workspace events. |
| [gws-modelarmor](../skills/gws-modelarmor/SKILL.md) | Google Model Armor: Filter user-generated content for safety. |
| [gws-workflow](../skills/gws-workflow/SKILL.md) | Google Workflow: Cross-service productivity workflows. |
| [gws-script](../skills/gws-script/SKILL.md) | Manage Google Apps Script projects. |
Comment thread
jpoehnelt marked this conversation as resolved.
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This pull request introduces a new gws-script skill, which seems unrelated to the primary goal of improving OS keychain integration for authentication. Mixing unrelated features in a single pull request increases its complexity, making it harder to review and test. It's a best practice to keep pull requests focused on a single, atomic change. Please consider moving the gws-script feature to a separate pull request.

References
  1. Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.


## Helpers

Expand Down Expand Up @@ -57,6 +58,7 @@ Shortcut commands for common operations.
| [gws-workflow-email-to-task](../skills/gws-workflow-email-to-task/SKILL.md) | Google Workflow: Convert a Gmail message into a Google Tasks entry. |
| [gws-workflow-weekly-digest](../skills/gws-workflow-weekly-digest/SKILL.md) | Google Workflow: Weekly summary: this week's meetings + unread email count. |
| [gws-workflow-file-announce](../skills/gws-workflow-file-announce/SKILL.md) | Google Workflow: Announce a Drive file in a Chat space. |
| [gws-script-push](../skills/gws-script-push/SKILL.md) | Google Apps Script: Upload local files to an Apps Script project. |

## Personas

Expand Down
Loading
Loading