From e5769098ef3b53b3321d9321b0ea0d4e4105c8a1 Mon Sep 17 00:00:00 2001 From: koushiro Date: Fri, 14 Oct 2022 16:26:32 +0800 Subject: [PATCH 1/6] pallet-sudo: add `CheckSudoKey` signed extension Signed-off-by: koushiro --- frame/sudo/src/extensions/check_sudo_key.rs | 106 ++++++++++++++++++++ frame/sudo/src/extensions/mod.rs | 18 ++++ frame/sudo/src/lib.rs | 11 +- 3 files changed, 134 insertions(+), 1 deletion(-) create mode 100644 frame/sudo/src/extensions/check_sudo_key.rs create mode 100644 frame/sudo/src/extensions/mod.rs diff --git a/frame/sudo/src/extensions/check_sudo_key.rs b/frame/sudo/src/extensions/check_sudo_key.rs new file mode 100644 index 0000000000000..14c41fa1286fd --- /dev/null +++ b/frame/sudo/src/extensions/check_sudo_key.rs @@ -0,0 +1,106 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +use crate::{Config, Pallet}; +use codec::{Decode, Encode}; +use frame_support::dispatch::DispatchInfo; +use scale_info::TypeInfo; +use sp_runtime::{ + traits::{DispatchInfoOf, Dispatchable, SignedExtension}, + transaction_validity::{ + InvalidTransaction, TransactionPriority, TransactionValidity, TransactionValidityError, + ValidTransaction, + }, +}; +use sp_std::{fmt, marker::PhantomData}; + +/// Ensure that signed transactions are only valid if they are signed by root. +#[derive(Clone, Eq, PartialEq, Encode, Decode, TypeInfo)] +#[scale_info(skip_type_params(T))] +pub struct CheckSudoKey(PhantomData); + +impl Default for CheckSudoKey { + fn default() -> Self { + Self(Default::default()) + } +} + +impl fmt::Debug for CheckSudoKey { + #[cfg(feature = "std")] + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "CheckSudoKey") + } + + #[cfg(not(feature = "std"))] + fn fmt(&self, _: &mut fmt::Formatter) -> fmt::Result { + Ok(()) + } +} + +impl CheckSudoKey { + /// Creates new `SignedExtension` to check sudo key. + pub fn new() -> Self { + Self::default() + } +} + +impl SignedExtension for CheckSudoKey +where + ::RuntimeCall: Dispatchable, +{ + const IDENTIFIER: &'static str = "CheckSudoKey"; + type AccountId = T::AccountId; + type Call = ::RuntimeCall; + type AdditionalSigned = (); + type Pre = (); + + fn additional_signed(&self) -> Result { + Ok(()) + } + + fn validate( + &self, + who: &Self::AccountId, + _call: &Self::Call, + info: &DispatchInfoOf, + _len: usize, + ) -> TransactionValidity { + let sudo_key: T::AccountId = match >::key() { + Some(account) => account, + None => return Err(InvalidTransaction::BadSigner.into()), + }; + + if *who == sudo_key { + Ok(ValidTransaction { + priority: info.weight.ref_time() as TransactionPriority, + ..Default::default() + }) + } else { + Err(InvalidTransaction::BadSigner.into()) + } + } + + fn pre_dispatch( + self, + who: &Self::AccountId, + call: &Self::Call, + info: &DispatchInfoOf, + len: usize, + ) -> Result { + self.validate(who, call, info, len).map(|_| ()) + } +} diff --git a/frame/sudo/src/extensions/mod.rs b/frame/sudo/src/extensions/mod.rs new file mode 100644 index 0000000000000..450ad6c9c7376 --- /dev/null +++ b/frame/sudo/src/extensions/mod.rs @@ -0,0 +1,18 @@ +// This file is part of Substrate. + +// Copyright (C) 2022 Parity Technologies (UK) Ltd. +// SPDX-License-Identifier: Apache-2.0 + +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +pub mod check_sudo_key; diff --git a/frame/sudo/src/lib.rs b/frame/sudo/src/lib.rs index 75d15d23c680a..689020b19ada6 100644 --- a/frame/sudo/src/lib.rs +++ b/frame/sudo/src/lib.rs @@ -59,7 +59,7 @@ //! use frame_system::pallet_prelude::*; //! //! #[pallet::pallet] -//! pub struct Pallet(_); +//! pub struct Pallet(PhantomData); //! //! #[pallet::config] //! pub trait Config: frame_system::Config {} @@ -79,6 +79,13 @@ //! # fn main() {} //! ``` //! +//! ### Signed Extensions +//! +//! The Sudo pallet defines the following extensions: +//! +//! - [`CheckSudoKey`]: Checks the signer of the transaction and ensure that the signed +//! transactions are only valid if they are signed by root +//! //! ## Genesis Config //! //! The Sudo pallet depends on the [`GenesisConfig`]. @@ -97,11 +104,13 @@ use sp_std::prelude::*; use frame_support::{dispatch::GetDispatchInfo, traits::UnfilteredDispatchable}; +mod extensions; #[cfg(test)] mod mock; #[cfg(test)] mod tests; +pub use extensions::check_sudo_key::CheckSudoKey; pub use pallet::*; type AccountIdLookupOf = <::Lookup as StaticLookup>::Source; From b2f5356da67de0a3bf604fad8541eb00370d4ad5 Mon Sep 17 00:00:00 2001 From: koushiro Date: Sat, 15 Oct 2022 00:52:19 +0800 Subject: [PATCH 2/6] Rename CheckSudoKey => CheckOnlySudo Signed-off-by: koushiro --- .../{check_sudo_key.rs => check_only_sudo.rs} | 10 +++++----- frame/sudo/src/extensions/mod.rs | 2 +- frame/sudo/src/lib.rs | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) rename frame/sudo/src/extensions/{check_sudo_key.rs => check_only_sudo.rs} (89%) diff --git a/frame/sudo/src/extensions/check_sudo_key.rs b/frame/sudo/src/extensions/check_only_sudo.rs similarity index 89% rename from frame/sudo/src/extensions/check_sudo_key.rs rename to frame/sudo/src/extensions/check_only_sudo.rs index 14c41fa1286fd..613783c68957e 100644 --- a/frame/sudo/src/extensions/check_sudo_key.rs +++ b/frame/sudo/src/extensions/check_only_sudo.rs @@ -31,15 +31,15 @@ use sp_std::{fmt, marker::PhantomData}; /// Ensure that signed transactions are only valid if they are signed by root. #[derive(Clone, Eq, PartialEq, Encode, Decode, TypeInfo)] #[scale_info(skip_type_params(T))] -pub struct CheckSudoKey(PhantomData); +pub struct CheckOnlySudo(PhantomData); -impl Default for CheckSudoKey { +impl Default for CheckOnlySudo { fn default() -> Self { Self(Default::default()) } } -impl fmt::Debug for CheckSudoKey { +impl fmt::Debug for CheckOnlySudo { #[cfg(feature = "std")] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "CheckSudoKey") @@ -51,14 +51,14 @@ impl fmt::Debug for CheckSudoKey { } } -impl CheckSudoKey { +impl CheckOnlySudo { /// Creates new `SignedExtension` to check sudo key. pub fn new() -> Self { Self::default() } } -impl SignedExtension for CheckSudoKey +impl SignedExtension for CheckOnlySudo where ::RuntimeCall: Dispatchable, { diff --git a/frame/sudo/src/extensions/mod.rs b/frame/sudo/src/extensions/mod.rs index 450ad6c9c7376..0b7ca6f9adb4e 100644 --- a/frame/sudo/src/extensions/mod.rs +++ b/frame/sudo/src/extensions/mod.rs @@ -15,4 +15,4 @@ // See the License for the specific language governing permissions and // limitations under the License. -pub mod check_sudo_key; +pub mod check_only_sudo; diff --git a/frame/sudo/src/lib.rs b/frame/sudo/src/lib.rs index 689020b19ada6..593f1b01299cf 100644 --- a/frame/sudo/src/lib.rs +++ b/frame/sudo/src/lib.rs @@ -83,7 +83,7 @@ //! //! The Sudo pallet defines the following extensions: //! -//! - [`CheckSudoKey`]: Checks the signer of the transaction and ensure that the signed +//! - [`CheckOnlySudo`]: Checks the signer of the transaction and ensure that the signed //! transactions are only valid if they are signed by root //! //! ## Genesis Config @@ -110,7 +110,7 @@ mod mock; #[cfg(test)] mod tests; -pub use extensions::check_sudo_key::CheckSudoKey; +pub use extensions::check_only_sudo::CheckOnlySudo; pub use pallet::*; type AccountIdLookupOf = <::Lookup as StaticLookup>::Source; From 9f1a6a5ef662e9550bba52505b516a88f3c4c76b Mon Sep 17 00:00:00 2001 From: koushiro Date: Thu, 3 Nov 2022 11:19:17 +0800 Subject: [PATCH 3/6] Rename extension name and Add some docs --- .../check_only_sudo.rs => extension.rs} | 20 +++++++++++-------- frame/sudo/src/extensions/mod.rs | 18 ----------------- frame/sudo/src/lib.rs | 14 +++++++------ 3 files changed, 20 insertions(+), 32 deletions(-) rename frame/sudo/src/{extensions/check_only_sudo.rs => extension.rs} (79%) delete mode 100644 frame/sudo/src/extensions/mod.rs diff --git a/frame/sudo/src/extensions/check_only_sudo.rs b/frame/sudo/src/extension.rs similarity index 79% rename from frame/sudo/src/extensions/check_only_sudo.rs rename to frame/sudo/src/extension.rs index 613783c68957e..271a94b30a778 100644 --- a/frame/sudo/src/extensions/check_only_sudo.rs +++ b/frame/sudo/src/extension.rs @@ -28,21 +28,25 @@ use sp_runtime::{ }; use sp_std::{fmt, marker::PhantomData}; -/// Ensure that signed transactions are only valid if they are signed by root. +/// Ensure that signed transactions are only valid if they are signed by sudo account. +/// +/// In the initial phase of a chain without any tokens, with this extension we could +/// reject spam transactions that are not signed by sudo account before they enter +/// transaction pool. #[derive(Clone, Eq, PartialEq, Encode, Decode, TypeInfo)] #[scale_info(skip_type_params(T))] -pub struct CheckOnlySudo(PhantomData); +pub struct CheckOnlySudoAccount(PhantomData); -impl Default for CheckOnlySudo { +impl Default for CheckOnlySudoAccount { fn default() -> Self { Self(Default::default()) } } -impl fmt::Debug for CheckOnlySudo { +impl fmt::Debug for CheckOnlySudoAccount { #[cfg(feature = "std")] fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "CheckSudoKey") + write!(f, "CheckOnlySudoAccount") } #[cfg(not(feature = "std"))] @@ -51,18 +55,18 @@ impl fmt::Debug for CheckOnlySudo { } } -impl CheckOnlySudo { +impl CheckOnlySudoAccount { /// Creates new `SignedExtension` to check sudo key. pub fn new() -> Self { Self::default() } } -impl SignedExtension for CheckOnlySudo +impl SignedExtension for CheckOnlySudoAccount where ::RuntimeCall: Dispatchable, { - const IDENTIFIER: &'static str = "CheckSudoKey"; + const IDENTIFIER: &'static str = "CheckOnlySudoAccount"; type AccountId = T::AccountId; type Call = ::RuntimeCall; type AdditionalSigned = (); diff --git a/frame/sudo/src/extensions/mod.rs b/frame/sudo/src/extensions/mod.rs deleted file mode 100644 index 0b7ca6f9adb4e..0000000000000 --- a/frame/sudo/src/extensions/mod.rs +++ /dev/null @@ -1,18 +0,0 @@ -// This file is part of Substrate. - -// Copyright (C) 2022 Parity Technologies (UK) Ltd. -// SPDX-License-Identifier: Apache-2.0 - -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -pub mod check_only_sudo; diff --git a/frame/sudo/src/lib.rs b/frame/sudo/src/lib.rs index 593f1b01299cf..fce01db3737cf 100644 --- a/frame/sudo/src/lib.rs +++ b/frame/sudo/src/lib.rs @@ -79,12 +79,14 @@ //! # fn main() {} //! ``` //! -//! ### Signed Extensions +//! ### Signed Extension //! -//! The Sudo pallet defines the following extensions: +//! The Sudo pallet defines the following extension: //! -//! - [`CheckOnlySudo`]: Checks the signer of the transaction and ensure that the signed -//! transactions are only valid if they are signed by root +//! - [`CheckOnlySudoAccount`]: Checks the signer of the transaction and ensure that the signed +//! transactions are only valid if they are signed by sudo account. +//! In the initial phase of a chain without any tokens, with this extension we could reject +//! spam transactions that are not signed by sudo account before they enter transaction pool. //! //! ## Genesis Config //! @@ -104,13 +106,13 @@ use sp_std::prelude::*; use frame_support::{dispatch::GetDispatchInfo, traits::UnfilteredDispatchable}; -mod extensions; +mod extension; #[cfg(test)] mod mock; #[cfg(test)] mod tests; -pub use extensions::check_only_sudo::CheckOnlySudo; +pub use extension::CheckOnlySudoAccount; pub use pallet::*; type AccountIdLookupOf = <::Lookup as StaticLookup>::Source; From 6938e8f9ffc885bdac9e967e8536eed1471dc839 Mon Sep 17 00:00:00 2001 From: koushiro Date: Thu, 3 Nov 2022 23:26:37 +0800 Subject: [PATCH 4/6] Apply review suggestions --- frame/sudo/src/extension.rs | 13 ++++++++----- frame/sudo/src/lib.rs | 6 ++---- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/frame/sudo/src/extension.rs b/frame/sudo/src/extension.rs index 271a94b30a778..87350de1aff6b 100644 --- a/frame/sudo/src/extension.rs +++ b/frame/sudo/src/extension.rs @@ -23,16 +23,19 @@ use sp_runtime::{ traits::{DispatchInfoOf, Dispatchable, SignedExtension}, transaction_validity::{ InvalidTransaction, TransactionPriority, TransactionValidity, TransactionValidityError, - ValidTransaction, + UnknownTransaction, ValidTransaction, }, }; use sp_std::{fmt, marker::PhantomData}; /// Ensure that signed transactions are only valid if they are signed by sudo account. /// -/// In the initial phase of a chain without any tokens, with this extension we could -/// reject spam transactions that are not signed by sudo account before they enter -/// transaction pool. +/// In the initial phase of a chain without any tokens you can not prevent accounts from sending +/// transactions. +/// These transactions would enter the transaction pool as the succeed the validation, but would +/// fail on applying them as they are not allowed/disabled/whatever. This would be some huge dos +/// vector to any kind of chain. This extension solves the dos vector by preventing any kind of +/// transaction entering the pool as long as it is not signed by the sudo account. #[derive(Clone, Eq, PartialEq, Encode, Decode, TypeInfo)] #[scale_info(skip_type_params(T))] pub struct CheckOnlySudoAccount(PhantomData); @@ -85,7 +88,7 @@ where ) -> TransactionValidity { let sudo_key: T::AccountId = match >::key() { Some(account) => account, - None => return Err(InvalidTransaction::BadSigner.into()), + None => return Err(UnknownTransaction::CannotLookup.into()), }; if *who == sudo_key { diff --git a/frame/sudo/src/lib.rs b/frame/sudo/src/lib.rs index fce01db3737cf..c18ced8911193 100644 --- a/frame/sudo/src/lib.rs +++ b/frame/sudo/src/lib.rs @@ -83,10 +83,8 @@ //! //! The Sudo pallet defines the following extension: //! -//! - [`CheckOnlySudoAccount`]: Checks the signer of the transaction and ensure that the signed -//! transactions are only valid if they are signed by sudo account. -//! In the initial phase of a chain without any tokens, with this extension we could reject -//! spam transactions that are not signed by sudo account before they enter transaction pool. +//! - [`CheckOnlySudoAccount`]: Ensures that the signed transactions are only valid if they are +//! signed by sudo account. //! //! ## Genesis Config //! From edb408e4f85b90b4f2d9d592a11203d29ce4f98b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Thu, 3 Nov 2022 23:17:29 +0100 Subject: [PATCH 5/6] Update frame/sudo/src/extension.rs Co-authored-by: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> --- frame/sudo/src/extension.rs | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/frame/sudo/src/extension.rs b/frame/sudo/src/extension.rs index 87350de1aff6b..d453dae888e42 100644 --- a/frame/sudo/src/extension.rs +++ b/frame/sudo/src/extension.rs @@ -86,19 +86,13 @@ where info: &DispatchInfoOf, _len: usize, ) -> TransactionValidity { - let sudo_key: T::AccountId = match >::key() { - Some(account) => account, - None => return Err(UnknownTransaction::CannotLookup.into()), - }; + let sudo_key: T::AccountId = >::key().ok_or(UnknownTransaction::CannotLookup)?; + ensure!(*who == sudo_key, InvalidTransaction::BadSigner); - if *who == sudo_key { - Ok(ValidTransaction { - priority: info.weight.ref_time() as TransactionPriority, - ..Default::default() - }) - } else { - Err(InvalidTransaction::BadSigner.into()) - } + Ok(ValidTransaction { + priority: info.weight.ref_time() as TransactionPriority, + ..Default::default() + }) } fn pre_dispatch( From 962f5a63a05a5c0c3f1b0a1db863d3b61ad33007 Mon Sep 17 00:00:00 2001 From: Jegor Sidorenko <5252494+jsidorenko@users.noreply.github.com> Date: Thu, 3 Nov 2022 23:39:25 +0100 Subject: [PATCH 6/6] Update frame/sudo/src/extension.rs --- frame/sudo/src/extension.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frame/sudo/src/extension.rs b/frame/sudo/src/extension.rs index d453dae888e42..068fa2ed928d5 100644 --- a/frame/sudo/src/extension.rs +++ b/frame/sudo/src/extension.rs @@ -17,7 +17,7 @@ use crate::{Config, Pallet}; use codec::{Decode, Encode}; -use frame_support::dispatch::DispatchInfo; +use frame_support::{dispatch::DispatchInfo, ensure}; use scale_info::TypeInfo; use sp_runtime::{ traits::{DispatchInfoOf, Dispatchable, SignedExtension},