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

Author: fnando

cmd/crates/soroban-test/tests/it/integration/secure_store.rs

@@ -35,6 +35,21 @@ async fn secure_store_key_management() {
         .stdout_as_str();
     assert!(secure_store_address.starts_with('G'));
 
+    // validate that the public key is cached on disk (so `keys address` and
+    // tx-signing hint derivation can skip the keychain on subsequent calls).
+    let identity_path = sandbox
+        .config_dir()
+        .join("identity")
+        .join(format!("{secure_key_name}.toml"));
+    let identity_toml = std::fs::read_to_string(&identity_path).unwrap_or_else(|err| {
+        panic!("expected identity file at {identity_path:?}: {err}");
+    });
+    assert!(
+        identity_toml.contains(&format!("public_key = \"{secure_store_address}\"")),
+        "expected public_key to be cached on disk after `keys generate --secure-store`, \
+         but identity file was:\n{identity_toml}"
+    );
+
     // use the secure store key to fund a new account
     let new_key_name = "new";
     sandbox

cmd/soroban-cli/src/commands/keys/add.rs

@@ -109,9 +109,13 @@ impl Cmd {
 
             let seed_phrase: SeedPhrase = secret_key.parse()?;
 
-            let secret =
-                secure_store::save_secret(print, &self.name, &seed_phrase, self.overwrite)?;
-            Ok(secret.parse()?)
+            Ok(secure_store::save_secret(
+                print,
+                &self.name,
+                &seed_phrase,
+                None,
+                self.overwrite,
+            )?)
         } else {
             let prompt = "Type a secret key or 12/24 word seed phrase:";
             let secret_key = read_password(print, prompt)?;

cmd/soroban-cli/src/commands/keys/generate.rs

@@ -116,9 +116,13 @@ impl Cmd {
     fn secret(&self, print: &Print) -> Result<Secret, Error> {
         let seed_phrase = self.seed_phrase()?;
         if self.secure_store {
-            let secret =
-                secure_store::save_secret(print, &self.name, &seed_phrase, self.overwrite)?;
-            Ok(secret.parse()?)
+            Ok(secure_store::save_secret(
+                print,
+                &self.name,
+                &seed_phrase,
+                self.hd_path,
+                self.overwrite,
+            )?)
         } else if self.as_secret {
             let secret: Secret = seed_phrase.into();
             Ok(secret.private_key(self.hd_path)?.into())
@@ -202,6 +206,34 @@ mod tests {
         assert!(matches!(identity, Key::Secret(Secret::SecureStore { .. })));
     }
 
+    #[cfg(feature = "additional-libs")]
+    #[tokio::test]
+    async fn test_generate_secure_store_caches_public_key_on_disk() {
+        use keyring::{mock, set_default_credential_builder};
+        set_default_credential_builder(mock::default_credential_builder());
+        let (test_locator, mut cmd) = set_up_test();
+        cmd.secure_store = true;
+        let global_args = global_args();
+
+        cmd.run(&global_args).await.unwrap();
+
+        let identity = test_locator.read_identity("test_name").unwrap();
+        match identity {
+            Key::Secret(Secret::SecureStore {
+                public_key,
+                hd_path,
+                ..
+            }) => {
+                assert!(
+                    public_key.is_some(),
+                    "public_key should be cached on disk after `keys generate --secure-store`"
+                );
+                assert_eq!(hd_path, None);
+            }
+            other => panic!("expected SecureStore variant, got {other:?}"),
+        }
+    }
+
     #[cfg(not(feature = "additional-libs"))]
     #[tokio::test]
     async fn test_storing_in_secure_store_returns_error_when_additional_libs_not_enabled() {

cmd/soroban-cli/src/commands/message/sign.rs

@@ -77,7 +77,9 @@ impl Cmd {
 
         // Get the signer
         let key_or_name = &self.sign_with_key;
-        let secret = self.locator.get_secret_key(key_or_name)?;
+        let secret = self
+            .locator
+            .get_secret_key_with_hd_path(key_or_name, self.hd_path)?;
         let signer = secret.signer(self.hd_path, print.clone()).await?;
         let public_key = signer.get_public_key()?;
 

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

@@ -116,9 +116,9 @@ impl UnresolvedMuxedAccount {
     ) -> Result<xdr::MuxedAccount, Error> {
         match self {
             UnresolvedMuxedAccount::Resolved(muxed_account) => Ok(muxed_account.clone()),
-            UnresolvedMuxedAccount::AliasOrSecret(alias_or_secret) => {
-                Ok(locator.read_key(alias_or_secret)?.muxed_account(hd_path)?)
-            }
+            UnresolvedMuxedAccount::AliasOrSecret(alias_or_secret) => Ok(locator
+                .read_key_with_secure_store_cache(alias_or_secret, hd_path)?
+                .muxed_account(hd_path)?),
             UnresolvedMuxedAccount::Ledger(_) => Err(Error::LedgerNotSupported),
         }
     }

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

@@ -299,6 +299,38 @@ impl Args {
             .or_else(|_| self.read_identity(key_or_name))
     }
 
+    /// Like [`Args::read_key`], but for a `SecureStore` identity loaded from disk
+    /// that lacks a cached public key, derive one via the keychain (one prompt)
+    /// and persist it back so subsequent reads avoid the keychain.
+    pub fn read_key_with_secure_store_cache(
+        &self,
+        key_or_name: &str,
+        hd_path: Option<usize>,
+    ) -> Result<Key, Error> {
+        if let Ok(literal) = key_or_name.parse::<Key>() {
+            return Ok(literal);
+        }
+        let key = self.read_identity(key_or_name)?;
+        if let Key::Secret(Secret::SecureStore {
+            entry_name,
+            public_key: None,
+            ..
+        }) = &key
+        {
+            let pk = secure_store::get_public_key(entry_name, hd_path)?;
+            let migrated = Key::Secret(Secret::SecureStore {
+                entry_name: entry_name.clone(),
+                public_key: Some(pk.to_string()),
+                hd_path,
+            });
+            // Best-effort write-back: if persistence fails we still return the
+            // freshly-derived value so the current call succeeds.
+            let _ = self.write_key(key_or_name, &migrated);
+            return Ok(migrated);
+        }
+        Ok(key)
+    }
+
     pub fn get_secret_key(&self, key_or_name: &str) -> Result<Secret, Error> {
         let key = self.read_key(key_or_name).map_err(|e| match e {
             Error::InvalidName(_) | Error::ConfigMissing(_, _) => Error::InvalidSigningKey,
@@ -310,6 +342,27 @@ impl Args {
         }
     }
 
+    /// Like [`Args::get_secret_key`], but if the secret is a `SecureStore`
+    /// identity loaded from disk without a cached public key, derive it for the
+    /// given `hd_path` and persist the cache. Use from signing paths so the
+    /// returned `Secret` already carries the data signing needs for the hint.
+    pub fn get_secret_key_with_hd_path(
+        &self,
+        key_or_name: &str,
+        hd_path: Option<usize>,
+    ) -> Result<Secret, Error> {
+        let key = self
+            .read_key_with_secure_store_cache(key_or_name, hd_path)
+            .map_err(|e| match e {
+                Error::InvalidName(_) | Error::ConfigMissing(_, _) => Error::InvalidSigningKey,
+                other => other,
+            })?;
+        match key {
+            Key::Secret(s) => Ok(s),
+            _ => Err(Error::InvalidSigningKey),
+        }
+    }
+
     pub fn get_public_key(
         &self,
         key_or_name: &str,
@@ -334,7 +387,7 @@ impl Args {
         let print = Print::new(global_args.quiet);
         let identity = self.read_identity(name)?;
 
-        if let Key::Secret(Secret::SecureStore { entry_name }) = identity {
+        if let Key::Secret(Secret::SecureStore { entry_name, .. }) = identity {
             secure_store::delete_secret(&print, &entry_name)?;
         }
 
@@ -1209,4 +1262,79 @@ mod tests {
             print_deprecation_warning(&local_dir);
         });
     }
+
+    mod secure_store_cache {
+        use super::super::*;
+
+        const TEST_PUBLIC_KEY: &str = "GAREAZZQWHOCBJS236KIE3AWYBVFLSBK7E5UW3ICI3TCRWQKT5LNLCEZ";
+        const TEST_SECRET_KEY: &str = "SBF5HLRREHMS36XZNTUSKZ6FTXDZGNXOHF4EXKUL5UCWZLPBX3NGJ4BH";
+
+        fn locator_with_tempdir() -> (tempfile::TempDir, Args) {
+            let dir = tempfile::tempdir().unwrap();
+            let args = Args {
+                config_dir: Some(dir.path().to_path_buf()),
+            };
+            (dir, args)
+        }
+
+        // The legacy-file -> derive-via-keychain -> write-back path is
+        // exercised end-to-end by the soroban-test integration test
+        // `secure_store_key_management`. The keyring crate's mock builder
+        // assigns each `Entry` instance its own in-memory credential
+        // (CredentialPersistence::EntryOnly), which makes the read-after-write
+        // round trip impossible to simulate in pure unit tests.
+
+        #[test]
+        fn passes_through_already_cached_identity_without_keychain_access() {
+            let (_dir, locator) = locator_with_tempdir();
+
+            // Entry name points to a non-existent keychain entry, so any
+            // keychain access would fail the test.
+            let already = Secret::SecureStore {
+                entry_name: "secure_store:org.stellar.cli-no-such-entry".to_string(),
+                public_key: Some(TEST_PUBLIC_KEY.to_string()),
+                hd_path: None,
+            };
+            locator.write_identity("already", &already).unwrap();
+
+            let key = locator
+                .read_key_with_secure_store_cache("already", None)
+                .unwrap();
+            match key {
+                Key::Secret(Secret::SecureStore {
+                    public_key: Some(pk),
+                    ..
+                }) => assert_eq!(pk, TEST_PUBLIC_KEY),
+                other => panic!("expected SecureStore, got {other:?}"),
+            }
+        }
+
+        #[test]
+        fn passes_through_non_secure_store_identity() {
+            let (_dir, locator) = locator_with_tempdir();
+
+            let secret = Secret::SecretKey {
+                secret_key: TEST_SECRET_KEY.to_string(),
+            };
+            locator.write_identity("plain", &secret).unwrap();
+
+            let key = locator
+                .read_key_with_secure_store_cache("plain", None)
+                .unwrap();
+            assert!(matches!(
+                key,
+                Key::Secret(Secret::SecretKey { ref secret_key }) if secret_key == TEST_SECRET_KEY
+            ));
+        }
+
+        #[test]
+        fn returns_literal_public_key_without_disk_lookup() {
+            let (_dir, locator) = locator_with_tempdir();
+
+            let key = locator
+                .read_key_with_secure_store_cache(TEST_PUBLIC_KEY, None)
+                .unwrap();
+            assert!(matches!(key, Key::PublicKey(_)));
+        }
+    }
 }