docs: Document safe use of Twox64Concat #601

weichweich · 2024-01-12T09:14:28Z

fixes https://github.com/KILTprotocol/ticket/issues/2529

Metadata Diff to Develop Branch

Peregrine Diff

!!! THE SUBWASM REDUCED DIFFER IS EXPERIMENTAL, DOUBLE CHECK THE RESULTS !!!
No change detected
SUMMARY:
- Compatible.......................: true
- Require transaction_version bump.: false

!!! THE SUBWASM REDUCED DIFFER IS EXPERIMENTAL, DOUBLE CHECK THE RESULTS !!!

Spiritnet Diff

!!! THE SUBWASM REDUCED DIFFER IS EXPERIMENTAL, DOUBLE CHECK THE RESULTS !!!
No change detected
SUMMARY:
- Compatible.......................: true
- Require transaction_version bump.: false

!!! THE SUBWASM REDUCED DIFFER IS EXPERIMENTAL, DOUBLE CHECK THE RESULTS !!!

Checklist:

I have verified that the code works
- No panics! (checked arithmetic ops, no indexing array[3] use get(3), ...)
I have verified that the code is easy to understand
- If not, I have left a well-balanced amount of inline comments
I have left the code in a better state
I have documented the changes (where applicable)
- Either PR or Ticket to update the Docs
- Link the PR/Ticket here

We can't ensure that the `Identifier` is not user choosable

weichweich · 2024-01-12T11:32:17Z

pallets/pallet-deposit-storage/src/lib.rs

+		_,
+		Twox64Concat,
+		<T as Config>::Namespace,
+		Blake2_128Concat,


note this change. I think the DepositKey might be chosable by the user

This follows the same reasoning as my other comments (not sure if above or below). The concrete type of a deposit key is defined by the runtime, but it could take user-specified values, so I am all in favor for this change.

ntn-x2

The public credentials pallet also has a pub type Credentials<T> = StorageDoubleMap<_, Twox64Concat, SubjectIdOf<T>, Blake2_128Concat, CredentialIdOf<T>, CredentialEntryOf<T>>; which, more than anything else, allows for a user-chosen SubjectId, which is typically an asset DID which, although it has a specific syntax, can still be crafted by the user.

pallets/pallet-dip-provider/src/lib.rs

pallets/pallet-dip-consumer/src/lib.rs

ntn-x2 · 2024-01-17T08:25:59Z

pallets/pallet-deposit-storage/src/lib.rs

+		_,
+		Twox64Concat,
+		<T as Config>::Namespace,
+		Blake2_128Concat,


This follows the same reasoning as my other comments (not sure if above or below). The concrete type of a deposit key is defined by the runtime, but it could take user-specified values, so I am all in favor for this change.

ntn-x2 · 2024-01-17T08:26:47Z

pallets/did/src/lib.rs

+	// SAFETY of Twox64Concat:
+	// The DID Identifier must be registered on chain first. To register a DID
+	// Identifier, you must provide a valid signature for the identifier or
+	// control the origin that corresponds to the identifier.


How does this play out with cross-chain origins? I know probably migration would be hard here, but it might still be worth it.

The DID that creates the endpoint still needs to be a valid accountId. If you would be able to choose an arbitrary accountId you could impersonate other accounts. So I don't think that this will ever be possible.

No, but the resulting account ID is given by our location converter. There might be a way for a chain to craft origins which are then converted into something else on our chain. That would be the same as crafting origins on our chain.

Creating specific AccountIds on our chain should never be possible? Even if the origin is coming from another chain.

Not saying that. But to have an account on our chain you must have a valid private key to generate the signature. On a different chain, "trying" with different accounts might be cheaper, e.g., if, by absurd, you simply create a chain to spam all possible account IDs. This is probably very theoretical, but still possible. All those origins would be converted into some sort of account ID on our chain, but we don't really know what's the "proof of ownership" for that account ID on the source chain. Maybe this is somehow prevented by having XCM fees in place, which is also a mitigation in our case.

weichweich · 2024-01-30T10:17:23Z

check Metadata and SDK compatibility before merging

ntn-x2 · 2024-03-06T15:53:44Z

This will break cross-chain DIP proofs since the storage keys (and the related proofs) have changed, hence we might want to change at least the DIP stuff to make it resistant.

Changing the storage hasher before we deploy on production, otherwise we would then need a migration strategy. I think we can disregard any migrations on our testnets, as the effort is not justified. I also lowered the limits of the DIP provider template, which were higher than Peregrine, so that it is faster to generate the benchmark worst case. Related to #601, based on top of #612.

ntn-x2 · 2024-05-20T10:57:23Z

DIP stuff was fixed in #613.

ntn-x2 · 2024-05-22T08:52:24Z

I'm putting this on hold until we get to a polkadot-sdk version that supports multi-block-migrations, after which we can revise this PR and perform the necessary migrations.

weichweich added 8 commits January 3, 2024 16:27

fix: switch to Blake2 hashing for ExternalAttestations

d138faf

docs(pallet-postit): add safety comment

2cf3adf

fix(pallet-dip-consumer): use Blake2_128Concat hasher

dcfc942

We can't ensure that the `Identifier` is not user choosable

fix: copy paste error

5158f25

fix(pallet-deposit): safe use of Twox64Concat

e15e9c5

chore: document safe use of Twox64

16edb75

fix: clippy

0cd7ad3

Update docs & revert instroduction of Blake2_128Concat

add4848

weichweich commented Jan 12, 2024

View reviewed changes

weichweich requested review from ntn-x2 and Ad96el January 12, 2024 11:49

weichweich marked this pull request as ready for review January 12, 2024 11:49

weichweich added 2 commits January 12, 2024 12:49

Merge branch 'develop' into aw-safe-twox-hashing

616f857

fmt

ef7c7f4

ntn-x2 reviewed Jan 17, 2024

View reviewed changes

Merge branch 'develop' into aw-safe-twox-hashing

7e4ffcc

Merge branch 'develop' into aw-safe-twox-hashing

6463c51

ntn-x2 mentioned this pull request Mar 6, 2024

fix: change DIP components storage hashers #613

Merged

ntn-x2 added the ✋on hold status: on hold label May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: Document safe use of Twox64Concat #601

docs: Document safe use of Twox64Concat #601

weichweich commented Jan 12, 2024 •

edited

weichweich Jan 12, 2024

ntn-x2 Jan 17, 2024

ntn-x2 left a comment

ntn-x2 Jan 17, 2024

ntn-x2 Jan 17, 2024

weichweich Jan 18, 2024

ntn-x2 Jan 18, 2024

weichweich Jan 18, 2024

ntn-x2 Jan 18, 2024

weichweich commented Jan 30, 2024

ntn-x2 commented Mar 6, 2024

ntn-x2 commented May 20, 2024 •

edited

ntn-x2 commented May 22, 2024

docs: Document safe use of Twox64Concat #601

Are you sure you want to change the base?

docs: Document safe use of Twox64Concat #601

Conversation

weichweich commented Jan 12, 2024 • edited

fixes https://github.com/KILTprotocol/ticket/issues/2529

Metadata Diff to Develop Branch

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntn-x2 left a 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

weichweich commented Jan 30, 2024

ntn-x2 commented Mar 6, 2024

ntn-x2 commented May 20, 2024 • edited

ntn-x2 commented May 22, 2024

weichweich commented Jan 12, 2024 •

edited

ntn-x2 commented May 20, 2024 •

edited