Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Provisioning Certification Keys (PCKs) #1051

Merged
merged 30 commits into from
Sep 25, 2024

Conversation

ameba23
Copy link
Contributor

@ameba23 ameba23 commented Sep 13, 2024

As part of the attestation feature #982 we need to handle PCK certificates when a validator joins. These contain the public key used to sign attestations, and are signed by Intel.

Rather than storing the whole certificate, my current proposal is to verify it once with Intel's public key, and then only store the public key with which we need to verify quotes.

However it could be argued we should keep the whole certificate including the signature - i am open to discuss this.

This PR handles these public keys and uses them in quote verification, but it does not handle extracting them from a PCK certificate when a validator joins, or verifying that the certificate is signed by Intel. This will come in another PR.

PCKs are stored as a field in the staking extension pallet's ServerInfo struct. This might not seem like the best home for them - i could have made a mapping of TSS account IDs to PCKs in the attestation pallet. But i deliberately did it like this because i want it to be impossible that an active validator does not have an associated PCK.

When generating mock quotes, the PCK keypair is derived from the TSS account ID. This saves us having to generate and store them somewhere, and makes it easy to compute the keypairs for the test accounts.

@ameba23 ameba23 marked this pull request as draft September 13, 2024 08:01
@ameba23 ameba23 self-assigned this Sep 18, 2024
@@ -64,6 +67,12 @@ async fn test_change_threshold_accounts() {
)
.await
.unwrap();

let provisioning_certification_key = {
let key = derive_mock_pck_verifying_key(&TSS_ACCOUNTS[0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now the PCK for the old TS account, even though the account ID has changed. I have made an issue for this: #1058

@@ -115,6 +115,7 @@ pub mod pallet {
pub tss_account: AccountId,
pub x25519_public_key: X25519PublicKey,
pub endpoint: TssServerURL,
pub provisioning_certification_key: VerifyingKey,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that this is a BoundedVec - it has caused all sorts of problems. I would have preferred to use a [u8; 33]. But i tried and i had difficulties with serializing / encoding. Just using Vec<u8> i find too permissive, so in the end i went with BoundedVec.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like all bounded vec does is just put a limit on a vec, we can do that manually, it is just everytime we touch it we would need to make sure we apply a size check

@ameba23 ameba23 marked this pull request as ready for review September 19, 2024 11:54
@@ -109,6 +109,10 @@ pub enum HashingAlgorithm {
/// A compressed, serialized [synedrion::ecdsa::VerifyingKey<k256::Secp256k1>]
pub type EncodedVerifyingKey = [u8; VERIFICATION_KEY_LENGTH as usize];

#[cfg(not(feature = "wasm"))]
pub type BoundedVecEncodedVerifyingKey =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Im not sure I see this anywhere else

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in the provisioning_certification_key constants and chainspecs

Copy link
Collaborator

@HCastano HCastano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't totally understand your comments around using Intel's public key once to avoid storing the whole cert, but this might be a bigger conversation around the quote/attestation flow from my end

Comment on lines +182 to +183
&server_info
.provisioning_certification_key
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More a note to myself than anything, but this is gonna have to get abstracted out similar to the X25519 key in #1063

pallets/staking/Cargo.toml Outdated Show resolved Hide resolved
crates/testing-utils/src/helpers.rs Show resolved Hide resolved
@@ -109,6 +109,10 @@ pub enum HashingAlgorithm {
/// A compressed, serialized [synedrion::ecdsa::VerifyingKey<k256::Secp256k1>]
pub type EncodedVerifyingKey = [u8; VERIFICATION_KEY_LENGTH as usize];

#[cfg(not(feature = "wasm"))]
pub type BoundedVecEncodedVerifyingKey =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's used in the provisioning_certification_key constants and chainspecs

@ameba23
Copy link
Contributor Author

ameba23 commented Sep 25, 2024

I don't totally understand your comments around using Intel's public key once to avoid storing the whole cert, but this might be a bigger conversation around the quote/attestation flow from my end

@HCastano i think we should chat about this as i have some code for handling the certificates when a validator initially joins (or changes their endpoint later), which should probably link in with what you are doing in 1063.

@ameba23 ameba23 mentioned this pull request Sep 25, 2024
@ameba23 ameba23 merged commit f286071 into master Sep 25, 2024
8 checks passed
@ameba23 ameba23 deleted the peg/provisioning-certification-key branch September 25, 2024 08:51
ameba23 added a commit that referenced this pull request Oct 2, 2024
* master:
  Bump clap from 4.5.18 to 4.5.19 in the patch-dependencies group (#1091)
  Avoid panic by checking that we have a non-signing validator before selecting one (#1083)
  Fix master build (#1088)
  Small fixes to `test-cli` (#1084)
  Pregenerate keyshares sets for all possible initial signer comittees (#1073)
  Fix master build (#1079)
  Bump reqwest from 0.12.7 to 0.12.8 in the patch-dependencies group (#1082)
  Block tss chain when signer (#1078)
  Run multiple test validator (#1074)
  Bump tempfile from 3.12.0 to 3.13.0 (#1076)
  Bump axum from 0.7.6 to 0.7.7 in the patch-dependencies group (#1075)
  Unignore register and sign integration test, and do a non-mock jumpstart (#1070)
  No unbonding when signer or next signer (#1031)
  Add `/relay_tx` endpoint (#1050)
  Fix how pre-generated keyshares are added for tests (#1061)
  Handle Provisioning Certification Keys (PCKs) (#1051)
  Bump async-trait from 0.1.82 to 0.1.83 in the patch-dependencies group (#1067)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants