Skip to content

fix(auth): verify keyring round-trip to prevent phantom key loss#370

Closed
mikeciesla wants to merge 2 commits intogoogleworkspace:mainfrom
mikeciesla:fix/keyring-roundtrip-verify
Closed

fix(auth): verify keyring round-trip to prevent phantom key loss#370
mikeciesla wants to merge 2 commits intogoogleworkspace:mainfrom
mikeciesla:fix/keyring-roundtrip-verify

Conversation

@mikeciesla
Copy link

@mikeciesla mikeciesla commented Mar 10, 2026

Summary

  • After every keyring::Entry::set_password(), immediately call get_password() and compare — only trust the keyring if the round-trip succeeds
  • Always persist the encryption key to ~/.config/gws/.encryption_key as a file backup, even when the keyring appears to work
  • Stop deleting the file backup when a keyring entry already exists (guards against future keyring failures)
  • Switch save_key_file to atomic write (tmp + rename) for crash safety

Problem

On macOS, keyring::Entry::set_password() can return Ok(()) without actually persisting to Keychain (phantom write). This was introduced in #345 which removed the file fallback when the keyring "succeeded". The result: the encryption key is lost between runs and all commands fail with:

Decryption failed. Credentials may have been created on a different machine.

Test plan

  • cargo test — 509 passed
  • cargo clippy -- -D warnings — clean
  • Manual test on macOS Sequoia 15.7.4 with phantom keyring writes:
    • gws auth login generates key and saves to file
    • gws drive files list decrypts successfully across process restarts
    • Key fingerprint is consistent across runs (verified with debug tracing)

@changeset-bot
Copy link

changeset-bot bot commented Mar 10, 2026

🦋 Changeset detected

Latest commit: 804a61b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@googleworkspace/cli Patch

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

@google-cla
Copy link

google-cla bot commented Mar 10, 2026

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.

@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello, 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 enhances the robustness of encryption key storage by addressing a critical issue where keyring writes, particularly on macOS, could falsely report success without actually persisting the data. The changes introduce immediate verification of keyring operations and establish a consistent file-based backup mechanism, preventing key loss and ensuring continuous access to encrypted credentials.

Highlights

  • Keyring Write Verification: Implemented a round-trip verification for keyring writes, immediately calling get_password() after set_password() to ensure the key was actually persisted, addressing phantom write issues on macOS.
  • Persistent File Backup: Ensured the encryption key is always persisted to ~/.config/gws/.encryption_key as a file backup, even when the keyring appears to function correctly.
  • File Backup Retention: Stopped deleting the local file backup when a keyring entry already exists or is successfully migrated, providing an additional layer of resilience against future keyring failures.
Changelog
  • .changeset/fix-keyring-roundtrip-verify.md
    • Added a new changeset file documenting the fix for keyring phantom writes and the introduction of round-trip verification and persistent file backups.
  • src/credential_store.rs
    • Updated key retrieval logic to retain the local key file as a backup instead of deleting it when a key is successfully read from the keyring.
    • Modified key migration from file to keyring to include a round-trip verification of the keyring write before deleting the original file.
    • Refactored new key generation to always persist the key to a local file as a backup and added round-trip verification for keyring writes, logging a warning if verification fails.
Activity
  • The author executed cargo test, resulting in 505 passed tests.
  • The author ran cargo clippy -- -D warnings, confirming a clean build with no warnings.
  • Manual testing was performed on macOS Sequoia 15.5 to verify the fix for phantom keyring writes, confirming successful decryption across process restarts and consistent key fingerprints.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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

  1. Review the Generative AI Prohibited Use Policy, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request aims to address a critical issue on macOS where phantom writes to the keyring could lead to encryption key loss, by implementing a round-trip verification and always maintaining a file-based backup. However, these changes introduce a significant security regression by permanently storing the encryption key in plain text on the file system, making user credentials vulnerable to local attackers. Additionally, there's a critical issue in the key migration logic where the file backup is still being deleted, which contradicts the PR's goal of maintaining a persistent backup.

I am having trouble creating individual review comments. Click here to see my feedback.

src/credential_store.rs (109-118)

critical

This block migrates a file-based key to the keyring but then deletes the file backup upon success. This contradicts the PR's stated goal to always preserve the file as a backup to guard against future keyring failures. Deleting the file here re-introduces the risk of key loss.

The logic should be updated to attempt migration but never delete the file backup, which would make it consistent with the other changes in this PR.

                                // Migrate file key into keyring, but keep the file as a backup.
                                let b64_key_trimmed = b64_key.trim();
                                if entry.set_password(b64_key_trimmed).is_err()
                                    || entry.get_password().ok().as_deref() != Some(b64_key_trimmed)
                                {
                                    eprintln!(
                                        "Warning: keyring write could not be verified during migration. "
                                        + "The file-based key will continue to be used."
                                    );
                                }

src/credential_store.rs (92-96)

security-high high

Removing the code that deletes the local encryption key file introduces a security regression. Previously, the application ensured that the encryption key was only stored in the secure OS keyring once migration was complete. By retaining the file backup permanently, the application now stores the encryption key in plain text (base64) on the file system alongside the encrypted credentials. This allows a local attacker with access to the user's home directory to bypass the security of the OS keyring and decrypt the credentials directly from the file system.

src/credential_store.rs (145)

security-high high

Always persisting the encryption key to a local file as a backup significantly increases the attack surface. The encryption key is stored in an unencrypted format (base64) in the same directory as the encrypted credentials. This undermines the security model of using an OS keyring, which is intended to provide a more secure, often hardware-backed, storage for sensitive keys. While this change addresses reliability issues on macOS, it does so at the cost of credential security. Consider only using the file as a temporary fallback when the keyring is unavailable, or encrypting the backup key using a platform-specific secure storage API.

…tom key loss

On macOS, keyring set_password() can return Ok without persisting.
This adds get_password() verification after every set_password() and
always saves the encryption key to file as a backup, so the key
survives between runs even when the OS keyring silently fails.
@mikeciesla mikeciesla force-pushed the fix/keyring-roundtrip-verify branch from f0be47f to abbf85a Compare March 10, 2026 12:13
@github-actions
Copy link
Contributor

/gemini review

- Stop deleting file backup in migration path (consistent with new philosophy)
- Switch save_key_file to atomic_write (tmp + rename) for crash safety
- Set 0600 permissions on key file after atomic write
- Add doc comment explaining the security trade-off
- Add tests for save_key_file: round-trip, permissions, overwrite, nested dirs
@github-actions
Copy link
Contributor

/gemini review

@jpoehnelt
Copy link
Member

Superseded by #359 which takes a different approach: instead of verifying the keyring roundtrip, it always preserves .encryption_key as a durable fallback and adds GOOGLE_WORKSPACE_CLI_KEYRING_BACKEND=file for environments without a keyring. Thank you for the contribution! See #359

@jpoehnelt jpoehnelt closed this Mar 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants