pallet-xcm: add support to authorize aliases #6336

acatangiu · 2024-11-01T16:44:53Z

Add calls to pallet-xcm for adding and removing authorization for a certain aliaser location to alias into the caller origin.

pallet-xcm also exposes an AuthorizedAliases filter implementation usable with xcm_executor::Config::Aliasers filter to easily allow runtimes to plug in the explicitly authorized aliases using the calls above.

Usually useful to allow your local account to be aliased into from a remote location also under your control (like your account on another chain).

One cool example is Alice on Para42 doing something on Asset Hub without having to transfer fees from Para42, but instead use her local Asset Hub account:

// called by Alice on Para42
pallet_xcm::send(
	Location::new(1, Parachain(1000)),
	Xcm(vec![
		AliasOrigin(AliceOnAH),
		WithdrawAsset(fees),
		PayFees(fees),
		DoWhatever
	])
);

Part of Empowered cross-chain origins.

Fixes XCM: Arbitrary Origin Aliases #722

bkontur

lgtm, left some small nits/questions

polkadot/xcm/pallet-xcm/src/lib.rs

polkadot/xcm/pallet-xcm/src/benchmarking.rs

polkadot/xcm/pallet-xcm/src/lib.rs

cumulus/parachains/runtimes/assets/asset-hub-westend/tests/tests.rs

polkadot/xcm/pallet-xcm/src/lib.rs

…orize-alias

acatangiu · 2024-11-07T12:02:58Z

bot bench polkadot-pallet --subcommand=xcm --runtime=westend --pallet=pallet_xcm
bot bench polkadot-pallet --subcommand=xcm --runtime=rococo --pallet=pallet_xcm

bot bench cumulus-assets --subcommand=xcm --runtime=asset-hub-westend --pallet=pallet_xcm
bot bench cumulus-assets --subcommand=xcm --runtime=asset-hub-rococo --pallet=pallet_xcm

…stend --target_dir=polkadot --pallet=pallet_xcm

…coco --target_dir=polkadot --pallet=pallet_xcm

…set-hub-westend --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm

…set-hub-rococo --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm

acatangiu · 2024-11-07T14:17:57Z

bot clean

polkadot/xcm/pallet-xcm/src/benchmarking.rs

franciscoaguirre · 2024-11-07T15:35:00Z

polkadot/xcm/pallet-xcm/src/lib.rs

+		/// their/your name. Once authorized using this call, the `aliaser` can freely impersonate
+		/// `origin` in XCM programs executed on the local chain.
+		#[pallet::call_index(14)]
+		pub fn add_authorized_alias(


Should we allow this indefinitely or always require an expiry? Thinking of the case where you do trust your account on another chain but maybe that chain gets compromised and you forgot to remove your authorization. Then the parachain can impersonate your account on their chain and in turn impersonate your account on this chain.

An expiry would be annoying but would make it safer. Apps could batch this with the actual action you want to take if they know the expiry is past.

I’m personally not a fan of expiry. It’s a big UX burden and doesn’t bring any value IMO. If the aliaser is compromised then the aliasee is compromised, one cannot rely on an expiry as protection. One should immediately remove the authorisation in such an unlikely case.

The idea is worth evaluating from a UX perspective depending on the following:
A) will users authorise these relationships for repeated actions (effectively needing them alive for longer periods), or
B) will they be used for very occasional one-time actions (so for added security they should either be explicitly removed or automatically expire)

If the usage pattern is B, then an expiry actually helps UX. If it is A, it hinders UX (and adds more complexity - would need to maintain an ordered list based on expirations and manage it on each block - complex and expensive).

I say we keep it simple for now and see where it goes, we can pivot to the more expensive option if needed.

I see expiry as a way to not allow users to forget that they allowed some location to impersonate them. I think it's a UX improvement to allow users to forget but still clean up after themselves.

The reason why I'm proposing this is because it's not only the location they explicitly allow that could be the problem, they open the surface area of bugs to bugs on the other chain. The main value in my opinion is that if a user on chain A allows an account from chain B to impersonate them and then forgets, a future vulnerability in chain B can't affect them anymore given that it expires.

I'd expect the usage pattern to be B, but if it is A we can still make the expiry refresh every time they make an action with it.

On the implementation side I don't see how it's so complex and expensive. You'd just keep the block number timeout and disallow aliases that are past that.

added expiry here a606607, let me know what you think

polkadot/xcm/pallet-xcm/src/lib.rs

polkadot/xcm/pallet-xcm/src/migration.rs

prdoc/pr_6336.prdoc

…orize-alias

muharem · 2024-11-12T17:20:08Z

polkadot/xcm/pallet-xcm/src/lib.rs

+	/// Entry of an authorized aliaser for a local origin. The aliaser `location` is only authorized
+	/// until its inner `expiry` block number.
+	#[derive(Clone, Encode, Decode, Debug, Eq, PartialEq, Ord, PartialOrd, TypeInfo)]
+	pub(crate) struct OriginAliaser {


I do not really see the reason to make stored types and storage items private. At the end when you need to use it for some reason you just have to copy past them and create an alias storage item.

not sure what you mean..

should define this other place? or just make it pub instead of pub(crate)?

Yes, I would make them public

muharem · 2024-11-12T17:27:03Z

polkadot/xcm/pallet-xcm/src/lib.rs

+	#[derive(Clone, Encode, Decode, Debug, Eq, PartialEq, Ord, PartialOrd, TypeInfo)]
+	pub(crate) struct OriginAliaser {
+		pub(crate) location: VersionedLocation,
+		pub(crate) expiry: Option<u64>,


why not to reference the actual block number type?

had some trouble storing BlockNumberFor<T>, then realized the interface is better to be generic - once we move to relay-block-number-provider that type might change again, but u64 will always be compatible

two times bigger than what we really use. when we change to configurable provider, the block number type wont change

muharem · 2024-11-12T17:32:22Z

polkadot/xcm/pallet-xcm/src/lib.rs

+				authorized_aliases
+					.try_push(aliaser)
+					.map_err(|_| Error::<T>::TooManyAuthorizedAliases)?;
+				// todo: hold storage deposit


do you plan to address this todo in the current PR?

just pushed something, want feedback before I clean it up all the way

muharem · 2024-11-12T17:32:35Z

polkadot/xcm/pallet-xcm/src/lib.rs

+				// todo: hold storage deposit
+			}
+			AuthorizedAliases::<T>::insert(&versioned_origin, authorized_aliases);
+			Ok(())


some event?

yes, still needs events

muharem · 2024-11-12T17:35:50Z

polkadot/xcm/pallet-xcm/src/lib.rs

+			let block_num = frame_system::Pallet::<T>::block_number().saturated_into::<u64>();
+			let mut writes = 0;
+			// need to iterate keys and modify map in separate steps to avoid undefined behavior
+			let keys: Vec<VersionedLocation> = AuthorizedAliases::<T>::iter_keys().collect();


this might be too many reads, we should check the weight_cutoff on every iteration

do we need to in practice? iterating over even a million keys should fit in practice, no?

I'm not even sure how we could do partial iterations, how to resume from where we left off...

in the context of this function (may be I missing a broader picture), it looks wrong. a client passes weight_cutoff, for this stage we ignore it. we do not know how small or big the weight_cutoff.

I think we can iterate one by one and if there was no enough weight to complete, we can return RemoveExpiredAliasAuthorizations(Some(cursor)).

muharem · 2024-11-12T17:36:54Z

polkadot/xcm/pallet-xcm/src/lib.rs

+		AuthorizedAliases::<T>::get(&target)
+			.iter()
+			.find(|&aliaser| {
+				let current_block =
+					frame_system::Pallet::<T>::block_number().saturated_into::<u64>();
+				let not_expired =
+					aliaser.expiry.map(|expiry| expiry < current_block).unwrap_or(true);
+				aliaser.location == origin && not_expired
+			})
+			.is_some()


it will be useful as the Pallet's function

muharem · 2024-11-12T17:38:25Z

polkadot/xcm/pallet-xcm/src/migration.rs

@@ -170,6 +170,47 @@ pub mod data {
 		}
 	}

+	/// Implementation of `NeedsMigration` for `AuthorizedAliases` data.
+	impl<M: Get<u32>, T: frame_system::Config> NeedsMigration


acatangiu added the T6-XCM This PR/Issue is related to XCM. label Nov 1, 2024

acatangiu self-assigned this Nov 1, 2024

acatangiu force-pushed the authorize-alias branch from ec00d1f to d0111c3 Compare November 6, 2024 14:42

acatangiu added 7 commits November 6, 2024 19:32

make docs for LocalOriginToLocation uniform across codebase

31f7879

pallet-xcm: add support for authorizing aliases

7a0af10

add stub weights

879d48b

add benchmark code

9103e27

add migration for authorized aliases

c29fc5d

fix pallet-xcm authorized aliases

e84293f

asset hub westend: add support for authorized aliases

0332aca

acatangiu force-pushed the authorize-alias branch from 35170ae to 0332aca Compare November 6, 2024 17:32

acatangiu marked this pull request as ready for review November 6, 2024 17:32

acatangiu requested a review from a team as a code owner November 6, 2024 17:32

acatangiu requested a review from muharem November 6, 2024 17:33

bkontur approved these changes Nov 7, 2024

View reviewed changes

acatangiu added 4 commits November 7, 2024 13:15

Merge branch 'master' of github.com:paritytech/polkadot-sdk into auth…

9447da4

…orize-alias

address comments and fix tests after rebase

4611498

add prdoc

709cf3e

Merge branch 'master' of github.com:paritytech/polkadot-sdk into auth…

200ccee

…orize-alias

acatangiu requested a review from franciscoaguirre November 7, 2024 11:59

command-bot and others added 5 commits November 7, 2024 12:44

".git/.scripts/commands/bench/bench.sh" --subcommand=xcm --runtime=we…

35b4859

…stend --target_dir=polkadot --pallet=pallet_xcm

".git/.scripts/commands/bench/bench.sh" --subcommand=xcm --runtime=ro…

79bfce2

…coco --target_dir=polkadot --pallet=pallet_xcm

".git/.scripts/commands/bench/bench.sh" --subcommand=xcm --runtime=as…

673c3f6

…set-hub-westend --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm

".git/.scripts/commands/bench/bench.sh" --subcommand=xcm --runtime=as…

ac5d4a6

…set-hub-rococo --runtime_dir=assets --target_dir=cumulus --pallet=pallet_xcm

fix benchmarks

70694dd

acatangiu enabled auto-merge November 7, 2024 15:37

franciscoaguirre reviewed Nov 7, 2024

View reviewed changes

Update polkadot/xcm/pallet-xcm/src/benchmarking.rs

61c8224

franciscoaguirre and others added 6 commits November 7, 2024 18:17

Update polkadot/xcm/pallet-xcm/src/lib.rs

9d59105

Update polkadot/xcm/pallet-xcm/src/lib.rs

1b647db

chore: change new_version to XCM_VERSION

7b58df8

Merge branch 'master' of github.com:paritytech/polkadot-sdk into auth…

8764367

…orize-alias

rename vars

c239f8d

pallet-xcm: add expiry to authorized aliases

a606607

muharem reviewed Nov 12, 2024

View reviewed changes

pallet-xcm: add storage deposit for authorized aliases

b7ef64c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pallet-xcm: add support to authorize aliases #6336

pallet-xcm: add support to authorize aliases #6336

acatangiu commented Nov 1, 2024 •

edited

Loading

bkontur left a comment

acatangiu commented Nov 7, 2024

acatangiu commented Nov 7, 2024

franciscoaguirre Nov 7, 2024

acatangiu Nov 7, 2024

franciscoaguirre Nov 7, 2024 •

edited

Loading

acatangiu Nov 12, 2024

muharem Nov 12, 2024

acatangiu Nov 12, 2024

muharem Nov 13, 2024

muharem Nov 12, 2024

acatangiu Nov 12, 2024

muharem Nov 13, 2024

muharem Nov 12, 2024

acatangiu Nov 12, 2024

muharem Nov 12, 2024

acatangiu Nov 12, 2024

muharem Nov 12, 2024

acatangiu Nov 12, 2024

muharem Nov 13, 2024

muharem Nov 12, 2024

muharem Nov 12, 2024

pallet-xcm: add support to authorize aliases #6336

Are you sure you want to change the base?

pallet-xcm: add support to authorize aliases #6336

Conversation

acatangiu commented Nov 1, 2024 • edited Loading

bkontur left a comment

Choose a reason for hiding this comment

acatangiu commented Nov 7, 2024

acatangiu commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciscoaguirre Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acatangiu commented Nov 1, 2024 •

edited

Loading

franciscoaguirre Nov 7, 2024 •

edited

Loading