fix(auth): enable native keyring backends on top of #359#373
fix(auth): enable native keyring backends on top of #359#373zssz wants to merge 5 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 11fdc10 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 resolves a critical authentication persistence issue where Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a critical authentication persistence bug by enabling native keyring backends and refactoring credential loading. The changes enhance security by utilizing strong cryptographic primitives and secure file permissions, with no high or critical security vulnerabilities identified. My feedback focuses on improving the robustness of the new test to prevent potential issues in CI environments.
|
/gemini review |
|
Thanks for this PR! #359 has been merged which adds Could you rebase this on top of the latest main (which includes #359) so we can evaluate the two together? The credential_store.rs has changed significantly. |
I rebased the branch onto Updated PR title and description aswell. |
32b0177 to
b0e72c5
Compare
|
/gemini review |
1 similar comment
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a critical issue where authentication credentials were not being persisted securely due to the keyring crate silently falling back to an in-memory mock backend. By enabling the native keyring features in Cargo.toml, the application will now correctly use the host OS's secure storage, with a robust file-based fallback. The additional fix to serialize a test that was subject to a race condition is also a good catch and correctly implemented. The changes are well-executed and significantly improve the security and reliability of credential management.
|
D-Bus dependency: This introduces a build-time dependency on libdbus-1-dev for Linux (which is why CI is failing). Many of our users run
|
|
/gemini review |
Implemented in I changed
That keeps I also regenerated |
There was a problem hiding this comment.
Code Review
This pull request correctly enables native keyring backends for macOS and Windows by using target-specific dependencies in Cargo.toml. This change addresses a critical issue where credentials could be silently stored in an in-memory mock backend instead of the secure OS store. The implementation is sound and follows best practices for platform-specific features in Rust. Additionally, the pull request includes a fix for a test race condition in src/auth_commands.rs by correctly marking a test that interacts with environment variables as serial. The changes are well-explained and improve the security and reliability of credential storage.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #373 +/- ##
==========================================
- Coverage 64.40% 64.39% -0.02%
==========================================
Files 38 38
Lines 15584 15583 -1
==========================================
- Hits 10037 10034 -3
- Misses 5547 5549 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly enables native keyring backends for macOS and Windows by using target-specific dependencies in Cargo.toml. This resolves the issue where the keyring crate could silently fall back to a non-persistent in-memory store. The changes are well-structured and directly address the motivation described. Additionally, the pull request includes a fix for a test race condition by serializing a test that modifies shared environment state, which is the correct approach. The changes look solid and improve the credential storage mechanism as intended.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where the keyring crate could silently fall back to a mock in-memory store on macOS and Windows. The fix uses target-specific dependencies in Cargo.toml to enable native keyring backends on these platforms. Additionally, a test is marked as serial to prevent race conditions related to environment variables, which should improve test suite stability. The changes appear correct and align with the pull request's goals.
|
@jpoehnelt should we move this forward? Let me know if you think it needs more changes. |
Description
This PR is rebased on top of the latest
main, including #359.Motivation:
#359 added
GOOGLE_WORKSPACE_CLI_KEYRING_BACKENDand made.encryption_keya durable fallback, but it did not change how thekeyringcrate itself is compiled. The remaining issue was thatkeyringcould still silently fall back to its in-memory mock backend on platforms where we intended to use the native OS keyring.What this PR changes:
keyringbackends only on supported desktop targets:apple-nativewindows-nativekeyring#359's file fallback behavior intact for Docker, CI, and other headless/minimal environments.Cargo.lockso the Linux D-Bus-related dependencies introduced by the earlier approach are no longer part of this PR.auth_commands::tests::config_dir_returns_gws_subdiras serial to avoid a race with other tests mutatingGOOGLE_WORKSPACE_CLI_CONFIG_DIRduring local verification.Gemini Reviewedjob does not hard-fail on fork PR review events when GitHub provides a read-only token.Why this fixes it:
backend=keyringnow compiles against a real native backend instead of risking the in-memory mock backend.#359's file-based fallback path.Note on
Cargo.lock:The lockfile changed because the dependency graph is now target-specific. After regenerating it, the Linux D-Bus-related crates from the previous approach are no longer part of this branch.
This does not add a new command or Discovery request path, so there is no relevant
--dry-runoutput for the template section below.Dry Run Output:
{ "not_applicable": "This change updates auth backend compilation, test isolation, and workflow behavior; it does not add a new command or API request body." }Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.