Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add support for OS specific keychains #1703

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

feat: add support for OS specific keychains #1703

wants to merge 40 commits into from

Conversation

willemneal
Copy link
Member

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

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/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
Open
@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
Open
5 tasks
@willemneal willemneal mentioned this pull request Nov 26, 2024
Open
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
@elizabethengelman elizabethengelman marked this pull request as ready for review December 3, 2024 14:08
elizabethengelman
elizabethengelman reviewed Dec 3, 2024
View reviewed changes
cmd/soroban-cli/src/commands/keys/generate.rs
@@ -39,6 +49,10 @@ pub struct Cmd {
#[arg(long, short = 's')]
pub as_secret: bool,

/// Save in OS-specific secure store
#[arg(long)]
pub secure_store: bool,
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to add an alias for this arg: keychain for mac, secret-service for linux and windows-credential-manager for windows?

.github/workflows/binaries.yml
@@ -46,7 +46,7 @@ jobs:
- run: rustup target add ${{ matrix.sys.target }}

- if: matrix.sys.target == 'aarch64-unknown-linux-gnu'
run: sudo apt-get update && sudo apt-get -y install gcc-aarch64-linux-gnu g++-aarch64-linux-gnu libudev-dev
run: sudo apt-get update && sudo apt-get -y install gcc-aarch64-linux-gnu g++-aarch64-linux-gnu libudev-dev libdbus-1-dev
Copy link
Contributor

Choose a reason for hiding this comment

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

The keyring crate requires this pkg as a dependency, so I am including it on several of these linux workflows. I just wanted to point this out as a dependency and see if there is any other way I should handle this.

@elizabethengelman elizabethengelman mentioned this pull request Dec 3, 2024
Open
20 tasks
@elizabethengelman elizabethengelman mentioned this pull request Dec 5, 2024
Open
@elizabethengelman elizabethengelman requested review from a team, leighmcculloch and Ifropc and removed request for a team December 13, 2024 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@elizabethengelman elizabethengelman elizabethengelman left review comments

@leighmcculloch leighmcculloch Awaiting requested review from leighmcculloch

@Ifropc Ifropc Awaiting requested review from Ifropc

At least 1 approving review is required to merge this pull request.

Assignees

@elizabethengelman elizabethengelman

Labels
None yet
Projects
DevX
Status: Needs Review
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