[crypto & al] Direct signing/verification for values #4846

huitseeker · 2020-06-30T17:46:21Z

What this does

We used to do:

let hash = msg.hash()
private_key.sign_message(hash)
signature.verify_msg(hash, public_key)
public_key.verify_msg(hash, signature)

Now we do:

private_key.sign(&message)
signature.verify(&message, public_key)
public_key.verify(&message, signature)

Throughout the code base.

Why was it this way?

Long story short: domain separation & weaknesses in serialization engine. We bypassed serialization (which was ambiguous, as of a while ago) and had a very specific way of constructing a hash value, which was sha3(sha3(domain_separation_bytes) || serialized_object_bytes), and then you signed that HashValue. See libra_crypto::hash::CryptoHasher for the domain separation details.

What are the signed bytes now?

We automatically derive them for you, but they're sha3(domain_separation_bytes) || serialized_object_bytes.

So signing is handled for existing `struct`s. What do I do if I want to define `MyFancyStruct` ?

use libra_crypto_derive::{CryptoHasher, LCSCryptoHash}

#[derive(Serialize, Deserialize, CryptoHasher, LCSCryptoHash)]
pub struct MyCryptoSignableCustomThingy {
...
}

priv_key.sign(my_crypto_signable_custom_thingy)

Is this in specs?

It's upcoming. The older signing/verification isn't there yet.

Why is this better?

It's simpler and more secure (you might pass the wrong hash in code, it's even a crypto gotcha).

Why is this worse?

Serialization is now on the critical path for signing / verification operations, which are exposed as infallible. Serialization can fail in some edge cases, so now they can panic. There's no clean way of recovering from this sort situation, but we are committed to and have a plan to push serialization failures into a never-visited corner (see e.g. #4928).

What's left?

See above re:spec. It's my very next item.

bors-libra · 2020-07-01T21:49:07Z

☔ The latest upstream changes (presumably 7aa3450) made this pull request unmergeable. Please resolve the merge conflicts.

ma2bd

Just random questions for my own understanding and a few minor comments.

testsuite/generate-format/src/consensus.rs

testsuite/generate-format/src/libra.rs

crypto/crypto/src/unit_tests/ed25519_test.rs

secure/storage/src/vault.rs

The strategy that underpins this is: - armoring serialization, as in diem#4250 - acknowledging signing is a bad place to recover from a serialization error.

ma2bd

The new API is very nice. Thanks for the changes!

ma2bd · 2020-07-07T14:21:48Z

crypto/crypto/src/traits.rs

-    ) -> Result<Self::SignatureMaterial, CryptoMaterialError>;
+    /// Note: this assumes serialization is unfaillible. See libra_common::lcs::ser
+    /// for a discussion of this assumption.
+    fn sign<T: CryptoHash + Serialize>(&self, message: &T) -> Self::SignatureMaterial;


ankushagarwal · 2020-07-07T14:36:42Z

@huitseeker Can you update the swiss-knife README.md before you land?

client/swiss-knife/README.md

This uses the new functionality to sign/verify any struct that implements libra_crypto::hash::CryptoHash + serde::ser::Serialize directly. Note that it makes the semantics of e.g. client/swiss-knife ambiguous, as the deserialization of a payload into an unknown type offered by this API is impossible: the new APis need to know (and use) the type to figure out the prefix that should be applied in signing that struct. `#[allow(deprecated)]` has been introduced in every place the old functionality needed to be maintained.

We want to reduce the uncertainty on how to sign/verify in Libra=> let's not leave those around.

- verify_struct_msg -> verify - batch_verify_struct_signatures -> batch_verify - batch_verify_aggregated_struct_signature -> batch_verify_aggregated_signatures

huitseeker · 2020-07-07T17:44:10Z

@bors-libra r=ankushagarwal

bors-libra · 2020-07-07T17:44:11Z

📌 Commit bd6c45d has been approved by ankushagarwal

bors-libra · 2020-07-07T17:44:22Z

⌛ Testing commit bd6c45d with merge 13290df...

Closes: #4846 Approved by: ankushagarwal

This uses the new functionality to sign/verify any struct that implements libra_crypto::hash::CryptoHash + serde::ser::Serialize directly. Note that it makes the semantics of e.g. client/swiss-knife ambiguous, as the deserialization of a payload into an unknown type offered by this API is impossible: the new APis need to know (and use) the type to figure out the prefix that should be applied in signing that struct. `#[allow(deprecated)]` has been introduced in every place the old functionality needed to be maintained. Closes: #4846 Approved by: ankushagarwal

We want to reduce the uncertainty on how to sign/verify in Libra=> let's not leave those around. Closes: #4846 Approved by: ankushagarwal

Closes: #4846 Approved by: ankushagarwal

- verify_struct_msg -> verify - batch_verify_struct_signatures -> batch_verify - batch_verify_aggregated_struct_signature -> batch_verify_aggregated_signatures Closes: #4846 Approved by: ankushagarwal

Closes: #4846 Approved by: ankushagarwal

github-actions · 2020-07-07T18:03:53Z

Cluster Test Result

all up : 1073 TPS, 4215 ms latency, 5100 ms p99 latency, no expired txns

Repro cmd:

./scripts/cti --tag land_13290dfe --run bench

bors-libra · 2020-07-07T18:05:56Z

☀️ Test successful - checks-actions_land_blocking_test, checks-circle_commit_workflow
Approved by: ankushagarwal
Pushing 13290df to master...

diem-cla-bot bot added the cla-signed label Jun 30, 2020

huitseeker requested review from davidiw, kchalkias and ma2bd June 30, 2020 17:46

huitseeker force-pushed the sign_stuff branch 3 times, most recently from d98cbbc to c433e8b Compare June 30, 2020 22:48

huitseeker force-pushed the sign_stuff branch 2 times, most recently from 274cf65 to 2d47094 Compare July 2, 2020 16:48

ma2bd reviewed Jul 2, 2020

View reviewed changes

testsuite/generate-format/src/consensus.rs Show resolved Hide resolved

testsuite/generate-format/src/libra.rs Outdated Show resolved Hide resolved

crypto/crypto/src/unit_tests/ed25519_test.rs Show resolved Hide resolved

secure/storage/src/vault.rs Outdated Show resolved Hide resolved

huitseeker force-pushed the sign_stuff branch 6 times, most recently from 1f5bb66 to 1a06510 Compare July 6, 2020 23:42

huitseeker requested review from ankushagarwal and thefallentree July 7, 2020 00:00

huitseeker added 2 commits July 6, 2020 20:03

[crypto] Make signing unfaillible

dfc27a4

The strategy that underpins this is: - armoring serialization, as in diem#4250 - acknowledging signing is a bad place to recover from a serialization error.

[crypto] Deprecate the sign_message, verify, HashValue-based APIs

336c5e3

huitseeker force-pushed the sign_stuff branch from e0e23c8 to a4a3f72 Compare July 7, 2020 00:20

huitseeker changed the title ~~[WIP][crypto] Towards generic signing/verification APIs~~ [crypto & al] Direct signing/verification for values Jul 7, 2020

huitseeker marked this pull request as ready for review July 7, 2020 00:48

ma2bd approved these changes Jul 7, 2020

View reviewed changes

ankushagarwal reviewed Jul 7, 2020

View reviewed changes

client/swiss-knife/README.md Outdated Show resolved Hide resolved

ankushagarwal reviewed Jul 7, 2020

View reviewed changes

client/swiss-knife/README.md Show resolved Hide resolved

ankushagarwal approved these changes Jul 7, 2020

View reviewed changes

huitseeker force-pushed the sign_stuff branch from 7c4a7c0 to 3fd35b0 Compare July 7, 2020 17:13

huitseeker force-pushed the sign_stuff branch from 3fd35b0 to 38eca64 Compare July 7, 2020 17:15

huitseeker added 6 commits July 7, 2020 13:15

Remove unused deprecated methods

ee5f77c

We want to reduce the uncertainty on how to sign/verify in Libra=> let's not leave those around.

Adapt client/swiss-knife to new signing & verification API

f6c4a3d

[crypto] Remove obsolete crypto methods

ca374da

[crypto] Renames

b3da0a3

- verify_struct_msg -> verify - batch_verify_struct_signatures -> batch_verify - batch_verify_aggregated_struct_signature -> batch_verify_aggregated_signatures

[client] Rewrite obsolete comment

bd6c45d

huitseeker force-pushed the sign_stuff branch from 38eca64 to bd6c45d Compare July 7, 2020 17:15

bors-libra pushed a commit that referenced this pull request Jul 7, 2020

[crypto] Deprecate the sign_message, verify, HashValue-based APIs

ea1489b

Closes: #4846 Approved by: ankushagarwal

bors-libra pushed a commit that referenced this pull request Jul 7, 2020

Remove unused deprecated methods

32cbcc7

We want to reduce the uncertainty on how to sign/verify in Libra=> let's not leave those around. Closes: #4846 Approved by: ankushagarwal

bors-libra pushed a commit that referenced this pull request Jul 7, 2020

Adapt client/swiss-knife to new signing & verification API

a70f718

Closes: #4846 Approved by: ankushagarwal

bors-libra pushed a commit that referenced this pull request Jul 7, 2020

[crypto] Remove obsolete crypto methods

74535fe

Closes: #4846 Approved by: ankushagarwal

bors-libra pushed a commit that referenced this pull request Jul 7, 2020

[client] Rewrite obsolete comment

13290df

Closes: #4846 Approved by: ankushagarwal

bors-libra closed this in bbdad54 Jul 7, 2020

huitseeker added the breaking Any changes that will require restarting the testnet label Jul 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[crypto & al] Direct signing/verification for values #4846

[crypto & al] Direct signing/verification for values #4846

huitseeker commented Jun 30, 2020 •

edited

Loading

bors-libra commented Jul 1, 2020

ma2bd left a comment

ma2bd left a comment

ma2bd Jul 7, 2020

ankushagarwal commented Jul 7, 2020

huitseeker commented Jul 7, 2020

bors-libra commented Jul 7, 2020

bors-libra commented Jul 7, 2020

github-actions bot commented Jul 7, 2020

bors-libra commented Jul 7, 2020

[crypto & al] Direct signing/verification for values #4846

[crypto & al] Direct signing/verification for values #4846

Conversation

huitseeker commented Jun 30, 2020 • edited Loading

What this does

Why was it this way?

What are the signed bytes now?

So signing is handled for existing structs. What do I do if I want to define MyFancyStruct ?

Is this in specs?

Why is this better?

Why is this worse?

What's left?

bors-libra commented Jul 1, 2020

ma2bd left a comment

Choose a reason for hiding this comment

ma2bd left a comment

Choose a reason for hiding this comment

ma2bd Jul 7, 2020

Choose a reason for hiding this comment

ankushagarwal commented Jul 7, 2020

huitseeker commented Jul 7, 2020

bors-libra commented Jul 7, 2020

bors-libra commented Jul 7, 2020

github-actions bot commented Jul 7, 2020

bors-libra commented Jul 7, 2020

huitseeker commented Jun 30, 2020 •

edited

Loading

So signing is handled for existing `struct`s. What do I do if I want to define `MyFancyStruct` ?