Skip to content
This repository has been archived by the owner on Nov 21, 2019. It is now read-only.

Update params in keystore files to be more consistent with geth and parity and/or change to use pbkdf2 #269

Open

Comments

@conor10
Copy link

conor10 commented Nov 3, 2016

In the wallet files you generate, the kdf field value is defined as "scrypt", however, based on the parameters provided, it should be defined as "pbkdf2" according to the Secret Storage Definition.

Additionally, it should not contain the "r" parameter in this definition, and the value of "prf" should be "hmac-sha256".

See the Geth implementation for reference.

The following test wallet file I created:

{
  "address": "8c1d4b092ca42428624a9acedfc25f6383f66385",
  "crypto": {
    "cipher": "aes-128-ctr",
    "ciphertext": "d79eb16f6052a14ae9ea648df151e100c1555cbfca729d2b06b6a97a399e7f7a",
    "cipherparams": {
      "iv": "d15541eff4cedbd2ac441c597820d3fb"
    },
    "kdf": "scrypt",
    "kdfparams": {
      "dklen": 32,
      "c": 262144,
      "prf": 1,
      "r": 8,
      "salt": "8b1b12076deb492a7275ae9c3d1c765109a10ec723424ba9764436b4e4a2790e"
    },
    "mac": "26648dfd1911424fa346ef2d40b3aab4b7e6dc6f8fb50fe5603d1f17ebab639f"
  },
  "id": "23c74c0f-a97b-43d9-9f5d-0b7d0cab3967",
  "version": 3
}

Should be:

{
  "address": "8c1d4b092ca42428624a9acedfc25f6383f66385",
  "crypto": {
    "cipher": "aes-128-ctr",
    "ciphertext": "d79eb16f6052a14ae9ea648df151e100c1555cbfca729d2b06b6a97a399e7f7a",
    "cipherparams": {
      "iv": "d15541eff4cedbd2ac441c597820d3fb"
    },
    "kdf": "pbkdf2",
    "kdfparams": {
      "dklen": 32,
      "c": 262144,
      "prf": "hmac-sha256",
      "salt": "8b1b12076deb492a7275ae9c3d1c765109a10ec723424ba9764436b4e4a2790e"
    },
    "mac": "26648dfd1911424fa346ef2d40b3aab4b7e6dc6f8fb50fe5603d1f17ebab639f"
  },
  "id": "23c74c0f-a97b-43d9-9f5d-0b7d0cab3967",
  "version": 3
}

@tayvano
Copy link
Contributor

tayvano commented Nov 3, 2016

So I just generated a brand new account with geth (Version: 1.4.18-stable-c72f5459) and got:

{
    "address":"45089867aa86167ccceec7979dc82ab80df0681b",
    "crypto":{
        "cipher":"aes-128-ctr",
        "ciphertext":"76e1f1d6875a85039146b51b81154ea2613c9be1cf0b9c94a9a5334347cab6",
        "cipherparams":{
            "iv":"670f933c844d3394e72a338b2f4ed6d5"
        },
        "kdf":"scrypt",
        "kdfparams":{
            "dklen":32,
            "n":262144,
            "p":1,
            "r":8,
            "salt":"412876678bc325d432a935240bda2ec06a3acf9a4e85d06c037b42d1c119ae63"
        },
        "mac":"6e28271571356bc181f58bdb3fe37104bcd031b3e8fb6cd85c3883cb300d178a"
    },
    "id":"4ae3aa3d-2530-4d71-b1f9-059b07eb2bed",
    "version":3
}

So either geth very recently changed to use pbkdf2 over scrypt (is there even a newer version out?), or there is something else causing the difference. Perhaps OS? I have no idea. Regardless, all the differences are a result of using scrypt rather than pbkdf2.

Parity v1.4.0 does use pbkdf2:

{
    "id":"9519478a-617e-9fcb-ed2f-436166724277",
    "version":3,
    "crypto":{
        "cipher":"aes-128-ctr",
        "cipherparams":{
            "iv":"e669ad811d6151e3fc4718f006912808"
        },
        "ciphertext":"1dbc674a8b90a475f563263e3cbdb6c1e8781b21ab0253b9ffc5897595abf47a",
        "kdf":"pbkdf2",
        "kdfparams":{
            "c":10240,
            "dklen":32,
            "prf":"hmac-sha256",
            "salt":"b5be927025bf793f9b7e5f2b01d15b76aa3c41ebc94f7ec3906d2c61ed535210"
        },
        "mac":"3c63efaf67caa16e38285995593c0ffad16aa71d62450053f2c27dcd34d2f1a8"
    },
    "address":"8e9d5e9ea741752c38b3bc6048cccc7214114a7a",
    "name":"9519478a-617e-9fcb-ed2f-436166724277",
    "meta":"{}"
}

https://github.com/ethereumjs/ethereumjs-wallet also defaults to scrypt.

Additionally, https://github.com/ethereumjs/ethereumjs-wallet#remarks-about-tov3 has some good remarks at the end highlighting different options.

I don't know. I'm sure it would be a minimal change. However, we are very hesitant to change anything with the derivation portion of our site. As you certainly know, derivation errors can be absolutely fatal in a wallet generation and the extensive testing and time can simply be replaced. We have to weigh this switch to the 8+ months w/o any reported derivation errors and endless tests. IMO, it's not worth it.

These are still valid v3 files that can be imported via geth and parity. Are there any strong arguments for pbkdf2?


The thing about prf has been noted. Not sure where that disappeared to but we'll look into it.

@conor10
Copy link
Author

conor10 commented Nov 3, 2016

I've been adding support for wallets in web3j, and noticed the above. It's not a huge issue for me, but thought you'd be interested to know.

@tayvano
Copy link
Contributor

tayvano commented Nov 3, 2016

Absolutely. Thank you for pointing it out. We'll keep this in mind as we move forward and probably switch it if we change anything else and then test it all extensively. 😄

@conor10
Copy link
Author

conor10 commented Nov 3, 2016

No problem. One other thing I've noticed - it looks like the "Crypto" parameter name should be "crypto" to be consistent with Geth/Parity.

@tayvano tayvano changed the title Incorrect kdfParams in wallet file Update params in keystore files to be more consistent with geth and parity and/or change to use pbkdf2 Nov 3, 2016
@tayvano
Copy link
Contributor

tayvano commented Nov 4, 2016

I just realized that our issue #259 may be pertinent to what you are working on. Essentially some clients can occasionally generate valid private keys that are 30 or 31 bytes - namely geth. I believe geth had updated now but you will likely still need to import these rare keys. Padding left to the required 32 is all that needs to be done. There are links in our thread to the other pertinent threads discussing the issue and solution.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.