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.

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

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

Closed

Conversation

georgepisaltu
Copy link
Contributor

Backport for #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 <[email protected]>
@georgepisaltu georgepisaltu requested a review from a team as a code owner June 5, 2024 10:26
@Morganamilo
Copy link
Contributor

This change is marked as major so is not suitable for backport. The existing release branches should stay stable and have no breaking changes.

@georgepisaltu
Copy link
Contributor Author

Ok, closing the PR then

@joepetrowski
Copy link
Contributor

@Morganamilo we have had some other "major" backports, even that changed the Config trait types. I know it is not perfect but this is important to ensure that actual breaking changes don't happen on the People Chain, and it's a good opportunity to do it before the affected feature is even in production at all.

@joepetrowski joepetrowski reopened this Jun 5, 2024
acatangiu
acatangiu previously approved these changes Jun 5, 2024
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we can keep everything sane by yanking the previous version of pallet-identity and releasing this new "patched" one.

The major breaking change here will not break compilation (API is unchanged); the interpretation of one of the parameters does indeed change, so it is only breaking for actual users of it - which there are none yet since this hasn't been deployed anywhere yet.

Patching the fix here allows us to use the right behavior on the first deployment.

@@ -1,6 +1,6 @@
[package]
name = "pallet-identity"
version = "29.0.1"
version = "30.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

30.0.0 is already released (guessing with sdk v1.8.0)

Suggested change
version = "30.0.0"
version = "29.0.2"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like latest on crates.io is v34.0.0, so there's some versions in between. Anyway, updated it to v29.0.2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Release tooling handles versioning. There's no need to change the crate or dep versions.

@acatangiu acatangiu self-requested a review June 5, 2024 14:12
@acatangiu acatangiu dismissed their stale review June 5, 2024 14:12

need to fix the version numbers first

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6410534

@Morganamilo
Copy link
Contributor

In this case we can keep everything sane by yanking the previous version of pallet-identity and releasing this new "patched" one.

The major breaking change here will not break compilation (API is unchanged); the interpretation of one of the parameters does indeed change, so it is only breaking for actual users of it - which there are none yet since this hasn't been deployed anywhere yet.

Patching the fix here allows us to use the right behavior on the first deployment.

If this crate is considered unreleased then in this case it's fine. But I don't know what you mean by it hasn't been deployed yet.

@georgepisaltu
Copy link
Contributor Author

it is only breaking for actual users of it - which there are none yet since this hasn't been deployed anywhere yet

Technically it is deployed on Kusama People chain, but there is no way to use the broken functionality because there are no registered username authorities. It is unreleased on Polkadot People chain, which is good because we have a chance to fix it. I think yanking the current version makes sense and we can just release this "patch" version on top of the yanked one, and do the same for the remaining release branches.

@bkchr bkchr closed this Jul 25, 2024
@bkchr bkchr deleted the george-identity-backport branch July 25, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants