feat(auth): use strict OS keychain integration on macOS and Windows#631
feat(auth): use strict OS keychain integration on macOS and Windows#631
Conversation
🦋 Changeset detectedLatest commit: 65b766a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the security posture of the CLI by enforcing native OS keychain usage for credential storage on macOS and Windows. It removes the reliance on insecure local fallback files on these platforms while maintaining necessary compatibility for Linux-based environments. Additionally, the PR introduces new documentation for Google Apps Script management capabilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request implements platform-specific keyring logic, enforcing strict native keychain integration on macOS and Windows while retaining file-based fallbacks for Linux to ensure compatibility in headless environments. It also introduces documentation for new Google Apps Script management skills. A review comment identified an inconsistency in error handling when persisting keys to the OS keyring on macOS and Windows, which could lead to decryption failures in subsequent sessions, and noted a potential TOCTOU race condition during the deletion of legacy encryption key files.
|
/gemini review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #631 +/- ##
==========================================
- Coverage 70.89% 70.86% -0.03%
==========================================
Files 44 44
Lines 20646 20646
==========================================
- Hits 14637 14631 -6
- Misses 6009 6015 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request implements strict OS keychain integration for macOS and Windows, ensuring that encryption keys are stored securely in native managers and legacy fallback files are removed. Linux deployments retain the file-based fallback for compatibility. Additionally, the PR introduces new 'gws-script' and 'gws-script-push' skills for managing Google Apps Script projects. Feedback was provided to refactor the key generation logic in the credential store to reduce code duplication and improve maintainability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements strict OS keychain integration for macOS and Windows to enhance credential security, while maintaining file-based fallbacks for Linux environments. It also introduces documentation for new script-related skills and improves test isolation. The review feedback focuses on security hardening, specifically recommending the sanitization of error messages to prevent terminal escape sequence injection and ensuring that legacy key file deletion is properly verified. Additionally, the inclusion of new skill documentation was flagged as scope creep that should be handled in a separate PR.
…fallbacks This commit removes the inadvertently included skills, uses output::sanitize_for_terminal to protect the user from escape sequence injection via keyring errors, and ensures removal of the fallback file properly alerts the user when an io error occurs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements strict OS keychain integration for macOS and Windows while maintaining a file-based fallback for Linux. It includes logic to automatically delete legacy encryption key files upon successful keyring access and isolates test environments to prevent race conditions. Feedback suggests improving error messages for keyring failures by recommending the file-based backend as an alternative and recommends separating the newly added Google Apps Script documentation into a dedicated pull request to maintain atomic changes.
| if let Err(e) = provider.set_password(&b64_key) { | ||
| anyhow::bail!( | ||
| "Failed to set key in OS keyring: {}", | ||
| sanitize_for_terminal(&e.to_string()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
For consistency and better user experience, it would be beneficial to include the suggestion to use the file-based backend in this error message, similar to the one on line 233. If setting the key in the OS keyring fails, the user is left without a clear way to proceed. Suggesting GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=file provides a helpful escape hatch.
| if let Err(e) = provider.set_password(&b64_key) { | |
| anyhow::bail!( | |
| "Failed to set key in OS keyring: {}", | |
| sanitize_for_terminal(&e.to_string()) | |
| ); | |
| } | |
| if let Err(e) = provider.set_password(&b64_key) { | |
| anyhow::bail!( | |
| "Failed to set key in OS keyring: {}. Set GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=file to use file storage.", | |
| sanitize_for_terminal(&e.to_string()) | |
| ); | |
| } |
| | [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. | |
There was a problem hiding this comment.
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
- Avoid introducing changes that are outside the primary goal of a pull request to prevent scope creep.
Closes #623.
The CLI no longer writes a fallback
.encryption_keytext 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_keyfile 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.