Feat/os keychain followup #1770

elizabethengelman · 2024-12-02T16:36:46Z

What

This PR is based on #1703 and adds some additional compatibility with the secure store like keys rm and keys add.

keys rm
keys add
manual test on Mac OS
- add the key: cargo run keys add --secure-store alice (will be prompted for a seed phrase)
- make sure we can get the address from the new key: cargo run keys address alice
- make sure we can sign & send with the new key:
```
    stello contract deploy --wasm target/test-wasms/hello_world.wasm  --build-only --network testnet --source alice | \
    stello tx simulate --network testnet --source alice | \
    stello tx sign --network testnet --sign-with-key alice | \
    stello tx send --network testnet
```
- remove the key: stello keys rm alice
- make sure it is no longer in the list or the keychain
manual test on Linux
manual test on Windows

Why

I wanted to keep #1703 from getting too big so it was easier to review.

Known limitations

[TODO or N/A]

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

willemneal

I still need to try this out locally, but correct me if I'm wrong, but it's possible to have the name used in keys add differ from what is used in entry-name?

elizabethengelman · 2024-12-23T21:50:21Z

@willemneal

... it's possible to have the name used in keys add differ from what is used in entry-name?

Good call! Yeah, at the moment, this is possible. It still ends up working because we are saving the value of --entry-name in the <NAME>.toml file, like this: entry_name = "secure_store:org.stellar.cli-<NAME>. And then getting that entry_name from the toml file when accessing the public key, or using the private key.

Though, this may be confusing. 🤔

Instead, we could remove the --entry-name flag, and when adding a new key, require that the key name is the same as the entry name in the OS's secure store. So, this command would change to be: cargo run keys add alice.

But, now I'm starting to wonder if we shouldn't allow users to add keys from the keychain at all. If we do, we'd need to make sure that users add the keys to their secure store with:

the key name prepended with secure_store:org.stellar.cli-
the key saved as a base64 encoded ed25519_dalek::SigningKey

And we could probably do that, but I'm not sure how necessary this feature is. 🤔

elizabethengelman · 2025-01-08T20:20:32Z

Since we are now planning to store the seed phrase instead of the signing key in the OS secure storage, I think that a more useful feature would be to allow a user to add a new key with their seed phrase, which is saved in secure storage instead of in the local file. So the command could look like this:

cargo run keys add <NAME> --secure-store

which would then prompt the user to enter their seed phrase:

Type a 12/24 word seed phrase:
...

cmd/soroban-cli/src/secure_store.rs

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

- this is the only place it is being used - also, this help with a cyclic dependency i was noticing when trying to use secure_store in secret and generate

elizabethengelman · 2025-01-14T18:52:18Z

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

        let path = self.config_locator.write_identity(&self.name, &secret)?;
        print.checkln(format!("Key saved with alias {:?} in {path:?}", self.name));
        Ok(())
    }
+
+    fn read_secret(&self, print: &Print) -> Result<Secret, Error> {


moved this here from config/secret.rs - i was seeing an interesting cyclical dependency, and moving this here helped that be a bit less confusing.

elizabethengelman · 2025-01-14T18:54:00Z

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

+            let prompt = "Type a 12 or 24 word seed phrase:";
+            let secret_key = read_password(print, prompt)?;
+
+            let seed_phrase: SeedPhrase = secret_key.parse()?;


At the moment I only added support for a user to add a new seed phrase to secure storage. I'm happy to make a change so that this supports adding a private key directly as well, but wanted to get some more eyes on this to see if folks thought it was worth the effort.

I think it's always a mistake to use a private key directly when storing keys. It has no recovery method and almost all wallets to my knowledge use a seed phrase so if users would be importing it wouldn't be a Secret Key.

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

elizabethengelman · 2025-01-14T18:58:18Z

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

@@ -0,0 +1,59 @@
+use sep5::SeedPhrase;


i'm not sure if this needs its own mod, or exactly where to put this, but I wanted to pull the code out where both add and generate could use it

willemneal · 2025-01-30T19:15:39Z

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

+}
+
+fn write_to_secure_store(
+    entry_name: &String,


Suggested change

entry_name: &String,

entry_name: &str,

willemneal · 2025-01-30T21:29:22Z

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

+        if let 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));
+                    }
+                },
+            }
+        }
        KeyType::Identity.remove(name, &self.config_dir()?)


Not sure this will be an issue, but it seems like it could be possible for the keyring to successfully delete the key but not the Identity file? I think we can assume this isn't the case since we read the id first.

willemneal · 2025-01-30T21:33:03Z

🎉

elizabethengelman mentioned this pull request Dec 2, 2024

feat: add support for OS specific keychains #1703

Merged

34 tasks

elizabethengelman force-pushed the feat/os-keychain-followup branch 2 times, most recently from 244508d to a97ccbb Compare December 2, 2024 20:59

elizabethengelman force-pushed the feat/os_keychain branch from 072a3af to e120595 Compare December 2, 2024 22:26

elizabethengelman force-pushed the feat/os-keychain-followup branch 3 times, most recently from df3543a to 3311a50 Compare December 3, 2024 17:18

elizabethengelman force-pushed the feat/os_keychain branch from 6a4b0ff to f263d8d Compare December 3, 2024 17:25

elizabethengelman force-pushed the feat/os-keychain-followup branch 4 times, most recently from c39bf60 to fe02078 Compare December 6, 2024 16:19

elizabethengelman marked this pull request as ready for review December 6, 2024 16:58

willemneal reviewed Dec 6, 2024

View reviewed changes

cmd/soroban-cli/src/config/locator.rs Outdated Show resolved Hide resolved

elizabethengelman force-pushed the feat/os-keychain-followup branch 2 times, most recently from f4e2913 to 3acc3d0 Compare December 13, 2024 14:31

elizabethengelman requested a review from leighmcculloch December 13, 2024 14:31

willemneal reviewed Dec 19, 2024

View reviewed changes

elizabethengelman commented Jan 8, 2025

View reviewed changes

cmd/soroban-cli/src/secure_store.rs Outdated Show resolved Hide resolved

elizabethengelman commented Jan 8, 2025

View reviewed changes

cmd/soroban-cli/src/config/secret.rs Outdated Show resolved Hide resolved

Base automatically changed from feat/os_keychain to main January 9, 2025 20:57

elizabethengelman added 7 commits January 13, 2025 17:04

Remove secure store entry when removing identity

4c743e2

Factor out a secure_store mod for code reuse

0914f06

Allow for adding a seed phrase key and storing in secure store

b1f7bca

Update doc comments

5428c3d

Move read_password to add.rs

16cfacd

- this is the only place it is being used - also, this help with a cyclic dependency i was noticing when trying to use secure_store in secret and generate

Cleanup & refactor

5b763f2

Use printer for printing prompt when reading password from cli

8e234bc

elizabethengelman force-pushed the feat/os-keychain-followup branch from 7b8d00c to 8e234bc Compare January 14, 2025 18:50

Simplify secure_store

b59fc3d

elizabethengelman commented Jan 14, 2025

View reviewed changes

elizabethengelman added 9 commits January 14, 2025 14:02

Apply formatting updates

9601f22

Clippy

c1423c3

Merge branch 'main' into feat/os-keychain-followup

9ff4022

Cleanup after merging main

6280d68

Fix after merging main

a7a4d66

Merge branch 'main' into feat/os-keychain-followup

ba652d0

Fix formatting

e983f6c

Merge branch 'main' into feat/os-keychain-followup

e470926

Fix: print 12 word phrase warning for secure store too

c66f567

sagpatil requested a review from fnando January 23, 2025 23:03

elizabethengelman added 6 commits January 24, 2025 11:20

Formatting

e14ff6d

Merge branch 'main' into feat/os-keychain-followup

e5eab8f

Clippy

a1e4ec4

Update generate docs

b4f4258

Merge branch 'main' into feat/os-keychain-followup

39716d0

Merge branch 'main' into feat/os-keychain-followup

8c11c93

willemneal reviewed Jan 30, 2025

View reviewed changes

willemneal approved these changes Jan 30, 2025

View reviewed changes

Merge branch 'main' into feat/os-keychain-followup

ef1833f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/os keychain followup #1770

Feat/os keychain followup #1770

elizabethengelman commented Dec 2, 2024 •

edited

Loading

willemneal left a comment

elizabethengelman commented Dec 23, 2024

elizabethengelman commented Jan 8, 2025 •

edited

Loading

elizabethengelman Jan 14, 2025

elizabethengelman Jan 14, 2025

willemneal Jan 30, 2025

elizabethengelman Jan 14, 2025

willemneal Jan 30, 2025

willemneal Jan 30, 2025

willemneal commented Jan 30, 2025

Feat/os keychain followup #1770

Are you sure you want to change the base?

Feat/os keychain followup #1770

Conversation

elizabethengelman commented Dec 2, 2024 • edited Loading

What

Why

Known limitations

willemneal left a comment

Choose a reason for hiding this comment

elizabethengelman commented Dec 23, 2024

elizabethengelman commented Jan 8, 2025 • edited Loading

elizabethengelman Jan 14, 2025

Choose a reason for hiding this comment

elizabethengelman Jan 14, 2025

Choose a reason for hiding this comment

willemneal Jan 30, 2025

Choose a reason for hiding this comment

elizabethengelman Jan 14, 2025

Choose a reason for hiding this comment

willemneal Jan 30, 2025

Choose a reason for hiding this comment

willemneal Jan 30, 2025

Choose a reason for hiding this comment

willemneal commented Jan 30, 2025

elizabethengelman commented Dec 2, 2024 •

edited

Loading

elizabethengelman commented Jan 8, 2025 •

edited

Loading