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

[conor10]

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]

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]

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]

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]

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]

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.