Skip to content

Commit

Permalink
Merge branch 'leon/master_key_signing_subnet_invariants' into 'master'
Browse files Browse the repository at this point in the history 
chore(registry): key signing subnet invariants

This MR adds a new invariant check that conects the `ChainKeyConfigs` with the chain key signing subnet lists.
Before, we would only check whether configs mentioned in the ecdsa_signing_subnet_list would match a EcdsaKeyConfig.
This is ok, since the Ecdsa data is still the source of truth.

In the future, we want to change the source of truth, and therefore, we need to do the same check on the `ChainKey` side as well.

This MR also adapts some tests, such that they pass under the new invariant as well.

Closes NNS1-3007 

Closes NNS1-3007

See merge request dfinity-lab/public/ic!19424
  • Loading branch information
@Sawchord
Sawchord committed May 28, 2024
2 parents a16c808 + 1e92a8c commit 07ca24a
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 14 deletions.
3 changes: 3 additions & 0 deletions rs/registry/canister/src/invariants/checks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,7 @@ mod tests {
|subnet_id, snapshot| {
vec![
apply_to_subnet_record(snapshot, subnet_id, |record| {
record.chain_key_config = Some(ChainKeyConfig::from(config.clone()));
record.ecdsa_config = Some(config);
}),
upsert_ck(ck_key_id.clone(), &[subnet_id]),
Expand Down Expand Up @@ -988,9 +989,11 @@ mod tests {
|subnet_id, snapshot| {
vec![
apply_to_subnet_record(snapshot, subnet_id, |record| {
record.chain_key_config = Some(ChainKeyConfig::from(config.clone()));
record.ecdsa_config = Some(config.clone());
}),
apply_to_subnet_record(snapshot, subnet_test_id(1), |record| {
record.chain_key_config = Some(ChainKeyConfig::from(config.clone()));
record.ecdsa_config = Some(config);
}),
upsert_ecdsa(ecdsa_key_id.clone(), &[subnet_test_id(1)]),
Expand Down
75 changes: 72 additions & 3 deletions rs/registry/canister/src/invariants/crypto.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,11 @@ use crate::invariants::{
};
use ic_base_types::{subnet_id_try_from_protobuf, NodeId, SubnetId};
use ic_crypto_utils_ni_dkg::extract_subnet_threshold_sig_public_key;
use ic_protobuf::registry::crypto::v1::{PublicKey, X509PublicKeyCert};
use ic_protobuf::registry::crypto::v1::{MasterPublicKeyId, PublicKey, X509PublicKeyCert};
use ic_protobuf::registry::subnet::v1::{CatchUpPackageContents, SubnetRecord};
use ic_registry_keys::{
get_ecdsa_key_id_from_signing_subnet_list_key, make_catch_up_package_contents_key,
get_ecdsa_key_id_from_signing_subnet_list_key,
get_master_public_key_id_from_signing_subnet_list_key, make_catch_up_package_contents_key,
make_crypto_threshold_signing_pubkey_key, make_node_record_key, make_subnet_record_key,
maybe_parse_crypto_node_key, maybe_parse_crypto_tls_cert_key, CRYPTO_RECORD_KEY_PREFIX,
CRYPTO_TLS_CERT_KEY_PREFIX, NODE_RECORD_KEY_PREFIX,
Expand All @@ -27,6 +28,8 @@ use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};
use dfn_core::println;
use ic_crypto_utils_basic_sig::conversions::derive_node_id;

use super::common::get_all_chain_key_signing_subnet_list_records;

#[cfg(test)]
mod tests;

Expand Down Expand Up @@ -65,6 +68,7 @@ pub(crate) fn check_node_crypto_keys_invariants(
check_no_orphaned_node_crypto_records(snapshot)?;
check_chain_key_configs(snapshot)?;
check_ecdsa_signing_subnet_lists(snapshot)?;
check_chain_key_signing_subnet_lists(snapshot)?;
check_high_threshold_public_key_matches_the_one_in_cup(snapshot)?;
Ok(())
}
Expand Down Expand Up @@ -315,7 +319,7 @@ fn check_chain_key_configs(snapshot: &RegistrySnapshot) -> Result<(), InvariantC
Ok(())
}

//TODO(NNS1-3039): extend to check chain key signing subnet lists
// TODO[NNS1-2986]: Remove this function after the migration has been performed.
fn check_ecdsa_signing_subnet_lists(
snapshot: &RegistrySnapshot,
) -> Result<(), InvariantCheckError> {
Expand Down Expand Up @@ -373,6 +377,71 @@ fn check_ecdsa_signing_subnet_lists(
})
}

/// Checks that the chain key signing subnet list is consistent with the chain key configurations
///
/// In particular, this function checks:
/// - That every subnet refered to by the signing subnet list exists
/// - That the subnet has a chain key configuration that contains the corresponding key
fn check_chain_key_signing_subnet_lists(
snapshot: &RegistrySnapshot,
) -> Result<(), InvariantCheckError> {
let subnet_records_map = get_subnet_records_map(snapshot);

get_all_chain_key_signing_subnet_list_records(snapshot)
.iter()
.try_for_each(|(key_id, chain_key_signing_subnet_list)| {
let master_key_id = get_master_public_key_id_from_signing_subnet_list_key(key_id)
.map_err(|err| InvariantCheckError {
msg: format!(
"Registry key_id {} could not be converted to an MasterPublicKeyId",
key_id,
),
source: Some(Box::new(err)),
})?;

chain_key_signing_subnet_list
.subnets
.iter()
.try_for_each(|subnet_id_bytes| {
let subnet_id = subnet_id_try_from_protobuf(subnet_id_bytes.clone())
.map_err(|err| InvariantCheckError {
msg: "Failed to deserialize subnet id from protobuf".to_string(),
source: Some(Box::new(err)),
})?;

if subnet_records_map
.get(&make_subnet_record_key(subnet_id).into_bytes())
.ok_or(InvariantCheckError {
msg: format!(
"A non-existent subnet {} was set as the holder of a key_id {}",
subnet_id, key_id
),
source: None,
})?
.chain_key_config
.as_ref()
.ok_or(InvariantCheckError {
msg: format!("The subnet {} does not have a ChainKeyConfig", subnet_id),
source: None,
})?
.key_configs
.iter()
.filter_map(|config| config.key_id.clone())
.any(|key| key == MasterPublicKeyId::from(&master_key_id)) {
Ok(())
} else {
Err(InvariantCheckError {
msg: format!(
"The subnet {} does not have the key with {} in its chain key configurations",
subnet_id, key_id
),
source: None,
})
}
})
})
}

fn check_no_orphaned_node_crypto_records(
snapshot: &RegistrySnapshot,
) -> Result<(), InvariantCheckError> {
Expand Down
22 changes: 11 additions & 11 deletions rs/registry/canister/tests/common/test_helpers.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use ic_registry_keys::{
};
use ic_registry_proto_data_provider::ProtoRegistryDataProvider;
use ic_registry_routing_table::RoutingTable;
use ic_registry_subnet_features::{EcdsaConfig, DEFAULT_ECDSA_MAX_QUEUE_SIZE};
use ic_registry_subnet_features::{ChainKeyConfig, EcdsaConfig, DEFAULT_ECDSA_MAX_QUEUE_SIZE};
use ic_registry_transport::pb::v1::RegistryAtomicMutateRequest;
use ic_types::ReplicaVersion;
use registry_canister::init::RegistryCanisterInitPayloadBuilder;
Expand Down Expand Up @@ -59,16 +59,16 @@ pub fn get_subnet_holding_ecdsa_keys(
..Default::default()
}
.into();
record.ecdsa_config = Some(
EcdsaConfig {
quadruples_to_create_in_advance: 1,
key_ids: ecdsa_key_ids.to_vec(),
max_queue_size: Some(DEFAULT_ECDSA_MAX_QUEUE_SIZE),
signature_request_timeout_ns: None,
idkg_key_rotation_period_ms: None,
}
.into(),
);

let ecdsa_config = EcdsaConfig {
quadruples_to_create_in_advance: 1,
key_ids: ecdsa_key_ids.to_vec(),
max_queue_size: Some(DEFAULT_ECDSA_MAX_QUEUE_SIZE),
signature_request_timeout_ns: None,
idkg_key_rotation_period_ms: None,
};
record.chain_key_config = Some(ChainKeyConfig::from(ecdsa_config.clone()).into());
record.ecdsa_config = Some(ecdsa_config.into());

record
}
Expand Down

0 comments on commit 07ca24a

Please sign in to comment.