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.

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

feat: add support for OS specific keychains #1703

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
49 commits
Select commit Hold shift + click to select a range
8c06970
feat: initial work into system keychain
willemneal Nov 5, 2024
883cb8c
chore: clean up
willemneal Nov 5, 2024
eb70734
Add KeyName struct in address
elizabethengelman Dec 2, 2024
d4e2500
Add Secret::Keychain
elizabethengelman Nov 14, 2024
3ee20b8
keys generate: allow for generating keys that are stored in keychain
elizabethengelman Nov 18, 2024
1a24c00
keys generate: Namespace keychain entry to identity name
elizabethengelman Nov 18, 2024
4a0c900
keys generate: don't allow 'keychain:' as a key name
elizabethengelman Nov 20, 2024
755c318
keys address: use keychain entry in secret to get the pub key
elizabethengelman Nov 20, 2024
dd07bf7
tx sign: allow a keychain identity sign a tx
elizabethengelman Nov 21, 2024
f042d19
Cleanup
elizabethengelman Nov 21, 2024
9cf5916
Use keyring mock for generate tests
elizabethengelman Nov 22, 2024
cd515d3
Refactor keyring: add keyring entry as StellarEntry field
elizabethengelman Nov 25, 2024
276558d
Add tests for keyring
elizabethengelman Nov 25, 2024
20c651b
Update config/secret tests
elizabethengelman Nov 25, 2024
66d1bfc
Cleanup
elizabethengelman Nov 25, 2024
8b9763e
Rename keychain arg to secure_store in generate
elizabethengelman Nov 25, 2024
fdefa2a
Rename Secret::Keychain to Secret::SecureStore
elizabethengelman Nov 25, 2024
36559ea
Rename SignerKind::Keychain to SignerKind::SecureStore
elizabethengelman Nov 25, 2024
f98b709
Use print for new fns in generate
elizabethengelman Dec 2, 2024
0f3106b
Return error when trying to get Secure Store secret
elizabethengelman Dec 2, 2024
e120595
Cleanup tests
elizabethengelman Dec 2, 2024
c4d6f99
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 2, 2024
c7e2cbc
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 3, 2024
57ba3a4
Install libdbus for rpc-tests and bindings-ts workflows
elizabethengelman Dec 3, 2024
0814e4b
Update generated docs
elizabethengelman Dec 3, 2024
74fa05b
Install libdbus for binaries workflow when target aarch64-unknown-lin…
elizabethengelman Dec 3, 2024
7ff1f6a
Clippy
elizabethengelman Dec 3, 2024
f263d8d
Install libdbus for rust workflow
elizabethengelman Dec 3, 2024
e2b7342
Install libdbus-1-dev in binaries workflow for build step
elizabethengelman Dec 3, 2024
dabbb27
Impl Display for KeyName
elizabethengelman Dec 3, 2024
d9131b4
Use resolve_muxed_account in resolve_secret
elizabethengelman Dec 3, 2024
3ad6b9b
Use resolve_muxed_account to get public key
elizabethengelman Dec 3, 2024
9d02b01
Clippy
elizabethengelman Dec 3, 2024
4bddd24
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 3, 2024
a72276d
fix: Sign tx hash instead of tx env with keychain
elizabethengelman Dec 4, 2024
65284cc
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 5, 2024
e9f86e7
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 9, 2024
4288630
Remove unused bin/secret
elizabethengelman Dec 9, 2024
6661b42
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 12, 2024
b2120d2
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 13, 2024
24e7b59
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 17, 2024
cfdb324
Merge branch 'main' into feat/os_keychain
elizabethengelman Dec 18, 2024
8e158a1
Merge branch 'main' into feat/os_keychain
willemneal Dec 19, 2024
2241352
Merge remote-tracking branch 'origin/main' into feat/os_keychain
willemneal Dec 20, 2024
03b84e8
Merge remote-tracking branch 'origin/main' into feat/os_keychain
elizabethengelman Dec 20, 2024
24269ce
Fix after merging with main
elizabethengelman Dec 20, 2024
e95fa3a
Apply suggestion from code review
elizabethengelman Dec 20, 2024
95d8b32
Limit key name length
elizabethengelman Dec 23, 2024
01c8753
Update public_key to work with secure storage keys
elizabethengelman Dec 23, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/binaries.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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.


- name: Setup vars
run: |
Expand All @@ -69,6 +69,7 @@ jobs:
env:
CARGO_TARGET_AARCH64_UNKNOWN_LINUX_GNU_LINKER: aarch64-linux-gnu-gcc
working-directory: ${{ env.BUILD_WORKING_DIR }}
run: sudo apt-get update && sudo apt-get -y install libdbus-1-dev
run: cargo build --target-dir="$GITHUB_WORKSPACE/target" --package ${{ matrix.crate.name }} --features opt --release --target ${{ matrix.sys.target }}

- name: Build provenance for attestation (release only)
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/bindings-ts.yml
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ jobs:
target/
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
- run: rustup update
- run: sudo apt install -y libdbus-1-dev
- run: cargo build
- run: rustup target add wasm32-unknown-unknown
- run: make build-test-wasms
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/rpc-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ jobs:
target/
key: ${{ runner.os }}-cargo-${{ hashFiles('**/Cargo.lock') }}
- run: rustup update
- run: sudo apt install -y libdbus-1-dev
- run: cargo build
- run: rustup target add wasm32-unknown-unknown
- run: make build-test-wasms
Expand Down
3 changes: 2 additions & 1 deletion .github/workflows/rust.yml
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ jobs:
- uses: actions/checkout@v4
- uses: stellar/actions/rust-cache@main
- run: rustup update
- run: sudo apt install -y libdbus-1-dev
- run: make generate-full-help-doc
- run: git add -N . && git diff HEAD --exit-code

Expand Down Expand Up @@ -90,7 +91,7 @@ jobs:
- run: rustup target add ${{ matrix.sys.target }}
- run: rustup target add wasm32-unknown-unknown
- if: runner.os == 'Linux'
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
- run: cargo clippy --all-targets --target ${{ matrix.sys.target }}
- run: make test
env:
Expand Down
111 changes: 111 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 3 additions & 2 deletions FULL_HELP_DOCS.md
Original file line number Diff line number Diff line change
Expand Up @@ -938,7 +938,7 @@ Create and manage identities including keys and addresses

###### **Subcommands:**

* `add` — Add a new identity (keypair, ledger, macOS keychain)
* `add` — Add a new identity (keypair, ledger, OS specific secure store)
* `address` — Given an identity return its address (public key)
* `fund` — Fund an identity on a test network
* `generate` — Generate a new identity with a seed phrase, currently 12 words
Expand All @@ -951,7 +951,7 @@ Create and manage identities including keys and addresses

## `stellar keys add`

Add a new identity (keypair, ledger, macOS keychain)
Add a new identity (keypair, ledger, OS specific secure store)

**Usage:** `stellar keys add [OPTIONS] <NAME>`

Expand Down Expand Up @@ -1023,6 +1023,7 @@ Generate a new identity with a seed phrase, currently 12 words
* `--no-fund` — Do not fund address
* `--seed <SEED>` — Optional seed to use when generating seed phrase. Random otherwise
* `-s`, `--as-secret` — Output the generated identity as a secret key
* `--secure-store` — Save in OS-specific secure store
* `--global` — Use global config
* `--config-dir <CONFIG_DIR>` — Location of config directory, default is "."
* `--hd-path <HD_PATH>` — When generating a secret key, which `hd_path` should be used from the original `seed_phrase`
Expand Down
7 changes: 6 additions & 1 deletion cmd/soroban-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,8 @@ rand = "0.8.5"
wasmparser = { workspace = true }
sha2 = { workspace = true }
csv = "1.1.6"
ed25519-dalek = { workspace = true }
# zeroize feature ensures that all sensitive data is zeroed out when dropped
ed25519-dalek = { workspace = true, features = ["zeroize"] }
reqwest = { version = "0.12.7", default-features = false, features = [
"rustls-tls",
"http2",
Expand Down Expand Up @@ -124,6 +125,10 @@ fqdn = "0.3.12"
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"] }
whoami = "1.5.2"


[build-dependencies]
crate-git-revision = "0.0.6"
Expand Down
7 changes: 6 additions & 1 deletion cmd/soroban-cli/src/commands/contract/arg_parsing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@ use crate::xdr::{

use crate::commands::txn_result::TxnResult;
use crate::config::{
self,
self, address,
sc_address::{self, UnresolvedScAddress},
secret,
};
use soroban_spec_tools::Spec;

Expand All @@ -41,6 +42,10 @@ pub enum Error {
Xdr(#[from] xdr::Error),
#[error(transparent)]
StrVal(#[from] soroban_spec_tools::Error),
#[error(transparent)]
Address(#[from] address::Error),
#[error(transparent)]
Secret(#[from] secret::Error),
#[error("Missing argument {0}")]
MissingArgument(String),
#[error("")]
Expand Down
10 changes: 7 additions & 3 deletions cmd/soroban-cli/src/commands/keys/add.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,28 @@ use clap::command;

use crate::{
commands::global,
config::{locator, secret},
config::{
address::{self, KeyName},
locator, secret,
},
print::Print,
};

#[derive(thiserror::Error, Debug)]
pub enum Error {
#[error(transparent)]
Secret(#[from] secret::Error),

#[error(transparent)]
Config(#[from] locator::Error),
#[error(transparent)]
Address(#[from] address::Error),
}

#[derive(Debug, clap::Parser, Clone)]
#[group(skip)]
pub struct Cmd {
/// Name of identity
pub name: String,
pub name: KeyName,

#[command(flatten)]
pub secrets: secret::Args,
Expand Down
16 changes: 13 additions & 3 deletions cmd/soroban-cli/src/commands/keys/address.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use crate::commands::config::secret;

use super::super::config::locator;
use clap::arg;

use crate::{
commands::config::{address, locator, secret},
config::UnresolvedMuxedAccount,
};

#[derive(thiserror::Error, Debug)]
pub enum Error {
#[error(transparent)]
Expand All @@ -13,6 +15,9 @@ pub enum Error {

#[error(transparent)]
StrKey(#[from] stellar_strkey::DecodeError),

#[error(transparent)]
Address(#[from] address::Error),
}

#[derive(Debug, clap::Parser, Clone)]
Expand Down Expand Up @@ -45,6 +50,11 @@ impl Cmd {
pub fn public_key(&self) -> Result<stellar_strkey::ed25519::PublicKey, Error> {
if let Ok(key) = stellar_strkey::ed25519::PublicKey::from_string(&self.name) {
Ok(key)
} else if let Ok(unresolved) = self.name.parse::<UnresolvedMuxedAccount>() {
let muxed = unresolved.resolve_muxed_account(&self.locator, self.hd_path)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

@willemneal When I was testing after merging main, I realized I needed to change this function. I added this else if branch to get the public key from the secure store without first getting the private key. Can you take a look and make sure this looks correct? I tested it locally but would love another set of eyes on it.

Ok(stellar_strkey::ed25519::PublicKey::from_string(
&muxed.to_string(),
)?)
} else {
Ok(stellar_strkey::ed25519::PublicKey::from_payload(
self.private_key()?.verifying_key().as_bytes(),
Expand Down
Loading
Loading