-
Notifications
You must be signed in to change notification settings - Fork 2
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
Conversation
@@ -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]); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
@@ -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 = |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this 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
&server_info | ||
.provisioning_certification_key |
There was a problem hiding this comment.
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
@@ -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 = |
There was a problem hiding this comment.
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
@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. |
* 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)
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.