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

Hash tx metadata before signing #2589

Merged
merged 3 commits into from
Apr 1, 2021
Merged

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Mar 31, 2021

Issue Number

ADP-800

Overview

When signing tx metadata using the signMetadata endpoint, hash the message first before signing.

@rvl rvl self-assigned this Mar 31, 2021
@@ -1115,9 +1115,10 @@ spec = describe "SHELLEY_WALLETS" $ do
let dummyChainCode = BS.replicate 32 0
let sig = CC.xsignature $ BL.toStrict $ getFromResponse id rSig
let key = unsafeXPub $ fst (getFromResponse #getApiVerificationKey rKey) <> dummyChainCode
let msg = unsafeFromHex "A10071706C65617365207369676E20746869732E"
let _msg = unsafeFromHex "A10071706C65617365207369676E20746869732E"
Copy link
Contributor

Choose a reason for hiding this comment

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

this one probably we want to delete, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the string is already shown in the comment above. I deleted it.

pure $
Signature $ BA.convert $
CC.sign encPwd (getRawKey addrK) $
hash @ByteString @Blake2b_256 $
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

lgtm! wonder if we can add some golden testing basing on some already existent test in cardano-node?

@rvl rvl force-pushed the rvl/adp-800/hash-sign-metadata branch from 1095188 to 9534e73 Compare April 1, 2021 07:47
@rvl
Copy link
Contributor Author

rvl commented Apr 1, 2021

I added a cheap "golden" signature check to the same integration test.

@rvl
Copy link
Contributor Author

rvl commented Apr 1, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request Apr 1, 2021
2589: Hash tx metadata before signing r=rvl a=rvl

### Issue Number

ADP-800

### Overview

When signing tx metadata using the `signMetadata` endpoint, hash the message first before signing.



Co-authored-by: Rodney Lorrimar <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 1, 2021

Build failed:

/nix/store/h253ilicq3kga1yj50aq28w6bbi58fny-stdenv-linux/setup: line 1312:   102 Killed                  $SETUP_HS build test:unit -j$(($NIX_BUILD_CORES > 4 ? 4 : $NIX_BUILD_CORES)) --ghc-option=-fexternal-interpreter --ghc-option=-pgmi --ghc-option=/nix/store/7hya6hh1bd98hpzgcqyy6pfkcqrsl0lj-iserv-wrapper/bin/iserv-wrapper --ghc-option=-L/nix/store/km0yd810a6yqhdp0fnprqzrvj6c90myl-mingw-w64-6.0.0-x86_64-w64-mingw32-pthreads-x86_64-w64-mingw32/lib --ghc-option=-L/nix/store/km0yd810a6yqhdp0fnprqzrvj6c90myl-mingw-w64-6.0.0-x86_64-w64-mingw32-pthreads-x86_64-w64-mingw32/bin --ghc-option=-L/nix/store/0wa4c1jirwkxrk8x0fsqaxx89j4razfr-gmp-6.2.1-x86_64-w64-mingw32/lib
builder for '/nix/store/8ywcqmpmxqxg0amx4axfhgjndiakpp9l-cardano-wallet-core-test-unit-2021.3.4-x86_64-w64-mingw32.drv' failed with exit code 137

The build process was killed - hmm!

@rvl rvl force-pushed the rvl/adp-800/hash-sign-metadata branch from 5bede68 to b023425 Compare April 1, 2021 11:48
@rvl
Copy link
Contributor Author

rvl commented Apr 1, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Apr 1, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 7df3079 into master Apr 1, 2021
@iohk-bors iohk-bors bot deleted the rvl/adp-800/hash-sign-metadata branch April 1, 2021 13:25
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.

2 participants