From 6c0cf23830d74c022ab121053da050059bd307e0 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Wed, 1 Mar 2023 13:47:01 +0100 Subject: [PATCH 1/4] add version checks on para ugprade --- pallets/parachain-system/src/lib.rs | 117 ++++++++++++++++++++++---- pallets/parachain-system/src/tests.rs | 37 +++++++- 2 files changed, 135 insertions(+), 19 deletions(-) diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 456ae5bf578..7cfd23c8712 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -16,18 +16,18 @@ #![cfg_attr(not(feature = "std"), no_std)] -//! cumulus-pallet-parachain-system is a base pallet for cumulus-based parachains. +//! `cumulus-pallet-parachain-system` is a base pallet for Cumulus-based parachains. //! -//! This pallet handles low-level details of being a parachain. It's responsibilities include: +//! This pallet handles low-level details of being a parachain. Its responsibilities include: //! -//! - ingestion of the parachain validation data -//! - ingestion of incoming downward and lateral messages and dispatching them -//! - coordinating upgrades with the relay-chain -//! - communication of parachain outputs, such as sent messages, signalling an upgrade, etc. +//! - ingestion of the parachain validation data; +//! - ingestion and dispatch of incoming downward and lateral messages; +//! - coordinating upgrades with the Relay Chain; and +//! - communication of parachain outputs, such as sent messages, signaling an upgrade, etc. //! //! Users must ensure that they register this pallet as an inherent provider. -use codec::Encode; +use codec::{Decode, Encode, MaxEncodedLen}; use cumulus_primitives_core::{ relay_chain, AbridgedHostConfiguration, ChannelStatus, CollationInfo, DmpMessageHandler, GetChannelInfo, InboundDownwardMessage, InboundHrmpMessage, MessageSendError, @@ -45,6 +45,7 @@ use frame_support::{ }; use frame_system::{ensure_none, ensure_root}; use polkadot_parachain::primitives::RelayChainBlockNumber; +use scale_info::TypeInfo; use sp_runtime::{ traits::{Block as BlockT, BlockNumberProvider, Hash}, transaction_validity::{ @@ -53,6 +54,7 @@ use sp_runtime::{ }, }; use sp_std::{cmp, collections::btree_map::BTreeMap, prelude::*}; +use sp_version::RuntimeVersion; use xcm::latest::XcmHash; mod migration; @@ -134,6 +136,36 @@ impl CheckAssociatedRelayNumber for AnyRelayNumber { fn check_associated_relay_number(_: RelayChainBlockNumber, _: RelayChainBlockNumber) {} } +/// Information needed when a new runtime binary is submitted and needs to be authorized before +/// replacing the current runtime. +#[derive(Decode, Encode, Default, PartialEq, Eq, MaxEncodedLen)] +struct CodeUpgradeAuthorization +where + T: Config, +{ + /// Hash of the new runtime binary. + code_hash: T::Hash, + /// Whether or not to carry out version checks. + check_version: bool, +} + +impl TypeInfo for CodeUpgradeAuthorization +where + T: Config, +{ + type Identity = Self; + + fn type_info() -> scale_info::Type { + scale_info::Type::builder() + .path(scale_info::Path::new("CodeUpgradeAuthorization", module_path!())) + .composite(scale_info::build::Fields::unnamed().field(|f| { + f.ty::().docs(&[ + "Raw code upgrade authorization bytes, encodes code_hash + check_version", + ]) + })) + } +} + #[frame_support::pallet] pub mod pallet { use super::*; @@ -442,17 +474,40 @@ pub mod pallet { Ok(()) } + /// Authorize an upgrade to a given `code_hash` for the runtime. The runtime can be supplied + /// later. + /// + /// The `check_version` parameter sets a boolean flag for whether or not the runtime's spec + /// version and name should be verified on upgrade. Since the authorization only has a hash, + /// it cannot actually perform the verification. + /// + /// This call requires Root origin. #[pallet::call_index(2)] #[pallet::weight((1_000_000, DispatchClass::Operational))] - pub fn authorize_upgrade(origin: OriginFor, code_hash: T::Hash) -> DispatchResult { + pub fn authorize_upgrade( + origin: OriginFor, + code_hash: T::Hash, + check_version: bool, + ) -> DispatchResult { ensure_root(origin)?; - - AuthorizedUpgrade::::put(&code_hash); + AuthorizedUpgrade::::put(CodeUpgradeAuthorization { + code_hash: code_hash.clone(), + check_version, + }); Self::deposit_event(Event::UpgradeAuthorized { code_hash }); Ok(()) } + /// Provide the preimage (runtime binary) `code` for an upgrade that has been authorized. + /// + /// If the authorization required a version check, this call will ensure the spec name + /// remains unchanged and that the spec version has increased. + /// + /// Note that this function will not apply the new `code`, but only attempt to schedule the + /// upgrade with the Relay Chain. + /// + /// The origin for this call should be unsigned. #[pallet::call_index(3)] #[pallet::weight(1_000_000)] pub fn enact_authorized_upgrade( @@ -487,16 +542,16 @@ pub mod pallet { #[pallet::error] pub enum Error { - /// Attempt to upgrade validation function while existing upgrade pending + /// Attempt to upgrade validation function while existing upgrade pending. OverlappingUpgrades, - /// Polkadot currently prohibits this parachain from upgrading its validation function + /// Polkadot currently prohibits this parachain from upgrading its validation function. ProhibitedByPolkadot, /// The supplied validation function has compiled into a blob larger than Polkadot is - /// willing to run + /// willing to run. TooBig, - /// The inherent which supplies the validation data did not run this block + /// The inherent which supplies the validation data did not run this block. ValidationDataNotAvailable, - /// The inherent which supplies the host configuration did not run this block + /// The inherent which supplies the host configuration did not run this block. HostConfigurationNotAvailable, /// No validation function upgrade is currently scheduled. NotScheduled, @@ -504,6 +559,13 @@ pub mod pallet { NothingAuthorized, /// The given code upgrade has not been authorized. Unauthorized, + /// Failed to extract (or decode) the runtime version from the new runtime. + FailedToExtractRuntimeVersion, + /// The name of specification for the new runtime does not match the current runtime's. + InvalidSpecName, + /// The specification version of the new runtime is not allowed to decrease from or remain + /// equal to the current runtime's. + SpecVersionNeedsToIncrease, } /// In case of a scheduled upgrade, this storage field contains the validation code to be applied. @@ -645,7 +707,7 @@ pub mod pallet { /// The next authorized upgrade, if there is one. #[pallet::storage] - pub(super) type AuthorizedUpgrade = StorageValue<_, T::Hash>; + pub(super) type AuthorizedUpgrade = StorageValue<_, CodeUpgradeAuthorization>; /// A custom head data that should be returned as result of `validate_block`. /// @@ -712,9 +774,28 @@ pub mod pallet { impl Pallet { fn validate_authorized_upgrade(code: &[u8]) -> Result { - let required_hash = AuthorizedUpgrade::::get().ok_or(Error::::NothingAuthorized)?; + let authorization = AuthorizedUpgrade::::get().ok_or(Error::::NothingAuthorized)?; + + // ensure that the actual hash matches the authorized hash let actual_hash = T::Hashing::hash(&code[..]); - ensure!(actual_hash == required_hash, Error::::Unauthorized); + ensure!(actual_hash == authorization.code_hash, Error::::Unauthorized); + + // check versions if required as part of the authorization + if authorization.check_version { + let current_version = T::Version::get(); + let new_version = sp_io::misc::runtime_version(code) + .and_then(|v| RuntimeVersion::decode(&mut &v[..]).ok()) + .ok_or(Error::::FailedToExtractRuntimeVersion)?; + + if new_version.spec_name != current_version.spec_name { + return Err(Error::::InvalidSpecName.into()) + } + + if new_version.spec_version <= current_version.spec_version { + return Err(Error::::SpecVersionNeedsToIncrease.into()) + } + } + Ok(actual_hash) } } diff --git a/pallets/parachain-system/src/tests.rs b/pallets/parachain-system/src/tests.rs index 871596ae8cb..13077453117 100755 --- a/pallets/parachain-system/src/tests.rs +++ b/pallets/parachain-system/src/tests.rs @@ -32,10 +32,11 @@ use frame_support::{ use frame_system::RawOrigin; use hex_literal::hex; use relay_chain::HrmpChannelId; -use sp_core::H256; +use sp_core::{blake2_256, H256}; use sp_runtime::{ testing::Header, traits::{BlakeTwo256, IdentityLookup}, + DispatchErrorWithPostInfo, }; use sp_version::RuntimeVersion; use std::cell::RefCell; @@ -974,3 +975,37 @@ fn test() { .add(1, || {}) .add(2, || {}); } + +#[test] +fn upgrade_version_checks_should_work() { + let test_data = vec![ + ("test", 0, 1, Err(Error::::SpecVersionNeedsToIncrease)), + ("test", 1, 0, Err(Error::::SpecVersionNeedsToIncrease)), + ("test", 1, 1, Err(Error::::SpecVersionNeedsToIncrease)), + ("test", 1, 2, Err(Error::::SpecVersionNeedsToIncrease)), + ("test2", 1, 1, Err(Error::::InvalidSpecName)), + ]; + + for (spec_name, spec_version, impl_version, expected) in test_data.into_iter() { + let version = RuntimeVersion { + spec_name: spec_name.into(), + spec_version, + impl_version, + ..Default::default() + }; + let read_runtime_version = ReadRuntimeVersion(version.encode()); + + let mut ext = new_test_ext(); + ext.register_extension(sp_core::traits::ReadRuntimeVersionExt::new(read_runtime_version)); + ext.execute_with(|| { + let new_code = vec![1, 2, 3, 4]; + let new_code_hash = sp_core::H256(blake2_256(&new_code)); + + let _authorize = + ParachainSystem::authorize_upgrade(RawOrigin::Root.into(), new_code_hash, true); + let res = ParachainSystem::enact_authorized_upgrade(RawOrigin::None.into(), new_code); + + assert_eq!(expected.map_err(DispatchErrorWithPostInfo::from), res); + }); + } +} From b51c167f28d03847ae12f4c79e0ddba02f2d07a7 Mon Sep 17 00:00:00 2001 From: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Date: Wed, 1 Mar 2023 14:18:31 +0100 Subject: [PATCH 2/4] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Bastian Köcher --- pallets/parachain-system/src/lib.rs | 35 ++++------------------------- 1 file changed, 4 insertions(+), 31 deletions(-) diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 7cfd23c8712..4ff5cf82f5b 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -138,7 +138,8 @@ impl CheckAssociatedRelayNumber for AnyRelayNumber { /// Information needed when a new runtime binary is submitted and needs to be authorized before /// replacing the current runtime. -#[derive(Decode, Encode, Default, PartialEq, Eq, MaxEncodedLen)] +#[derive(Decode, Encode, Default, PartialEq, Eq, MaxEncodedLen, TypeInfo)] +#[scale_info(skip_type_params(T))] struct CodeUpgradeAuthorization where T: Config, @@ -149,23 +150,6 @@ where check_version: bool, } -impl TypeInfo for CodeUpgradeAuthorization -where - T: Config, -{ - type Identity = Self; - - fn type_info() -> scale_info::Type { - scale_info::Type::builder() - .path(scale_info::Path::new("CodeUpgradeAuthorization", module_path!())) - .composite(scale_info::build::Fields::unnamed().field(|f| { - f.ty::().docs(&[ - "Raw code upgrade authorization bytes, encodes code_hash + check_version", - ]) - })) - } -} - #[frame_support::pallet] pub mod pallet { use super::*; @@ -507,7 +491,7 @@ pub mod pallet { /// Note that this function will not apply the new `code`, but only attempt to schedule the /// upgrade with the Relay Chain. /// - /// The origin for this call should be unsigned. + /// All origins are allowed. #[pallet::call_index(3)] #[pallet::weight(1_000_000)] pub fn enact_authorized_upgrade( @@ -782,18 +766,7 @@ impl Pallet { // check versions if required as part of the authorization if authorization.check_version { - let current_version = T::Version::get(); - let new_version = sp_io::misc::runtime_version(code) - .and_then(|v| RuntimeVersion::decode(&mut &v[..]).ok()) - .ok_or(Error::::FailedToExtractRuntimeVersion)?; - - if new_version.spec_name != current_version.spec_name { - return Err(Error::::InvalidSpecName.into()) - } - - if new_version.spec_version <= current_version.spec_version { - return Err(Error::::SpecVersionNeedsToIncrease.into()) - } + frame_system::Pallet::::can_set_code(code)?; } Ok(actual_hash) From cff79a2334cdea8cab9793eeded0ca8b977964d2 Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Wed, 1 Mar 2023 16:22:18 +0100 Subject: [PATCH 3/4] remove unneeded imports and errors --- pallets/parachain-system/src/lib.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 4ff5cf82f5b..3a693b0911a 100644 --- a/pallets/parachain-system/src/lib.rs +++ b/pallets/parachain-system/src/lib.rs @@ -54,7 +54,6 @@ use sp_runtime::{ }, }; use sp_std::{cmp, collections::btree_map::BTreeMap, prelude::*}; -use sp_version::RuntimeVersion; use xcm::latest::XcmHash; mod migration; @@ -543,13 +542,6 @@ pub mod pallet { NothingAuthorized, /// The given code upgrade has not been authorized. Unauthorized, - /// Failed to extract (or decode) the runtime version from the new runtime. - FailedToExtractRuntimeVersion, - /// The name of specification for the new runtime does not match the current runtime's. - InvalidSpecName, - /// The specification version of the new runtime is not allowed to decrease from or remain - /// equal to the current runtime's. - SpecVersionNeedsToIncrease, } /// In case of a scheduled upgrade, this storage field contains the validation code to be applied. From 80d7b3fb4f0379dd0857a52e256cf600544f66cd Mon Sep 17 00:00:00 2001 From: joepetrowski Date: Wed, 1 Mar 2023 16:33:47 +0100 Subject: [PATCH 4/4] fix test error path --- pallets/parachain-system/src/tests.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pallets/parachain-system/src/tests.rs b/pallets/parachain-system/src/tests.rs index 13077453117..70e4c106bf2 100755 --- a/pallets/parachain-system/src/tests.rs +++ b/pallets/parachain-system/src/tests.rs @@ -979,11 +979,11 @@ fn test() { #[test] fn upgrade_version_checks_should_work() { let test_data = vec![ - ("test", 0, 1, Err(Error::::SpecVersionNeedsToIncrease)), - ("test", 1, 0, Err(Error::::SpecVersionNeedsToIncrease)), - ("test", 1, 1, Err(Error::::SpecVersionNeedsToIncrease)), - ("test", 1, 2, Err(Error::::SpecVersionNeedsToIncrease)), - ("test2", 1, 1, Err(Error::::InvalidSpecName)), + ("test", 0, 1, Err(frame_system::Error::::SpecVersionNeedsToIncrease)), + ("test", 1, 0, Err(frame_system::Error::::SpecVersionNeedsToIncrease)), + ("test", 1, 1, Err(frame_system::Error::::SpecVersionNeedsToIncrease)), + ("test", 1, 2, Err(frame_system::Error::::SpecVersionNeedsToIncrease)), + ("test2", 1, 1, Err(frame_system::Error::::InvalidSpecName)), ]; for (spec_name, spec_version, impl_version, expected) in test_data.into_iter() {