Extract PCK certificate chain from quotes #1209

ameba23 · 2024-12-10T11:16:59Z

I recently figured out the PCK certificate chain is actually included in the quote, encoded as PEM. I have an open PR to tdx-quote which allows getting the certificate chain from a quote, and i have moved the cert chain parsing logic over there: entropyxyz/tdx-quote#12

This means that the extrinsics validate and change_threshold_accounts don't need to take both a quote and a certificate chain - just the quote.

My proposal is to change AttestationHandler::verify_quote to validate the quote with whatever PCK can be extracted from its certificate chain, and return the PCK upon successful verification - the staking pallet can then store it in the ServerInfo.

This also adds a production feature flag to entropy which propagates down to pallet-attestation. If present, real PCK cert chain verification will be used (previously we always mocked certificate chain verification). By default it is not used.

Closes #1208 by adding a custom error type to the attestation handler trait.

If this gets approved, i think there is maybe some refactoring that could be done as a followup - i'm not totally sure but i don't think the staking extension pallet needs to be a dependency of the attestation pallet anymore. Which means we can have the attestation pallet be a dependency of the staking extension pallet, and move the AttestationHandler trait from entropy-shared to the attestation pallet.

…rt of quote verification

HCastano

Took a sneak peek here, seems reasonable. I like that we're able to move some of this out to the tdx-quote crate

HCastano · 2024-12-12T14:22:48Z

runtime/Cargo.toml

@@ -273,3 +273,6 @@ try-runtime=[
  "pallet-utility/try-runtime",
  "pallet-vesting/try-runtime",
 ]
+# Enables real PCK certificate chain verification - which means TSS nodes much be runnning on TDX


Suggested change

# Enables real PCK certificate chain verification - which means TSS nodes much be runnning on TDX

# Enables real PCK certificate chain verification - which means TSS nodes must be runnning on TDX

HCastano · 2024-12-12T14:23:14Z

Cargo.lock

Did you do a full cargo update here? This seems like a lot of changes

Yeah that was an accident. I couldn't update the git version tdx-quote with cargo update tdx-quote because some crates were using the git version and others the release version. So i lazily just did cargo update without thinking of the implications. Im not sure whether to revert it or just see if things still work and leave it in if the do.

In the end i have reverted the lockfile as there was cargo-deny related license issues popping up

node/cli/Cargo.toml

Co-authored-by: Hernando Castano <[email protected]>

ameba23 · 2024-12-17T08:15:24Z

crates/client/Cargo.toml

 entropy-testing-utils={ path="../testing-utils" }
-tdx-quote            ={ version="0.0.1", features=["mock"] }
+tdx-quote={ git="https://github.com/entropyxyz/tdx-quote.git", branch="peg/cert-chain-parse", features=[


I'm planning to re-publish this crate if this PR gets approved. Ideally i would also like to fix entropyxyz/tdx-quote#13 and entropyxyz/tdx-quote#11 before publishing.

ameba23 · 2024-12-17T09:42:20Z

crates/shared/src/types.rs

+/// An error when verifying a quote
+#[cfg(not(feature = "wasm"))]
+#[derive(Eq, PartialEq)]
+pub enum VerifyQuoteError {


I added this custom error type because i couldn't figure out how to get the useful information out of a sp_runtime::DispathError without adding a lot of extra code.

ameba23 · 2024-12-17T09:47:56Z

pallets/staking/src/lib.rs

    }

-    impl<T> From<pck::PckParseVerifyError> for Error<T> {
-        fn from(error: pck::PckParseVerifyError) -> Self {
+    impl<T> From<VerifyQuoteError> for Error<T> {


Maybe its possible to remove the need for this impl by implementing PalletError for VerifyQuoteError, so that we can have a tuple enum variant of pallet::<T>::Error containing it. I would maybe try this in a followup - but i am afraid of going over the maximum encoded size of 4 bytes.

It seems like it's a compile time check anyways, so yeah would be worth trying

HCastano

Nice work, looks good 👍

I'd need to double check the diff with v0.3.0 to be sure, but this probably needs a CHANGELOG entry and a "Bump spec_version" tag.

Which means we can have the attestation pallet be a dependency of the staking extension pallet, and move the AttestationHandler trait from entropy-shared to the attestation pallet.

It would be nice to keep this flexible using associated types, but I'd be curious to see what you have in mind for this first before commenting further.

HCastano · 2024-12-17T21:45:44Z

crates/shared/src/types.rs

-    ) -> Result<(), sp_runtime::DispatchError> {
-        Ok(())
+    ) -> Result<BoundedVecEncodedVerifyingKey, VerifyQuoteError> {
+        Ok(sp_runtime::BoundedVec::new())


We may want to specify a size of 33 bytes here in case a caller is assuming a "valid"-ish key back

HCastano · 2024-12-17T22:16:10Z

pallets/staking/src/lib.rs

-    impl<T> From<pck::PckParseVerifyError> for Error<T> {
-        fn from(error: pck::PckParseVerifyError) -> Self {
+    impl<T> From<VerifyQuoteError> for Error<T> {
+        fn from(error: VerifyQuoteError) -> Self {


Ah, so it was really frustrating trying to debug errors stemming from the quote verification code because they'd all basically be type erased into the pallet error. While this is pretty ugly it is quite useful.

Maybe worth a comment explaining this for future readers.

HCastano · 2024-12-17T22:16:42Z

pallets/staking/src/lib.rs

    }

-    impl<T> From<pck::PckParseVerifyError> for Error<T> {
-        fn from(error: pck::PckParseVerifyError) -> Self {
+    impl<T> From<VerifyQuoteError> for Error<T> {


It seems like it's a compile time check anyways, so yeah would be worth trying

crates/shared/src/types.rs

ameba23 · 2024-12-18T07:55:15Z

I'd need to double check the diff with v0.3.0 to be sure, but this probably needs a CHANGELOG entry and a "Bump spec_version" tag.

Im still not super clear exactly what kinds of runtime changes mean we need to bump the spec version but i added that label for now as i figured better to have it and not need it than need it and not have it.

HCastano · 2024-12-18T16:33:48Z

crates/shared/src/types.rs

-    ) -> Result<(), sp_runtime::DispatchError> {
-        Ok(())
+    ) -> Result<BoundedVecEncodedVerifyingKey, VerifyQuoteError> {
+        // Ok(sp_runtime::BoundedVec::new())


Suggested change

// Ok(sp_runtime::BoundedVec::new())

HCastano · 2024-12-18T16:36:17Z

CHANGELOG.md

+- In ([#1209]()) a `production` feature flag was added to `entropy` which if enabled will use
+  non-mock verification of PCK certificate chains in TDX quotes, meaning TSS servers must be running
+  on TDX hardware


Suggested change

- In ([#1209]()) a `production` feature flag was added to `entropy` which if enabled will use

non-mock verification of PCK certificate chains in TDX quotes, meaning TSS servers must be running

on TDX hardware

These sections should just have PR titles.

If anything we could maybe mention it in the Breaking Changes, but nbd. Can always add it later when prepping the release.

Minor edits missed from PR review

WIP move PCK cert chain verification to attestation pallet

f961716

ameba23 marked this pull request as draft December 10, 2024 11:17

ameba23 added 4 commits December 12, 2024 12:56

Switch things around for PCK cert chain verification happenning as pa…

eda5589

…rt of quote verification

Fix error handling in entropy-tss

2884836

Use git version of tdx-quote

c7bc598

Use git version of tdx-quote

dac3595

HCastano reviewed Dec 12, 2024

View reviewed changes

ameba23 and others added 18 commits December 13, 2024 08:47

Update node/cli/Cargo.toml

84e0849

Co-authored-by: Hernando Castano <[email protected]>

Fix staking pallet cargo.toml

92a1397

Taplo

3fefc22

Update tdx-quote, rm unwraps

f1fe7bf

Improve error handling in attestation pallet

2072ec6

Bump tdx-quote again following improvement of error type

dd463c6

Update tdx-quote following pck_verify takes reference

9c4bdfd

Revert cargo.lock, fix ensure! macro call

3680d62

Fix staking pallet config in other pallets mock files

20d57e6

Update metadata

072b411

Fix client test

971052b

Update test-cli

4ba9241

Fixes for tests

ee23433

Fix staking pallet benchmarking

546b034

Fix staking pallet benchmarking

7a2f36f

Merge master

6f17e5b

Fix client following merge master

fe7291a

Typo

d1f075f

ameba23 commented Dec 17, 2024

View reviewed changes

ameba23 added 3 commits December 17, 2024 10:07

Error handling

02d0a61

Update staking pallet tests

0456568

Comments, changelog

5efd1ae

ameba23 commented Dec 17, 2024

View reviewed changes

ameba23 added 5 commits December 17, 2024 11:14

VerifyQuoteError implements Debug

30a522b

Rm PCK cert chain from JoiningServerInfo

a680883

Pull chain metadata

dc2222c

Clippy

096036e

Clippy

a674da9

ameba23 marked this pull request as ready for review December 17, 2024 18:17

HCastano approved these changes Dec 17, 2024

View reviewed changes

ameba23 added 2 commits December 18, 2024 08:52

Add to changelog

4c2c586

Minor changes following review comments

f1334e6

ameba23 added the Bump `spec_version` A change which affects the current runtime behaviour label Dec 18, 2024

ameba23 added 2 commits December 18, 2024 09:11

Merge master

172d2e0

Use specific commit of tdx-quote rather than branch

0c55178

HCastano reviewed Dec 18, 2024

View reviewed changes

ameba23 merged commit 664e406 into master Dec 18, 2024
8 checks passed

ameba23 deleted the peg/extract-pck-cert-from-quotes branch December 18, 2024 16:50

ameba23 mentioned this pull request Dec 19, 2024

Minor edits missed from review of #1209 #1226

Merged

HCastano pushed a commit that referenced this pull request Dec 19, 2024

Minor edits missed from review of #1209 (#1226)

1c1bc75

Minor edits missed from PR review

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract PCK certificate chain from quotes #1209

Extract PCK certificate chain from quotes #1209

ameba23 commented Dec 10, 2024 •

edited

Loading

HCastano left a comment

HCastano Dec 12, 2024

HCastano Dec 12, 2024

ameba23 Dec 13, 2024

ameba23 Dec 13, 2024

ameba23 Dec 17, 2024

ameba23 Dec 17, 2024

ameba23 Dec 17, 2024

HCastano Dec 17, 2024

HCastano left a comment

HCastano Dec 17, 2024

HCastano Dec 17, 2024

HCastano Dec 17, 2024

ameba23 commented Dec 18, 2024

HCastano Dec 18, 2024

HCastano Dec 18, 2024

	# Enables real PCK certificate chain verification - which means TSS nodes much be runnning on TDX
	# Enables real PCK certificate chain verification - which means TSS nodes must be runnning on TDX

	- In ([#1209]()) a `production` feature flag was added to `entropy` which if enabled will use
	non-mock verification of PCK certificate chains in TDX quotes, meaning TSS servers must be running
	on TDX hardware

Extract PCK certificate chain from quotes #1209

Extract PCK certificate chain from quotes #1209

Conversation

ameba23 commented Dec 10, 2024 • edited Loading

HCastano 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

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

HCastano 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

ameba23 commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameba23 commented Dec 10, 2024 •

edited

Loading