additional-libs feature for keyring and stellar-ledger dependencies (#1977)

elizabethengelman · web-flow · commit 14de5393343a · 2025-04-22T07:55:47.000-07:00
* Add an `additional-libs` feature for keyring and stellar-ledger crates

* Return an error if a user tries to use a ledger or secure store without additional-libs
diff --git a/.github/workflows/binaries.yml b/.github/workflows/binaries.yml
@@ -56,7 +56,7 @@ jobs:
 
     - name: Build
       run: |
-        cargo build --package ${{ matrix.crate.name }} --features opt --release --target ${{ matrix.sys.target }}
+        cargo build --package ${{ matrix.crate.name }} --features opt,additional-libs --release --target ${{ matrix.sys.target }}
     - name: Build provenance for binary attestation (release only)
       if: github.event_name == 'release'
       uses: actions/attest-build-provenance@v2
diff --git a/.github/workflows/ledger-emulator.yml b/.github/workflows/ledger-emulator.yml
@@ -26,6 +26,6 @@ jobs:
                   sudo apt update && sudo apt install -y libudev-dev libdbus-1-dev
             - run: |
                   cargo test --manifest-path cmd/crates/stellar-ledger/Cargo.toml --features "emulator-tests" -- --nocapture
-            - run: cargo build --features emulator-tests
+            - run: cargo build --features emulator-tests,additional-libs
             - run: |
                   cargo test --features emulator-tests --package soroban-test --test it -- emulator
diff --git a/Makefile b/Makefile
@@ -49,6 +49,7 @@ generate-full-help-doc:
 
 test: build-test
 	cargo test --workspace --exclude soroban-test
+	cargo test --workspace --exclude soroban-test --features additional-libs
 	cargo test -p soroban-test -- --skip integration::
 
 e2e-test:
diff --git a/cmd/soroban-cli/Cargo.toml b/cmd/soroban-cli/Cargo.toml
@@ -40,6 +40,7 @@ version_lt_23 = []
 version_gte_23 = []
 opt = ["dep:wasm-opt"]
 emulator-tests = ["stellar-ledger/emulator-tests"]
+additional-libs = ["dep:keyring", "dep:stellar-ledger"]
 
 [dependencies]
 stellar-xdr = { workspace = true, features = ["cli"] }
@@ -52,7 +53,7 @@ soroban-ledger-snapshot = { workspace = true }
 stellar-strkey = { workspace = true }
 soroban-sdk = { workspace = true }
 soroban-rpc = { workspace = true }
-stellar-ledger = { workspace = true }
+stellar-ledger = { workspace = true, optional = true }
 
 clap = { workspace = true, features = [
     "derive",
@@ -131,7 +132,7 @@ open = "5.3.0"
 url = "2.5.2"
 wasm-gen = "0.1.4"
 zeroize = "1.8.1"
-keyring = { version = "3", features = ["apple-native", "windows-native", "sync-secret-service"] }
+keyring = { version = "3", features = ["apple-native", "windows-native", "sync-secret-service"], optional = true }
 whoami = "1.5.2"
 serde_with = "3.11.0"
 
diff --git a/cmd/soroban-cli/src/commands/keys/add.rs b/cmd/soroban-cli/src/commands/keys/add.rs
@@ -81,7 +81,8 @@ impl Cmd {
 
             let seed_phrase: SeedPhrase = secret_key.parse()?;
 
-            Ok(secure_store::save_secret(print, &self.name, seed_phrase)?)
+            let secret = secure_store::save_secret(print, &self.name, &seed_phrase)?;
+            Ok(secret.parse()?)
         } else {
             let prompt = "Type a secret key or 12/24 word seed phrase:";
             let secret_key = read_password(print, prompt)?;
diff --git a/cmd/soroban-cli/src/commands/keys/generate.rs b/cmd/soroban-cli/src/commands/keys/generate.rs
@@ -127,7 +127,8 @@ impl Cmd {
     fn secret(&self, print: &Print) -> Result<Secret, Error> {
         let seed_phrase = self.seed_phrase()?;
         if self.secure_store {
-            Ok(secure_store::save_secret(print, &self.name, seed_phrase)?)
+            let secret = secure_store::save_secret(print, &self.name, &seed_phrase)?;
+            Ok(secret.parse()?)
         } else if self.as_secret {
             let secret: Secret = seed_phrase.into();
             Ok(secret.private_key(self.hd_path)?.into())
@@ -144,7 +145,6 @@ impl Cmd {
 #[cfg(test)]
 mod tests {
     use crate::config::{address::KeyName, key::Key, secret::Secret};
-    use keyring::{mock, set_default_credential_builder};
 
     fn set_up_test() -> (super::locator::Args, super::Cmd) {
         let temp_dir = tempfile::tempdir().unwrap();
@@ -200,8 +200,10 @@ mod tests {
         assert!(matches!(identity, Key::Secret(Secret::SecretKey { .. })));
     }
 
+    #[cfg(feature = "additional-libs")]
     #[tokio::test]
     async fn test_storing_secret_in_secure_store() {
+        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;
@@ -212,4 +214,26 @@ mod tests {
         let identity = test_locator.read_identity("test_name").unwrap();
         assert!(matches!(identity, Key::Secret(Secret::SecureStore { .. })));
     }
+
+    #[cfg(not(feature = "additional-libs"))]
+    #[tokio::test]
+    async fn test_storing_in_secure_store_returns_error_when_additional_libs_not_enabled() {
+        let (test_locator, mut cmd) = set_up_test();
+        cmd.secure_store = true;
+        let global_args = global_args();
+
+        let result = cmd.run(&global_args).await;
+        assert!(result.is_err());
+        assert_eq!(
+            result.unwrap_err().to_string(),
+            format!("Secure Store keys are not allowed: additional-libs feature must be enabled")
+        );
+
+        let identity_result = test_locator.read_identity("test_name");
+        assert!(identity_result.is_err());
+        assert_eq!(
+            identity_result.unwrap_err().to_string(),
+            format!("Failed to find config identity for test_name")
+        );
+    }
 }
diff --git a/cmd/soroban-cli/src/config/address.rs b/cmd/soroban-cli/src/config/address.rs
@@ -48,6 +48,8 @@ pub enum Error {
     InvalidKeyName(String),
     #[error("Ledger not supported in this context")]
     LedgerNotSupported,
+    #[error(transparent)]
+    Ledger(#[from] signer::ledger::Error),
 }
 
 impl FromStr for UnresolvedMuxedAccount {
@@ -85,7 +87,7 @@ impl UnresolvedMuxedAccount {
     ) -> Result<xdr::MuxedAccount, Error> {
         match self {
             UnresolvedMuxedAccount::Ledger(hd_path) => Ok(xdr::MuxedAccount::Ed25519(
-                ledger(*hd_path).await?.public_key().await?.0.into(),
+                ledger::new(*hd_path).await?.public_key().await?.0.into(),
             )),
             UnresolvedMuxedAccount::Resolved(_) | UnresolvedMuxedAccount::AliasOrSecret(_) => {
                 self.resolve_muxed_account_sync(locator, hd_path)
diff --git a/cmd/soroban-cli/src/config/locator.rs b/cmd/soroban-cli/src/config/locator.rs
@@ -15,7 +15,7 @@ use stellar_strkey::{Contract, DecodeError};
 use crate::{
     commands::{global, HEADING_GLOBAL},
     print::Print,
-    signer::{self, keyring::StellarEntry},
+    signer::secure_store,
     utils::find_config_dir,
     xdr, Pwd,
 };
@@ -95,7 +95,7 @@ pub enum Error {
     #[error("Key cannot {0} cannot overlap with contract alias")]
     KeyCannotOverlapWithContractAlias(String),
     #[error(transparent)]
-    Keyring(#[from] signer::keyring::Error),
+    SecureStore(#[from] secure_store::Error),
     #[error("Only private keys and seed phrases are supported for getting private keys {0}")]
     SecretKeyOnly(String),
     #[error(transparent)]
@@ -301,20 +301,10 @@ impl Args {
         let identity = self.read_identity(name)?;
 
         if let Key::Secret(Secret::SecureStore { entry_name }) = identity {
-            let entry = StellarEntry::new(&entry_name)?;
-            match entry.delete_seed_phrase() {
-                Ok(()) => {}
-                Err(e) => match e {
-                    signer::keyring::Error::Keyring(keyring::Error::NoEntry) => {
-                        print.infoln("This key was already removed from the secure store. Removing the cli config file.");
-                    }
-                    _ => {
-                        return Err(Error::Keyring(e));
-                    }
-                },
-            }
+            secure_store::delete_secret(&print, &entry_name)?;
         }
 
+        print.infoln("Removing the key's cli config file");
         KeyType::Identity.remove(name, &self.config_dir()?)
     }
 
diff --git a/cmd/soroban-cli/src/config/secret.rs b/cmd/soroban-cli/src/config/secret.rs
@@ -7,7 +7,7 @@ use stellar_strkey::ed25519::{PrivateKey, PublicKey};
 
 use crate::{
     print::Print,
-    signer::{self, keyring, ledger, LocalKey, SecureStoreEntry, Signer, SignerKind},
+    signer::{self, ledger, secure_store, LocalKey, SecureStoreEntry, Signer, SignerKind},
     utils,
 };
 
@@ -25,14 +25,14 @@ pub enum Error {
     InvalidSecretOrSeedPhrase,
     #[error(transparent)]
     Signer(#[from] signer::Error),
-
     #[error("Ledger does not reveal secret key")]
     LedgerDoesNotRevealSecretKey,
-
     #[error(transparent)]
-    Keyring(#[from] keyring::Error),
+    SecureStore(#[from] secure_store::Error),
     #[error("Secure Store does not reveal secret key")]
     SecureStoreDoesNotRevealSecretKey,
+    #[error(transparent)]
+    Ledger(#[from] signer::ledger::Error),
 }
 
 #[derive(Debug, clap::Args, Clone)]
@@ -72,7 +72,7 @@ impl FromStr for Secret {
             })
         } else if s == "ledger" {
             Ok(Secret::Ledger)
-        } else if s.starts_with(keyring::SECURE_STORE_ENTRY_PREFIX) {
+        } else if s.starts_with(secure_store::ENTRY_PREFIX) {
             Ok(Secret::SecureStore {
                 entry_name: s.to_string(),
             })
@@ -123,8 +123,7 @@ impl Secret {
 
     pub fn public_key(&self, index: Option<usize>) -> Result<PublicKey, Error> {
         if let Secret::SecureStore { entry_name } = self {
-            let entry = keyring::StellarEntry::new(entry_name)?;
-            Ok(entry.get_public_key(index)?)
+            Ok(secure_store::get_public_key(entry_name, index)?)
         } else {
             let key = self.key_pair(index)?;
             Ok(stellar_strkey::ed25519::PublicKey::from_payload(
@@ -144,7 +143,7 @@ impl Secret {
                     .unwrap_or_default()
                     .try_into()
                     .expect("uszie bigger than u32");
-                SignerKind::Ledger(ledger(hd_path).await?)
+                SignerKind::Ledger(ledger::new(hd_path).await?)
             }
             Secret::SecureStore { entry_name } => SignerKind::SecureStore(SecureStoreEntry {
                 name: entry_name.to_string(),
diff --git a/cmd/soroban-cli/src/config/sign_with.rs b/cmd/soroban-cli/src/config/sign_with.rs
@@ -29,6 +29,8 @@ pub enum Error {
     StrKey(#[from] stellar_strkey::DecodeError),
     #[error(transparent)]
     Xdr(#[from] xdr::Error),
+    #[error(transparent)]
+    Ledger(#[from] signer::ledger::Error),
 }
 
 #[derive(Debug, clap::Args, Clone, Default)]
@@ -72,7 +74,7 @@ impl Args {
                 print,
             }
         } else if self.sign_with_ledger {
-            let ledger = ledger(
+            let ledger = ledger::new(
                 self.hd_path
                     .unwrap_or_default()
                     .try_into()
diff --git a/cmd/soroban-cli/src/signer/keyring.rs b/cmd/soroban-cli/src/signer/keyring.rs
@@ -1,39 +1,71 @@
+use crate::print::Print;
 use ed25519_dalek::Signer;
 use keyring::Entry;
 use sep5::seed_phrase::SeedPhrase;
 use zeroize::Zeroize;
 
-pub(crate) const SECURE_STORE_ENTRY_PREFIX: &str = "secure_store:";
-pub(crate) const SECURE_STORE_ENTRY_SERVICE: &str = "org.stellar.cli";
-
 #[derive(thiserror::Error, Debug)]
 pub enum Error {
+    #[cfg(feature = "additional-libs")]
     #[error(transparent)]
     Keyring(#[from] keyring::Error),
+
     #[error(transparent)]
     Sep5(#[from] sep5::error::Error),
+
+    #[error("Secure Store keys are not allowed: additional-libs feature must be enabled")]
+    FeatureNotEnabled,
 }
 
 pub struct StellarEntry {
+    name: String,
+    #[cfg(feature = "additional-libs")]
     keyring: Entry,
 }
 
 impl StellarEntry {
     pub fn new(name: &str) -> Result<Self, Error> {
         Ok(StellarEntry {
+            name: name.to_string(),
             keyring: Entry::new(name, &whoami::username())?,
         })
     }
 
-    pub fn set_seed_phrase(&self, seed_phrase: SeedPhrase) -> Result<(), Error> {
+    pub fn write(&self, seed_phrase: SeedPhrase, print: &Print) -> Result<(), Error> {
+        if let Ok(key) = self.get_public_key(None) {
+            print.warnln(format!(
+                "A key for {0} already exists in your operating system's secure store: {1}",
+                self.name, key
+            ));
+        } else {
+            print.infoln(format!(
+                "Saving a new key to your operating system's secure store: {0}",
+                self.name
+            ));
+            self.set_seed_phrase(seed_phrase)?;
+        };
+        Ok(())
+    }
+
+    fn set_seed_phrase(&self, seed_phrase: SeedPhrase) -> Result<(), Error> {
         let mut data = seed_phrase.seed_phrase.into_phrase();
+
         self.keyring.set_password(&data)?;
         data.zeroize();
         Ok(())
     }
 
-    pub fn delete_seed_phrase(&self) -> Result<(), Error> {
-        Ok(self.keyring.delete_credential()?)
+    pub fn delete_seed_phrase(&self, print: &Print) -> Result<(), Error> {
+        match self.keyring.delete_credential() {
+            Ok(()) => Ok(()),
+            Err(e) => match e {
+                keyring::Error::NoEntry => {
+                    print.infoln("This key was already removed from the secure store.");
+                    Ok(())
+                }
+                _ => Err(Error::Keyring(e)),
+            },
+        }
     }
 
     fn get_seed_phrase(&self) -> Result<SeedPhrase, Error> {
@@ -86,8 +118,11 @@ impl StellarEntry {
     }
 }
 
+#[cfg(feature = "additional-libs")]
 #[cfg(test)]
 mod test {
+    use crate::print;
+
     use super::*;
     use keyring::{mock, set_default_credential_builder};
 
@@ -165,7 +200,8 @@ mod test {
         assert!(get_seed_phrase_result.is_ok());
 
         // delete the password
-        let delete_seed_phrase_result = entry.delete_seed_phrase();
+        let print = print::Print::new(true);
+        let delete_seed_phrase_result = entry.delete_seed_phrase(&print);
         assert!(delete_seed_phrase_result.is_ok());
 
         // confirm the entry is gone
diff --git a/cmd/soroban-cli/src/signer/ledger.rs b/cmd/soroban-cli/src/signer/ledger.rs
diff --git a/cmd/soroban-cli/src/signer/mod.rs b/cmd/soroban-cli/src/signer/mod.rs
diff --git a/cmd/soroban-cli/src/signer/secure_store.rs b/cmd/soroban-cli/src/signer/secure_store.rs
diff --git a/cmd/stellar-cli/Cargo.toml b/cmd/stellar-cli/Cargo.toml
