Remove keypair from client state #594

patrickkuo · 2022-02-28T23:52:52Z

Removed key pair from client state and add a SignatureCallback to the ClientAPI to get signature from the application layer.

Removed key pair from ClientState
Removed key usage in TransactionData and Transaction
Added SignatureCallback type for signature requests during transaction creation.

oxade · 2022-03-01T21:41:56Z

This is cool. But I don't think I know enough about cryptography to say if the approach is good or secure
I'll try to provide feedback on the code but not functionality

huitseeker

Thanks for doing this! I love to better master where keys live in the client / gateway.

I left a bunch of comments, here are the three main ones:

the lock usage,
the question on add_key,
the definition of SignatureCallBack.

huitseeker · 2022-03-02T00:22:49Z

sui/src/wallet_commands.rs

-    pub fn create_account_state(&mut self, owner: &SuiAddress) -> SuiResult {
-        let kp = Box::pin(self.config.get_account_cfg_info(owner)?.key_pair.copy());
-        self.address_manager.create_account_state(*owner, kp)
-    }


This is so beautiful I could cry. 🥲

huitseeker · 2022-03-02T00:33:15Z

sui/src/keystore.rs

+    keystore: Arc<Mutex<Box<dyn Keystore>>>,
+    address: SuiAddress,
+}
+


The SuiKeyStoreSigner will only ever read from the KeyStore, never write to it: a RwLock is a better choice than a Mutex.

huitseeker · 2022-03-02T00:36:32Z

sui/src/keystore.rs

+    pub fn load_or_create(path: &Path) -> Result<Self, anyhow::Error> {
+        let keys: Vec<KeyPair> = if path.exists() {
+            let reader = BufReader::new(File::open(path)?);
+            serde_json::from_reader(reader)?
+        } else {
+            Vec::new()
+        };
+
+        let keys = keys
+            .into_iter()
+            .map(|key| (SuiAddress::from(key.public_key_bytes()), key))
+            .collect();
+
+        Ok(Self {
+            keys,
+            path: path.to_path_buf(),
+        })
+    }


CC-ing @asonnino @oxade, who may end up on the receiving end of a request to build benchmarks, for clarity (benchmarks require knowing on how to provision machines with the keys they need).

sui/src/keystore.rs

huitseeker · 2022-03-02T00:54:29Z

sui/src/keystore.rs

+            .ok_or_else(|| anyhow!("Cannot find key for address: [{}]", address))?
+            .sign(msg))
+    }
+    fn add_key(&mut self, keypair: KeyPair) -> Result<(), anyhow::Error> {


Outside of unit tests, what's the scenario, here: where would that key come from?

Do we really need an add_key(&mut self, keypair: KeyPair) -> Result<(), anyhow::Error>, or do we need a add_random_key<R: CyrptoRng + RngCore>(&mut self, rng: &mut R) -> Result<PublicKey, anyhow::Error>?

huitseeker · 2022-03-02T01:23:13Z

sui_core/src/client.rs

-    pub fn secret(&self) -> &dyn signature::Signer<Signature> {
-        &*self.secret
-    }
-


huitseeker · 2022-03-02T01:26:33Z

sui_types/src/messages.rs

+    pub fn from_data(data: TransactionData, keypair: &KeyPair) -> Self {
+        let signature = Signature::new(&data, keypair);
+        Self::new(data, signature)


Even for testing, this could easily take a signer (&dyn signature::Signer<Signature>) in argument position (and then everything that produces such a signer is testable using this).

And then the KeyPair import disappears.

sui/src/keystore.rs

huitseeker · 2022-03-02T02:00:48Z

sui_core/src/client.rs

+    dyn FnOnce(SuiAddress, TransactionData) -> BoxFuture<'a, Result<Signature, anyhow::Error>>
+        + Send
+        + Sync
+        + 'a,
+>;


So, this works, but it's really "flat", i.e. you need this complex type to capture all the genericity of "anything that has a function sendable between threads and taking an Address & TransactionData and allow me to wait on the result which is ...".

You could also express this complex notion in more manageable pieces by having something like this:¹

#[async_trait] trait SignatureCallBack { async fn sign(&self, &SuiAddress, TransactionData) -> Result<Signature, signature::Error>; }

and then at places where you'd be using it you'd require the following type²:

pub type StableSyncSignatureCallBack = Pin<Box<dyn SignatureCallBack + Send + Sync>>;

What I believe this amounts to is letting async_trait take care of some of the complexity for you: it will hide the complexity of the second dyn in your signature, the one behind the BoxFuture.

Footnotes

there's one detail, which is that it's important that the error type not be too expressive in the returned signature, hence the choice of signature::Error instead of anyhow::Error. That's a detail we can discuss later. ↩

feel free to choose a better name ↩

hmm.... but I won't be able to use closure if I use the trait instead of FnOnce...
closure is useful if the caller want to validate TransactionData before signing, I am investigation if I can do the same with the trait implementation.

Apparently rust have a RFC for async closure but it's still not stable, will be much easier when this is available lol https://github.com/rust-lang/rfcs/blob/master/text/2394-async_await.md

patrickkuo · 2022-03-02T10:42:08Z

the Keystore changes was merged from another PR #590, Github changed base automatically and this PR was missing a rebase 🙇‍♂️, I will address all keystore related comments in a separate branch

patrickkuo · 2022-03-03T16:53:46Z

@huitseeker, thanks for the comments! I have replaced the closure callback implementation with a SignatureCallback trait, I have also included keystore.rs refactoring here as the changes is quite small.

This PR is ready for round 2, thanks 🙏

huitseeker

I think the SignatureCallback trait area needs a bit of pedagogic rustdoc love, but this overall looks dope, and pretty close to the finish line.

huitseeker · 2022-03-03T21:05:44Z

sui/src/keystore.rs


 #[derive(Serialize, Deserialize)]
+#[non_exhaustive]
+// This will work on user signatures, but not suitable for authority signatures.
+// TODO: refactor keystore to support authority signature.


I'm actually in favor of the keystore never supporting the authority signature (I would be very cross with the people running an authority whose private key lies on an unencrypted file on the actual machine). To me that's an ANTI-TODO.

my thought is we will have many keystore implementation in the future, HSMKeystore will be one of it, and user can choose what type of keystore to use by config.

I will file an issue to encrypt the current keystore file used by the wallet, password protected is better then nothing lol

my thought is we will have many keystore implementation in the future, HSMKeystore will be one of it, and user can choose what type of keystore to use by config.

If we're talking about authorities, I'm pretty sure an HSM connector is not going to be a "store" of any kind, (it will just be an impl AsyncTransactionSigner), and it will live in an entirely different part of the code base. The key store in its current form is kind of ok for stubbing the signing component of a wallet. For an authority, it's a different thing, you'd want something like TKMS.

I really think we should remove this TODO comment: it will give somebody ideas, and then we'll have to explain why the present keystore should just not be used for authorities after they write the PR rather than before.

Sure, I have removed the TODO

huitseeker · 2022-03-03T21:07:20Z

sui/src/keystore.rs

        Ok(self
            .keys
            .get(address)
-            .ok_or_else(|| anyhow!("Cannot find key for address: [{}]", address))?
+            .ok_or_else(|| {
+                signature::Error::from_source(format!("Cannot find key for address: [{}]", address))


huitseeker · 2022-03-03T21:11:18Z

sui/src/wallet_commands.rs

+        struct SimpleSignatureCallback {
+            keystore: Arc<RwLock<Box<dyn Keystore>>>,
+        }
+        #[async_trait]
+        // A simple signer callback implementation, which signs the content without validation.
+        impl SignatureCallback for SimpleSignatureCallback {
+            async fn sign(
+                &self,
+                address: &SuiAddress,
+                data: TransactionData,
+            ) -> Result<Signature, signature::Error> {
+                let keystore = self.keystore.read().unwrap();
+                let mut msg = Vec::new();
+                data.write(&mut msg);
+                keystore.sign(address, &msg)
+            }
+        }


I think it's fine to have those be a struct private to the wallet_commands module (i.e. right outside this impl WalletCommands block), and may actually help defining a registry of such impls in the future.

huitseeker · 2022-03-03T21:19:24Z

sui_core/src/client.rs

@@ -40,7 +40,16 @@ use self::client_responses::{MergeCoinResponse, SplitCoinResponse};
 ///
 /// Typically instantiated with Box::pin(keypair) where keypair is a `KeyPair`
 ///
-pub type StableSyncSigner = Pin<Box<dyn signature::Signer<Signature> + Send + Sync>>;
+#[async_trait]
+pub trait SignatureCallback {


I'm miffed by two things:

the SignatureCallback is really an asynchronous signing function keyed to an address. The name doesn't help me figure that out. AddressableAsyncSigner, though ugly, is a bit better. I'm open to suggestions.

the comment no longer matches what it's commenting on at all. I think both SignatureCallback and StableSyncSignatureCallback should have doc comments. Moreover, I think the comment for SignatureCallback should include help for the implementers, including suggesting this is a good place to do TX validation (which you have mentioned in PR comments).

I have renamed it to AsyncTransactionSigner, probably more clear on what it's signing.

patrickkuo · 2022-03-04T01:08:27Z

I have addressed all the comments, this PR is ready for more review (hopefully last) :)

huitseeker

Great work, thanks @patrickkuo ! The improvements to the key store are great, and seeing keys drop from the client is tip top!

lxfind · 2022-03-04T18:07:23Z

This is awesome. Thanks @patrickkuo !

patrickkuo requested review from huitseeker, sblackshear, lxfind, arun-koshy and oxade March 1, 2022 00:44

patrickkuo force-pushed the pat/wallet_key_store branch from 9c1a473 to 274b91c Compare March 1, 2022 13:05

patrickkuo force-pushed the pat/remove_keypair_from_client_state branch 2 times, most recently from d0089c6 to e397e3d Compare March 1, 2022 13:42

patrickkuo mentioned this pull request Mar 1, 2022

Refactor ClientState API #584

Merged

patrickkuo force-pushed the pat/wallet_key_store branch from a848f80 to be46f49 Compare March 1, 2022 16:46

patrickkuo force-pushed the pat/remove_keypair_from_client_state branch from 18c9e5d to 5251767 Compare March 1, 2022 18:33

patrickkuo marked this pull request as ready for review March 1, 2022 18:55

lxfind mentioned this pull request Mar 1, 2022

[Gateway Service] [umbrella] Implement full Gateway Service API #615

Closed

21 tasks

patrickkuo requested a review from kchalkias March 1, 2022 22:17

Base automatically changed from pat/wallet_key_store to main March 1, 2022 23:49

huitseeker reviewed Mar 2, 2022

View reviewed changes

patrickkuo force-pushed the pat/remove_keypair_from_client_state branch 2 times, most recently from 483a0f0 to a441a6b Compare March 3, 2022 13:33

huitseeker reviewed Mar 3, 2022

View reviewed changes

patrickkuo mentioned this pull request Mar 3, 2022

[fastx] Replace KeyPair With Signer & Load Keys From File #380

Closed

patrickkuo force-pushed the pat/remove_keypair_from_client_state branch from 4c67431 to 24daba4 Compare March 4, 2022 01:04

patrickkuo linked an issue Mar 4, 2022 that may be closed by this pull request

[fastx] Replace KeyPair With Signer & Load Keys From File #380

Closed

patrickkuo requested a review from huitseeker March 4, 2022 15:15

patrickkuo added 2 commits March 4, 2022 15:23

Remove keypair from client state

c289393

remove get_or_create_client_state from wallet context

92dfb6f

patrickkuo force-pushed the pat/remove_keypair_from_client_state branch from 68df38f to 92dfb6f Compare March 4, 2022 15:26

huitseeker approved these changes Mar 4, 2022

View reviewed changes

patrickkuo merged commit 0a76477 into main Mar 4, 2022

patrickkuo deleted the pat/remove_keypair_from_client_state branch March 4, 2022 18:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove keypair from client state #594

Remove keypair from client state #594

patrickkuo commented Feb 28, 2022 •

edited

Loading

oxade commented Mar 1, 2022

huitseeker left a comment •

edited

Loading

huitseeker Mar 2, 2022

huitseeker Mar 2, 2022

huitseeker Mar 2, 2022

huitseeker Mar 2, 2022

huitseeker Mar 2, 2022

huitseeker Mar 2, 2022

huitseeker Mar 2, 2022

patrickkuo Mar 2, 2022 •

edited

Loading

patrickkuo commented Mar 2, 2022 •

edited

Loading

patrickkuo commented Mar 3, 2022

huitseeker left a comment

huitseeker Mar 3, 2022

patrickkuo Mar 3, 2022

huitseeker Mar 4, 2022 •

edited

Loading

patrickkuo Mar 4, 2022

huitseeker Mar 3, 2022

huitseeker Mar 3, 2022 •

edited

Loading

huitseeker Mar 3, 2022

patrickkuo Mar 4, 2022

patrickkuo commented Mar 4, 2022

huitseeker left a comment

lxfind commented Mar 4, 2022

Remove keypair from client state #594

Remove keypair from client state #594

Conversation

patrickkuo commented Feb 28, 2022 • edited Loading

oxade commented Mar 1, 2022

huitseeker left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

patrickkuo Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

patrickkuo commented Mar 2, 2022 • edited Loading

patrickkuo commented Mar 3, 2022

huitseeker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Mar 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickkuo commented Mar 4, 2022

huitseeker left a comment

Choose a reason for hiding this comment

lxfind commented Mar 4, 2022

patrickkuo commented Feb 28, 2022 •

edited

Loading

huitseeker left a comment •

edited

Loading

patrickkuo Mar 2, 2022 •

edited

Loading

patrickkuo commented Mar 2, 2022 •

edited

Loading

huitseeker Mar 4, 2022 •

edited

Loading

huitseeker Mar 3, 2022 •

edited

Loading