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

Make `MultiSigner` use compressed ECDSA public key #4502

Merged
merged 4 commits into from Dec 31, 2019
Merged

Conversation

@bkchr
Copy link
Contributor

bkchr commented Dec 26, 2019

MultiSigner did not compressed the ECDSA public key before hashing it but MultiSignature expected that the public key is compressed when verifying a signature. Thus, the signature verification could never succeed.

This pr changes the following:

  • MultiSigner compresses the public key before hashing it with blake256.
  • Verify for ecdsa::Signature compresses given public key before comparing it.
  • ecdsa::Public is now an enum that distinguishes between the full and compressed format.

Fixes: #4498

@@ -246,7 +246,9 @@ impl traits::IdentifyAccount for MultiSigner {
match self {
MultiSigner::Ed25519(who) => <[u8; 32]>::from(who).into(),
MultiSigner::Sr25519(who) => <[u8; 32]>::from(who).into(),
MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256(who.as_ref()).into(),
MultiSigner::Ecdsa(who) => sp_io::hashing::blake2_256(
&who.as_compressed().expect("what should we do?")[..],

This comment has been minimized.

Copy link
@bkchr

bkchr Dec 27, 2019

Author Contributor

@gavofyork what should we do here? I think the best would be to propagate the error via IdentifyAccount.

This comment has been minimized.

Copy link
@gavofyork

gavofyork Dec 30, 2019

Member

i believe this cannot fail for any valid keypair, so .expect should be fine

@bkchr bkchr changed the title Don't use compressed ecdsa public key in verify Make `MultiSigner` Dec 27, 2019
@bkchr bkchr changed the title Make `MultiSigner` Make `MultiSigner` use compressed ECDSA public key Dec 27, 2019
@gavofyork

This comment has been minimized.

Copy link
Member

gavofyork commented Dec 30, 2019

but... what's the point of supporting uncompressed public key format?

@bkchr

This comment has been minimized.

Copy link
Contributor Author

bkchr commented Dec 30, 2019

but... what's the point of supporting uncompressed public key format?

Good question, I just followed the initial implementation. However, I think that for ergonomic reasons the support for both sounds reasonable to me. If you say that people only use the compressed version, I can remove the support for full public keys.

@gavofyork

This comment has been minimized.

Copy link
Member

gavofyork commented Dec 30, 2019

it does leave this annoying .expect thing in.

@gavofyork gavofyork merged commit 20a9b15 into master Dec 31, 2019
13 of 14 checks passed
13 of 14 checks passed
continuous-integration/gitlab-check_polkadot Build stage: build; status: failed
Details
continuous-integration/gitlab-cargo-check-benches Build stage: test; status: success
Details
continuous-integration/gitlab-cargo-check-subkey Build stage: test; status: success
Details
continuous-integration/gitlab-check-line-width Build stage: test; status: success
Details
continuous-integration/gitlab-check-runtime Build stage: test; status: success
Details
continuous-integration/gitlab-check-web-wasm Build stage: test; status: success
Details
continuous-integration/gitlab-check_warnings Build stage: build; status: success
Details
continuous-integration/gitlab-node-exits Build stage: test; status: success
Details
continuous-integration/gitlab-test-dependency-rules Build stage: test; status: success
Details
continuous-integration/gitlab-test-frame-staking Build stage: test; status: success
Details
continuous-integration/gitlab-test-full-crypto-feature Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable Build stage: test; status: success
Details
continuous-integration/gitlab-test-linux-stable-int Build stage: test; status: success
Details
continuous-integration/gitlab-test-wasmtime Build stage: test; status: success
Details
@gavofyork gavofyork deleted the bkchr-fix-ecdsa-verify branch Dec 31, 2019
bkchr added a commit that referenced this pull request Jan 17, 2020
Some fixes after: #4502

This removes the unwanted `expect`s from `MultiSigner`. Instead we
convert from full to compressed in `TryFrom` and can return an error on
invalid input.
gavofyork added a commit that referenced this pull request Jan 20, 2020
Some fixes after: #4502

This removes the unwanted `expect`s from `MultiSigner`. Instead we
convert from full to compressed in `TryFrom` and can return an error on
invalid input.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.