fix(credential_store): preserve .encryption_key file for keyring-less environments#357
fix(credential_store): preserve .encryption_key file for keyring-less environments#357
Conversation
… environments The fix for #344 introduced automatic deletion of .encryption_key when the OS keyring succeeds. In Docker/ephemeral-keyring environments, this permanently loses the encryption key on container restart. Changes: - Never delete .encryption_key — it persists as a safe fallback - Always save new keys to file, even when keyring set_password succeeds - Extract resolve_key() with KeyringProvider trait for testability - Add 13 new unit tests covering all keyring/file scenarios Fixes #344 (regression)
🦋 Changeset detectedLatest commit: 5e6070a 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 addresses a critical regression that caused encryption keys to be permanently lost in environments where OS keyrings are not persistent, such as Docker containers. The changes ensure that the 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
|
|
Superseded — switching to a gogcli-style backend selection pattern with env var control instead of removing the keyring entirely. |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses a regression where the encryption key could be lost in ephemeral environments like Docker. The refactoring to introduce a KeyringProvider trait and add comprehensive unit tests is excellent. I've found one potential race condition during initial key generation that could lead to data loss, and have provided a suggestion to resolve it.
| let key = generate_random_key(); | ||
| let b64_key = STANDARD.encode(key); | ||
| save_key_file(key_file, &b64_key)?; | ||
| Ok(key) |
There was a problem hiding this comment.
This key generation logic has a race condition when multiple processes run for the first time simultaneously, which can lead to unrecoverable encrypted data.
For example:
- Process A generates
key_A. - Process B generates
key_B. - Process A writes
key_Ato.encryption_key. - Process B writes
key_Bto.encryption_key, overwritingkey_A.
If Process A has already used key_A to encrypt data, that data is now lost.
To fix this, you can generate a key, write it, and then immediately re-read the file. This ensures all processes converge on using the single key that 'won' the race and was persisted to disk.
Note: This fix assumes that save_key_file performs an atomic write (e.g., using fs_util::atomic_write) to prevent reading a partially written file. The current implementation of save_key_file is not atomic and should also be updated to prevent data corruption on crashes.
| let key = generate_random_key(); | |
| let b64_key = STANDARD.encode(key); | |
| save_key_file(key_file, &b64_key)?; | |
| Ok(key) | |
| let key_candidate = generate_random_key(); | |
| let b64_key = STANDARD.encode(key_candidate); | |
| save_key_file(key_file, &b64_key)?; | |
| // Re-read the key from disk to safely handle concurrent initializations. | |
| // This ensures all processes converge on the key that won the race. | |
| read_key_file(key_file) | |
| .ok_or_else(|| anyhow::anyhow!("Failed to read key file after writing.")) |
Summary
The fix for #344 introduced automatic deletion of
.encryption_keywhen the OS keyring succeeds. In Docker/ephemeral-keyring environments, this permanently loses the encryption key on container restart, makingcredentials.encundecryptable.Changes
.encryption_key— the file persists as a safe fallback. The keyring remains authoritative when available.set_passwordsucceeds. This ensures the key survives keyring loss (e.g. Docker container restart).resolve_key()with aKeyringProvidertrait for testability.Root Cause
The
NoEntry+ existing file path (line 113-114) migrated the file key into the keyring, then deleted the file. In Docker,set_password()can succeed within a session but the keyring doesn't persist across container restarts. On next start, both the keyring and the file are gone → key lost forever.Test Results
Fixes #344 (regression)