Collateral handling for DCAP #1134

Niederb · 2023-01-03T09:11:26Z

This PR contains a lot of boilerplate code for handling the collateral data:

Getting the collateral data from SGX
Converting the C data structure into something more rust like
Sending the collateral to the chain via extrinsics
Next step would be something like Collateral updater component #1104
DCAP is still de-activated and the pallet is not ready yet anyway, but I think it is ready for a first review

OverOrion

Overall a nice job, I had some remarks, will check some parts more thoroughly (mostly the unsafe parts) the next time.

core-primitives/attestation-handler/src/collateral.rs

enclave-runtime/src/attestation.rs

core-primitives/enclave-api/src/remote_attestation.rs

core-primitives/node-api/metadata/src/metadata_mocks.rs

core-primitives/attestation-handler/src/attestation_handler.rs

clangenb

Thanks a lot, it looks very clean to me! I have some minor questions, but otherwise I am highly satisfied.

core-primitives/attestation-handler/src/collateral.rs

core-primitives/enclave-api/src/remote_attestation.rs

core-primitives/utils/src/hex.rs

service/src/account_funding.rs

service/src/main.rs

core-primitives/attestation-handler/src/collateral.rs

clangenb

Thanks for the fixes, looks good to me now!

service/src/main.rs

core-primitives/attestation-handler/Cargo.toml

core-primitives/attestation-handler/src/collateral.rs

OverOrion · 2023-01-23T06:57:42Z

core-primitives/attestation-handler/src/collateral.rs

+		let json = String::from_utf8_lossy(data);
+		let value: serde_json::Value = serde_json::from_str(&json).ok()?;


On the call site data is often a C terminated string, and I encountered an issue, where the terminating \0 was still there and serde failed with unexpected ending character (a simple println!() did not show it, it needed to be debug formatted).

For this reason I suggest this:

Suggested change

let json = String::from_utf8_lossy(data);

let value: serde_json::Value = serde_json::from_str(&json).ok()?;

let json = CStr::from_bytes_with_nul(data)

.expect("CStr::from_bytes_with_nul failed")

.to_str()

.expect("Must not contain invalid UTF-8 data");

let value: serde_json::Value = serde_json::from_str(&json).ok()?;

Can you explain, why it is safe to put an expect here instead of returning an error?

~~Of course~~:

from_bytes_with_nul().expect() is safe, because SgxQlQveCollateral is the Rust version of sgx_ql_qve_collateral_t, which states the following:
/** This is the data provided to the quote verifier by the verifying platform software. They are NULL terminated strings. [...] */

to_str().expect() ~~is safe, because~~ I could not find an explicit guarantee regarding the fields always being valid UTF-8 sequences. -> The second .expect() should be .ok()? instead, right?

Nice catch @clangenb, sorry for assuming too much!

Thanks for the explanation. However, I am still unsure. We must never panic in the enclave, as this might leak sensitive data. So I prefer that the function doesn't assume that it is called in the correct context. I prefer to be defensive here and also handle the error case. In general, I only allow expect, when the context is static, and it does really not allow panics. Then we can do something like: expect("does not overflow because of this check above blabla: qed")

Thank you for the clarifications, absolutely understandable, then returning the error is the way to go.

CStr::from_bytes_with_nul(..) fails if the null terminator is missing. In order to keep it more general I would suggest the following solution:

let json = String::from_utf8_lossy(data); // Remove potential C-style null terminators let json = json.trim_matches(char::from(0)); let value: serde_json::Value = serde_json::from_str(json).ok()?;

Alright, I pushed the changes.

OverOrion

LGTM, thank you!

Co-authored-by: Szilárd Parrag <szilard.parrag@gmail.com>

…l terminators

* First version that manages to register a quoting enclave * Move collateral to attestation-handler * Work on collateral handling * Use the real collateral data for quoting enclave * Make the dump-ra CLI command work * Work towards register_tcb_info * Add all the boilerplate code to register TCB info * Reduce code duplication for extrinsic encoding * Reduce code duplication for extrinsic sending * Extract method for collateral * Remove duplicated code * Fix clippy issues * Fix compilation error in teeracle * Return certificate and dcap quote * Cleanup * Switch to updated docker image * Disable DCAP for now * Register collateral only for DCAP * Register collateral only for DCAP * Cleanup * Update core-primitives/attestation-handler/src/attestation_handler.rs Co-authored-by: Szilárd Parrag <szilard.parrag@gmail.com> * Update enclave-runtime/src/attestation.rs Co-authored-by: Szilárd Parrag <szilard.parrag@gmail.com> * Extract shared logic into separate method * Improve documentation * Extract DCAP logic into separate function * Get rid of two unwrap() calls * Move getting the call_ids into the shared function * Use the correct Error for metadata * Add type alias for Fmspc * Improve separate_json_data_and_signature and add unit test * Fix clippy issues * Incorporate review feedback * Fix unsafe * Switch implementation of separate_json_data_and_signature * Make clippy happy * Add missing `use` * Add missing feature `preserve_order` to `serde_json_sgx` * Add compiler flag for std case * Make separate_json_data_and_signature robust to potential C-style null terminators Co-authored-by: Szilárd Parrag <szilard.parrag@gmail.com>

Niederb force-pushed the tn/collateral-handling branch from 35a789e to 78e1fcf Compare January 3, 2023 14:46

Niederb requested review from clangenb and OverOrion January 5, 2023 13:18

OverOrion reviewed Jan 5, 2023

View reviewed changes

clangenb reviewed Jan 13, 2023

View reviewed changes

OverOrion mentioned this pull request Jan 16, 2023

Create type alias for [u8; 2] #1151

Open

OverOrion reviewed Jan 16, 2023

View reviewed changes

core-primitives/attestation-handler/src/collateral.rs Outdated Show resolved Hide resolved

Niederb requested review from OverOrion and clangenb January 18, 2023 10:02

clangenb approved these changes Jan 18, 2023

View reviewed changes

service/src/main.rs Show resolved Hide resolved

core-primitives/attestation-handler/Cargo.toml Show resolved Hide resolved

OverOrion reviewed Jan 19, 2023

View reviewed changes

core-primitives/attestation-handler/src/collateral.rs Show resolved Hide resolved

OverOrion reviewed Jan 23, 2023

View reviewed changes

Niederb requested a review from OverOrion January 25, 2023 07:27

OverOrion approved these changes Jan 25, 2023

View reviewed changes

Niederb marked this pull request as ready for review January 26, 2023 06:35

Niederb added 15 commits January 26, 2023 07:37

First version that manages to register a quoting enclave

e0c4f54

Move collateral to attestation-handler

f48cde3

Work on collateral handling

072aeeb

Use the real collateral data for quoting enclave

6130fb8

Make the dump-ra CLI command work

ddabfbd

Work towards register_tcb_info

57e1548

Add all the boilerplate code to register TCB info

af8e855

Reduce code duplication for extrinsic encoding

72a2605

Reduce code duplication for extrinsic sending

9f5077d

Extract method for collateral

831090a

Remove duplicated code

6fc0edd

Fix clippy issues

bcbde65

Fix compilation error in teeracle

13034d7

Return certificate and dcap quote

66ac9a3

Cleanup

26e2ac3

Niederb and others added 20 commits January 26, 2023 07:37

Cleanup

0e58658

Update core-primitives/attestation-handler/src/attestation_handler.rs

f2aae17

Co-authored-by: Szilárd Parrag <szilard.parrag@gmail.com>

Update enclave-runtime/src/attestation.rs

837e4e8

Co-authored-by: Szilárd Parrag <szilard.parrag@gmail.com>

Extract shared logic into separate method

3513c8a

Improve documentation

bd57bcd

Extract DCAP logic into separate function

7519e93

Get rid of two unwrap() calls

2fcdf53

Move getting the call_ids into the shared function

cb3d476

Use the correct Error for metadata

b467d50

Add type alias for Fmspc

27413f1

Improve separate_json_data_and_signature and add unit test

48c508e

Fix clippy issues

fb4ed84

Incorporate review feedback

fa5a887

Fix unsafe

c23738d

Switch implementation of separate_json_data_and_signature

85ae349

Make clippy happy

9beacf8

Add missing use

2b870ea

Add missing feature preserve_order to serde_json_sgx

52b4525

Add compiler flag for std case

6cbd250

Make separate_json_data_and_signature robust to potential C-style nul…

90c466d

…l terminators

Niederb force-pushed the tn/collateral-handling branch from d2e4fb1 to 90c466d Compare January 26, 2023 06:37

Niederb added A0-core Affects a core part B1-releasenotes C1-low 📌 Does not elevate a release containing this beyond "low priority" E3-hardmerge PR introduces a lot changes in a lot of files. Requires some effort to merge or rebase to. labels Jan 26, 2023

Niederb self-assigned this Jan 26, 2023

clangenb merged commit 39624bc into master Jan 26, 2023

Kailai-Wang mentioned this pull request Jan 27, 2023

No panic in enclave litentry/litentry-parachain#1227

Closed

clangenb deleted the tn/collateral-handling branch February 20, 2023 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collateral handling for DCAP #1134

Collateral handling for DCAP #1134

Niederb commented Jan 3, 2023 •

edited

Loading

OverOrion left a comment

clangenb left a comment

clangenb left a comment

OverOrion Jan 23, 2023 •

edited

Loading

clangenb Jan 23, 2023

OverOrion Jan 23, 2023

clangenb Jan 23, 2023 •

edited

Loading

OverOrion Jan 23, 2023

Niederb Jan 23, 2023

Niederb Jan 25, 2023

OverOrion left a comment

		let json = String::from_utf8_lossy(data);
		let value: serde_json::Value = serde_json::from_str(&json).ok()?;

-		let json = String::from_utf8_lossy(data);
-		let value: serde_json::Value = serde_json::from_str(&json).ok()?;
+        let json = CStr::from_bytes_with_nul(data)
+			.expect("CStr::from_bytes_with_nul failed")
+			.to_str()
+			.expect("Must not contain invalid UTF-8 data");
+		let value: serde_json::Value = serde_json::from_str(&json).ok()?;

Collateral handling for DCAP #1134

Collateral handling for DCAP #1134

Conversation

Niederb commented Jan 3, 2023 • edited Loading

OverOrion left a comment

Choose a reason for hiding this comment

clangenb left a comment

Choose a reason for hiding this comment

clangenb left a comment

Choose a reason for hiding this comment

OverOrion Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

clangenb Jan 23, 2023

Choose a reason for hiding this comment

OverOrion Jan 23, 2023

Choose a reason for hiding this comment

clangenb Jan 23, 2023 • edited Loading

Choose a reason for hiding this comment

OverOrion Jan 23, 2023

Choose a reason for hiding this comment

Niederb Jan 23, 2023

Choose a reason for hiding this comment

Niederb Jan 25, 2023

Choose a reason for hiding this comment

OverOrion left a comment

Choose a reason for hiding this comment

Niederb commented Jan 3, 2023 •

edited

Loading

OverOrion Jan 23, 2023 •

edited

Loading

clangenb Jan 23, 2023 •

edited

Loading