Conversation
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves Linux support for the Rust CLI by adding a GNOME Keyring-based keystore backend (Secret Service + polkit), and tightening in-memory handling of keypair bytes via zeroization.
Changes:
- Add a Linux-only GNOME Keyring backend (Secret Service D-Bus API) gated by polkit authorization for keypair reads.
- Change
KeystoreBackend::load_keypairto returnZeroizing<Vec<u8>>and update backends/callers accordingly. - Update docs and release pipeline (adds a Snap publish job; adds a
rust-toolchain.toml).
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| rust/rust-toolchain.toml | Pins Rust toolchain to stable for the Rust workspace. |
| rust/crates/keystore/src/lib.rs | Re-exports Zeroizing and updates load_keypair API to return zeroizing bytes. |
| rust/crates/keystore/src/backends/onepassword.rs | Updates backend to return Zeroizing<Vec<u8>>. |
| rust/crates/keystore/src/backends/apple_keychain.rs | Updates backend to return Zeroizing<Vec<u8>>. |
| rust/crates/keystore/src/backends/mod.rs | Adds Linux-only gnome_keyring module. |
| rust/crates/keystore/src/backends/gnome_keyring.rs | Implements Secret Service + polkit-based Linux keystore backend. |
| rust/crates/keystore/Cargo.toml | Adds zeroize and Linux-only deps for Secret Service + D-Bus + tokio. |
| rust/crates/core/src/signer.rs | Adds gnome-keyring: source support for loading signers. |
| rust/crates/core/src/keystore.rs | Re-exports GnomeKeyring on Linux. |
| rust/crates/core/src/config.rs | Prefers GNOME Keyring default source on Linux when present. |
| rust/crates/core/src/accounts.rs | Adds GnomeKeyring keystore type and source formatting. |
| rust/crates/cli/src/commands/setup.rs | Adds interactive/setup support for GNOME Keyring backend on Linux. |
| rust/crates/cli/src/commands/export.rs | Keeps exported key bytes zeroized while in memory. |
| rust/crates/cli/src/commands/destroy.rs | Adds delete/discovery support for GNOME Keyring backend on Linux. |
| rust/config/polkit/sh.pay.unlock-keypair.policy | Adds required polkit action definition for GNOME Keyring auth gate. |
| rust/Cargo.toml | Adds workspace zeroize dependency. |
| rust/Cargo.lock | Locks new dependencies (secret-service, zbus, etc.). |
| README.md | Documents GNOME Keyring support and the polkit install step. |
| .github/workflows/release-cli.yml | Adds a Snap Store publish job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn connect() -> Result<SecretService<'static>> { | ||
| SecretService::connect(EncryptionType::Plain) | ||
| .await | ||
| .map_err(|e| Error::Backend(format!("Secret Service unavailable: {e}"))) | ||
| } |
There was a problem hiding this comment.
connect() uses EncryptionType::Plain, which disables Secret Service session encryption and can expose key bytes on the D-Bus. Prefer an encrypted session type (or the crate default) so secrets are protected in transit between client and keyring service.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Done — changed EncryptionType::Plain to EncryptionType::Dh in the connect() helper so all Secret Service sessions (not just the is_available() probe) use an encrypted D-Bus channel. Commit: e0b6a0e.
| /// Requires the action file to be installed: | ||
| /// sudo cp rust/config/polkit/sh.pay.unlock-keypair.policy /usr/share/polkit-1/actions/ | ||
| async fn polkit_authenticate(_reason: &str) -> Result<()> { | ||
| use zbus::zvariant::{OwnedValue, Value}; | ||
|
|
||
| let conn = zbus::Connection::system() | ||
| .await | ||
| .map_err(|e| Error::Backend(format!("D-Bus system bus: {e}")))?; | ||
|
|
||
| let pid = std::process::id(); | ||
| let start_time = process_start_time()?; | ||
|
|
||
| // Subject: the current process ("unix-process" with pid + start-time). | ||
| // start-time prevents PID reuse attacks. | ||
| let subject_details: HashMap<String, OwnedValue> = [ | ||
| ("pid".to_owned(), OwnedValue::from(Value::new(pid))), | ||
| ( | ||
| "start-time".to_owned(), | ||
| OwnedValue::from(Value::new(start_time)), | ||
| ), | ||
| ] | ||
| .into(); | ||
|
|
||
| // details a{ss}: must be empty for unprivileged callers — only uid 0 or | ||
| // the action owner may pass custom details to CheckAuthorization. | ||
| let details: HashMap<String, String> = HashMap::new(); |
There was a problem hiding this comment.
polkit_authenticate takes a reason argument but it’s unused, so callers passing a custom reason won’t affect the auth prompt. Either plumb reason into the polkit details (if supported for unprivileged callers) or remove/rename the parameter and adjust upstream docs to avoid implying the reason is displayed.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
… D-Bus session Agent-Logs-Url: https://github.com/solana-foundation/pay/sessions/2397098d-b0fb-4da5-a692-695dce568de7 Co-authored-by: lgalabru <87777+lgalabru@users.noreply.github.com>
No description provided.