Signed responses #621

DavidM-D · 2024-05-30T13:55:32Z

Make the smart contract verify that the responses we're getting back from the nodes are valid.

A large part of the change is move dependencies into crypto_shared so it can be used in the contract for verification.

This has a breaking change in that payloads are no longer little endian and are big endian (standard in our industry).

A subsequent PR will:

Get the native NEAR verification function working
Expose the well typed API
Unreverse the epsilon value
Force validation of scalars (not added in this PR)
Abstract the "Serializable" trait from its concrete types

CHANGELOG.md

ailisp · 2024-05-31T08:45:37Z

crypto-shared/src/kdf.rs

+    let derivation_path = format!("{EPSILON_DERIVATION_PREFIX}{},{}", predecessor_id, path);
+    let mut hasher = Sha256::new();
+    hasher.update(derivation_path);
+    let mut bytes = hasher.finalize();


consider native near_sdk::env::sha256 instead of wasm-side sha256 to be more efficient?

ailisp · 2024-05-31T08:50:01Z

crypto-shared/src/kdf.rs

+    anyhow::bail!("cannot use either recovery id (0 or 1) to recover pubic key")
+}
+
+// #[cfg(not(target_arch = "wasm32"))]


This and // #[cfg(target_arch = "wasm32")] conditional compile seems unnecessary, as near-sdk handles mock of ecrecover etc. on non wasm case (by importing vmlogic from nearcore): https://github.com/near/near-sdk-rs/blob/master/near-sdk/src/environment/mock/mocked_blockchain.rs

Ah excellent point

Do you mind if I move this to the next PR? The merge conflicts are killing me

So the contract code can inherit it (also maybe include this in a client library later?)

It's not working with the CI

volovyks

Great work @DavidM-D !

contract/Cargo.toml

contract/src/lib.rs

volovyks · 2024-05-31T12:35:27Z

crypto-shared/src/kdf.rs

+    let mut hasher = Sha256::new();
+    hasher.update(derivation_path);
+    let mut bytes = hasher.finalize();
+    // Due to a previous bug in our Scalar conversion code, this hash was reversed, we reverse it here to preserve compatibility, but will likely change this later.


What is the reason for postponing this breaking change? Isn't now the best time to do it?

I'm just trying to break up this PR a bit, I'll get it through before the 0.3.0 release

volovyks · 2024-05-31T13:05:11Z

integration-tests/chain-signatures/tests/actions/mod.rs

-    let payload_hash_scalar = k256::Scalar::from_bytes(&payload_hash); // TODO: why do we need both reversed and not reversed versions?
-    let mut payload_hash_reversed: [u8; 32] = payload_hash;
-    payload_hash_reversed.reverse();
+    payload_hash.reverse();


We are still reversing the hash here.

This code was and still is a bit of a mess.

Previously converting a Scalar to bytes reversed the bytes and converted to a Scalar.

The big_r and s values were generated using chain signatures from an older commit, therefore the signature is generated against a reversed hash.

You now have to reverse the bytes to generate the same Scalar and therefore Signature and this tests that. I'll write a comment to this effect, after this change has rolled out we should update this test using new values.

Co-authored-by: Serhii Volovyk <[email protected]>

volovyks · 2024-05-31T18:09:10Z

contract/src/lib.rs

        match self {
            Self::V0(mpc_contract) => mpc_contract.pending_requests.get(request),
        }
    }
+


Do we need this?

volovyks · 2024-05-31T18:09:46Z

contract/src/lib.rs

@@ -70,36 +73,53 @@ impl Default for VersionedMpcContract {
    }
 }

+#[derive(BorshDeserialize, BorshSerialize, Serialize, Deserialize, Debug, Clone)]
+pub struct SignatureRequest {


Why do we need SignatureRequest and SignRequest? Can we use 1 struct?

SignatureRequest describes all the information about a request required by the signing protocol, SigRequest is the structure of the request sent to the contract, so they're different. That being said, we can probably come up with better names.

volovyks

We can merge this to avoid future conflicts and unblock yield/resume. Great contribution!

github-actions · 2024-05-31T18:49:39Z

Terraform Feature Environment Destroy (dev-621)

Terraform Initialization ⚙️`success`

Terraform Destroy `success`

Show Destroy Plan



No changes. No objects need to be destroyed.

Either you have not created any objects yet or the existing objects were
already deleted outside of Terraform.

Destroy complete! Resources: 0 destroyed.

Pusher: @DavidM-D, Action: pull_request, Working Directory: ``, Workflow: Terraform Feature Env (Destroy)

* Moved kdf out of the node So the contract code can inherit it (also maybe include this in a client library later?) * Migrated contract over to using API with context * Fix test imports * Successfully merged into phuongs mega PR * Builds with signature verification * Working tests with a compatible API * All tests pass (with spurious reversing) * Test rogue responses fail * Generate recovery ID on node * Switched to use native near function to verify sig * Cleanup * Removed Multichain Signature type * Add missing import in wasm builds * Added a changelog explaining the API changes * Cleanup comments * Move into_eth_sig back up the dep tree * Migrate recovery ID functions up the tree * Clippy fix * Do the native function in a seperate PR It's not working with the CI * Fix fmt * Clippy fixes * Compiling after rebase * Update contract/src/lib.rs Co-authored-by: Serhii Volovyk <[email protected]> * Fixed types and removed unneeded type * Simplify API * Shrink diff * Comment explaining the mad test --------- Co-authored-by: Serhii Volovyk <[email protected]>

DavidM-D force-pushed the dmd/signed-response branch from 2c7b5a8 to 771c29d Compare May 30, 2024 13:56

DavidM-D requested review from volovyks and ChaoticTempest May 30, 2024 15:45

DavidM-D marked this pull request as ready for review May 30, 2024 15:46

ailisp reviewed May 31, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ailisp reviewed May 31, 2024

View reviewed changes

DavidM-D added 21 commits May 31, 2024 14:48

Moved kdf out of the node

f9ab397

So the contract code can inherit it (also maybe include this in a client library later?)

Migrated contract over to using API with context

7f8e67c

Fix test imports

96cf7f9

Successfully merged into phuongs mega PR

8618f40

Builds with signature verification

54f07de

Working tests with a compatible API

571e161

All tests pass (with spurious reversing)

bbde683

Test rogue responses fail

342ae27

Generate recovery ID on node

77c9fbe

Switched to use native near function to verify sig

af4736c

Cleanup

2f66f09

Removed Multichain Signature type

f1db016

Add missing import in wasm builds

ac6db8d

Added a changelog explaining the API changes

9de7726

Cleanup comments

d92d857

Move into_eth_sig back up the dep tree

6d1098d

Migrate recovery ID functions up the tree

d095f83

Clippy fix

b4f5208

Do the native function in a seperate PR

3390649

It's not working with the CI

Fix fmt

1781fa3

Clippy fixes

942aca4

volovyks reviewed May 31, 2024

View reviewed changes

DavidM-D force-pushed the dmd/signed-response branch from ad5976c to 942aca4 Compare May 31, 2024 13:06

DavidM-D and others added 6 commits May 31, 2024 15:59

Compiling after rebase

4eda1a1

Update contract/src/lib.rs

89ea373

Co-authored-by: Serhii Volovyk <[email protected]>

Fixed types and removed unneeded type

0d4d4c6

Simplify API

7b5bd96

Shrink diff

08806d5

Comment explaining the mad test

aed35b6

volovyks reviewed May 31, 2024

View reviewed changes

volovyks approved these changes May 31, 2024

View reviewed changes

DavidM-D merged commit 22c85cf into develop May 31, 2024
6 checks passed

DavidM-D deleted the dmd/signed-response branch May 31, 2024 18:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Signed responses #621

Signed responses #621

DavidM-D commented May 30, 2024 •

edited

Loading

ailisp May 31, 2024

DavidM-D May 31, 2024

ailisp May 31, 2024

DavidM-D May 31, 2024

DavidM-D May 31, 2024

volovyks left a comment

volovyks May 31, 2024

DavidM-D May 31, 2024

volovyks May 31, 2024

DavidM-D May 31, 2024

volovyks May 31, 2024

volovyks May 31, 2024

DavidM-D May 31, 2024

volovyks left a comment

github-actions bot commented May 31, 2024

Signed responses #621

Signed responses #621

Conversation

DavidM-D commented May 30, 2024 • 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

volovyks left a 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

Choose a reason for hiding this comment

volovyks left a comment

Choose a reason for hiding this comment

github-actions bot commented May 31, 2024

Terraform Feature Environment Destroy (dev-621)

Terraform Initialization ⚙️success

Terraform Destroy success

DavidM-D commented May 30, 2024 •

edited

Loading

Terraform Initialization ⚙️`success`

Terraform Destroy `success`