Skip to content

Cache secure-store public key in identity files.#2533

Open
fnando wants to merge 2 commits intomainfrom
secure-store-cache-public-key
Open

Cache secure-store public key in identity files.#2533
fnando wants to merge 2 commits intomainfrom
secure-store-cache-public-key

Conversation

@fnando
Copy link
Copy Markdown
Member

@fnando fnando commented Apr 30, 2026

What

  • Add optional public_key and hd_path fields to the SecureStore variant of Secret, serialized into the identity TOML so address/hint lookups don't need to unlock the OS keychain.
  • Lazily migrate legacy identity files on first read: derive the public key once via the keychain and persist it back to disk. The migration is best-effort — if the write-back fails the current call still succeeds, and the next invocation will retry.
  • Add read_key_with_secure_store_cache and get_secret_key_with_hd_path on locator::Args so callers in message sign, sign_with, and address resolution can pass through the requested hd_path and use the cached public key when it matches.
  • Update Secret::public_key and Secret::signer to return the cached public key when present (and the requested hd_path matches the cached one), so SecureStoreEntry can compute the signature hint without re-opening the keychain.

Why

On macOS (and other keyring backends), every secure-store operation prompts the user to unlock the keychain. Today even read-only flows trigger prompts:

  • stellar keys address <name> prompts, even though the public key never changes.
  • stellar tx sign prompts to compute the signature hint before prompting again to actually sign.
  • Any command that resolves a MuxedAccount from a secure-store alias prompts.

Caching the public key on disk eliminates prompts for read-only address/hint queries entirely.

Close #2446

Known limitations

  • Legacy identity files written before this change are migrated on first read (one keychain prompt, then persisted). Users on read-only filesystems will keep seeing the prompt on every invocation.
  • The on-disk cache is keyed by hd_path. A read at a different hd_path than the cached one falls through to the keychain (one prompt) but does not overwrite the cached entry. This keeps the common single-path case fast without invalidating data on multi-path users. To also cache hd_path, add a separate entry with stellar keys add name --hd-path=PATH.
  • The keyring crate's mock builder isolates each Entry to its own in-memory credential, so the legacy-file → derive → write-back round trip is covered by the secure_store_key_management soroban-test integration test rather than a pure unit test.

@fnando fnando self-assigned this Apr 30, 2026
Copilot AI review requested due to automatic review settings April 30, 2026 00:36
@fnando fnando added this to DevX Apr 30, 2026
@github-project-automation github-project-automation Bot moved this to Backlog (Not Ready) in DevX Apr 30, 2026
@fnando fnando moved this from Backlog (Not Ready) to Needs Review in DevX Apr 30, 2026
@fnando fnando force-pushed the secure-store-cache-public-key branch from 70bf705 to 5a9a6d0 Compare April 30, 2026 00:37
@fnando fnando force-pushed the secure-store-cache-public-key branch from 5a9a6d0 to bf77937 Compare April 30, 2026 00:41
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR reduces OS keychain prompts for secure-store identities by caching the derived public key (and associated hd_path) in the on-disk identity TOML, and introducing best-effort lazy migration for legacy identity files.

Changes:

  • Extend Secret::SecureStore to optionally persist public_key and hd_path in identity TOML, and use the cache for public-key / signature-hint lookups when it matches.
  • Populate the cache when creating secure-store identities (keys generate --secure-store, keys add --secure-store) and add best-effort lazy migration on first read for legacy identity files.
  • Thread the requested hd_path through signing and address-resolution paths to maximize cache hits; add unit + integration coverage for caching behavior.

Reviewed changes

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

Show a summary per file
File Description
cmd/soroban-cli/src/signer/secure_store.rs Change secure-store save to return a Secret::SecureStore containing cached public key + hd_path.
cmd/soroban-cli/src/signer/mod.rs Allow SecureStoreEntry to use an optional cached public key for hint derivation.
cmd/soroban-cli/src/config/sign_with.rs Use new locator method that passes through hd_path and triggers cache migration when needed.
cmd/soroban-cli/src/config/secret.rs Add cached fields to Secret::SecureStore; use cache in public_key() and signer(); add unit tests.
cmd/soroban-cli/src/config/locator.rs Add read_key_with_secure_store_cache and get_secret_key_with_hd_path (lazy migration + persistence).
cmd/soroban-cli/src/config/address.rs Route muxed-account resolution through the cache-aware locator read.
cmd/soroban-cli/src/commands/message/sign.rs Use cache-aware secret lookup for message signing flow.
cmd/soroban-cli/src/commands/keys/generate.rs Populate cache when generating secure-store keys; add test asserting cache persistence.
cmd/soroban-cli/src/commands/keys/add.rs Update secure-store save call to new signature returning Secret.
cmd/crates/soroban-test/tests/it/integration/secure_store.rs Integration assertion that identity TOML contains cached public_key.

Comment thread cmd/soroban-cli/src/config/secret.rs Outdated
Comment thread cmd/soroban-cli/src/config/secret.rs Outdated
Comment thread cmd/soroban-cli/src/config/secret.rs Outdated
Comment thread cmd/soroban-cli/src/config/secret.rs Outdated
Comment thread cmd/soroban-cli/src/signer/secure_store.rs Outdated
Copy link
Copy Markdown
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

I can't tell, but does the cached public key get updated every time the secret key is read? I don't think so, but I could be wrong. It would be good for that to happen if not, so that the public key cache can't be poisoned through some nefarious actor and then never updated.

print,
&self.name,
&seed_phrase,
None,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Intriguing to me that the add command does not support the --hd-path flag when the generate command does. It doesn't look like that's changing as a part of this PR so it's out of scope to change here, but noticing it here now because this parameter is set to None.

print,
&self.name,
&seed_phrase,
self.hd_path,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The add command doesn't pass an HD path. Is it a bug that the generate command does?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

Store public key separately from secret-store

3 participants