From 3977f389cce4a00fd7100f95262e0563622b9aa4 Mon Sep 17 00:00:00 2001
From: georgepisaltu <52418509+georgepisaltu@users.noreply.github.com>
Date: Wed, 5 Jun 2024 10:38:01 +0300
Subject: [PATCH] [Identity] Remove double encoding username signature payload
(#4646)
In order to receive a username in `pallet-identity`, users have to,
among other things, provide a signature of the desired username. Right
now, there is an [extra encoding
step](https://github.com/paritytech/polkadot-sdk/blob/4ab078d6754147ce731523292dd1882f8a7b5775/substrate/frame/identity/src/lib.rs#L1119)
when generating the payload to sign.
Encoding a `Vec` adds extra bytes related to the length, which changes
the payload. This is unnecessary and confusing as users expect the
payload to sign to be just the username bytes. This PR fixes this issue
by validating the signature directly against the username bytes.
---------
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
---
Cargo.lock | 2 +-
prdoc/pr_4646.prdoc | 20 ++++++++
substrate/frame/identity/Cargo.toml | 2 +-
substrate/frame/identity/src/benchmarking.rs | 9 ++--
substrate/frame/identity/src/lib.rs | 7 ++-
substrate/frame/identity/src/tests.rs | 51 +++++++++-----------
6 files changed, 51 insertions(+), 40 deletions(-)
create mode 100644 prdoc/pr_4646.prdoc
diff --git a/Cargo.lock b/Cargo.lock
index fe0de516f10..82aac5fb2a8 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -10469,7 +10469,7 @@ dependencies = [
[[package]]
name = "pallet-identity"
-version = "28.0.0"
+version = "29.0.0"
dependencies = [
"enumflags2",
"frame-benchmarking",
diff --git a/prdoc/pr_4646.prdoc b/prdoc/pr_4646.prdoc
new file mode 100644
index 00000000000..0252deb57bd
--- /dev/null
+++ b/prdoc/pr_4646.prdoc
@@ -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
diff --git a/substrate/frame/identity/Cargo.toml b/substrate/frame/identity/Cargo.toml
index e0bce8a77bd..987e418048d 100644
--- a/substrate/frame/identity/Cargo.toml
+++ b/substrate/frame/identity/Cargo.toml
@@ -1,6 +1,6 @@
[package]
name = "pallet-identity"
-version = "28.0.0"
+version = "29.0.0"
authors.workspace = true
edition.workspace = true
license = "Apache-2.0"
diff --git a/substrate/frame/identity/src/benchmarking.rs b/substrate/frame/identity/src/benchmarking.rs
index cdcdb952261..957549b19f8 100644
--- a/substrate/frame/identity/src/benchmarking.rs
+++ b/substrate/frame/identity/src/benchmarking.rs
@@ -22,7 +22,6 @@
use super::*;
use crate::Pallet as Identity;
-use codec::Encode;
use frame_benchmarking::{account, v2::*, whitelisted_caller, BenchmarkError};
use frame_support::{
assert_ok, ensure,
@@ -623,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()));
diff --git a/substrate/frame/identity/src/lib.rs b/substrate/frame/identity/src/lib.rs
index 5a36101cc2f..50d6de32ac6 100644
--- a/substrate/frame/identity/src/lib.rs
+++ b/substrate/frame/identity/src/lib.rs
@@ -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.
@@ -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
diff --git a/substrate/frame/identity/src/tests.rs b/substrate/frame/identity/src/tests.rs
index 60579a23b91..b1a953d487c 100644
--- a/substrate/frame/identity/src/tests.rs
+++ b/substrate/frame/identity/src/tests.rs
@@ -1042,13 +1042,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),
@@ -1093,13 +1093,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);
@@ -1156,13 +1156,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
));
@@ -1174,7 +1174,7 @@ 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());
@@ -1182,7 +1182,7 @@ fn set_username_with_bytes_signature_should_work() {
// 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
));
@@ -1401,9 +1401,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()),
@@ -1427,9 +1426,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),
@@ -1510,10 +1508,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(),
@@ -1525,10 +1521,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(),
@@ -1633,13 +1627,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);
@@ -1657,11 +1651,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(
--
GitLab