Add quote guards to `ServerInfo` related extrinsics #1123

HCastano · 2024-10-21T21:08:20Z

This is one of the follow-ups to #1109, in which we will enforce that validators who want
to change certain info about their validator will need to pass an attestation check
first.

For the change_endpoint() extrinsic we assume that that the underlying hardware and
Account ID stay the same, so a quote from the existing TSS Account ID is expected.

For the change_threshold_accounts() extrinsic we assume that the underlying hardware
has changed, so a quote from a new TSS account ID is expected.

Doesn't work yet though, but this is at least a good checkpoint

Faling to verify the PCK though...

HCastano · 2024-10-28T20:04:23Z

crates/client/src/tests.rs

+        let mut pck_seeder = StdRng::from_seed(tss_public_key.0);
+        let pck = tdx_quote::SigningKey::random(&mut pck_seeder);


@ameba23 sorry since this test is a bit of a mess right now, but do you have any idea why the PCK verification might be failing? Should I be using a different input than the tss_public_key for the seeder?

I've commented out the line in the Attestation crate where this fails.

Okay so in this case we're using the old PCK for quote verification. I thought the PCK was a fixed thing, but since it's probably tied to the hardware it does need to change then.

Sorry i somehow missed this.

Yes the idea was to use tss account id as the seed.

I guess with change_endpoint its pretty likely that the hardware is changing so PCK would also change (unless the hardware is moving to a different internet connection). Not sure about change_threshold_account.

With change_endpoint we don't necessarily need to be changing the hardware. Maybe we're just putting our existing node behind a FQDN, or behind a load balancer.

With change_threshold_account if we make the change to generate the TSS account ID in the enclave then this would very likely indicate that the hardware has also changed.

That said, what if in the second case we just spin up a second instance of entropy-tss on the same hardware. This would result in a different account_id and x25519_public_key, but would it have a different PCK? I'm not familiar enough to know.

That said, what if in the second case we just spin up a second instance of entropy-tss on the same hardware. This would result in a different account_id and x25519_public_key, but would it have a different PCK? I'm not familiar enough to know.

In my understanding PCK would be the same but the 'attestation key' should change. A quote contains two signatures: the quote data is signed with the attestation key, and the attestation public key is signed with the PCK. I chose not to store the attestation public key on chain, because we can already see from the quote itself that it has been endorsed by the PCK - so the PCK is the thing we are interested in.

But this does mean that there can be several TSS nodes associated with the same PCK. I can't think of a reason why this would be a problem but worth being aware of.

It looks like if the TSS Account ID and X25519 public keys are changing then we're probably on different hardware so the PCK will also change. This also gets the client test for the extrisnic passing.

This matches what `validate()` does and prevents us from having an invalid PCK become part of `ServerInfo`.

This reverts commit fe7aadd.

ameba23

🌟 Great

ameba23 · 2024-11-05T08:11:57Z

crates/test-cli/src/lib.rs

+        ///
+        /// The quote format is specified in:
+        /// https://download.01.org/intel-sgx/latest/dcap-latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf
+        quote: String,


For a followup:

Maybe we wanna have these be given as hex. I think otherwise they need to be given as [234, 1, 43...].

It will be easiest if we have them be given in whatever format the TSS server gives them to the operator in the quote generation endpoint, which doesn't yet exist. So probably we can fix this once it does exist.

ameba23 · 2024-11-05T08:30:50Z

crates/client/src/tests.rs

@@ -36,7 +42,33 @@ async fn test_change_endpoint() {
    let api = get_api(&substrate_context.node_proc.ws_url).await.unwrap();
    let rpc = get_rpc(&substrate_context.node_proc.ws_url).await.unwrap();

-    let result = change_endpoint(&api, &rpc, one.into(), "new_endpoint".to_string()).await.unwrap();
+    // By using this `Alice` account we can skip the `request_attestation` step since this is


Ah ok, i was thinking we were going to ditch having the pending attestation set at genesis because it wasn't used anymore

ameba23 · 2024-11-05T08:37:07Z

crates/client/src/client.rs

+        attestee = ?attestee.public(),
+    )
+)]
+pub async fn request_attestation(


For followup: If this is going to also be called by entropy-tss, which i think we probably will want to when we add a generate quote endpoint - it will need to go in a different module as stuff in this one is behind the full-client feature flag.

ameba23 · 2024-11-05T08:37:56Z

crates/client/src/client.rs

+    let result =
+        submit_transaction_with_pair(api, rpc, &attestee, &request_attestation, None).await?;
+    let result_event = result
+        .find_first::<entropy::attestation::events::AttestationIssued>()?


Thats nice that you can get the nonce like this - i didnt understand that from the last PR.

ameba23 · 2024-11-05T08:46:54Z

crates/test-cli/src/lib.rs

@@ -155,6 +160,13 @@ enum CliCommand {
        new_tss_account: String,
        /// New x25519 public key
        new_x25519_public_key: String,
+        /// The new Provisioning Certification Key (PCK) certificate chain to be used for the TSS.
+        new_pck_certificate_chain: Vec<String>,


For followup: If given as strings, it probably makes sense for them to be PEM encoded as this is the standard for encoding x509 certificates as utf-8. This would mean adding a dependency to the test-cli to decode them. Otherwise we could have them be read in from files i guess.

We can probably leave this until we develop tooling for operators to be able to retrieve these certificates and see what makes most sense then.

HCastano added 3 commits October 21, 2024 16:05

Add quote guard to change_endpoint extrinsic

fe47faa

Add quote guard to change_threshold_accounts extrinsic

6c18434

Bump metadata

641134b

HCastano mentioned this pull request Oct 23, 2024

Remove declare synced #1134

Merged

HCastano added 8 commits October 28, 2024 11:27

Merge branch 'master' into hc/add-quote-guards

ae73f0d

RustFmt

b2e5d01

Get entropy-client test for change_endpoint working

8613c80

Get change_threshold_account test compiling

fdff83a

Doesn't work yet though, but this is at least a good checkpoint

Add way to request_attestation from client

abec5a9

Almost have change_threshold_account test working

9296095

Faling to verify the PCK though...

TaploFmt

d5f1545

Be a bit more descriptive with the TSS public key variable

8daaa24

HCastano commented Oct 28, 2024

View reviewed changes

HCastano added 17 commits October 30, 2024 14:32

Make update_threshold_account() use updated PCK

2e39ce9

It looks like if the TSS Account ID and X25519 public keys are changing then we're probably on different hardware so the PCK will also change. This also gets the client test for the extrisnic passing.

Clean up change_threshold_account() test

edc2f7c

Clean up the request_attestation client method a bit

ed5db5e

Get entropy-test-cli compiling again

cc71358

Remove unnecessary .clone()

68c8f4c

Update test-cli for new extrinsic arguments

f7571c5

Get staking tests working again

5a0283c

Get Staking benchmarks compiling

3518cd0

Get change_endpoint benchmark working

70da5e3

Get change_threshold_accounts benchmark working

a4f41c7

RustFmt benches

f70db4b

Use better mock endpoint

c68a4ed

Switch to requiring a PCK chain instead of a certificate directly

f237a73

This matches what `validate()` does and prevents us from having an invalid PCK become part of `ServerInfo`.

Bump metadata

212628d

Update client to use PCK certificate chains

21bdfa7

Update the test-cli to use PCK certificate chains

492b9d6

Undo some formatting changes

2434d4d

HCastano added 2 commits November 4, 2024 16:14

Variables mystery amount

1628f8a

Add CHANGELOG entry

fe7aadd

HCastano added the Bump `transaction_version` A change which affects how existing extrinsics are created (e.g parameter changes) label Nov 4, 2024

HCastano added 2 commits November 4, 2024 16:23

Revert "Add CHANGELOG entry"

5a7ab4d

This reverts commit fe7aadd.

Updated CHANGELOG without formatting

dd695c9

HCastano marked this pull request as ready for review November 4, 2024 21:26

HCastano requested review from ameba23 and JesseAbram November 4, 2024 21:27

JesseAbram approved these changes Nov 4, 2024

View reviewed changes

ameba23 approved these changes Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add quote guards to `ServerInfo` related extrinsics #1123

Add quote guards to `ServerInfo` related extrinsics #1123

HCastano commented Oct 21, 2024 •

edited

Loading

HCastano Oct 28, 2024 •

edited

Loading

HCastano Oct 29, 2024

ameba23 Oct 29, 2024

HCastano Oct 30, 2024

ameba23 Nov 5, 2024

ameba23 left a comment

ameba23 Nov 5, 2024

ameba23 Nov 5, 2024

ameba23 Nov 5, 2024

ameba23 Nov 5, 2024

ameba23 Nov 5, 2024

		let mut pck_seeder = StdRng::from_seed(tss_public_key.0);
		let pck = tdx_quote::SigningKey::random(&mut pck_seeder);

Add quote guards to ServerInfo related extrinsics #1123

Are you sure you want to change the base?

Add quote guards to ServerInfo related extrinsics #1123

Conversation

HCastano commented Oct 21, 2024 • edited Loading

HCastano Oct 28, 2024 • 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

ameba23 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

Add quote guards to `ServerInfo` related extrinsics #1123

Add quote guards to `ServerInfo` related extrinsics #1123

HCastano commented Oct 21, 2024 •

edited

Loading

HCastano Oct 28, 2024 •

edited

Loading