-
Notifications
You must be signed in to change notification settings - Fork 121
Fix/soroban auth signing #2230
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
Fix/soroban auth signing #2230
Conversation
…an async function more easily
we should be able to have resolve_signer (in arg_parsing) always return a Signer instead of a SigningKey
8b92954 to
a5308f6
Compare
cb8bd29 to
27be163
Compare
86be245 to
eb5ba54
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates Soroban authorization signing to use abstract Signer implementations (local, secure store, etc.) instead of accessing private keys directly, enabling secure store signers for both source and auth signing.
- Switches signing of invoke transactions and auth entries to use Signer, not raw private keys.
- Makes argument parsing/build helpers async and returns Signer instances for address arguments.
- Adds Signer utilities for public key retrieval and payload signing, with secure store support.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/soroban-cli/src/signer/mod.rs | Introduces async signer-based auth signing, adds get_public_key/sign_payload to Signer, and secure store integrations. |
| cmd/soroban-cli/src/config/mod.rs | Provides a source_signer() helper and updates auth signing to use Signer and async flow. |
| cmd/soroban-cli/src/commands/contract/invoke.rs | Awaits async arg parsing helpers. |
| cmd/soroban-cli/src/commands/contract/deploy/wasm.rs | Awaits async constructor parameter builder. |
| cmd/soroban-cli/src/commands/contract/arg_parsing.rs | Converts builders to async, returns Vec for auth-capable addresses, and resolves signers via secure store/local. |
Co-authored-by: Copilot <[email protected]>
These fns will need to be made into async fns again if/when we implement this for a ledger device.
|
the currently failing bindings typescript test should be fixed once #2268 is merged in |
mootz12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not all that familiar with keychain for storing keys, but I assume this is expected? Bombadil is my secure store key on mac.
stellar-cli % devcli keys secret bombadil
❌ error: Secure Store does not reveal secret key
@mootz12 Yep, that is correct! When we first implemented this, I think we made the decision to not expose the private key or seed phrase for Secure Store keys. For signing txs, we're able to send the tx to the keyring crate (which manages the secure store integration) to sign it, and then returned the signed tx. So we don't necessarily need access to the private key directly. |
mootz12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG2M with the expectation #2291 is a fast follow
What
Closes #2039
Why
Previously we were trying to get a signer key's private key directly to sign an invoke tx and when signing auth entries, but this doesn't work for secure store or ledger device signers since they don't expose their private key directly.
This PR updates the invoke tx signing, and signing auth entries to use a signer to sign, rather than a signing key directly so we don't need explicit access to the private key.
local source key, local auth key
local source key, secure auth key
❌ error: Missing signing key for account GB765REIZB4KQEN5TYFEPN3FQQMGIZHVXE5SKDD3REULDSTJWTR5E7GLsecure source key, local auth key
❌ error: Secure Store does not reveal secret keysecure source key, secure auth key
❌ error: Secure Store does not reveal secret keyKnown limitations
This PR does not address ledger devices yet, that will be a follow up PR to avoid this one getting too large.