From 3249186fe643f62ca95769e2217f858dde803ab6 Mon Sep 17 00:00:00 2001 From: joe petrowski <25483142+joepetrowski@users.noreply.github.com> Date: Thu, 2 Mar 2023 16:48:14 +0100 Subject: [PATCH] Add Version Checks on Para Upgrade (#2261) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * add version checks on para ugprade * Apply suggestions from code review Co-authored-by: Bastian Köcher * remove unneeded imports and errors * fix test error path --------- Co-authored-by: Bastian Köcher --- pallets/parachain-system/src/lib.rs | 82 +++++++++++++++++++++------ pallets/parachain-system/src/tests.rs | 37 +++++++++++- 2 files changed, 100 insertions(+), 19 deletions(-) diff --git a/pallets/parachain-system/src/lib.rs b/pallets/parachain-system/src/lib.rs index 456ae5bf578..3a693b0911a 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::{ @@ -134,6 +135,20 @@ 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, TypeInfo)] +#[scale_info(skip_type_params(T))] +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, +} + #[frame_support::pallet] pub mod pallet { use super::*; @@ -442,17 +457,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. + /// + /// All origins are allowed. #[pallet::call_index(3)] #[pallet::weight(1_000_000)] pub fn enact_authorized_upgrade( @@ -487,16 +525,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, @@ -645,7 +683,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 +750,17 @@ 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 { + frame_system::Pallet::::can_set_code(code)?; + } + Ok(actual_hash) } } diff --git a/pallets/parachain-system/src/tests.rs b/pallets/parachain-system/src/tests.rs index 871596ae8cb..70e4c106bf2 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(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() { + 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); + }); + } +}