[Identity] Remove double encoding username signature payload#4646
[Identity] Remove double encoding username signature payload#4646georgepisaltu merged 11 commits intoparitytech:masterfrom
Conversation
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
georgepisaltu
added
the
T2-pallets
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
|
The CI pipeline was cancelled due to failure one of the required jobs. |
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
prdoc/pr_4646.prdoc
Outdated
|
|
||
| crates: | ||
| - name: pallet-identity | ||
| bump: minor |
There was a problem hiding this comment.
Isn't this breaking the Identity::set_username_for() call? The API is not changed per-se, but IIUC the caller has to provide a "different" signature now.
| bump: minor | |
| bump: major |
There was a problem hiding this comment.
The API is the same and the caller has to construct the payload to be signed differently, but at the same time:
- this is a bug and the payload to be signed should have never had the leading length bytes
- this username feature is relatively new and unused (hence why nobody complained about the bug)
Given that the actual code is backwards compatible, I didn't mark it as a major change, but because of the behavior change it's not a patch either, so I went with minor. Do you still think it should be marked as a major change? I still have to update the toml version anyway.
There was a problem hiding this comment.
this is a bug and the payload to be signed should have never had the leading length bytes
Was it working before? If yes, then I see it as a breaking/major change.
Like you correctly say, at the API definition code contract level, it is "backward compatible", but in reality it isn't (any existing tool/lib/UI will continue to "work" but provide incorrect signatures).
If it was not working before because of this bug, and now it does, it can even be a patch fix.
this username feature is relatively new and unused (hence why nobody complained about the bug)
I'm glad we caught this early, that means nobody will complain about the potential major change.
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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>
…ech#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>
…ech#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>
…ech#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>
5 participants
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 when generating the payload to sign.Encoding a
Vecadds 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.