Problem (WIP #1616): keypackage not verified in nodejointx / council node data #1668

yihuang · 2020-05-26T12:07:17Z

Solution:

record most recent isv_svn, warn when a new version appears
add mock data and fix unit/integration tests temporarily.

TBD: generate real keypackage when construct node-join tx, or prepare genesis.

codecov · 2020-05-26T16:54:51Z

Codecov Report

Merging #1668 into master will increase coverage by 4.62%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1668      +/-   ##
==========================================
+ Coverage   67.23%   71.86%   +4.62%     
==========================================
  Files         191      184       -7     
  Lines       23516    21908    -1608     
==========================================
- Hits        15811    15744      -67     
+ Misses       7705     6164    -1541

Impacted Files	Coverage Δ
...in-tx-enclave-next/enclave-ra/ra-client/src/lib.rs	`100.00% <ø> (ø)`
chain-tx-enclave-next/mls/src/lib.rs	`100.00% <ø> (ø)`
client-rpc/src/rpc/staking_rpc.rs	`0.00% <0.00%> (ø)`
dev-utils/src/commands/init_command.rs	`0.00% <0.00%> (ø)`
chain-core/src/init/config.rs	`78.89% <58.97%> (+0.80%)`	⬆️
chain-abci/src/app/app_init.rs	`81.96% <76.92%> (-0.40%)`	⬇️
chain-abci/src/staking/tx.rs	`90.98% <80.00%> (+0.30%)`	⬆️
...-enclave-next/enclave-ra/ra-client/src/verifier.rs	`44.20% <83.33%> (-1.19%)`	⬇️
test-common/src/block_generator.rs	`88.60% <88.88%> (+0.25%)`	⬆️
chain-abci/src/app/mod.rs	`83.92% <100.00%> (+0.07%)`	⬆️
... and 27 more

tomtau · 2020-05-27T01:24:17Z

chain-abci/src/staking/tx.rs

+        mut recent_isv_svn: u16,
        tx: &NodeJoinRequestTx,
-    ) -> Result<(), PublicTxError> {
+    ) -> Result<u16, PublicTxError> {


why it needs it mut when it also returns it?

It'll get assigned if info.quote.report_body.isv_svn > recent_isv_svn, and returned at the end.

how about
recent_isv_svn:&mut u16 ?
for readibility

changing recent_isv_svn inside this function can be error-prone,
can be refactored using tutple?

Used a new variable to return now, it would be less confusing.

tomtau · 2020-05-27T01:36:06Z

chain-tx-enclave-next/enclave-ra/ra-client/src/config.rs

@@ -19,7 +19,8 @@ impl<'a> EnclaveCertVerifierConfig<'a> {
    pub fn new() -> Self {
        Self {
            signing_ca_cert_pem: IAS_CERT.into(),
-            valid_enclave_quote_statuses: vec!["OK".into()].into(),
+            // FIXME remove SW_HARDENING_NEEDED after https://github.com/rust-lang/llvm-project/pull/58


I think SW_HARDENING_NEEDED will still appear as a response, so it may not be possible to remove it ("OK" or "UP-TO-DATE" will only appear on unaffected platforms which are only some very recent CPUs, i.e. mid-2019 and newer) -- it's more of a reminder that one should apply those software hardening measures: https://software.intel.com/security-software-guidance/insights/deep-dive-load-value-injection#mitigationguidelines

Since it affects many platforms, applying the mitigations and releasing only one version with them for all platforms is ok.

Oh, then OK, UP-TO-DATE, SW_HARDENING_NEEDED all should be considered as valid responses?

The API docs (https://api.trustedservices.intel.com/documents/sgx-attestation-api-spec.pdf) don't mention "UP-TO-DATE" (it's only mentioned in that LVI mitigation article), so perhaps only "OK" and "SW_HARDENING_NEEDED"

leejw51crypto · 2020-05-27T02:11:29Z

chain-abci/src/staking/tx.rs

@@ -24,9 +24,9 @@ impl StakingTable {
        heap: &mut impl StoreStaking,
        block_time: Timespec,
        max_evidence_age: Timespec,
-        ra_verifier: &EnclaveCertVerifier,
+        mut recent_isv_svn: u16,


some full-word comment about "isv_svn" would be useful,
or consider full word variable

added a little bit more comment here about isv svn:
https://github.com/crypto-com/chain/pull/1668/files#diff-3cfbc20473df3695f7d44c7c3d8c1508R57

leejw51crypto · 2020-05-27T02:12:47Z

chain-abci/src/storage/mod.rs

+    NodeJoin {
+        address: StakedStateAddress,
+        council_node: CouncilNode,
+        /// most recent isv_svn


this remark for documentation?
// ?

leejw51crypto

lgtm

tomtau · 2020-05-27T02:51:10Z

chain-core/src/init/config.rs

@@ -2,85 +2,61 @@ use crate::common::Timespec;
 use crate::init::address::RedeemAddress;


just one note that genesis processing may need more thought / design: crypto-org-chain/chain-docs#141 (comment)

tomtau · 2020-05-27T03:04:22Z

bors r+

1668: Problem (WIP #1616): keypackage not verified in nodejointx / council node data r=tomtau a=yihuang Solution: - record most recent isv_svn, warn when a new version appears - add mock data and fix unit/integration tests temporarily. TBD: generate real keypackage when construct node-join tx, or prepare genesis. Co-authored-by: yihuang <[email protected]>

bors · 2020-05-27T03:52:44Z

Build failed:

continuous-integration/travis-ci/push

tomtau

check-docker-app-hash.sh related stuff + fuzzer may also need updating?

tomtau · 2020-05-27T04:51:20Z

bors r+

1668: Problem (WIP #1616): keypackage not verified in nodejointx / council node data r=tomtau a=yihuang Solution: - record most recent isv_svn, warn when a new version appears - add mock data and fix unit/integration tests temporarily. TBD: generate real keypackage when construct node-join tx, or prepare genesis. Co-authored-by: yihuang <[email protected]>

bors · 2020-05-27T04:53:10Z

Canceled.

tomtau · 2020-05-27T06:34:24Z

chain-abci/fuzz/Cargo.lock

@@ -0,0 +1,3908 @@
+# This file is automatically @generated by Cargo.


needed? or could this go to .gitignore?

I think general guideline is like libraries should ignore Cargo.lock but binaries/applications should check-in Cargo.lock, I guess fuzz can be considered as application?

Maybe we can put the fuzz crate in workspace, it'll share the same Cargo.lock file?

EDIT: oh, fuzz itself is a workspace.

Removed the lock file.

tomtau · 2020-05-27T06:37:47Z

A few clippy suggestions: https://travis-ci.org/github/crypto-com/chain/jobs/691601119#L1686

yihuang · 2020-05-27T07:14:06Z

A few clippy suggestions: https://travis-ci.org/github/crypto-com/chain/jobs/691601119#L1686

Done

…/ council node data Solution: - record most recent isv_svn in chain state, warn when a new version appears - use mock keypackage and make unit/integration tests running tempararily

tomtau · 2020-05-27T07:40:44Z

bors r+

bors · 2020-05-27T08:25:39Z

Build succeeded:

yihuang requested review from calvinaco, devashishdxt and tomtau as code owners May 26, 2020 12:07

yihuang force-pushed the issue1616 branch 6 times, most recently from c96f316 to cfca8dc Compare May 26, 2020 16:27

tomtau requested review from linfeng-crypto and leejw51crypto May 27, 2020 01:00

tomtau reviewed May 27, 2020

View reviewed changes

leejw51crypto reviewed May 27, 2020

View reviewed changes

leejw51crypto approved these changes May 27, 2020

View reviewed changes

tomtau reviewed May 27, 2020

View reviewed changes

yihuang force-pushed the issue1616 branch 2 times, most recently from 70c427e to 44d7a67 Compare May 27, 2020 03:01

tomtau self-requested a review May 27, 2020 03:58

tomtau suggested changes May 27, 2020

View reviewed changes

yihuang force-pushed the issue1616 branch 2 times, most recently from 9e7c685 to 5f1020b Compare May 27, 2020 04:40

yihuang force-pushed the issue1616 branch from 5f1020b to 5865db7 Compare May 27, 2020 04:53

yihuang force-pushed the issue1616 branch from 5865db7 to a199fbf Compare May 27, 2020 04:56

tomtau reviewed May 27, 2020

View reviewed changes

yihuang force-pushed the issue1616 branch from a199fbf to 6981ae5 Compare May 27, 2020 07:14

yihuang requested a review from tomtau May 27, 2020 07:14

Problem (WIP crypto-com#1616): keypackage not verified in nodejointx …

dcbc252

…/ council node data Solution: - record most recent isv_svn in chain state, warn when a new version appears - use mock keypackage and make unit/integration tests running tempararily

yihuang force-pushed the issue1616 branch from 6981ae5 to dcbc252 Compare May 27, 2020 07:18

bors bot merged commit 9018951 into crypto-com:master May 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Problem (WIP #1616): keypackage not verified in nodejointx / council node data #1668

Problem (WIP #1616): keypackage not verified in nodejointx / council node data #1668

yihuang commented May 26, 2020 •

edited

Loading

codecov bot commented May 26, 2020 •

edited

Loading

tomtau May 27, 2020

yihuang May 27, 2020

leejw51crypto May 27, 2020 •

edited

Loading

yihuang May 27, 2020

tomtau May 27, 2020

yihuang May 27, 2020

tomtau May 27, 2020

yihuang May 27, 2020

leejw51crypto May 27, 2020 •

edited

Loading

yihuang May 27, 2020

leejw51crypto May 27, 2020

yihuang May 27, 2020

leejw51crypto left a comment

tomtau May 27, 2020

tomtau commented May 27, 2020

bors bot commented May 27, 2020

tomtau left a comment

tomtau commented May 27, 2020

bors bot commented May 27, 2020

tomtau May 27, 2020

yihuang May 27, 2020

yihuang May 27, 2020 •

edited

Loading

yihuang May 27, 2020

tomtau commented May 27, 2020

yihuang commented May 27, 2020

tomtau commented May 27, 2020

bors bot commented May 27, 2020

		@@ -2,85 +2,61 @@ use crate::common::Timespec;
		use crate::init::address::RedeemAddress;

		@@ -0,0 +1,3908 @@
		# This file is automatically @generated by Cargo.

Problem (WIP #1616): keypackage not verified in nodejointx / council node data #1668

Problem (WIP #1616): keypackage not verified in nodejointx / council node data #1668

Conversation

yihuang commented May 26, 2020 • edited Loading

codecov bot commented May 26, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leejw51crypto May 27, 2020 • 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

leejw51crypto May 27, 2020 • 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

leejw51crypto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau commented May 27, 2020

bors bot commented May 27, 2020

tomtau left a comment

Choose a reason for hiding this comment

tomtau commented May 27, 2020

bors bot commented May 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang May 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomtau commented May 27, 2020

yihuang commented May 27, 2020

tomtau commented May 27, 2020

bors bot commented May 27, 2020

yihuang commented May 26, 2020 •

edited

Loading

codecov bot commented May 26, 2020 •

edited

Loading

leejw51crypto May 27, 2020 •

edited

Loading

leejw51crypto May 27, 2020 •

edited

Loading

yihuang May 27, 2020 •

edited

Loading