runtime-sdk: Add confidential store #639

jberci · 2021-11-12T13:34:04Z

Closes #333

codecov · 2021-11-12T13:35:25Z

Codecov Report

Merging #639 (b207472) into main (fa6849d) will decrease coverage by 0.71%.
The diff coverage is 68.29%.

❗ Current head b207472 differs from pull request most recent head 6801b89. Consider uploading reports for the commit 6801b89 to get more accurate results

@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
- Coverage   72.20%   71.48%   -0.72%     
==========================================
  Files         111      112       +1     
  Lines        8214     8421     +207     
==========================================
+ Hits         5931     6020      +89     
- Misses       2260     2378     +118     
  Partials       23       23

Impacted Files	Coverage Δ
...ime-sdk/modules/contracts/src/abi/oasis/storage.rs	`58.76% <ø> (ø)`
runtime-sdk/modules/contracts/src/lib.rs	`66.12% <ø> (-4.62%)`	⬇️
runtime-sdk/src/dispatcher.rs	`4.51% <0.00%> (-0.02%)`	⬇️
runtime-sdk/src/runtime.rs	`18.18% <0.00%> (ø)`
runtime-sdk/src/storage/mod.rs	`100.00% <ø> (ø)`
runtime-sdk/src/keymanager.rs	`18.00% <14.28%> (+18.00%)`	⬆️
runtime-sdk/src/context.rs	`86.98% <50.00%> (+1.10%)`	⬆️
runtime-sdk/src/testing/keymanager.rs	`70.00% <70.00%> (ø)`
runtime-sdk/src/storage/confidential.rs	`70.52% <70.52%> (ø)`
runtime-sdk/src/testing/mock.rs	`82.35% <75.00%> (-8.28%)`	⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa6849d...6801b89. Read the comment docs.

runtime-sdk/src/storage/confidential.rs

kostko

Would also be great to have some benchmarks (using cargo bench) to show overhead compared to plain store.

runtime-sdk/src/storage/confidential.rs

runtime-sdk/src/storage/mod.rs

runtime-sdk/modules/contracts/src/store.rs

kostko

It would be nicer if the key manager was a trait and the mocked version would be completely separate.

runtime-sdk/modules/contracts/src/store.rs

kostko · 2022-01-19T14:46:27Z

runtime-sdk/src/lib.rs

@@ -1,4 +1,5 @@
 //! Oasis runtime SDK.
+#![feature(test)]


Why is this needed?

runtime-sdk/modules/contracts/src/store.rs

runtime-sdk/src/storage/confidential.rs

runtime-sdk/modules/contracts/src/store.rs

runtime-sdk/src/storage/confidential.rs

kostko

Looks good, some minor comments.

Can you also add unit tests where the underlying storage is corrupted to make sure that the right thing happens. E.g. store something using the confidential store and then corrupt it and try to perform a get.

Also could you add a unit test for the iterator?

runtime-sdk/modules/contracts/src/store.rs

runtime-sdk/src/storage/confidential.rs

kostko · 2022-02-12T08:22:35Z

runtime-sdk/modules/contracts/src/store.rs


 /// Create a contract instance store.
+///
+/// Confidential stores are only available in transaction contexts.


What happens outside a transaction context?

Yawning · 2022-02-15T12:44:32Z

runtime-sdk/src/storage/confidential.rs

+        let actual_key = Zeroizing::new(key);
+
+        // Derive a nonce key for nonces used to encrypt storage keys in the store:
+        // nonce_key = KDF(key)


Hm, HMAC as a KDF isn't amazing, but it's probably ok? This is only used for nonce derivation right?

Yawning · 2022-02-15T12:48:05Z

runtime-sdk/src/storage/confidential.rs

+        (nonce, key)
+    }
+
+    fn make_value(&mut self, plain_value: &[u8]) -> (Nonce, Vec<u8>) {


This seems kind of overkill to mask "the code accessed the same key", but I can't think of a nice way to handle this off the top of my head.

kostko reviewed Nov 15, 2021

View reviewed changes

runtime-sdk/src/storage/confidential.rs Outdated Show resolved Hide resolved

runtime-sdk/src/storage/confidential.rs Outdated Show resolved Hide resolved

runtime-sdk/src/storage/confidential.rs Outdated Show resolved Hide resolved

jberci force-pushed the jberci/feature/confidential-store branch from ffd510e to 5d6c7d1 Compare November 30, 2021 20:56

kostko reviewed Dec 6, 2021

View reviewed changes

runtime-sdk/src/storage/confidential.rs Outdated Show resolved Hide resolved

runtime-sdk/src/storage/mod.rs Outdated Show resolved Hide resolved

runtime-sdk/modules/contracts/src/store.rs Outdated Show resolved Hide resolved

jberci force-pushed the jberci/feature/confidential-store branch from 5d6c7d1 to b765818 Compare December 10, 2021 22:22

kostko reviewed Dec 21, 2021

View reviewed changes

runtime-sdk/modules/contracts/src/store.rs Show resolved Hide resolved

jberci force-pushed the jberci/feature/confidential-store branch from b765818 to b22998f Compare January 18, 2022 16:40

jberci force-pushed the jberci/feature/confidential-store branch 3 times, most recently from 0afa0c2 to eace426 Compare January 31, 2022 16:18

kostko reviewed Feb 1, 2022

View reviewed changes

jberci force-pushed the jberci/feature/confidential-store branch from eace426 to a3e9f23 Compare February 2, 2022 17:36

kostko reviewed Feb 3, 2022

View reviewed changes

runtime-sdk/modules/contracts/src/store.rs Outdated Show resolved Hide resolved

runtime-sdk/src/storage/confidential.rs Outdated Show resolved Hide resolved

jberci force-pushed the jberci/feature/confidential-store branch 2 times, most recently from 6b7a0ce to b207472 Compare February 8, 2022 02:27

kostko reviewed Feb 9, 2022

View reviewed changes

runtime-sdk/modules/contracts/src/store.rs Outdated Show resolved Hide resolved

runtime-sdk/modules/contracts/src/store.rs Outdated Show resolved Hide resolved

jberci force-pushed the jberci/feature/confidential-store branch 3 times, most recently from ede1609 to e05bd03 Compare February 9, 2022 23:15

kostko reviewed Feb 10, 2022

View reviewed changes

jberci force-pushed the jberci/feature/confidential-store branch 2 times, most recently from c91a20b to 597717b Compare February 10, 2022 13:56

jberci marked this pull request as ready for review February 10, 2022 17:31

jberci requested review from pro-wh and ptrus as code owners February 10, 2022 17:31

kostko reviewed Feb 11, 2022

View reviewed changes

jberci force-pushed the jberci/feature/confidential-store branch 2 times, most recently from 61696bf to 485fdec Compare February 11, 2022 20:10

kostko approved these changes Feb 12, 2022

View reviewed changes

jberci force-pushed the jberci/feature/confidential-store branch from 485fdec to c6a82e8 Compare February 14, 2022 16:40

kostko approved these changes Feb 14, 2022

View reviewed changes

Yawning reviewed Feb 15, 2022

View reviewed changes

runtime-sdk: Add confidential store

6801b89

jberci force-pushed the jberci/feature/confidential-store branch from c6a82e8 to 6801b89 Compare February 15, 2022 16:06

jberci enabled auto-merge February 15, 2022 16:21

jberci merged commit 54d3685 into main Feb 15, 2022

jberci deleted the jberci/feature/confidential-store branch February 15, 2022 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

runtime-sdk: Add confidential store #639

runtime-sdk: Add confidential store #639

jberci commented Nov 12, 2021

codecov bot commented Nov 12, 2021 •

edited

Loading

kostko left a comment

kostko left a comment

kostko Jan 19, 2022

kostko left a comment

kostko Feb 12, 2022

Yawning Feb 15, 2022

Yawning Feb 15, 2022

runtime-sdk: Add confidential store #639

runtime-sdk: Add confidential store #639

Conversation

jberci commented Nov 12, 2021

codecov bot commented Nov 12, 2021 • edited Loading

Codecov Report

kostko left a comment

Choose a reason for hiding this comment

kostko left a comment

Choose a reason for hiding this comment

kostko Jan 19, 2022

Choose a reason for hiding this comment

kostko left a comment

Choose a reason for hiding this comment

kostko Feb 12, 2022

Choose a reason for hiding this comment

Yawning Feb 15, 2022

Choose a reason for hiding this comment

Yawning Feb 15, 2022

Choose a reason for hiding this comment

codecov bot commented Nov 12, 2021 •

edited

Loading