fix(auth): verify keyring round-trip to prevent phantom key loss#370
fix(auth): verify keyring round-trip to prevent phantom key loss#370mikeciesla wants to merge 2 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 804a61b 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 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
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 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)
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)
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)
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.
f0be47f to
abbf85a
Compare
|
/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
|
/gemini review |
Summary
keyring::Entry::set_password(), immediately callget_password()and compare — only trust the keyring if the round-trip succeeds~/.config/gws/.encryption_keyas a file backup, even when the keyring appears to worksave_key_fileto atomic write (tmp + rename) for crash safetyProblem
On macOS,
keyring::Entry::set_password()can returnOk(())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:Test plan
cargo test— 509 passedcargo clippy -- -D warnings— cleangws auth logingenerates key and saves to filegws drive files listdecrypts successfully across process restarts