Fix/secure store prompting #2313

elizabethengelman · 2025-12-03T16:17:00Z

What

Caches seedphrase on the secure store entry, to prevent multiple prompts from the OS for permission to use the key from keychain.

This required plumbing the caching the key that is retrieved in config::locator layer so that the cached seedphrase could be reused for each step of the command.

Why

We were seeing multiple prompts each time a secure store key was used. For example, for the command, we were being prompted 6 times from the keychain to approve the command's actions.

cargo run contract invoke --source my-secure-store-key --network local --id hello-world -- inc

Known limitations

On my mac, each time I access my keychain it prompts for permission twice - once for access to the keychain, and once for access to the specific key. I am not sure if it is possible to get this down to just one prompt.

when getting the Secret's public key and signer

deny.toml

cmd/soroban-cli/src/signer/mod.rs

cmd/soroban-cli/src/config/locator.rs

cmd/soroban-cli/src/config/secret.rs

cmd/soroban-cli/src/signer/keyring.rs

cmd/soroban-cli/src/signer/secure_store.rs

cmd/soroban-cli/src/signer/secure_store_entry.rs

cmd/soroban-cli/src/signer/keyring.rs

…2324) ### What Upgrade stellar-strkey from 0.0.13 to 0.0.15, enable the cli feature, and add a new `strkey` subcommand to the CLI. ### Why Expose strkey encoding and decoding functionality directly in the CLI, matching the existing pattern used for XDR. For advanced use cases it is helpful to be able to extract components out of addresses. The use cases this is most relevant is cross-chain protocols who prefer to work with the raw payloads within the addresses.

mootz12

Looks good overall. Few minor comments.

Couple overall thoughts:

We might want to consider just storing the pubkey for all accessed keys (on CachedKey or something)? This would be a pretty agnostic way to reduce key interactions.
If 1 is easy to do, we could consider just retaining the StellarEntry cache layer and not storing seed phrase in memory at all. Thus, we would be down to 3 interactions (entry access, pubkey derivation, and signing data).

cc @leighmcculloch @fnando

cmd/soroban-cli/src/signer/keyring.rs

mootz12 · 2025-12-15T20:28:07Z

cmd/soroban-cli/src/signer/keyring.rs


    fn get_seed_phrase(&self) -> Result<SeedPhrase, Error> {
-        Ok(self.keyring.get_password()?.parse()?)
+        let mut guard = self.inner.cached_seed.lock().unwrap();


check for MutexPoison?

fnando · 2025-12-15T20:59:02Z

cmd/soroban-cli/src/signer/secure_store_entry.rs

+        Err(Error::FeatureNotEnabled)
+    }
+
+    pub fn sign_tx_data(_data: &[u8]) -> Result<Vec<u8>, Error> {


Suggested change

pub fn sign_tx_data(_data: &[u8]) -> Result<Vec<u8>, Error> {

pub fn sign_tx_data(&self, _data: &[u8]) -> Result<Vec<u8>, Error> {

Copilot

Pull request overview

This PR addresses issue #2291 by implementing caching for secure store keys to reduce the number of OS keychain prompts from 6 to a minimum during command execution. The implementation adds two levels of caching: caching the seed phrase within StellarEntry to avoid repeated keychain reads, and caching the SecureStoreEntry itself within the Secret enum and locator::Args to reuse the same entry object across multiple operations.

Key Changes:

Refactored secure store functionality from function-based (secure_store.rs) to object-oriented (secure_store_entry.rs) to enable state caching
Added seed phrase caching in StellarEntry using Mutex<Option<SeedPhrase>>
Added SecureStoreEntry caching in Secret enum using Arc<OnceLock<SecureStoreEntry>>
Added key caching in locator::Args to prevent multiple filesystem reads

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
cmd/soroban-cli/src/signer/secure_store_entry.rs	New file containing object-oriented SecureStoreEntry with Arc-wrapped StellarEntry for caching
cmd/soroban-cli/src/signer/secure_store.rs	Removed - replaced by secure_store_entry.rs
cmd/soroban-cli/src/signer/mod.rs	Updated to import secure_store_entry module; moved SecureStoreEntry struct to dedicated file
cmd/soroban-cli/src/signer/keyring.rs	Added seed phrase caching with Mutex; wrapped inner fields in Arc for shared ownership
cmd/soroban-cli/src/config/secret.rs	Added cached_entry field to Secret::SecureStore; added caching helper method; fixed typo in comment
cmd/soroban-cli/src/config/locator.rs	Added cached_keys HashMap to prevent repeated filesystem reads for same key
cmd/soroban-cli/src/config/key.rs	Removed PartialEq/Eq; added Clone; added custom equality assertion for tests
cmd/soroban-cli/src/commands/keys/generate.rs	Updated to use SecureStoreEntry::create_and_save; added cached_keys initialization in tests
cmd/soroban-cli/src/commands/keys/add.rs	Updated to use SecureStoreEntry::create_and_save
cmd/crates/soroban-test/tests/it/integration/hello_world.rs	Added cached_keys initialization for test config
cmd/crates/soroban-test/src/lib.rs	Added cached_keys initialization for TestEnv

Copilot · 2025-12-15T21:02:39Z

cmd/soroban-cli/src/signer/keyring.rs


    fn get_seed_phrase(&self) -> Result<SeedPhrase, Error> {
-        Ok(self.keyring.get_password()?.parse()?)
+        let mut guard = self.inner.cached_seed.lock().unwrap();


Inconsistent error handling: Line 96 uses unwrap() which will panic on mutex poisoning, while lines 80-81 use map_err to properly handle the poisoned mutex case. For consistency and better error handling, change this to use map_err(|_| Error::MutexPoison)? like the delete_seed_phrase method does.

Suggested change

let mut guard = self.inner.cached_seed.lock().unwrap();

let mut guard = self.inner.cached_seed.lock().map_err(|_| Error::MutexPoison)?;

Copilot · 2025-12-15T21:02:39Z

cmd/soroban-cli/src/config/locator.rs

-        key_or_name
+        // check cache for key & return it if its there
+        if let Some(arc) = self.cached_keys.get() {
+            let map = arc.lock().unwrap();


Potential panic from unwrap() call on mutex lock. If the mutex is poisoned, this will panic instead of returning a proper error. Consider using map_err to convert the poison error into a proper Error type, similar to how it's handled in keyring.rs line 80-81.

Copilot · 2025-12-15T21:02:40Z

cmd/soroban-cli/src/config/locator.rs

+        let arc = self
+            .cached_keys
+            .get_or_init(|| Arc::new(Mutex::new(HashMap::new())));
+        let mut map = arc.lock().unwrap();


Potential panic from unwrap() call on mutex lock. If the mutex is poisoned, this will panic instead of returning a proper error. Consider using map_err to convert the poison error into a proper Error type, similar to how it's handled in keyring.rs line 80-81.

Suggested change

let mut map = arc.lock().unwrap();

let mut map = arc

.lock()

.map_err(|e| Error::ConfigIo(format!("Failed to lock cached_keys mutex: {e}")))?;

Copilot · 2025-12-15T21:02:40Z

cmd/soroban-cli/src/config/key.rs

+                ) => {
+                    assert_eq!(e_seed_phrase, a_seed_phrase);
+                }
+                (Secret::Ledger, Secret::Ledger) => todo!(),


Placeholder todo!() in test code will panic if Ledger equality check is ever executed. Either implement the equality check for Ledger keys or add a comment explaining why this case doesn't need to be handled in current tests.

Suggested change

(Secret::Ledger, Secret::Ledger) => todo!(),

// The Secret::Ledger variant is not constructed or compared in current tests,

Copilot · 2025-12-15T21:02:40Z

cmd/soroban-cli/src/config/locator.rs

    pub config_dir: Option<PathBuf>,
+
+    #[clap(skip)]
+    // This saves us from reading the same key from the file system more than once for one cmd


Missing documentation for the cached_keys field. This field serves an important purpose of preventing multiple filesystem reads for the same key during a command execution. Consider adding a doc comment explaining its purpose and usage pattern.

Suggested change

// This saves us from reading the same key from the file system more than once for one cmd

/// Caches loaded keys to avoid reading the same key from the filesystem multiple times during a single command execution.

/// This field is initialized once per command and shared via an `Arc<Mutex<...>>` to ensure thread-safe access.

/// It improves performance by preventing redundant disk I/O when the same key is requested multiple times.

Copilot · 2025-12-15T21:02:41Z

cmd/soroban-cli/src/config/secret.rs

+    SecureStore {
+        entry_name: String,
+        #[serde(skip)]
+        #[serde(default)]


Missing documentation for the cached_entry field. This field is crucial for the PR's purpose of reducing OS keychain prompts by caching the SecureStoreEntry. Add a doc comment explaining that it caches the SecureStoreEntry to prevent multiple OS prompts for keychain access.

Suggested change

#[serde(default)]

#[serde(default)]

/// Caches the SecureStoreEntry to prevent multiple OS prompts for keychain access.

Copilot · 2025-12-15T21:02:41Z

cmd/soroban-cli/src/signer/secure_store_entry.rs

+        Err(Error::FeatureNotEnabled)
+    }
+
+    pub fn sign_tx_data(_data: &[u8]) -> Result<Vec<u8>, Error> {


Missing &self parameter in method signature. The method signature should be pub fn sign_tx_data(&self, _data: &[u8]) to match the feature-enabled version and allow calling it on an instance of SecureStoreEntry.

Suggested change

pub fn sign_tx_data(_data: &[u8]) -> Result<Vec<u8>, Error> {

pub fn sign_tx_data(&self, _data: &[u8]) -> Result<Vec<u8>, Error> {

Copilot · 2025-12-15T21:02:42Z

cmd/soroban-cli/src/signer/keyring.rs

    fn set_seed_phrase(&self, seed_phrase: SeedPhrase) -> Result<(), Error> {
        let mut data = seed_phrase.seed_phrase.into_phrase();

-        self.keyring.set_password(&data)?;
+        self.inner.keyring.set_password(&data)?;
        data.zeroize();
        Ok(())
    }


Cache not populated after writing new seed phrase. When a new seed phrase is written via set_seed_phrase, the cached_seed is not updated to contain the new value. This means the next read will still trigger a keyring access. Consider caching the seed phrase after successful write to reduce OS prompts.

Copilot · 2025-12-15T21:02:42Z

cmd/soroban-cli/src/config/secret.rs

+    fn cached_secure_store_entry(
+        hd_path: Option<usize>,
+        entry_name: &str,
+        cached_entry: &Arc<OnceLock<SecureStoreEntry>>,
+    ) -> Result<SecureStoreEntry, Error> {
+        let entry = if let Some(e) = cached_entry.get() {
+            e.clone()
+        } else {
+            let e = SecureStoreEntry::new(entry_name.to_owned(), hd_path)?;
+            // It's fine if set fails because another thread initialized it concurrently.
+            let _ = cached_entry.set(e.clone());
+            e
+        };
+        Ok(entry)
+    }


Cache key collision issue: The cached SecureStoreEntry is stored with only the entry_name as the key, but SecureStoreEntry includes an hd_path field. If the same entry_name is used with different hd_path values (e.g., calling public_key with index 0, then with index 1), the cache will incorrectly return the first cached entry with the wrong hd_path, leading to using the wrong key derivation path. The cache should either be keyed by both entry_name and hd_path, or the hd_path should be checked when retrieving from cache.

Copilot · 2025-12-15T21:02:42Z

cmd/soroban-cli/src/config/key.rs

+                    },
+                ) => {
+                    assert_eq!(e_entry_name, a_entry_name);
+                    assert!(Arc::ptr_eq(e_cached_entry, a_cached_entry));


Incorrect pointer comparison logic in test. After deserialization from TOML, a new Secret::SecureStore instance is created with a new Arc (line 99), so Arc::ptr_eq will always return false. This test assertion doesn't correctly verify that deserialization works properly. Consider checking that both cached_entry fields are empty OnceLocks instead, or remove this assertion since the cache is runtime state that shouldn't persist through serialization.

Suggested change

assert!(Arc::ptr_eq(e_cached_entry, a_cached_entry));

// Do not compare cached_entry pointer identity; cache is runtime-only and not serialized.

elizabethengelman · 2025-12-19T15:28:35Z

After talking with @mootz12 & @fnando I think we've decided that we don't want to merge this PR in as is. Prior to this we've done a lot of good work to make sure we aren't storing private keys or seed phrases in memory at all, and it doesn't seem worth changing that to remove some UX annoyance that can be remedied.

For users on mac os, they can choose "Allow all" which would eliminate all of the repetitive prompts from keychain
Or, like @mootz12 mentioned above, we could instead cache the identity's pubkey which would remove at least 1 set of extract keychain prompts.

Also, I'll note that when testing on an ubuntu VM i didn't see this multiple prompting issue, so it's probably not worth risking secure storage of a key/seed phrase for one OS's implementation of a secure storage.

I'll close this PR for now, and perhaps the fix of caching a pubkey can be implemented instead.

elizabethengelman added 4 commits December 2, 2025 17:10

Store StellarEntry on SecureStoryEntry and reuse it

d7b9c18

Create a SecureStoreEntry constructor

6e56981

Cache the seedphrase in StellarEntry

beed676

Cache public key in StellarEntry

019ae15

github-project-automation bot added this to DevX Dec 3, 2025

github-project-automation bot moved this to Backlog (Not Ready) in DevX Dec 3, 2025

elizabethengelman mentioned this pull request Dec 3, 2025

Investigate multiple prompting for secure storage key #2291

Open

elizabethengelman changed the title ~~Fix/secure store promting~~ Fix/secure store prompting Dec 3, 2025

elizabethengelman added 19 commits December 4, 2025 13:24

Pull SecureStoreEntry into its own mod

bf61b15

And remove the need for separate secure_store mod

ff5bfb8

Cargo fmt

3cb4abf

Fix typo

426062f

WIP - cache key lookup in locator

5815575

Use get_or_init the cached stellar entry

f75b923

when getting the Secret's public key and signer

Cleanup

0af0a3f

Remove unused name field on SecureStoreEntry

a7934f8

Remove unnecessary cached public key in StellarEntry

af9d430

Cleanup

df86683

Cleanup unwraps

c174c24

Fix to compile without additional-libs feature

a36c403

Merge branch 'main' into fix/secure-store-promting

968e5db

Fixup after merge

fb770cf

Clippy

4c30730

Update key test to account for not implementing PartialEq

9a38824

Fmt

db79631

Clippy for ubuntu builds

379d408

Clear the cached seed when a key is deleted

1b928c6

elizabethengelman marked this pull request as ready for review December 10, 2025 19:40

elizabethengelman marked this pull request as draft December 10, 2025 19:40

Fix integration test

569432b