From 6c1843445b744d988de8ab22cea734016a8c93d5 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 21 Oct 2024 16:19:16 -0400 Subject: [PATCH] Add quote guard to `change_threshold_accounts` extrinsic --- pallets/staking/src/lib.rs | 41 +++++++++++++++++--- pallets/staking/src/tests.rs | 75 +++++++++++++++++++++++++++++++++--- 2 files changed, 105 insertions(+), 11 deletions(-) diff --git a/pallets/staking/src/lib.rs b/pallets/staking/src/lib.rs index 7ba1cfe1b..47f4eac3f 100644 --- a/pallets/staking/src/lib.rs +++ b/pallets/staking/src/lib.rs @@ -373,6 +373,7 @@ pub mod pallet { quote: Vec, ) -> DispatchResult { let who = ensure_signed(origin)?; + ensure!( endpoint.len() as u32 <= T::MaxEndpointLength::get(), Error::::EndpointTooLong @@ -399,6 +400,7 @@ pub mod pallet { ); server_info.endpoint.clone_from(&endpoint); + Ok(()) } else { Err(Error::::NoBond) @@ -409,21 +411,34 @@ pub mod pallet { Ok(()) } - /// Allows a validator to change their threshold key so can confirm done when coms manager - /// `new_account`: nodes's threshold account + /// Allows a validator to change their assocated threshold server AccountID and X25519 + /// public key. + /// + /// # Expects TDX Quote + /// + /// A valid TDX quote must be passed along in order to ensure that the validator is running + /// TDX hardware. In order for the chain to be aware that a quote is expected from the + /// validator `pallet_attestation::request_attestation()` must be called first. + /// + /// The **new** TSS AccountID must be used when requesting this quote. + /// + /// The quote format is specified in: + /// https://download.01.org/intel-sgx/latest/dcap-latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf #[pallet::call_index(1)] #[pallet::weight(::WeightInfo::change_threshold_accounts(MAX_SIGNERS as u32))] pub fn change_threshold_accounts( origin: OriginFor, tss_account: T::AccountId, x25519_public_key: X25519PublicKey, + quote: Vec, ) -> DispatchResultWithPostInfo { + let who = ensure_signed(origin)?; + ensure!( !ThresholdToStash::::contains_key(&tss_account), Error::::TssAccountAlreadyExists ); - let who = ensure_signed(origin)?; let stash = Self::get_stash(&who)?; let validator_id = ::ValidatorId::try_from(stash) .or(Err(Error::::InvalidValidatorId))?; @@ -437,17 +452,33 @@ pub mod pallet { let new_server_info: ServerInfo = ThresholdServers::::try_mutate(&validator_id, |maybe_server_info| { if let Some(server_info) = maybe_server_info { + // Before we modify the `server_info`, we want to check that the validator is + // still running TDX hardware. + ensure!( + >::verify_quote( + &tss_account.clone(), + x25519_public_key, + server_info.provisioning_certification_key.clone(), + quote + ) + .is_ok(), + Error::::FailedAttestationCheck + ); + server_info.tss_account = tss_account.clone(); server_info.x25519_public_key = x25519_public_key; ThresholdToStash::::insert(&tss_account, &validator_id); + Ok(server_info.clone()) } else { Err(Error::::NoBond) } })?; + Self::deposit_event(Event::ThresholdAccountChanged(validator_id, new_server_info)); - Ok(Some(::WeightInfo::change_threshold_accounts(signers.len() as u32)) - .into()) + + let actual_weight = ::WeightInfo::change_threshold_accounts(signers.len() as u32); + Ok(Some(actual_weight).into()) } /// Wraps's Substrate's `unbond` extrinsic but checks to make sure targeted account is not a signer or next signer diff --git a/pallets/staking/src/tests.rs b/pallets/staking/src/tests.rs index 52a8858e7..0a410c7e4 100644 --- a/pallets/staking/src/tests.rs +++ b/pallets/staking/src/tests.rs @@ -179,7 +179,11 @@ fn it_changes_endpoint() { VALID_QUOTE.to_vec(), )); - assert_ok!(Staking::change_endpoint(RuntimeOrigin::signed(1), endpoint.clone(), VALID_QUOTE.to_vec())); + assert_ok!(Staking::change_endpoint( + RuntimeOrigin::signed(1), + endpoint.clone(), + VALID_QUOTE.to_vec() + )); assert_eq!(Staking::threshold_server(1).unwrap().endpoint, endpoint); assert_noop!( @@ -243,12 +247,22 @@ fn it_changes_threshold_account() { VALID_QUOTE.to_vec(), )); - assert_ok!(Staking::change_threshold_accounts(RuntimeOrigin::signed(1), 4, NULL_ARR)); + assert_ok!(Staking::change_threshold_accounts( + RuntimeOrigin::signed(1), + 4, + NULL_ARR, + VALID_QUOTE.to_vec() + )); assert_eq!(Staking::threshold_server(1).unwrap().tss_account, 4); assert_eq!(Staking::threshold_to_stash(4).unwrap(), 1); assert_noop!( - Staking::change_threshold_accounts(RuntimeOrigin::signed(4), 5, NULL_ARR), + Staking::change_threshold_accounts( + RuntimeOrigin::signed(4), + 5, + NULL_ARR, + VALID_QUOTE.to_vec() + ), Error::::NotController ); @@ -273,18 +287,62 @@ fn it_changes_threshold_account() { )); assert_noop!( - Staking::change_threshold_accounts(RuntimeOrigin::signed(1), 5, NULL_ARR), + Staking::change_threshold_accounts( + RuntimeOrigin::signed(1), + 5, + NULL_ARR, + VALID_QUOTE.to_vec() + ), Error::::TssAccountAlreadyExists ); Signers::::put(vec![1]); assert_noop!( - Staking::change_threshold_accounts(RuntimeOrigin::signed(1), 9, NULL_ARR,), + Staking::change_threshold_accounts( + RuntimeOrigin::signed(1), + 9, + NULL_ARR, + VALID_QUOTE.to_vec() + ), Error::::NoChangingThresholdAccountWhenSigner ); }); } +#[test] +fn it_doesnt_allow_changing_threshold_account_with_invalid_quote() { + new_test_ext().execute_with(|| { + assert_ok!(FrameStaking::bond( + RuntimeOrigin::signed(1), + 100u64, + pallet_staking::RewardDestination::Account(1), + )); + + let server_info = ServerInfo { + tss_account: 3, + x25519_public_key: NULL_ARR, + endpoint: vec![20], + provisioning_certification_key: BoundedVec::with_max_capacity(), + }; + assert_ok!(Staking::validate( + RuntimeOrigin::signed(1), + pallet_staking::ValidatorPrefs::default(), + server_info.clone(), + VALID_QUOTE.to_vec(), + )); + + assert_noop!( + Staking::change_threshold_accounts( + RuntimeOrigin::signed(1), + 4, + NULL_ARR, + INVALID_QUOTE.to_vec() + ), + Error::::FailedAttestationCheck + ); + }) +} + #[test] fn it_will_not_allow_existing_tss_account_when_changing_threshold_account() { new_test_ext().execute_with(|| { @@ -328,7 +386,12 @@ fn it_will_not_allow_existing_tss_account_when_changing_threshold_account() { )); assert_noop!( - Staking::change_threshold_accounts(RuntimeOrigin::signed(1), 5, NULL_ARR), + Staking::change_threshold_accounts( + RuntimeOrigin::signed(1), + 5, + NULL_ARR, + VALID_QUOTE.to_vec() + ), Error::::TssAccountAlreadyExists ); });