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

Implement tranformation from mnemonic phrase to seed #12

Closed
sobuch opened this issue Oct 12, 2019 · 14 comments
Closed

Implement tranformation from mnemonic phrase to seed #12

sobuch opened this issue Oct 12, 2019 · 14 comments
Assignees
Labels

Comments

@sobuch
Copy link
Collaborator

sobuch commented Oct 12, 2019

No description provided.

@mvondracek
Copy link
Owner

We need to test program behaviour when mnemonic isn't valid UTF-8. #21 (comment)

@mvondracek
Copy link
Owner

mvondracek commented Oct 19, 2019

Please add validation of user's password.

salt should be about 16 or more bytes
https://docs.python.org/3.7/library/hashlib.html#hashlib.pbkdf2_hmac

def _generate_seed(mnemonic: str, seed_password: str = '') -> bytes:
"""Generate seed from provided mnemonic phrase.
Seed can be protected by password. If a seed should not be protected, the password is treated as `''`
(empty string) by default.
:rtype: bytes
:return: Seed
"""
# the encoding of both inputs should be UTF-8 NFKD
mnemonic = mnemonic.encode() # encoding string into bytes, UTF-8 by default
passphrase = "mnemonic" + seed_password
passphrase = passphrase.encode()
return pbkdf2_hmac('sha512', mnemonic, passphrase, PBKDF2_ROUNDS, SEED_LEN)

passphrase = "mnemonic" + seed_password

passphrase is used as salt.

seed_password should be at least 8 bytes long, otherwise raise instance of ValueError.

@lsolodkova
Copy link
Collaborator

lsolodkova commented Oct 19, 2019

salt should be about 16 or more bytes

Funny, but BIP-39 specification allows empty passwords.

If a passphrase is not present, an empty string "" is used instead.

Anyway, it's up to you, one more line of code isn't a problem.

@mvondracek
Copy link
Owner

I didn't check BIP-39, sorry. Thanks for info. Validation of passwrod is not needed as we should stick to the standard. :)

@mvondracek
Copy link
Owner

Related: #29

@mvondracek
Copy link
Owner

Concerning #29, what if we limit maximum password length to... let say 256 UTF-8 characters?

@lsolodkova
Copy link
Collaborator

lsolodkova commented Oct 19, 2019

It happens that for Latin (in particular, English) phrases all is fine; for Japanese (and other non-Latin), though, we have problems. Added some tests in 7310479.

what if we limit

If I don't see the code, I don't know how to break it. Btw, who must check the length of the password?

@mvondracek
Copy link
Owner

@lsolodkova , @sobuch

During today's lecture, some students asked if they can use PBKDF2 from standard library of Go and from standard library of Java, doc. Švenda told them they cannot -- they have to implement it and they can use only functions for hashes. So I told him that I explicitly asked about Python's PBKDF2 after previous lecture and that it was accepted. The result is there was some misunderstanding and we have to implement PBKDF2. We talked about it and it makes sense, because teams working in pure C would have great disadvantage.

Seed generation has to be implemented without hashlib.pbkdf2_hmac, just with hashing functions. It's _generate_seed or Mnemonic.toSeed depending on when #36 gets merged.

Related: #16

@lsolodkova
Copy link
Collaborator

depending on when #36 gets merged.

Should I work on that now or wait for #36 ?

@sobuch
Copy link
Collaborator Author

sobuch commented Oct 21, 2019

you could review #36 and then we can merge it :)

@mvondracek
Copy link
Owner

@lsolodkova #36 is already merged. Please have a look at the seed generation.

I also updated branch fix-_generate_seed_invalid-password-too-long from dev and fixed conflicts. Please keep in mind bug #29 when you implement PBKDF2 and set some reasonable limit for password.

@mvondracek
Copy link
Owner

#42 (comment)

@mvondracek
Copy link
Owner

I think this can be closed, what about you, @lsolodkova?

@lsolodkova
Copy link
Collaborator

lsolodkova commented Oct 26, 2019

Of course. Looking forward to any bugs and issues:)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants