fix(auth): preserve Linux encryption key fallback#371
fix(auth): preserve Linux encryption key fallback#371mkrueger12 wants to merge 1 commit intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: de50550 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 |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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 regression affecting Linux users, specifically those in headless or unstable keyring environments, where previously stored encrypted credentials could become inaccessible. By implementing a conservative strategy to always maintain a local fallback encryption key on Linux, the change ensures the durability and reliability of credential decryption without fully reverting the benefits of OS keyring integration. 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 correctly addresses a critical regression on Linux where encrypted credentials could become unreadable. The approach of maintaining a fallback encryption key file on Linux, even when the OS keyring is accessible, is a sound and conservative fix for the reported issue. The changes are well-contained within credential_store.rs and are accompanied by good unit tests.
I have one high-severity comment regarding a potential race condition in the key generation logic that could affect concurrent executions of the CLI. While this issue pre-dates this PR, it is relevant to the modified code and could lead to a similar data loss scenario.
| // Keep a local fallback on Linux because Secret | ||
| // Service availability can be transient across fresh | ||
| // processes in headless environments. | ||
| sync_key_file_after_keyring_success(&key_file, &b64_key); |
There was a problem hiding this comment.
The get_or_create_key function appears to have a race condition when multiple gws processes run concurrently and the encryption key does not yet exist. The OnceLock only prevents races within a single process, but two separate processes could still race to create the key.
For example:
- Process A starts, finds no key.
- Process B starts, finds no key.
- Process A generates key K_A, saves it to the keyring and/or file.
- Process B generates key K_B, saves it, overwriting K_A.
If Process A has already used K_A to encrypt credentials, those credentials will become undecryptable because the key has been replaced by K_B.
To resolve this, consider using a file lock (e.g., with the fs2 crate) around the key creation logic to ensure that only one process can create the key at a time. This would make the key generation atomic across processes.
|
/gemini review |
Signed-off-by: Max <max@backlandlabs.io>
|
Addressed the race condition comment. What changed:
Validation run locally after the update:
Latest commit on the branch: |
|
/gemini review |
Description
Fix a Linux credential storage regression in
v0.9.1where encrypted OAuth credentials can become undecryptable on the same machine after the key-storage change in5872dbe.This PR keeps a stable local
.encryption_keyfallback on Linux even when OS keyring operations appear to succeed.Fixes #364.
Dry Run Output:
// Not applicable: this PR does not add a new feature or command.Problem
Issue #364 reports a Linux environment where:
credentials.encexistsgws auth statusshowsencrypted_credentials_exists: trueencryption_valid: falseFailed to decrypt credentials: Decryption failed. Credentials may have been created on a different machineThe environment is a Debian/headless-ish setup with DBus present and
libsecretinstalled, but without a clearly stable desktop keyring session.Root cause
Commit
5872dbechanged the key persistence behavior to stop keeping.encryption_keyonce the OS keyring was available.That is fine when the keyring is reliably available across fresh processes, but it is too optimistic for some Linux/Secret Service setups:
.encryption_keyfallbackcredentials.encis still present, but it can no longer be decryptedIn other words, the write path can succeed once while the read path is not stable across invocations.
What this PR changes
Linux behavior
On Linux, after a successful keyring read/write, keep
.encryption_keysynchronized as a stable fallback instead of removing it.This applies to:
Non-Linux behavior
Keep the current behavior:
Why this approach
The regression appears to be Linux-specific and tied to environments where Secret Service is partially available but not reliably retrievable across processes.
Keeping the fallback on Linux is a conservative durability fix:
credentials.encunreadable on the same machineSecurity tradeoff
This does slightly reduce defense-in-depth on Linux compared to keyring-only storage, because the AES key may also exist on disk in
.encryption_key.That said:
0600, parent dir0700)A stricter future approach could try to prove keyring stability across fresh processes before deleting the fallback. For this regression fix, preserving the fallback on Linux seemed like the least surprising and safest behavior.
Tests and validation
Added coverage for the platform strategy in
credential_store:sync_key_file_after_keyring_success_matches_platform_strategylinux_strategy_persists_fallback_key_materialAlso added the required changeset file:
.changeset/polite-eggs-burn.mdCommands run locally:
cargo fmt --all cargo test credential_store -- --nocapture cargo clippy -- -D warningsReproduction path validated during investigation
Checked out
v0.9.1and traced the regression to the current logic insrc/credential_store.rs:.encryption_keyThat matches the failure mode reported in #364.
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.