Skip to content

Commit

Permalink
Merge branch 'eichhorl/ck-conf-invariant' into 'master'
Browse files Browse the repository at this point in the history
feat(registry): NNS1-3050 NNS1-3051 Add registry invariant for `ChainKeyConfig`

We add a registry invariant for chain key configs, which asserts:
1. The subnet record entry can be deserialized (all mandatory fields exist)
2. The number of pre-signatures to create in advance cannot be zero
3. There are no duplicate key IDs present for each subnet.

Additionally, we extend `ic-admin` to read the new chain key config and subnet signing list. 

See merge request dfinity-lab/public/ic!19118
  • Loading branch information
eichhorl committed May 15, 2024
2 parents 9eca78e + 9b8c978 commit e38345f
Show file tree
Hide file tree
Showing 9 changed files with 335 additions and 38 deletions.
26 changes: 24 additions & 2 deletions rs/registry/admin/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,8 @@ use ic_protobuf::registry::{
};
use ic_registry_client::client::RegistryClientImpl;
use ic_registry_client_helpers::{
crypto::CryptoRegistry, deserialize_registry_value, ecdsa_keys::EcdsaKeysRegistry,
hostos_version::HostosRegistry, subnet::SubnetRegistry,
chain_keys::ChainKeysRegistry, crypto::CryptoRegistry, deserialize_registry_value,
ecdsa_keys::EcdsaKeysRegistry, hostos_version::HostosRegistry, subnet::SubnetRegistry,
};
use ic_registry_keys::{
get_node_operator_id_from_record_key, get_node_record_node_id, is_node_operator_record_key,
Expand Down Expand Up @@ -444,6 +444,8 @@ enum SubCommand {
ProposeToInsertSnsWasmUpgradePathEntries(ProposeToInsertSnsWasmUpgradePathEntriesCmd),
/// Get the ECDSA key ids and their signing subnets
GetEcdsaSigningSubnets,
/// Get the Master public key ids and their signing subnets
GetChainKeySigningSubnets,
/// Propose to update the list of SNS Subnet IDs that SNS-WASM deploys SNS instances to
ProposeToUpdateSnsSubnetIdsInSnsWasm(ProposeToUpdateSnsSubnetIdsInSnsWasmCmd),
/// Propose to update the list of Principals that are allowed to deploy SNS instances
Expand Down Expand Up @@ -4793,6 +4795,26 @@ async fn main() {
println!("KeyId {:?}: {:?}", key_id, subnets);
}
}
SubCommand::GetChainKeySigningSubnets => {
let registry_client = make_registry_client(
reachable_nns_urls,
opts.verify_nns_responses,
opts.nns_public_key_pem_file,
);

// maximum number of retries, let the user ctrl+c if necessary
registry_client
.try_polling_latest_version(usize::MAX)
.unwrap();

let signing_subnets = registry_client
.get_chain_key_signing_subnets(registry_client.get_latest_version())
.unwrap()
.unwrap();
for (key_id, subnets) in signing_subnets.iter() {
println!("KeyId {:?}: {:?}", key_id, subnets);
}
}
SubCommand::ProposeToUpdateElectedReplicaVersions(cmd)
| SubCommand::ProposeToReviseElectedGuestosVersions(cmd) => {
let (proposer, sender) = cmd.proposer_and_sender(sender);
Expand Down
6 changes: 4 additions & 2 deletions rs/registry/admin/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,10 @@ impl From<&SubnetRecordProto> for SubnetRecord {
.ecdsa_config
.as_ref()
.map(|c| c.clone().try_into().unwrap()),
// TODO[NNS1-2969]: Use this field rather than ecdsa_config.
chain_key_config: None,
chain_key_config: value
.chain_key_config
.as_ref()
.map(|c| c.clone().try_into().unwrap()),
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion rs/registry/canister/src/invariants/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ mod tests {
key_configs: vec![KeyConfig {
key_id: Some(MasterPublicKeyId {
key_id: Some(master_public_key_id::KeyId::Ecdsa(EcdsaKeyId {
curve: 0,
curve: 1,
name: "test_curve_1".to_string(),
})),
}),
Expand Down
60 changes: 58 additions & 2 deletions rs/registry/canister/src/invariants/crypto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,18 @@ 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::subnet::v1::CatchUpPackageContents;
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,
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,
};
use ic_registry_subnet_features::ChainKeyConfig;
use ic_types::crypto::threshold_sig::ThresholdSigPublicKey;
use ic_types::crypto::KeyPurpose;
use prost::Message;
use std::collections::{BTreeMap, HashMap, HashSet};
use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet};

#[cfg(target_arch = "wasm32")]
use dfn_core::println;
Expand Down Expand Up @@ -62,6 +63,7 @@ pub(crate) fn check_node_crypto_keys_invariants(
) -> Result<(), InvariantCheckError> {
check_node_crypto_keys_exist_and_are_unique(snapshot)?;
check_no_orphaned_node_crypto_records(snapshot)?;
check_chain_key_configs(snapshot)?;
check_ecdsa_signing_subnet_lists(snapshot)?;
check_high_threshold_public_key_matches_the_one_in_cup(snapshot)?;
Ok(())
Expand Down Expand Up @@ -259,6 +261,60 @@ fn get_all_nodes_public_keys_and_certs(
Ok((pks, certs))
}

fn check_chain_key_configs(snapshot: &RegistrySnapshot) -> Result<(), InvariantCheckError> {
let mut subnet_records_map = get_subnet_records_map(snapshot);
let subnet_id_list = get_subnet_ids_from_snapshot(snapshot);
for subnet_id in subnet_id_list {
// Subnets in the subnet list have a subnet record
let subnet_record: SubnetRecord = subnet_records_map
.remove(&make_subnet_record_key(subnet_id).into_bytes())
.unwrap_or_else(|| {
panic!(
"Subnet {:} is in subnet list but no record exists",
subnet_id
)
});

let Some(chain_key_config_pb) = subnet_record.chain_key_config else {
continue;
};

let chain_key_config =
ChainKeyConfig::try_from(chain_key_config_pb.clone()).map_err(|err| {
InvariantCheckError {
msg: format!(
"ChainKeyConfig {:?} of subnet {:} could not be deserialized: {}",
chain_key_config_pb, subnet_id, err,
),
source: None,
}
})?;

let mut key_ids = BTreeSet::new();
for key_config in chain_key_config.key_configs {
if key_config.pre_signatures_to_create_in_advance == 0 {
return Err(InvariantCheckError {
msg: format!(
"`pre_signatures_to_create_in_advance` of subnet {:} cannot be zero.",
subnet_id,
),
source: None,
});
}
if !key_ids.insert(key_config.key_id.clone()) {
return Err(InvariantCheckError {
msg: format!(
"ChainKeyConfig of subnet {:} contains multiple entries for key ID {}.",
subnet_id, key_config.key_id,
),
source: None,
});
}
}
}
Ok(())
}

//TODO(NNS1-3039): extend to check chain key signing subnet lists
fn check_ecdsa_signing_subnet_lists(
snapshot: &RegistrySnapshot,
Expand Down
155 changes: 154 additions & 1 deletion rs/registry/canister/src/invariants/crypto/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -599,14 +599,167 @@ fn run_test_orphaned_crypto_keys(

mod ecdsa_signing_subnet_lists {
use super::*;
use crate::{
common::test_helpers::invariant_compliant_registry, mutations::common::encode_or_panic,
};
use ic_base_types::{subnet_id_into_protobuf, SubnetId};
use ic_management_canister_types::{EcdsaCurve, EcdsaKeyId};
use ic_protobuf::registry::crypto::v1::EcdsaSigningSubnetList;
use ic_protobuf::registry::subnet::v1::{EcdsaConfig, SubnetRecord};
use ic_protobuf::registry::crypto::v1::{self as pb, master_public_key_id, MasterPublicKeyId};
use ic_protobuf::registry::subnet::v1::{ChainKeyConfig, EcdsaConfig, KeyConfig, SubnetRecord};
use ic_registry_transport::pb::v1::RegistryMutation;
use ic_registry_transport::upsert;
use ic_test_utilities_types::ids::{node_test_id, subnet_test_id};
use ic_types::PrincipalId;
use rand::Rng;

fn invariant_compliant_chain_key_config() -> ChainKeyConfig {
ChainKeyConfig {
key_configs: vec![
KeyConfig {
key_id: Some(MasterPublicKeyId {
key_id: Some(master_public_key_id::KeyId::Ecdsa(pb::EcdsaKeyId {
curve: 1,
name: "ecdsa_key".to_string(),
})),
}),
pre_signatures_to_create_in_advance: Some(456),
max_queue_size: Some(100),
},
KeyConfig {
key_id: Some(MasterPublicKeyId {
key_id: Some(master_public_key_id::KeyId::Schnorr(pb::SchnorrKeyId {
algorithm: 1,
name: "schnorr_key".to_string(),
})),
}),
pre_signatures_to_create_in_advance: Some(456),
max_queue_size: Some(100),
},
],
signature_request_timeout_ns: Some(10_000),
idkg_key_rotation_period_ms: Some(20_000),
}
}

fn check_chain_key_config_invariant(config: ChainKeyConfig) {
let registry = invariant_compliant_registry(0);

let list = registry.get_subnet_list_record();
let nns_id = SubnetId::from(PrincipalId::try_from(list.subnets.first().unwrap()).unwrap());
let mut subnet = registry.get_subnet_or_panic(nns_id);
subnet.chain_key_config = Some(config);

let new_subnet = upsert(
make_subnet_record_key(nns_id).into_bytes(),
encode_or_panic(&subnet),
);
registry.check_global_state_invariants(&[new_subnet]);
}

#[test]
fn should_succeed_with_invariant_compliant_config() {
check_chain_key_config_invariant(invariant_compliant_chain_key_config());
}

#[test]
#[should_panic(
expected = "Missing required struct field: KeyConfig::pre_signatures_to_create_in_advance"
)]
fn should_fail_if_missing_pre_signatures() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs[1].pre_signatures_to_create_in_advance = None;
check_chain_key_config_invariant(config);
}

#[test]
#[should_panic(
expected = "`pre_signatures_to_create_in_advance` of subnet ya35z-hhham-aaaaa-aaaap-yai cannot be zero."
)]
fn should_fail_if_pre_signatures_is_zero() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs[0].pre_signatures_to_create_in_advance = Some(0);
check_chain_key_config_invariant(config);
}

#[test]
#[should_panic(expected = "Missing required struct field: KeyConfig::max_queue_size")]
fn should_fail_if_missing_queue_size() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs[1].max_queue_size = None;
check_chain_key_config_invariant(config);
}

#[test]
#[should_panic(expected = "Missing required struct field: KeyConfig::key_id")]
fn should_fail_if_missing_key_id() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs[1].key_id = None;
check_chain_key_config_invariant(config);
}

#[test]
#[should_panic(expected = "Unable to convert 2 to an EcdsaCurve")]
fn should_fail_if_unkown_ecdsa_curve() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs[1].key_id = Some(MasterPublicKeyId {
key_id: Some(master_public_key_id::KeyId::Ecdsa(pb::EcdsaKeyId {
curve: 2,
name: "ecdsa_key".to_string(),
})),
});
check_chain_key_config_invariant(config);
}

#[test]
#[should_panic(expected = "Unable to convert 3 to a SchnorrAlgorithm")]
fn should_fail_if_unkown_schnorr_algorithm() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs[1].key_id = Some(MasterPublicKeyId {
key_id: Some(master_public_key_id::KeyId::Schnorr(pb::SchnorrKeyId {
algorithm: 3,
name: "schnorr_key".to_string(),
})),
});
check_chain_key_config_invariant(config);
}

#[test]
#[should_panic(expected = "Unable to convert Unspecified to an EcdsaCurve")]
fn should_fail_if_unspecified_ecdsa_curve() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs[1].key_id = Some(MasterPublicKeyId {
key_id: Some(master_public_key_id::KeyId::Ecdsa(pb::EcdsaKeyId {
curve: 0,
name: "ecdsa_key".to_string(),
})),
});
check_chain_key_config_invariant(config);
}

#[test]
#[should_panic(expected = "Unable to convert Unspecified to a SchnorrAlgorithm")]
fn should_fail_if_unspecified_schnorr_algorithm() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs[1].key_id = Some(MasterPublicKeyId {
key_id: Some(master_public_key_id::KeyId::Schnorr(pb::SchnorrKeyId {
algorithm: 0,
name: "schnorr_key".to_string(),
})),
});
check_chain_key_config_invariant(config);
}

#[test]
#[should_panic(
expected = "ChainKeyConfig of subnet ya35z-hhham-aaaaa-aaaap-yai contains multiple entries for key ID schnorr:Bip340Secp256k1:schnorr_key."
)]
fn should_fail_if_duplicate_key_ids() {
let mut config = invariant_compliant_chain_key_config();
config.key_configs.push(config.key_configs[1].clone());
check_chain_key_config_invariant(config);
}

#[test]
fn should_succeed_for_valid_snapshot() {
let setup = Setup::builder()
Expand Down
2 changes: 1 addition & 1 deletion rs/registry/helpers/src/chain_keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::collections::BTreeMap;

use ic_interfaces_registry::{RegistryClient, RegistryClientResult};
use ic_management_canister_types::MasterPublicKeyId;
use ic_protobuf::registry::crypto::v1::EcdsaSigningSubnetList;
use ic_protobuf::registry::crypto::v1::ChainKeySigningSubnetList;
use ic_registry_keys::{
get_master_public_key_id_from_signing_subnet_list_key, CHAIN_KEY_SIGNING_SUBNET_LIST_KEY_PREFIX,
};
Expand Down
1 change: 1 addition & 0 deletions rs/registry/helpers/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
//! to the respective crate/component at some point in the future.
pub mod api_boundary_node;
pub mod chain_keys;
pub mod crypto;
pub mod ecdsa_keys;
pub mod firewall;
Expand Down
24 changes: 23 additions & 1 deletion rs/registry/helpers/src/subnet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ use ic_registry_keys::{
make_catch_up_package_contents_key, make_node_record_key, make_replica_version_key,
make_subnet_list_record_key, make_subnet_record_key, ROOT_SUBNET_ID_KEY,
};
use ic_registry_subnet_features::{EcdsaConfig, SubnetFeatures};
use ic_registry_subnet_features::{ChainKeyConfig, EcdsaConfig, SubnetFeatures};
use ic_types::{
registry::RegistryClientError::DecodeError, Height, NodeId, PrincipalId,
PrincipalIdBlobParseError, RegistryVersion, ReplicaVersion, SubnetId,
Expand Down Expand Up @@ -90,6 +90,13 @@ pub trait SubnetRegistry {
version: RegistryVersion,
) -> RegistryClientResult<EcdsaConfig>;

/// Returns chain key config
fn get_chain_key_config(
&self,
subnet_id: SubnetId,
version: RegistryVersion,
) -> RegistryClientResult<ChainKeyConfig>;

/// Returns notarization delay settings:
/// - the unit delay for blockmaker;
/// - the initial delay for notary, to give time to rank-0 block
Expand Down Expand Up @@ -286,6 +293,21 @@ impl<T: RegistryClient + ?Sized> SubnetRegistry for T {
Ok(subnet.and_then(|subnet| subnet.ecdsa_config.map(|config| config.try_into().unwrap())))
}

fn get_chain_key_config(
&self,
subnet_id: SubnetId,
version: RegistryVersion,
) -> RegistryClientResult<ChainKeyConfig> {
let bytes = self.get_value(&make_subnet_record_key(subnet_id), version);
let subnet = deserialize_registry_value::<SubnetRecord>(bytes)?;
subnet
.and_then(|subnet| subnet.chain_key_config.map(ChainKeyConfig::try_from))
.transpose()
.map_err(|err| DecodeError {
error: format!("get_chain_key_config() failed with {}", err),
})
}

fn get_notarization_delay_settings(
&self,
subnet_id: SubnetId,
Expand Down
Loading

0 comments on commit e38345f

Please sign in to comment.