Problem: no gen keypackage command in dev-utils (fix #1692) #1738

leejw51crypto · 2020-06-08T07:04:26Z

added keyspace generation in dev-utils.

yihuang · 2020-06-08T07:44:05Z

chain-tx-enclave-next/mls/src/main.rs

+        certificate_validity_secs: 86400,
+    });
+    if config.is_err() {
+        println!("cannot connect ra-sp-server, run ra-sp-server beforehand e.g.) ra-sp-server --quote-type Unlinkable --ias-key $IAS_API_KEY --spid $SPID")


print out the error itself?

to notify keypackage failure just when user create it,
otherwise, it's very hard for user to know which step is wrong

yihuang · 2020-06-08T07:46:11Z

chain-tx-enclave-next/mls/src/main.rs

+        .keypackage
+        .verify(&*ra_client::ENCLAVE_CERT_VERIFIER, now);
+    if let Err(value) = verication_result {
+        println!("verification_fail {}", value);


Write the error into the stderr, and exit with a non zero code.

codecov · 2020-06-08T07:52:30Z

Codecov Report

Merging #1738 into master will decrease coverage by 0.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1738      +/-   ##
==========================================
- Coverage   65.71%   65.66%   -0.05%     
==========================================
  Files         204      205       +1     
  Lines       25294    25310      +16     
==========================================
- Hits        16622    16621       -1     
- Misses       8672     8689      +17

Impacted Files	Coverage Δ
...-enclave-next/enclave-ra/ra-client/src/verifier.rs	`43.24% <ø> (ø)`
chain-tx-enclave-next/mls/src/main.rs	`0.00% <ø> (ø)`
client-cli/src/command/transaction_command.rs	`0.00% <0.00%> (ø)`
client-common/src/lib.rs	`50.00% <ø> (ø)`
client-rpc/src/rpc/staking_rpc.rs	`0.00% <ø> (ø)`
dev-utils/src/commands/genesis_command.rs	`2.59% <ø> (ø)`
dev-utils/src/commands/init_command.rs	`0.00% <ø> (ø)`
dev-utils/src/commands/keypackage_command.rs	`0.00% <0.00%> (ø)`
dev-utils/src/dev_utils.rs	`0.00% <0.00%> (ø)`
dev-utils/src/keypackage.rs	`0.00% <0.00%> (ø)`
... and 6 more

tomtau

client-* can be made simple by not including the development-only utilities, like gen_package -- it can just accept the generated payload?

tomtau · 2020-06-09T02:06:31Z

dev-utils/src/commands/genesis_command.rs

@@ -22,8 +22,9 @@ use client_common::tendermint::types::{Genesis, Time};
 use client_common::{ErrorKind, Result, ResultExt};

 use crate::commands::genesis_dev_config::GenesisDevConfig;
+use client_common::gen_keypackage;


perhaps no need to have that function in client_common, it can be moved to dev-utils?

ok,
i'll also remove it from client-cli, because no longer necessary.
user can use dev-utils to create key-package.

working on now

gen-package is being used in client-rpc, so i will not remove it.
only remove in client-cli

why is it used in client-rpc?

yes, it's used in integration test.
can be replaced with dev-utils

ok to be replaced then in the integration test?

yes, i'll change integration test,
and remove from client-rpc.

leejw51crypto · 2020-06-10T07:20:14Z

removed gen-keypackage in client-rpc,
now modifying integration test
fixed integration test

leejw51crypto · 2020-06-10T16:31:23Z

./dev-utils keypackage generate
./dev-utils keypackage verify

added another category in dev-utils

tomtau

one small fmt issue: https://travis-ci.org/github/crypto-com/chain/jobs/696918322#L1408

tomtau · 2020-06-11T02:29:09Z

bors try

bors · 2020-06-11T03:13:38Z

try

Build failed:

continuous-integration/travis-ci/push

leejw51crypto · 2020-06-11T03:28:24Z

i'm checking travis issue

tomtau · 2020-06-11T03:43:50Z

@leejw51crypto travis issue should be fixable with cargo fmt?

leejw51crypto · 2020-06-11T03:44:20Z

yes, i'm checking again

tomtau · 2020-06-11T06:21:04Z

bors try

bors · 2020-06-11T08:45:39Z

try

Timed out.

tomtau · 2020-06-12T01:52:09Z

.drone.yml

@@ -59,7 +59,7 @@ steps:
  commands:
  - export CARGO_HOME=$PWD/drone/cargo
  - export CARGO_TARGET_DIR=$PWD/drone/target
-  - export PATH=$CARGO_HOME/bin:$PATH
+  - export PATH=$CARGO_HOME/bin:$CARGO_TARGET_DIR/debug:$PATH


Is this necessary?

yes, because it need to call 'dev-utils', without it, cannot call 'dev-utils'

The run_multinode.sh will add the target dir into PATH, it'll also take BUILD_PROFILE into account.

but, when i run drone local, PATH didn't work.
also this PATH is static, i think it's simpler just to include inside .drone.yml,
not inside runtime bash script.
this is for simplicity.

if it's located to other script, hard to catch bug when runtime error occurs,
because you don't know exact PATH env status just like in checking time.

leejw51crypto · 2020-06-12T03:25:13Z

updated hmac

tomtau · 2020-06-12T03:29:30Z

bors r+

bors · 2020-06-12T04:36:56Z

Merge conflict.

yihuang · 2020-06-12T04:52:44Z

client-cli/src/command/transaction_command.rs

-                } else {
-                    break kp;
-                }
+                /* TODO : use dev-utils to verify*/


why not verify by calling the function?

because mls, ra-client moved to dev-utils, cannot call verification directly.
also client-core will be compiled via WASM

i'm adding verification now

yihuang · 2020-06-12T04:54:26Z

client-rpc/src/rpc/staking_rpc.rs

@@ -479,9 +468,6 @@ fn get_node_metadata(
        .err_kind(ErrorKind::InvalidInput, || "invalid base64")
        .map_err(to_rpc_error)?;

-    #[cfg(not(feature = "mock-enclave"))]
-    verify_keypackage(&keypackage).map_err(to_rpc_error)?;
-


Why remove?

yes, to remove dependency of mls,ra-client in client-core

But we should verify this in client.

it's supported in dev-utils
dev-utils keypackage verify
also i added verification in generation process
so if sgx remote attestation fails, generation itself stops

because it is not necessary to continue with failed remote attestation result.
it is much easier just to notify to user in generating keypackage.
so when remote attestation fails, user will resolve by configuration in bios or reinstall sgx-driver, and aesm_service.

so commands are
dev-utils keypackage generate <- generate keypacakge via ra-sp-server
dev-utils keypackage verify <- verify

Solution: add gen-keypackage tidy up more detail in message tidy up print error in stderr remove gen-keypackage in client-cli fix integration test add path for dev-utils tidy up update hmac

leejw51crypto · 2020-06-12T05:54:34Z

rebased for merge conflict

tomtau · 2020-06-12T06:30:00Z

bors retry

bors · 2020-06-12T07:01:55Z

Build succeeded:

leejw51crypto requested review from calvinaco, devashishdxt and tomtau as code owners June 8, 2020 07:04

tomtau requested review from yihuang and linfeng-crypto June 8, 2020 07:04

leejw51 force-pushed the cro-1692 branch from 1fd949f to 2cbc9a4 Compare June 8, 2020 07:08

yihuang reviewed Jun 8, 2020

View reviewed changes

leejw51 force-pushed the cro-1692 branch 2 times, most recently from eaa675b to aacd2b1 Compare June 8, 2020 15:25

tomtau suggested changes Jun 9, 2020

View reviewed changes

leejw51crypto force-pushed the cro-1692 branch from aacd2b1 to 88264a8 Compare June 9, 2020 09:47

leejw51 force-pushed the cro-1692 branch from 88264a8 to d0d66a6 Compare June 9, 2020 10:15

leejw51crypto force-pushed the cro-1692 branch from d0d66a6 to e3153f9 Compare June 9, 2020 13:49

leejw51 force-pushed the cro-1692 branch from e3153f9 to 34e191f Compare June 10, 2020 06:13

leejw51 force-pushed the cro-1692 branch from 34e191f to 1a2ce2c Compare June 10, 2020 16:28

tomtau self-requested a review June 11, 2020 02:00

tomtau suggested changes Jun 11, 2020

View reviewed changes

leejw51 force-pushed the cro-1692 branch from 1a2ce2c to 9b8c719 Compare June 11, 2020 02:05

bors bot added a commit that referenced this pull request Jun 11, 2020

Try #1738:

54af898

leejw51 force-pushed the cro-1692 branch from 9b8c719 to dad05da Compare June 11, 2020 03:45

leejw51crypto force-pushed the cro-1692 branch from dad05da to 69aada3 Compare June 11, 2020 06:15

bors bot added a commit that referenced this pull request Jun 11, 2020

Try #1738:

ebdeae6

tomtau reviewed Jun 12, 2020

View reviewed changes

leejw51 force-pushed the cro-1692 branch from 69aada3 to 87a885b Compare June 12, 2020 03:24

yihuang reviewed Jun 12, 2020

View reviewed changes

Problem: no gen keypackage command in dev-utils (fix crypto-com#1692)

55b0f13

Solution: add gen-keypackage tidy up more detail in message tidy up print error in stderr remove gen-keypackage in client-cli fix integration test add path for dev-utils tidy up update hmac

leejw51 force-pushed the cro-1692 branch from 87a885b to 55b0f13 Compare June 12, 2020 05:53

bors bot merged commit b17886c into crypto-com:master Jun 12, 2020

Problem: no gen keypackage command in dev-utils (fix #1692) #1738

Problem: no gen keypackage command in dev-utils (fix #1692) #1738

Conversation

leejw51crypto commented Jun 8, 2020

Choose a reason for hiding this comment

leejw51crypto Jun 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto Jun 8, 2020 • edited

Choose a reason for hiding this comment

codecov bot commented Jun 8, 2020 • edited

Codecov Report

tomtau 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

leejw51crypto Jun 9, 2020 • edited

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

leejw51crypto Jun 10, 2020 • edited

Choose a reason for hiding this comment

leejw51crypto commented Jun 10, 2020 • edited

leejw51crypto commented Jun 10, 2020

tomtau left a comment

Choose a reason for hiding this comment

tomtau commented Jun 11, 2020

bors bot commented Jun 11, 2020

try

leejw51crypto commented Jun 11, 2020

tomtau commented Jun 11, 2020

leejw51crypto commented Jun 11, 2020

tomtau commented Jun 11, 2020

bors bot commented Jun 11, 2020

try

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto Jun 12, 2020 • edited

Choose a reason for hiding this comment

leejw51crypto commented Jun 12, 2020

tomtau commented Jun 12, 2020

bors bot commented Jun 12, 2020

yihuang Jun 12, 2020 • edited

Choose a reason for hiding this comment

leejw51crypto Jun 12, 2020 • edited

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

leejw51crypto Jun 12, 2020 • edited

Choose a reason for hiding this comment

leejw51crypto commented Jun 12, 2020

tomtau commented Jun 12, 2020

bors bot commented Jun 12, 2020

leejw51crypto Jun 8, 2020 •

edited

leejw51crypto Jun 8, 2020 •

edited

codecov bot commented Jun 8, 2020 •

edited

leejw51crypto Jun 9, 2020 •

edited

leejw51crypto Jun 10, 2020 •

edited

leejw51crypto commented Jun 10, 2020 •

edited

leejw51crypto Jun 12, 2020 •

edited

yihuang Jun 12, 2020 •

edited

leejw51crypto Jun 12, 2020 •

edited

leejw51crypto Jun 12, 2020 •

edited