Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Backport pallet-identity double encoding on signature payload fix into crates io release 1.7.0 #4702

Closed
wants to merge 4 commits into from
Closed

Backport pallet-identity double encoding on signature payload fix into crates io release 1.7.0 #4702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
49edb86
[Identity] Remove double encoding username signature payload (#4646)
georgepisaltu Jun 5, 2024
06508f2
Set pallet-identity version to v29.0.2
georgepisaltu Jun 5, 2024
4ad27f4
Fix bench compilation warning
georgepisaltu Jun 5, 2024
b551625
Bump identity version in deps
georgepisaltu Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cumulus/parachains/integration-tests/emulated/tests/people/people-rococo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ sp-runtime = { path = "../../../../../../../substrate/primitives/runtime", defau
frame-support = { path = "../../../../../../../substrate/frame/support", default-features = false, version = "29.0.2" }
pallet-balances = { path = "../../../../../../../substrate/frame/balances", default-features = false, version = "29.0.2" }
pallet-message-queue = { path = "../../../../../../../substrate/frame/message-queue", default-features = false, version = "32.0.0" }
pallet-identity = { path = "../../../../../../../substrate/frame/identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../../../../../../../substrate/frame/identity", default-features = false, version = "29.0.2" }

# Polkadot
xcm = { package = "staging-xcm", path = "../../../../../../../polkadot/xcm", default-features = false, version = "8.0.1" }
Expand Down
2 changes: 1 addition & 1 deletion cumulus/parachains/integration-tests/emulated/tests/people/people-westend/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ sp-runtime = { path = "../../../../../../../substrate/primitives/runtime", defau
frame-support = { path = "../../../../../../../substrate/frame/support", default-features = false, version = "29.0.2" }
pallet-balances = { path = "../../../../../../../substrate/frame/balances", default-features = false, version = "29.0.2" }
pallet-message-queue = { path = "../../../../../../../substrate/frame/message-queue", default-features = false, version = "32.0.0" }
pallet-identity = { path = "../../../../../../../substrate/frame/identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../../../../../../../substrate/frame/identity", default-features = false, version = "29.0.2" }

# Polkadot
xcm = { package = "staging-xcm", path = "../../../../../../../polkadot/xcm", default-features = false, version = "8.0.1" }
Expand Down
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/people/people-rococo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ frame-try-runtime = { path = "../../../../../substrate/frame/try-runtime", defau
pallet-aura = { path = "../../../../../substrate/frame/aura", default-features = false, version = "28.0.0" }
pallet-authorship = { path = "../../../../../substrate/frame/authorship", default-features = false, version = "29.0.0" }
pallet-balances = { path = "../../../../../substrate/frame/balances", default-features = false, version = "29.0.2" }
pallet-identity = { path = "../../../../../substrate/frame/identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../../../../../substrate/frame/identity", default-features = false, version = "29.0.2" }
pallet-message-queue = { path = "../../../../../substrate/frame/message-queue", default-features = false, version = "32.0.0" }
pallet-multisig = { path = "../../../../../substrate/frame/multisig", default-features = false, version = "29.0.0" }
pallet-session = { path = "../../../../../substrate/frame/session", default-features = false, version = "29.0.0" }
Expand Down
2 changes: 1 addition & 1 deletion cumulus/parachains/runtimes/people/people-westend/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ frame-try-runtime = { path = "../../../../../substrate/frame/try-runtime", defau
pallet-aura = { path = "../../../../../substrate/frame/aura", default-features = false, version = "28.0.0" }
pallet-authorship = { path = "../../../../../substrate/frame/authorship", default-features = false, version = "29.0.0" }
pallet-balances = { path = "../../../../../substrate/frame/balances", default-features = false, version = "29.0.2" }
pallet-identity = { path = "../../../../../substrate/frame/identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../../../../../substrate/frame/identity", default-features = false, version = "29.0.2" }
pallet-message-queue = { path = "../../../../../substrate/frame/message-queue", default-features = false, version = "32.0.0" }
pallet-multisig = { path = "../../../../../substrate/frame/multisig", default-features = false, version = "29.0.0" }
pallet-session = { path = "../../../../../substrate/frame/session", default-features = false, version = "29.0.0" }
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/common/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ pallet-authorship = { path = "../../../substrate/frame/authorship", default-feat
pallet-balances = { path = "../../../substrate/frame/balances", default-features = false, version = "29.0.2" }
pallet-broker = { path = "../../../substrate/frame/broker", default-features = false, version = "0.7.2" }
pallet-fast-unstake = { path = "../../../substrate/frame/fast-unstake", default-features = false, version = "28.0.0" }
pallet-identity = { path = "../../../substrate/frame/identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../../../substrate/frame/identity", default-features = false, version = "29.0.2" }
pallet-session = { path = "../../../substrate/frame/session", default-features = false, version = "29.0.0" }
frame-support = { path = "../../../substrate/frame/support", default-features = false, version = "29.0.2" }
pallet-staking = { path = "../../../substrate/frame/staking", default-features = false, version = "29.0.3" }
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/rococo/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ pallet-elections-phragmen = { path = "../../../substrate/frame/elections-phragme
pallet-asset-rate = { path = "../../../substrate/frame/asset-rate", default-features = false, version = "8.0.0" }
frame-executive = { path = "../../../substrate/frame/executive", default-features = false, version = "29.0.0" }
pallet-grandpa = { path = "../../../substrate/frame/grandpa", default-features = false, version = "29.0.0" }
pallet-identity = { path = "../../../substrate/frame/identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../../../substrate/frame/identity", default-features = false, version = "29.0.2" }
pallet-im-online = { path = "../../../substrate/frame/im-online", default-features = false, version = "28.0.0" }
pallet-indices = { path = "../../../substrate/frame/indices", default-features = false, version = "29.0.0" }
pallet-membership = { path = "../../../substrate/frame/membership", default-features = false, version = "29.0.0" }
Expand Down
2 changes: 1 addition & 1 deletion polkadot/runtime/westend/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ pallet-elections-phragmen = { package = "pallet-elections-phragmen", path = "../
pallet-election-provider-multi-phase = { path = "../../../substrate/frame/election-provider-multi-phase", default-features = false, version = "28.0.0" }
pallet-fast-unstake = { path = "../../../substrate/frame/fast-unstake", default-features = false, version = "28.0.0" }
pallet-grandpa = { path = "../../../substrate/frame/grandpa", default-features = false, version = "29.0.0" }
pallet-identity = { path = "../../../substrate/frame/identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../../../substrate/frame/identity", default-features = false, version = "29.0.2" }
pallet-im-online = { path = "../../../substrate/frame/im-online", default-features = false, version = "28.0.0" }
pallet-indices = { path = "../../../substrate/frame/indices", default-features = false, version = "29.0.0" }
pallet-membership = { path = "../../../substrate/frame/membership", default-features = false, version = "29.0.0" }
Expand Down
20 changes: 20 additions & 0 deletions prdoc/pr_4646.prdoc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json

title: "[Identity] Remove double encoding username signature payload"

doc:
- audience: Runtime Dev
description: |
The signature payload for setting a username for an account in `pallet-identity` is now just
the raw bytes of said username (still including the suffix), removing the need to first
encode these bytes before signing.
- audience: Runtime User
description: |
The signature payload for setting a username for an account in `pallet-identity` is now just
the raw bytes of said username (still including the suffix), removing the need to first
encode these bytes before signing.

crates:
- name: pallet-identity
bump: major
2 changes: 1 addition & 1 deletion substrate/bin/node/runtime/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ pallet-nis = { path = "../../../frame/nis", default-features = false, version =
pallet-grandpa = { path = "../../../frame/grandpa", default-features = false, version = "29.0.0" }
pallet-im-online = { path = "../../../frame/im-online", default-features = false, version = "28.0.0" }
pallet-indices = { path = "../../../frame/indices", default-features = false, version = "29.0.0" }
pallet-identity = { path = "../../../frame/identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../../../frame/identity", default-features = false, version = "29.0.2" }
pallet-lottery = { path = "../../../frame/lottery", default-features = false, version = "29.0.0" }
pallet-membership = { path = "../../../frame/membership", default-features = false, version = "29.0.0" }
pallet-message-queue = { path = "../../../frame/message-queue", default-features = false, version = "32.0.0" }
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/alliance/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ frame-benchmarking = { path = "../benchmarking", default-features = false, optio
frame-support = { path = "../support", default-features = false, version = "29.0.2" }
frame-system = { path = "../system", default-features = false, version = "29.0.0" }

pallet-identity = { path = "../identity", default-features = false, version = "29.0.1" }
pallet-identity = { path = "../identity", default-features = false, version = "29.0.2" }
pallet-collective = { path = "../collective", default-features = false, optional = true, version = "29.0.0" }

[dev-dependencies]
Expand Down
2 changes: 1 addition & 1 deletion substrate/frame/identity/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "pallet-identity"
version = "29.0.1"
version = "29.0.2"
authors.workspace = true
edition.workspace = true
license = "Apache-2.0"
Expand Down
13 changes: 5 additions & 8 deletions substrate/frame/identity/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,7 @@
use super::*;

use crate::Pallet as Identity;
use codec::Encode;
use frame_benchmarking::{
account, impl_benchmark_test_suite, v2::*, whitelisted_caller, BenchmarkError,
};
use frame_benchmarking::{account, v2::*, whitelisted_caller, BenchmarkError};
use frame_support::{
assert_ok, ensure,
traits::{EnsureOrigin, Get, OnFinalize, OnInitialize},
Expand Down Expand Up @@ -625,17 +622,17 @@ mod benchmarks {

let username = bench_username();
let bounded_username = bounded_username::<T>(username.clone(), suffix.clone());
let encoded_username = Encode::encode(&bounded_username.to_vec());

let public = sr25519_generate(0.into(), None);
let who_account: T::AccountId = MultiSigner::Sr25519(public).into_account().into();
let who_lookup = T::Lookup::unlookup(who_account.clone());

let signature =
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
let signature = MultiSignature::Sr25519(
sr25519_sign(0.into(), &public, &bounded_username[..]).unwrap(),
);

// Verify signature here to avoid surprise errors at runtime
assert!(signature.verify(&encoded_username[..], &public.into()));
assert!(signature.verify(&bounded_username[..], &public.into()));

#[extrinsic_call]
_(RawOrigin::Signed(authority.clone()), who_lookup, username, Some(signature.into()));
Expand Down
7 changes: 3 additions & 4 deletions substrate/frame/identity/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1116,8 +1116,7 @@ pub mod pallet {
if let Some(s) = signature {
// Account has pre-signed an authorization. Verify the signature provided and grant
// the username directly.
let encoded = Encode::encode(&bounded_username.to_vec());
Self::validate_signature(&encoded, &s, &who)?;
Self::validate_signature(&bounded_username[..], &s, &who)?;
Self::insert_username(&who, bounded_username);
} else {
// The user must accept the username, therefore, queue it.
Expand Down Expand Up @@ -1267,12 +1266,12 @@ impl<T: Config> Pallet<T> {

/// Validate a signature. Supports signatures on raw `data` or `data` wrapped in HTML `<Bytes>`.
pub fn validate_signature(
data: &Vec<u8>,
data: &[u8],
signature: &T::OffchainSignature,
signer: &T::AccountId,
) -> DispatchResult {
// Happy path, user has signed the raw data.
if signature.verify(&data[..], &signer) {
if signature.verify(data, &signer) {
return Ok(())
}
// NOTE: for security reasons modern UIs implicitly wrap the data requested to sign into
Expand Down
51 changes: 22 additions & 29 deletions substrate/frame/identity/src/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1061,13 +1061,13 @@ fn set_username_with_signature_without_existing_identity_should_work() {

// set up username
let (username, username_to_sign) = test_username_of(b"42".to_vec(), suffix);
let encoded_username = Encode::encode(&username_to_sign.to_vec());

// set up user and sign message
let public = sr25519_generate(0.into(), None);
let who_account: AccountIdOf<Test> = MultiSigner::Sr25519(public).into_account().into();
let signature =
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
let signature = MultiSignature::Sr25519(
sr25519_sign(0.into(), &public, &username_to_sign[..]).unwrap(),
);

assert_ok!(Identity::set_username_for(
RuntimeOrigin::signed(authority),
Expand Down Expand Up @@ -1112,13 +1112,13 @@ fn set_username_with_signature_with_existing_identity_should_work() {

// set up username
let (username, username_to_sign) = test_username_of(b"42".to_vec(), suffix);
let encoded_username = Encode::encode(&username_to_sign.to_vec());

// set up user and sign message
let public = sr25519_generate(0.into(), None);
let who_account: AccountIdOf<Test> = MultiSigner::Sr25519(public).into_account().into();
let signature =
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
let signature = MultiSignature::Sr25519(
sr25519_sign(0.into(), &public, &username_to_sign[..]).unwrap(),
);

// Set an identity for who. They need some balance though.
Balances::make_free_balance_be(&who_account, 1000);
Expand Down Expand Up @@ -1175,13 +1175,13 @@ fn set_username_with_bytes_signature_should_work() {
let unwrapped_username = username_to_sign.to_vec();

// Sign an unwrapped version, as in `username.suffix`.
let unwrapped_encoded = Encode::encode(&unwrapped_username);
let signature_on_unwrapped =
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &unwrapped_encoded).unwrap());
let signature_on_unwrapped = MultiSignature::Sr25519(
sr25519_sign(0.into(), &public, &unwrapped_username[..]).unwrap(),
);

// Trivial
assert_ok!(Identity::validate_signature(
&unwrapped_encoded,
&unwrapped_username,
&signature_on_unwrapped,
&who_account
));
Expand All @@ -1193,15 +1193,15 @@ fn set_username_with_bytes_signature_should_work() {
let mut wrapped_username: Vec<u8> =
Vec::with_capacity(unwrapped_username.len() + prehtml.len() + posthtml.len());
wrapped_username.extend(prehtml);
wrapped_username.extend(unwrapped_encoded.clone());
wrapped_username.extend(&unwrapped_username);
wrapped_username.extend(posthtml);
let signature_on_wrapped =
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &wrapped_username).unwrap());

// We want to call `validate_signature` on the *unwrapped* username, but the signature on
// the *wrapped* data.
assert_ok!(Identity::validate_signature(
&unwrapped_encoded,
&unwrapped_username,
&signature_on_wrapped,
&who_account
));
Expand Down Expand Up @@ -1420,9 +1420,8 @@ fn setting_primary_should_work() {

// set up username
let (first_username, first_to_sign) = test_username_of(b"42".to_vec(), suffix.clone());
let encoded_username = Encode::encode(&first_to_sign.to_vec());
let first_signature =
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &first_to_sign[..]).unwrap());

assert_ok!(Identity::set_username_for(
RuntimeOrigin::signed(authority.clone()),
Expand All @@ -1446,9 +1445,8 @@ fn setting_primary_should_work() {

// set up username
let (second_username, second_to_sign) = test_username_of(b"101".to_vec(), suffix);
let encoded_username = Encode::encode(&second_to_sign.to_vec());
let second_signature =
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &second_to_sign[..]).unwrap());

assert_ok!(Identity::set_username_for(
RuntimeOrigin::signed(authority),
Expand Down Expand Up @@ -1529,10 +1527,8 @@ fn must_own_primary() {
let pi_account: AccountIdOf<Test> = MultiSigner::Sr25519(pi_public).into_account().into();
let (pi_username, pi_to_sign) =
test_username_of(b"username314159".to_vec(), suffix.clone());
let encoded_pi_username = Encode::encode(&pi_to_sign.to_vec());
let pi_signature = MultiSignature::Sr25519(
sr25519_sign(0.into(), &pi_public, &encoded_pi_username).unwrap(),
);
let pi_signature =
MultiSignature::Sr25519(sr25519_sign(0.into(), &pi_public, &pi_to_sign[..]).unwrap());
assert_ok!(Identity::set_username_for(
RuntimeOrigin::signed(authority.clone()),
pi_account.clone(),
Expand All @@ -1544,10 +1540,8 @@ fn must_own_primary() {
let e_public = sr25519_generate(1.into(), None);
let e_account: AccountIdOf<Test> = MultiSigner::Sr25519(e_public).into_account().into();
let (e_username, e_to_sign) = test_username_of(b"username271828".to_vec(), suffix.clone());
let encoded_e_username = Encode::encode(&e_to_sign.to_vec());
let e_signature = MultiSignature::Sr25519(
sr25519_sign(1.into(), &e_public, &encoded_e_username).unwrap(),
);
let e_signature =
MultiSignature::Sr25519(sr25519_sign(1.into(), &e_public, &e_to_sign[..]).unwrap());
assert_ok!(Identity::set_username_for(
RuntimeOrigin::signed(authority.clone()),
e_account.clone(),
Expand Down Expand Up @@ -1652,13 +1646,13 @@ fn removing_dangling_usernames_should_work() {

// set up username
let (username, username_to_sign) = test_username_of(b"42".to_vec(), suffix.clone());
let encoded_username = Encode::encode(&username_to_sign.to_vec());

// set up user and sign message
let public = sr25519_generate(0.into(), None);
let who_account: AccountIdOf<Test> = MultiSigner::Sr25519(public).into_account().into();
let signature =
MultiSignature::Sr25519(sr25519_sign(0.into(), &public, &encoded_username).unwrap());
let signature = MultiSignature::Sr25519(
sr25519_sign(0.into(), &public, &username_to_sign[..]).unwrap(),
);

// Set an identity for who. They need some balance though.
Balances::make_free_balance_be(&who_account, 1000);
Expand All @@ -1676,11 +1670,10 @@ fn removing_dangling_usernames_should_work() {

// Now they set up a second username.
let (username_two, username_two_to_sign) = test_username_of(b"43".to_vec(), suffix);
let encoded_username_two = Encode::encode(&username_two_to_sign.to_vec());

// set up user and sign message
let signature_two = MultiSignature::Sr25519(
sr25519_sign(0.into(), &public, &encoded_username_two).unwrap(),
sr25519_sign(0.into(), &public, &username_two_to_sign[..]).unwrap(),
);

assert_ok!(Identity::set_username_for(
Expand Down
Loading