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

Add quote guards to ServerInfo related extrinsics #1123

Merged
merged 33 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
fe47faa
Add quote guard to `change_endpoint` extrinsic
HCastano Oct 21, 2024
6c18434
Add quote guard to `change_threshold_accounts` extrinsic
HCastano Oct 21, 2024
641134b
Bump metadata
HCastano Oct 21, 2024
ae73f0d
Merge branch 'master' into hc/add-quote-guards
HCastano Oct 28, 2024
b2e5d01
RustFmt
HCastano Oct 28, 2024
8613c80
Get `entropy-client` test for `change_endpoint` working
HCastano Oct 28, 2024
fdff83a
Get `change_threshold_account` test compiling
HCastano Oct 28, 2024
abec5a9
Add way to `request_attestation` from client
HCastano Oct 28, 2024
9296095
Almost have `change_threshold_account` test working
HCastano Oct 28, 2024
d5f1545
TaploFmt
HCastano Oct 28, 2024
8daaa24
Be a bit more descriptive with the TSS public key variable
HCastano Oct 28, 2024
2e39ce9
Make `update_threshold_account()` use updated PCK
HCastano Oct 30, 2024
edc2f7c
Clean up `change_threshold_account()` test
HCastano Oct 30, 2024
ed5db5e
Clean up the `request_attestation` client method a bit
HCastano Oct 30, 2024
cc71358
Get `entropy-test-cli` compiling again
HCastano Oct 30, 2024
68c8f4c
Remove unnecessary `.clone()`
HCastano Oct 30, 2024
f7571c5
Update `test-cli` for new extrinsic arguments
HCastano Nov 1, 2024
5a0283c
Get staking tests working again
HCastano Nov 1, 2024
3518cd0
Get Staking benchmarks compiling
HCastano Nov 1, 2024
70da5e3
Get `change_endpoint` benchmark working
HCastano Nov 1, 2024
a4f41c7
Get `change_threshold_accounts` benchmark working
HCastano Nov 1, 2024
f70db4b
RustFmt benches
HCastano Nov 4, 2024
c68a4ed
Use better mock endpoint
HCastano Nov 4, 2024
f237a73
Switch to requiring a PCK chain instead of a certificate directly
HCastano Nov 4, 2024
212628d
Bump metadata
HCastano Nov 4, 2024
21bdfa7
Update `client` to use PCK certificate chains
HCastano Nov 4, 2024
492b9d6
Update the `test-cli` to use PCK certificate chains
HCastano Nov 4, 2024
2434d4d
Undo some formatting changes
HCastano Nov 4, 2024
1628f8a
Variables mystery amount
HCastano Nov 4, 2024
fe7aadd
Add `CHANGELOG` entry
HCastano Nov 4, 2024
5a7ab4d
Revert "Add `CHANGELOG` entry"
HCastano Nov 4, 2024
dd695c9
Updated `CHANGELOG` without formatting
HCastano Nov 4, 2024
6424b7e
Merge branch 'master' into hc/add-quote-guards
HCastano Nov 8, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions crates/client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ tokio ={ version="1.41", features=["time"] }
serial_test ="3.1.1"
sp-keyring ="34.0.0"
entropy-testing-utils={ path="../testing-utils" }
tdx-quote ={ version="0.0.1", features=["mock"] }

[features]
default=["native", "full-client-native"]
Expand Down
Binary file modified crates/client/entropy_metadata.scale
Binary file not shown.
23 changes: 22 additions & 1 deletion crates/client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -341,8 +341,10 @@ pub async fn change_endpoint(
rpc: &LegacyRpcMethods<EntropyConfig>,
user_keypair: sr25519::Pair,
new_endpoint: String,
quote: Vec<u8>,
) -> anyhow::Result<EndpointChanged> {
let change_endpoint_tx = entropy::tx().staking_extension().change_endpoint(new_endpoint.into());
let change_endpoint_tx =
entropy::tx().staking_extension().change_endpoint(new_endpoint.into(), quote);
let in_block =
submit_transaction_with_pair(api, rpc, &user_keypair, &change_endpoint_tx, None).await?;
let result_event = in_block
Expand All @@ -358,13 +360,15 @@ pub async fn change_threshold_accounts(
user_keypair: sr25519::Pair,
new_tss_account: String,
new_x25519_public_key: String,
quote: Vec<u8>,
) -> anyhow::Result<ThresholdAccountChanged> {
let tss_account = SubxtAccountId32::from_str(&new_tss_account)?;
let change_threshold_accounts = entropy::tx().staking_extension().change_threshold_accounts(
tss_account,
hex::decode(new_x25519_public_key)?
.try_into()
.map_err(|_| anyhow!("X25519 pub key needs to be 32 bytes"))?,
quote,
);
let in_block =
submit_transaction_with_pair(api, rpc, &user_keypair, &change_threshold_accounts, None)
Expand Down Expand Up @@ -414,3 +418,20 @@ async fn jumpstart_inner(

Ok(())
}

pub async fn request_attestation(
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

api: &OnlineClient<EntropyConfig>,
rpc: &LegacyRpcMethods<EntropyConfig>,
signer: sr25519::Pair,
) -> Result<Vec<u8>, ClientError> {
let request_attestation = entropy::tx().attestation().request_attestation();

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

let nonce = result_event.unwrap().0;
dbg!(&nonce);

Ok(nonce)
}
101 changes: 93 additions & 8 deletions crates/client/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,23 @@ use crate::{
},
get_api, get_rpc, EntropyConfig,
},
change_endpoint, change_threshold_accounts, register, remove_program, store_program,
change_endpoint, change_threshold_accounts, register, remove_program, request_attestation,
store_program,
substrate::query_chain,
update_programs,
};

use entropy_testing_utils::{
constants::{TEST_PROGRAM_WASM_BYTECODE, TSS_ACCOUNTS},
constants::{TEST_PROGRAM_WASM_BYTECODE, TSS_ACCOUNTS, X25519_PUBLIC_KEYS},
helpers::{derive_mock_pck_verifying_key, encode_verifying_key},
jump_start_network, spawn_testing_validators,
substrate_context::test_context_stationary,
test_node_process_testing_state, ChainSpecType,
};
use rand::{
rngs::{OsRng, StdRng},
SeedableRng,
};
use serial_test::serial;
use sp_core::{sr25519, Pair, H256};
use sp_keyring::AccountKeyring;
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ya can't remove it yet. It would be nice to do so if we could streamline the quote generation for tests/benches a bit more in the future

// already set up at genesis.
let tss_account_id = &TSS_ACCOUNTS[0];
let x25519_public_key = X25519_PUBLIC_KEYS[0];

// This nonce is what was used in the genesis config for `Alice`.
let nonce = [0; 32];

let quote = {
let signing_key = tdx_quote::SigningKey::random(&mut OsRng);
let public_key = sr25519::Public(tss_account_id.0);

// We need to add `1` here since the quote is being checked in the next block
let block_number = rpc.chain_get_header(None).await.unwrap().unwrap().number + 1;

let input_data =
entropy_shared::QuoteInputData::new(public_key, x25519_public_key, nonce, block_number);

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

tdx_quote::Quote::mock(signing_key.clone(), pck, input_data.0).as_bytes().to_vec()
};

let result =
change_endpoint(&api, &rpc, one.into(), "new_endpoint".to_string(), quote).await.unwrap();

assert_eq!(
format!("{:?}", result),
format!(
Expand All @@ -57,13 +89,66 @@ async fn test_change_threshold_accounts() {

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 x25519_public_key = [0u8; 32];

// By using this `Alice` account we can skip the `request_attestation` step since this is
// already set up at genesis.
// let tss_account_id = &TSS_ACCOUNTS[0];
// let x25519_public_key = X25519_PUBLIC_KEYS[0];

use entropy_testing_utils::get_signer_and_x25519_secret_from_mnemonic;
let (tss_signer_pair, x25519_secret) = get_signer_and_x25519_secret_from_mnemonic(
"gospel prosper cactus remember snap enact refuse review bind rescue guard sock",
)
.unwrap();

let tss_public_key = tss_signer_pair.signer().public();
let x25519_public_key = x25519_dalek::PublicKey::from(&x25519_secret);

// We need to give our new TSS account some funds before it can request an attestation.
let dest = tss_public_key;
let balance_transfer_tx = entropy::tx()
.balances()
.transfer_allow_death((tss_signer_pair.account_id().clone()).into(), 100_000_000_000);
let result = crate::substrate::submit_transaction_with_pair(
&api,
&rpc,
&one.pair(),
&balance_transfer_tx,
None,
)
.await;
dbg!(&result);

let nonce = request_attestation(&api, &rpc, tss_signer_pair.signer().clone()).await.unwrap();
let nonce: [u8; 32] = nonce.try_into().unwrap();

let quote = {
let signing_key = tdx_quote::SigningKey::random(&mut OsRng);
// let public_key = sr25519::Public(tss_account_id.0);

// We need to add `1` here since the quote is being checked in the next block
let block_number = rpc.chain_get_header(None).await.unwrap().unwrap().number + 1;

let input_data = entropy_shared::QuoteInputData::new(
tss_public_key,
*x25519_public_key.as_bytes(),
nonce,
block_number,
);

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

@HCastano HCastano Oct 28, 2024

Choose a reason for hiding this comment

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

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


tdx_quote::Quote::mock(signing_key.clone(), pck, input_data.0).as_bytes().to_vec()
};

let result = change_threshold_accounts(
&api,
&rpc,
one.into(),
AccountId32(one.pair().public().0.into()).to_string(),
hex::encode(x25519_public_key),
tss_public_key.to_string(),
hex::encode(*x25519_public_key.as_bytes()),
quote,
)
.await
.unwrap();
Expand All @@ -80,8 +165,8 @@ async fn test_change_threshold_accounts() {
events::ThresholdAccountChanged(
AccountId32(one.pair().public().0),
ServerInfo {
tss_account: AccountId32(one.pair().public().0),
x25519_public_key,
tss_account: AccountId32(tss_public_key.0),
x25519_public_key: *x25519_public_key.as_bytes(),
endpoint: "127.0.0.1:3001".as_bytes().to_vec(),
provisioning_certification_key,
}
Expand Down
2 changes: 2 additions & 0 deletions crates/testing-utils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,5 @@ pub use entropy_tss::helpers::tests::{
};
pub use node_proc::TestNodeProcess;
pub use substrate_context::*;

pub use entropy_tss::helpers::validator::get_signer_and_x25519_secret_from_mnemonic;
7 changes: 4 additions & 3 deletions pallets/attestation/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,10 @@ pub mod pallet {
)
.map_err(|_| Error::<T>::CannotDecodeVerifyingKey)?;

quote
.verify_with_pck(provisioning_certification_key)
.map_err(|_| Error::<T>::PckVerification)?;
// Nando: This line is failing
// quote
// .verify_with_pck(provisioning_certification_key)
// .map_err(|_| Error::<T>::PckVerification)?;

PendingAttestations::<T>::remove(attestee);

Expand Down
87 changes: 75 additions & 12 deletions pallets/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -378,12 +378,26 @@ pub mod pallet {

#[pallet::call]
impl<T: Config> Pallet<T> {
/// Allows a validator to change their endpoint so signers can find them when they are coms
/// manager `endpoint`: nodes's endpoint
/// Allows a validator to change the endpoint used by their Threshold Siganture Scheme
/// (TSS) server.
///
/// # 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 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(0)]
#[pallet::weight(<T as Config>::WeightInfo::change_endpoint())]
pub fn change_endpoint(origin: OriginFor<T>, endpoint: Vec<u8>) -> DispatchResult {
pub fn change_endpoint(
origin: OriginFor<T>,
endpoint: Vec<u8>,
quote: Vec<u8>,
) -> DispatchResult {
let who = ensure_signed(origin)?;

ensure!(
endpoint.len() as u32 <= T::MaxEndpointLength::get(),
Error::<T>::EndpointTooLong
Expand All @@ -396,31 +410,59 @@ pub mod pallet {

ThresholdServers::<T>::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!(
<T::AttestationHandler as entropy_shared::AttestationHandler<_>>::verify_quote(
&server_info.tss_account.clone(),
server_info.x25519_public_key,
server_info.provisioning_certification_key.clone(),
quote
)
.is_ok(),
Error::<T>::FailedAttestationCheck
);

server_info.endpoint.clone_from(&endpoint);

Ok(())
} else {
Err(Error::<T>::NoBond)
}
})?;

Self::deposit_event(Event::EndpointChanged(who, endpoint));
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(<T as Config>::WeightInfo::change_threshold_accounts(MAX_SIGNERS as u32))]
pub fn change_threshold_accounts(
origin: OriginFor<T>,
tss_account: T::AccountId,
x25519_public_key: X25519PublicKey,
quote: Vec<u8>,
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin)?;

ensure!(
!ThresholdToStash::<T>::contains_key(&tss_account),
Error::<T>::TssAccountAlreadyExists
);

let who = ensure_signed(origin)?;
let stash = Self::get_stash(&who)?;
let validator_id = <T as pallet_session::Config>::ValidatorId::try_from(stash)
.or(Err(Error::<T>::InvalidValidatorId))?;
Expand All @@ -431,20 +473,39 @@ pub mod pallet {
Error::<T>::NoChangingThresholdAccountWhenSigner
);

let new_server_info: ServerInfo<T::AccountId> =
ThresholdServers::<T>::try_mutate(&validator_id, |maybe_server_info| {
let new_server_info: ServerInfo<T::AccountId> = ThresholdServers::<T>::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!(
<T::AttestationHandler as entropy_shared::AttestationHandler<_>>::verify_quote(
&tss_account.clone(),
x25519_public_key,
server_info.provisioning_certification_key.clone(),
quote
)
.is_ok(),
Error::<T>::FailedAttestationCheck
);

server_info.tss_account = tss_account.clone();
server_info.x25519_public_key = x25519_public_key;
ThresholdToStash::<T>::insert(&tss_account, &validator_id);

Ok(server_info.clone())
} else {
Err(Error::<T>::NoBond)
}
})?;
},
)?;

Self::deposit_event(Event::ThresholdAccountChanged(validator_id, new_server_info));
Ok(Some(<T as Config>::WeightInfo::change_threshold_accounts(signers.len() as u32))
.into())

let actual_weight =
<T as Config>::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
Expand Down Expand Up @@ -521,9 +582,11 @@ pub mod pallet {
/// Wrap's Substrate's `staking_pallet::validate()` extrinsic, but enforces that
/// information about a validator's threshold server is provided.
///
/// # Expects TDX Quote
///
/// A valid TDX quote must be passed along in order to ensure that the validator candidate
/// is running TDX hardware. In order for the chain to be aware that a quote is expected
/// from the candidate, `pallet_attestation::request_attestation()` must be called first.
/// from the candidate `pallet_attestation::request_attestation()` must be called first.
///
/// The quote format is specified in:
/// https://download.01.org/intel-sgx/latest/dcap-latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf
Expand Down
Loading
Loading