The filename of keys makes a difference as to whether or not they can be unlocked

[stephantual]

I found today by playing around with geth that the filename format of the keys makes a difference as to whether or not they can be unlocked.

Expected behaviour: I can place any file with a valid encrypted in key in /keystore and it will be possible to unlock the account with the correct password.

Actual behaviour (OSX, latest master, build from source): if i rename a key file to 'blah_blah' then obviously it's not recognised by ./geth account list and therefore using it is out of the question, which is expected.

However, if i rename the key file to say, '367670f9a85e63377c9cc43a2ff6fe78b33b01c4', then the account will appear in the account list, but even with the correct password, i cannot unlock it.

Renaming it to the proper format, irrespective of timestamp used, fixes it. (modifying the timestamp does however move it up and down in the array of accounts, which I believe is expected).

Why this is a problem: I think a lot of people are securing their keys by:

• printing out the content of the keyfile
• deleting the file ... in order to put their funds in 'cold storage;

So these people are gong to be rather unhappy when they try to recover their keys later down the line and can't get geth to unlock the account even though the keys are valid and so are the passwords.

Temporary workaround: create a brand new account, then copy the format of the file, change the date slightly, and then suffic with the proper public key. It allows you to unlock the file again (thankfully).

[zelig]

Indeed, you found an inconsistency. The keystore is meant to recognise the old (prefrontier) format where keys were in their own subdirectories: keystore/367670f9a85e63377c9cc43a2ff6fe78b33b01c4/367670f9a85e63377c9cc43a2ff6fe78b33b01c4

However when we list the accounts we carelessly included the pattern keystore/367670f9a85e63377c9cc43a2ff6fe78b33b01c4

So what would you like to see?

Expected behaviour: I can place any file with a valid encrypted in key in /keystore and it will be possible to unlock the account with the correct password.

this is certainly possible but this is not a hotfix since this would require us to parse all files even for listing the accounts. I guess this is probably the correct/safer behaviour anyway.

All in all, we defo need to at least fix the above pattern. I would lean towards not supporting other filepath formats but the previous and current ones.

Down the line this whole key management nonsense needs to go and be replaced by some more stardard and/or device/OS-integrated solution.

[stephantual]

Hi Viktor, thank you for your reply :)

So there are two distinct things here:

One, the naming convention which could be changed to parse all files or parse only acceptable ones (ie, the old and new ones). That's not the end of the world either way.

Second - and I'd love to be proven wrong on this - that a key that's not named as expected but still going in the list of accounts cannot be unlocked with the correct password.

I tried this:

• Name the key so that's it's recognized but non-standards
• Try to unlock the account by sending a trx, fails
• Rename the key to the UTC format
• Try to unlock the account by sending a trx , succeeds.

For the sake of testing I had just generated the key and the password was 'password' (so I wouldn't type it wrong).

[zelig]

sorry stephan if i was not clear, yes your case is indeed a bug. Only correctly named keyiles (old and new) should be accepted but listing mistakenly matches the pattern (keyfiles/<key>), and yes when you unlock it the pattern match is "incorrect" and does not find your file so it won't unlock and on top does not even give a correct error message. The correct fix would be to disallow your naming convention. Since you found this first, probably noone else is affected, so I suggest we go down this simple restrictive route for now. (which means, please conform to the new scheme :)

Admittedly I am recommending the solution that is easiest for us. This is because if we put more into this now, IMHO that should be directed to finding a proper way to handle keys. I will talk to the team about this.

[klmoney]

Hi, for the specific situation that Stephan mentions, this bug would be less of a problem if Geth allowed the export of unencrypted private keys. Then a user would print and store the private key string and when time to rebuild the geth keyfile with the correct filename format, would simply use “geth account import”. Having said that, I do appreciate the risks of holding an unencrypted private key.

[stephantual]

Thanks a lot @zelig and I'm thrilled I found an original bug :smile: I like your solution! :+1:

[infamousrev]

I agree with stephantual that the behaviour that it could simply detect any filename would be very useful.

I think anyone making a backup of their key will probably want to store it in a hard to recognize way, for example encrypt it another round to make the contents look like it is random data and rename it to familyphoto.jpg or something like that.

This is important because if the file name has to contain your wallet address then someone who manages to get his hands on your backup can use a wallet explorer to see the balance and might use that as a reason to threaten you for the password.

Having such a complex and mandatory naming scheme will make sure that tons of new users who try to backup their key in this way will have trouble restoring it.

[Gustav-Simonsson]

The files in the keystore are not meant to be fiddled with by users. They are internal and have no guarantees other than what the key spec at https://github.com/ethereum/wiki/wiki/Web3-Secret-Storage-Definition defines.

Fun fact: the spec recommends for file name what older versions of the keystore used - a random UUID (unrelated to the actual key) assigned to each key.

I would recommend that the keys in the keystore remain regarded as internal and for users to not directly handle them. Instead we should have geth commands for key export/import where exported files have a privacy-preserving file name and also excludes the address field from the JSON file content.

[karalabe]

Open for an eternity, but this issue was finally addressed on develop.