Byron wallet restoration from xprv endpoint integration test #1476

paweljakubas · 2020-03-23T12:54:54Z

Issue Number

Overview

I have added encryptPasswordWithScrypt
I have added encryptPasswordWithScrypt properties in unit test
I have extended DSL to check new endpoint
I have added integration test to ByronWallets in core-integration
I have removed wrong 64-byte requirement for passphrase hash
I have added integration test to ByronTransactions in byron

Comments

piotr-iohk · 2020-03-23T14:40:00Z

@paweljakubas, @KtorZ
If I'm not mistaken the idea is that input for this enpoint will be produced by the script that was created here -> input-output-hk/cardano-sl#4278?

If that'e the case there is some data from the script produced while testing that could be used for testing the endpoint (input-output-hk/cardano-sl#4278 (comment) or input-output-hk/cardano-sl#4278 (comment)).

However trying this (input-output-hk/cardano-sl#4278 (comment)) data on testnet is not successful. Mostly results in:

{
  "code": "bad_request",
  "message": "Error in $['passphrase_hash']: Invalid encryption hash: expecting a hex-encoded value that is 64 bytes in length."
}

but also errors about invalid Json or name (more details below), which is unexpected because I was able to create this data using cardano-wallet on cardano-sl and then exported it using export-wallets script. Not sure if the issue is in the enpoint or in the script from input-output-hk/cardano-sl#4278. (but it seems they need to be tuned to work together)

Details of the request/response using this data:

curl  -vX POST http://localhost:8090/v2/byron-wallets \
  -H "Content-Type: application/json; charset=utf-8" \
  -d '{
	"style": "random",
		  "encrypted_root_private_key": "65b0a8a43341078aa25e499457902bed3e7d63a74d6079a563e08f0a7e2854dc918ed1b98c2e262d837276cce3fa8c3825ae5aa0f24535168b597127bd62461b323dba7f9c2beb5dbf24b528b6981e83c2e21a4aa7684746d86b763191aada4f980a05ec790380f10c8d05df95750bdd482870ed294ffc2f6356da69d0287950",
		  "name": "`~`!@#$%^&*()_+-=<>,./?;':\"\"'{}[]\\|❤️ 💔 💌 💕 💞   💓 💗 💖 💘 💝 💟 💜 💛 💚 💙0️⃣ 1️⃣ 2️⃣ 3️⃣ 4️⃣ 5️⃣ 6️⃣ 7️⃣ 8️⃣ 9️⃣ 🔟🇺🇸🇷🇺🇸 🇦🇫🇦🇲🇸",
		  "passphrase_hash": "31347c387c317c57434338486c412b6e3834734c426e55427450447a4a59536e6a7742794b4f5662387171534977476f48557a7a773d3d7c497944797147756c6b493733494f4d66544e615034572b7447396b78485a54665870795a572f436733316f3d"
  }' --http1.1  |  jq

👇

{
  "code": "bad_request",
  "message": "I couldn't understand the content of your message. If your message is intended to be in JSON format, please check that the JSON is valid."
}

curl  -vX POST http://localhost:8090/v2/byron-wallets \
  -H "Content-Type: application/json; charset=utf-8" \
  -d '{
	"style": "random",
		"encrypted_root_private_key": "1dd082ced0f831afe5ab065450ed136ac06e412adf0a06edfdca5fb21eb777df68e62e7b2b3fee8c7d9c21e8cf1dd7bf655b7684d7149f52d1f6ec0f723aab9436db71be813be0462930225922f1d11a544db877b1a5bd5dddfc73e469ad3980fd27e3436a49ba158810ecbb1612006d79eac41dd8ebe0ab67d184fed8d7479f",
		"name": "亜哀挨愛曖悪握圧扱宛嵐安案暗以衣位囲医依委威為畏胃尉異移萎偉椅彙意違維慰  遺緯域育一壱逸茨芋引印因咽姻員院淫陰飲隠韻右宇羽雨唄鬱畝浦運雲永泳英映栄\n営詠影鋭衛易疫益液駅悦越謁  閲円延沿炎怨宴媛援園煙猿遠鉛塩演縁艶汚王凹\r\n央応往押旺欧殴桜翁奥横岡屋億憶臆虞乙俺卸音恩温穏下化火加  可仮何花佳価果河苛科架夏家荷華菓貨渦過嫁暇禍靴寡歌箇稼課蚊牙瓦我画芽賀雅餓介回灰会快戒改怪拐悔海界  皆械絵開階塊楷解潰壊懐諧貝外劾害崖涯街慨蓋該概骸垣柿各角拡革格核殻郭覚較隔閣確獲嚇穫学岳楽額顎掛潟  括活喝渇割葛滑褐轄且株釜鎌刈干刊甘汗缶\r",
		"passphrase_hash": "31347c387c317c574343746d337732414a464a4f6a326d506c334a544c5a3533427057366263457838394c76344c2b6e70545752673d3d7c427359646477565656304b786c567a46666264706a69495a6f654f505430565a624f4a47492f42537771593d"
   }' --http1.1  |  jq

👇

{
  "code": "bad_request",
  "message": "Error in $.name: name is too long: expected at most 255 characters"
}

curl  -vX POST http://localhost:8090/v2/byron-wallets \
  -H "Content-Type: application/json; charset=utf-8" \
  -d '{
	"style": "random",
		"encrypted_root_private_key": "edb251441107f6d70613ab89eec818753ff4390ffdd446e9ffe431becb7b22d8e7e5a6875624548a249f489c333f397b8da55fcba32d764166f2b402e5086525dfecf921cac5281a07e607297b20541818e166ff4e127fac8c837860703e7220820256aef6576cd2053a10b64c82d58d471262bbf2c95bb9f246d41daf2b3713",
		"name": "ثم نفس سقطت وبالتحديد،, جزيرتي باستخدام أن دنو. إذ هنا؟ الستار وتنصيب كان. أهّل ايطاليا، بريطانيا-فرنسا قد أخذ. سليمان، إتفاقية بين ما, يذكر الحدود أي بعد, معاملة بولندا، الإطلاق عل إيو.",
		"passphrase_hash": "31347c387c317c5743415a67386d325345577275505874656452636277583637524e55793763722f7778414b5a494e466755596b513d3d7c694d6f66593074445164794b375549745a796c6677392b425831626f6f73433672523532684630305932553d"
   }' --http1.1  |  jq

👇

{
  "code": "bad_request",
  "message": "Error in $['passphrase_hash']: Invalid encryption hash: expecting a hex-encoded value that is 64 bytes in length."
}

curl  -vX POST http://localhost:8090/v2/byron-wallets \
  -H "Content-Type: application/json; charset=utf-8" \
  -d '{
	"style": "random",
		"encrypted_root_private_key": "75bea242039182aea099c443ba601572fc68e03eba46309353453148671695dc458df8b715e515784120aa14da9ea671b5b10b8cb29528dac0ecd593812d4ac084443bfc8dace09c03e96fbef07d8853ca7774f633d97a91eb2243c9bcf1a6f36290b278271c8a782b5bfc42aef90984ab2fecad286669f51482871546d655ad",
		"name": "aąbcćdeęfghijklłmnoóprsś\r\ntuvwyzżźAĄBCĆDEĘFGHIJKLŁMNOP\rRSŚTUVWYZŻŹ",
		"passphrase_hash": "31347c387c317c57434374303455384757434b3171646b7439635a2b65336e3343595837487a4831593531487a5166795141644b673d3d7c4a6837513162473550647a783442372b50414374776676494a3158616f757642334b5a6b5033493763676f3d"
	  }' --http1.1  |  jq

👇

{
  "code": "bad_request",
  "message": "Error in $['passphrase_hash']: Invalid encryption hash: expecting a hex-encoded value that is 64 bytes in length."
}

curl  -vX POST http://localhost:8090/v2/byron-wallets   -H "Content-Type: application/json; charset=utf-8"   -d '{
"style": "random",
"encrypted_root_private_key": "5d80b8cf019053034580f81474da1c490873679891581f0dce6d341183df31c49d79de812c33a46a7f79eaf2f436c618d3efc45c70d9a18af57fb2282dd93775bdd4035be38887881fb3813f2c317a82a749ce135275c8ec5567830213a3cf0e55396dcf70a6002a075eb1b550d6ba1955781a8b57c68629721c7669eb7b0513",
"name": "АаБбВвГгДдЕеЁёЖжЗз ИиЙйКкЛлМмНнО оПпРрСсТтУуФф ХхЦцЧчШшЩщЪъ ЫыЬьЭэЮюЯяІ ѢѲѴѵѳѣі",
"passphrase_hash": "31347c387c317c5743445145374e6156312b78304455325930736737667335314c6f366c5648682b4435744e74334559434c367a513d3d7c454d6471313478504f67614979573945355539426973337a49326e2f5a787a532f587230483746473377593d"
  }' --http1.1  |  jq

👇

{
  "code": "bad_request",
  "message": "Error in $['passphrase_hash']: Invalid encryption hash: expecting a hex-encoded value that is 64 bytes in length."
}

paweljakubas · 2020-03-23T18:07:39Z

@piotr-iohk I was aware of this restriction (hence this PR). When designing this U/S we were thinking that 64-byte passphrase hash is requirement. But it turn out to be wrong. Which is corrected in 0c3ccb4

KtorZ · 2020-03-23T18:25:27Z

@piotr-iohk If I'm not mistaken the idea is that input for this enpoint will be produced by the script that was created here -> input-output-hk/cardano-sl#4278?

Correct ✔️

but also errors about invalid Json or name (more details below), which is unexpected because I was able to create this data using cardano-wallet on cardano-sl and then exported it using export-wallets script.

Damn. I didn't thought about the length of the name... I just checked, in -sl, there's no actual upper limit, so names could in practice be of any arbitrary length... We can't possibly accept that on the new implementation. But I am not sure that rejecting restoration would help.

Truncating the name down to 255 characters could make sense, don't you think @piotr-iohk @paweljakubas ? It's quite an edge-case and, what we do really want is users to be able to identify their wallets.

Also, to be checked with Daedalus but, there's maybe a limit on the frontend, so all Daedalus users should actually do quite okay and not run into the issue at all. So maybe rejecting is fine after all and truncating isn't needed.

paweljakubas · 2020-03-23T19:31:18Z

there is limitation on wallet name in daedalus : https://github.com/input-output-hk/daedalus/blob/318e5ec583ed3c0220b2f801cbf3b6f88713464c/source/renderer/app/utils/validations.js#L6

paweljakubas · 2020-03-23T19:31:44Z

bors try

iohk-bors · 2020-03-23T20:09:49Z

try

Build failed

buildkite/cardano-wallet

paweljakubas · 2020-03-23T22:07:46Z

bors try

iohk-bors · 2020-03-23T22:38:29Z

try

Build failed

buildkite/cardano-wallet

paweljakubas · 2020-03-23T22:40:53Z

two times failing because of :

  | expected: Status {statusCode = 202, statusMessage = "Accepted"}
  | but got: Status {statusCode = 500, statusMessage = "Internal Server Error"}

KtorZ · 2020-03-24T09:25:51Z

lib/byron/test/integration/Test/Integration/Byron/Scenario/API/Transactions.hs

+            \597a425834515177666475467578436b4d485569733d7c78324d646738\
+            \49554a3232507235676531393575445a76583646552b7757395a6a6a2f\
+            \51303054356c654751794279732f7662753367526d726c316c657a7150\
+            \43676d364e6758476d4d2f4b6438343265304b4945773d3d"


Where does this hash come from 😶 ?

I added tests for "cardano-wallet" here : https://github.com/input-output-hk/cardano-wallet/pull/1476/files#diff-131f348ed80ae192131796acae9042efR273

moved to DSL with proper comment : c8e89ed

lib/core-integration/src/Test/Integration/Framework/DSL.hs

KtorZ · 2020-03-24T09:43:03Z

specifications/api/swagger.yaml

@@ -270,15 +270,13 @@ x-walletPassphrase: &walletPassphrase

 x-walletPassphraseHash: &walletPassphraseHash
  description: |
-    A hash of master passphrase. The hash should be a 64-byte output of a Scrypt function with the following parameters:
+    A hash of master passphrase. The hash should be an output of a Scrypt function with the following parameters:


We do know the size however right, it's actually a 100 bytes? So 200 hex-encoded. Isn't it.

when looking at https://github.com/input-output-hk/cardano-wallet/pull/1476/files#diff-131f348ed80ae192131796acae9042efR247-R280
I see that the hex-encoded strings are the same in length, but the one added by you ("patate") is different. I used this to have it :

let p = Passphrase @"raw" $ BA.convert $ T.encodeUtf8 PASSWD hp <- encryptPasswordWithScrypt p TR.trace ("hp:"<>show (hex hp))

KtorZ · 2020-03-24T09:51:55Z

lib/core-integration/src/Test/Integration/Scenario/API/ByronWallets.hs

+            ("Byron Wallet", rootXPrv, passwHash)
+        rd2 <- request
+              @ApiByronWallet ctx (Link.deleteWallet @'Byron w2) Default Empty
+        expectResponseCode @IO HTTP.status204 rd2


What's the difference between this scenario and the one in "BYRON_RESTORE_01 - one recipient". To me, it sounds like the latter makes this one redundant (as it includes it already, but has more steps)

yes, BYRON_RESTORE_01 is more powerful and check everything what is here, and even more

addressed in c8e89ed

KtorZ · 2020-03-24T09:52:37Z

lib/core/src/Cardano/Wallet/Api/Types.hs

@@ -685,7 +684,14 @@ instance ToText (ApiT XPrv) where
        . getApiT

 instance FromText (ApiT (Hash "encryption"))  where
-    fromText = fmap ApiT . hashFromText 64


Why not hashFromText 100 🤔 Can the Scrypt output actually be of different sizes ?

if i is 100 then we should have hashFromText 100 but I have this comment : #1476 (comment)

KtorZ · 2020-03-24T09:54:54Z

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivationSpec.hs

+    Hash . Scrypt.getEncryptedPass <$>
+    Scrypt.encryptPassIO Scrypt.defaultParams (Scrypt.Pass $ BA.convert passwd)
+  where
+    (Passphrase passwd) = preparePassphrase EncryptWithScrypt p


side note, Passphrase has an instance of ByteArrayAccess, to BA.convert can be called on it directly, no need to pattern match on this output 👍

Passphrase is BA.ScrubbedBytes inside and Scrypt.Pass requires B8.ByteString - hence I used BA.convert here

KtorZ · 2020-03-24T09:55:05Z

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivationSpec.hs

+    p /= p' ==> monadicIO $ liftIO $ do
+        hp <- encryptPasswordWithScrypt p
+        checkPassphrase EncryptWithScrypt p' hp
+            `shouldBe` Left ErrWrongPassphrase


KtorZ · 2020-03-24T09:55:09Z

lib/core/test/unit/Cardano/Wallet/Primitive/AddressDerivationSpec.hs

+    -> Property
+prop_passphraseFromScryptRoundtrip p = monadicIO $ liftIO $ do
+    hp <- encryptPasswordWithScrypt p
+    checkPassphrase EncryptWithScrypt p hp `shouldBe` Right ()


KtorZ · 2020-03-24T10:00:37Z

One thing that would be worth testing as well is, what happens if we submit an "invalid" passphrase hash? Something that wasn't generated from Scrypt. The behavior should be that we basically fail to use this wallet because we'll never possibly get the passphrase. It'll be good to make sure that the scrypt function doesn't throw when presented an illed-formed passphrase :s

lib/byron/test/integration/Test/Integration/Byron/Scenario/API/Transactions.hs

paweljakubas · 2020-03-24T15:21:34Z

@piotr-iohk @KtorZ I added integration test with corrupt passphrase hash, but correct rootXPrv and it restored in a sense that available balance is like faucet wallet, but was expecting different :

  1) API Specifications, BYRON_RESTORATION, BYRON_RESTORE_03 - restoring wallet from corrupted hash gives 0 balance
       expected: Quantity {getQuantity = 0}
        but got: Quantity {getQuantity = 1000000000000}

The test was added here : 359c914

BUT it seems to be ok, because restoration is basing on master key as specified here : https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/src/Cardano/Wallet/Api/Server.hs#L910

Is this as expected?

piotr-iohk · 2020-03-24T15:34:25Z

re: #1476 (comment)

Is this as expected?

I wonder if we should allow restoring in case of corrupt passphrase... it makes the wallet useless I suppose, cause the passphrase is ... unavailable, isn't it.

KtorZ · 2020-03-24T15:42:22Z

@paweljakubas @piotr-iohk that's a feature of Random wallets in the Byron era indeed. Good to know:

The Haskell XPrv type that we manipulate is actually made of:
- A public key
- A private key
- A chain code
In a XPrv, only the private key part is actually encrypted, the public key is not.
To restore a Random wallet, one need to know the wallet root public key, from which is derived a payload passphrase that encrypts the underlying address derivation paths. If we can decrypt the derivation path, it means the address is ours.
It's because of 2. & 3. that this kind of XPrv import is possible through the wallet. We can extract the public key from an encrypted XPrv and use it to restore the wallet. We can't however spend funds without the spending passphrase, because the unencrypted private key is required for that.
This is messed up.

Thus @paweljakubas , this is indeed expected. You do not need the passphrase to actually restore the wallet. So, the passphrase hash can be complete garbage, one would still be able to restore the wallet with only the XPrv. @piotr-iohk you're right, having a corrupt passphrase will make the wallet unusable (or at least, unable to sign transactions). But the question is how to tell whether a given passphrase hash is corrupted or not? This is tricky because:

a. From @paweljakubas' tests, it seems that the Scrypt Pass data-types can be of variable length. Although, there's probably a spectrum that is safe to consider.

b. In theory, you can only really know whether the passphrase is correct when you actually get your hand on the passphrase (i.e., when submitting a transaction) 🙃

paweljakubas · 2020-03-24T15:53:03Z

@KtorZ thanks for confirmation and comment. It would be good then to add test restoring wallet with wrong passphrase hash and show that making tx is not possible with it.

correct property tests

nix patch

compilation correction

correct expectation plus nix patch

paweljakubas · 2020-03-25T16:02:28Z

bors try

iohk-bors · 2020-03-25T16:43:31Z

try

Build succeeded

buildkite/cardano-wallet

KtorZ

LGTM

paweljakubas force-pushed the paweljakubas/1436/byron-wallet-restoration-from-xprv-endpoint-integration-test branch from b8220c7 to 9aa7e4a Compare March 23, 2020 13:51

paweljakubas force-pushed the paweljakubas/1436/byron-wallet-restoration-from-xprv-endpoint-integration-test branch from 9aa7e4a to 0c3ccb4 Compare March 23, 2020 17:47

paweljakubas requested a review from KtorZ March 23, 2020 17:49

paweljakubas self-assigned this Mar 23, 2020

paweljakubas added this to the (ADP-210) Restore byron wallets from xpriv+passwd hash milestone Mar 23, 2020

iohk-bors bot added a commit that referenced this pull request Mar 23, 2020

Try #1476:

5d365d2

iohk-bors bot added a commit that referenced this pull request Mar 23, 2020

Try #1476:

6e534d9

paweljakubas force-pushed the paweljakubas/1436/byron-wallet-restoration-from-xprv-endpoint-integration-test branch 2 times, most recently from c3a07e7 to 325ff5d Compare March 24, 2020 09:18

KtorZ reviewed Mar 24, 2020

View reviewed changes

lib/core-integration/src/Test/Integration/Framework/DSL.hs Show resolved Hide resolved

KtorZ reviewed Mar 24, 2020

View reviewed changes

lib/byron/test/integration/Test/Integration/Byron/Scenario/API/Transactions.hs Show resolved Hide resolved

paweljakubas force-pushed the paweljakubas/1436/byron-wallet-restoration-from-xprv-endpoint-integration-test branch 2 times, most recently from e3a3d5e to 30ea64d Compare March 24, 2020 12:25

paweljakubas force-pushed the paweljakubas/1436/byron-wallet-restoration-from-xprv-endpoint-integration-test branch from 359c914 to f457078 Compare March 24, 2020 15:55

paweljakubas added 14 commits March 25, 2020 12:08

introduce passphrase hash

5267da6

add encryptPasswordWithScrypt

8045045

add password from pwd success property

11f410e

correct property tests

add more golden tests for legacy passwords

751d91d

add DSL helper in integration tests

3add0f9

finish integration test

61ce6c6

correct 64-byte restriction on passphrase hash

a54d997

add illustrative node integration test

6179b62

nix patch

c7a2ea6

relocate encryptPasswordWithScrypt to spec

0b9faf3

nix patch

cleaning integration tests

411d0c8

add second integration test

94d328f

compilation correction

add restoration test with wrong hash

2f0f34d

correct expectation plus nix patch

add wrong passphrase hash test

e721b26

paweljakubas force-pushed the paweljakubas/1436/byron-wallet-restoration-from-xprv-endpoint-integration-test branch from f457078 to e721b26 Compare March 25, 2020 11:49

correct tests - now all pass

846d5f1

paweljakubas requested a review from KtorZ March 25, 2020 15:57

iohk-bors bot added a commit that referenced this pull request Mar 25, 2020

Try #1476:

5f0fad0

KtorZ approved these changes Mar 25, 2020

View reviewed changes

KtorZ merged commit 79247ad into master Mar 25, 2020

KtorZ deleted the paweljakubas/1436/byron-wallet-restoration-from-xprv-endpoint-integration-test branch March 25, 2020 16:58

Byron wallet restoration from xprv endpoint integration test #1476

Byron wallet restoration from xprv endpoint integration test #1476

Uh oh!

Conversation

paweljakubas commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Number

Overview

Comments

Uh oh!

piotr-iohk commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paweljakubas commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KtorZ commented Mar 23, 2020

Uh oh!

paweljakubas commented Mar 23, 2020

Uh oh!

paweljakubas commented Mar 23, 2020

Uh oh!

iohk-bors bot commented Mar 23, 2020

try

Build failed

Uh oh!

paweljakubas commented Mar 23, 2020

Uh oh!

iohk-bors bot commented Mar 23, 2020

try

Build failed

Uh oh!

paweljakubas commented Mar 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KtorZ commented Mar 24, 2020

Uh oh!

Uh oh!

paweljakubas commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

piotr-iohk commented Mar 24, 2020

Uh oh!

KtorZ commented Mar 24, 2020

Uh oh!

paweljakubas commented Mar 24, 2020

Uh oh!

paweljakubas commented Mar 25, 2020

Uh oh!

iohk-bors bot commented Mar 25, 2020

try

Build succeeded

paweljakubas commented Mar 23, 2020 •

edited

Loading

piotr-iohk commented Mar 23, 2020 •

edited

Loading

paweljakubas commented Mar 23, 2020 •

edited

Loading

paweljakubas commented Mar 24, 2020 •

edited

Loading