Skip to content

feat: add support for OS specific keychains - #1703

Merged
elizabethengelman merged 55 commits into
mainfrom
feat/os_keychain
Jan 9, 2025
Merged

feat: add support for OS specific keychains#1703
elizabethengelman merged 55 commits into
mainfrom
feat/os_keychain

Conversation

@willemneal

@willemneal willemneal commented Nov 5, 2024 •
edited by elizabethengelman
Loading

Copy link
Copy Markdown
Contributor

Addresses #1481
I updated this branch to point at main instead of feat/add_stellar_ledger so we wouldn't have too many PRs stacked on each other.

This adds support for using secrets stored in OS specific key rings.

Update to system test: stellar-deprecated/system-test#109

  • key generate (no fund)
  • keys generate (fund)
  • keys fund
  • keys address
  • keys show: should not display the secure store's secret and instead return an error: ❌ error: Secure Store does not reveal secret key
  • tx sign
  • keys rm and keys add will be addressed in Feat/os keychain followup #1770
  • manual test on Mac OS
    • cargo run keys generate --secure-store carol --network local --no-fund
    • cargo run keys generate --secure-store dean --network local --fund
    • cargo run keys fund carol --network local
    • cargo run keys address carol
    • cargo run keys show carol
    • tx sign: cargo run contract deploy --wasm target/wasm32-unknown-unknown/test-wasms/test_hello_world.wasm --build-only --network local --source carol | \ cargo run tx sign --network local --sign-with-key carol
    • tx simulate: cargo run contract deploy --wasm target/test-wasms/hello_world.wasm --build-only --network testnet --source fred-testnet | \ cargo run tx simulate --network testnet --source fred-testnet
      this command is not working on a local network or with the test-wasms, it does work on testnet with wasm generated from a new contract
    • tx simulate, sign, send: cargo run contract deploy --wasm target/test-wasms/hello_world.wasm --build-only --network testnet --source fred-testnet | \ cargo run tx simulate --network testnet --source fred-testnet | \ cargo run tx sign --network testnet --sign-with-key fred-testnet | \ cargo run tx send --network testnet
      same as above
  • manual test on Linux (tested with UTM vm)
    • cargo run keys generate --secure-store --network testnet --no-fund alice
    • cargo run keys generate --secure-store --network testnet --fund bob
    • cargo run keys fund --network testnet alice
    • cargo run keys address alice
    • cargo run keys show alice
    • tx sign: cargo run contract deploy --wasm target/wasm32-unknown-unknown/test-wasms/test_hello_world.wasm --build-only --network testnet --source alice | \ cargo run tx sign --network local --sign-with-key alice
    • tx simulate: cargo run contract deploy --wasm target/wasm32-unknown-unknown/test-wasms/test_hello_world.wasm --build-only --network testnet --source alice | \ cargo run tx simulate --network testnet --source alice
    • tx simulate, sign, send
  • manual test on Windows(trying to test on windows via UTM, but have run into some issues with spinning up the windows vm)
    • cargo run keys generate --secure-store --network testnet --no-fund alice
    • cargo run keys generate --secure-store --network testnet --fund bob
    • cargo run keys fund --network testnet alice
    • cargo run keys address alice
    • cargo run keys show alice
    • tx sign: cargo run contract deploy --wasm target/wasm32-unknown-unknown/test-wasms/test_hello_world.wasm --build-only --network testnet --source alice | \ cargo run tx sign --network local --sign-with-key alice
    • tx simulate: cargo run contract deploy --wasm target/wasm32-unknown-unknown/test-wasms/test_hello_world.wasm --build-only --network testnet --source alice | \ cargo run tx simulate --network testnet --source alice
    • tx simulate, sign, send

@willemneal willemneal self-assigned this Nov 5, 2024
@janewang
janewang removed the request for review from elizabethengelman November 5, 2024 21:15
@willemneal
willemneal marked this pull request as draft November 12, 2024 21:42
@elizabethengelman elizabethengelman linked an issue Nov 20, 2024 that may be closed by this pull request
Sign with system keychain #1481
Closed
@willemneal willemneal linked an issue Nov 26, 2024 that may be closed by this pull request
[Epic] Implement more robust signing, which includes ledger, and lab #1591
Closed
5 tasks
@willemneal willemneal mentioned this pull request Nov 26, 2024
Closed
5 tasks
willemneal and others added 19 commits December 2, 2024 16:47
@willemneal @elizabethengelman
feat: initial work into system keychain
8c06970
@willemneal @elizabethengelman
chore: clean up
883cb8c
@elizabethengelman
Add KeyName struct in address
eb70734
@elizabethengelman
Add Secret::Keychain
d4e2500
@elizabethengelman
keys generate: allow for generating keys that are stored in keychain
3ee20b8
@elizabethengelman
keys generate: Namespace keychain entry to identity name
1a24c00
@elizabethengelman
keys generate: don't allow 'keychain:' as a key name
4a0c900
@elizabethengelman
keys address: use keychain entry in secret to get the pub key
755c318
@elizabethengelman
tx sign: allow a keychain identity sign a tx
dd07bf7
@elizabethengelman
Cleanup
f042d19
@elizabethengelman
Use keyring mock for generate tests
9cf5916
@elizabethengelman
Refactor keyring: add keyring entry as StellarEntry field
cd515d3 
- previously we were creating a new keyring entry for each interaction
with the keyring
- this change will allow us use a mock keyring entry for testing
@elizabethengelman
Add tests for keyring
276558d
@elizabethengelman
Update config/secret tests
20c651b
@elizabethengelman
Cleanup
66d1bfc
@elizabethengelman
Rename keychain arg to secure_store in generate
8b9763e
@elizabethengelman
Rename Secret::Keychain to Secret::SecureStore
fdefa2a
@elizabethengelman
Rename SignerKind::Keychain to SignerKind::SecureStore
36559ea
@elizabethengelman
Use print for new fns in generate
f98b709
willemneal
willemneal commented Dec 19, 2024
View reviewed changes
Comment thread cmd/soroban-cli/src/config/address.rs
willemneal
willemneal commented Dec 19, 2024
View reviewed changes
Comment thread cmd/soroban-cli/src/signer/keyring.rs Outdated
@willemneal

willemneal commented Dec 19, 2024

Copy link
Copy Markdown
Contributor Author

@elizabethengelman I can't review my own PR and can't figure out how to transfer to you, but besides my two suggestions, I think this is amazing and am excited to roll this out. I do think this should go first and then ledger since this is more widely applicable!

elizabethengelman
elizabethengelman reviewed Dec 23, 2024
View reviewed changes
Comment thread cmd/soroban-cli/src/commands/keys/address.rs Outdated
willemneal
willemneal commented Jan 7, 2025
View reviewed changes
Comment thread cmd/soroban-cli/src/config/address.rs Outdated
willemneal
willemneal commented Jan 7, 2025
View reviewed changes
Comment thread cmd/soroban-cli/src/commands/keys/generate.rs Outdated
willemneal
willemneal commented Jan 7, 2025
View reviewed changes
Comment thread cmd/soroban-cli/src/config/secret.rs
@willemneal
feat: store seedphrase instead of private key
aecc5ac 
This will allow for exporting the phrase later
@willemneal
fix: clean up
63f247e
@elizabethengelman elizabethengelman mentioned this pull request Jan 8, 2025
Closed
@elizabethengelman

elizabethengelman commented Jan 8, 2025

Copy link
Copy Markdown
Contributor

@willemneal I'm planning to merge this in this afternoon - what do you think? I think that the e2e test should be fixed by stellar-deprecated/system-test#109

@elizabethengelman
elizabethengelman merged commit c055eac into main Jan 9, 2025
@elizabethengelman
elizabethengelman deleted the feat/os_keychain branch January 9, 2025 20:57
@fnando fnando mentioned this pull request Jan 21, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leighmcculloch leighmcculloch Awaiting requested review from leighmcculloch
@ifropc ifropc Awaiting requested review from ifropc
1 more reviewer
@elizabethengelman elizabethengelman elizabethengelman approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@elizabethengelman elizabethengelman

Labels

None yet

Projects

DevX
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Epic] Implement more robust signing, which includes ledger, and lab Sign with system keychain

2 participants

@willemneal @elizabethengelman