PBKDF2 and BIP39 Support

[jimtendo]

This is a PBKDF2 pure JS implementation to progress towards supporting BIP39 mnemonics (I'll branch mnemonics off of this - want to try to keep reviews small and make sure I'm on the right track code-style-wise).

It's based on this implementation https://github.com/browserify/pbkdf2/blob/master/lib/sync-browser.js with a change to support derivedKeyLength that is less than the hmacLength (both NodeJS's PBKDF2 implementation and Python's support this - the test vectors I've found also have this).

The interface is a it clunky - I've tried to keep to the "under three params" lint rule and, unfortunately, we need to know the HMAC length in advance (we could determine this at run-time by "trying it", but I'm not sure that feels proper or efficient?j). Currently, the Pbkdf2PHmac interface breaks the functional/no-mixed-type linter rule too - but I'm not too sure what's a good approach for this (maybe I remove this interface and just pass those params direct into function? Let me know if you have any suggestions).

The implementation itself breaks many linter rules. At the moment, I've set a lot of these to be ignored as there's some precedent for this in the CashAddress implementation and rewriting around them may be less efficient considering we're dealing with an algo that iterates into the thousands in practice. Let me know what's preferred here and I can try to rewrite.

Overall, if you could take a look and just let me know which parts you'd like refactored and whether the general direction (e.g. the tests) look appropriate, that'd be great. I'm still digesting and learning the styles from elsewhere in Libauth.

[bitjson]

Hey Jim, really fantastic work, thank you! 🚀

On first review the code and interface look great, and code/comment style is excellent!

Did you consider turning pbkdf2HmacSha256 and pbkdf2HmacSha512 into exported functions themselves? So the pbkdf2 function might become something more like instantiateHmacFunction, maybe instantiatePbkdf2Function?

Since your ultimate goal with this is to add BIP39 support, if you're willing, I'd love to see that in this PR too! That would let us confirm that the interface is exactly what we need + improve test coverage.

[jimtendo]

@bitjson No idea how to get rid of this one.

Algo is almost verbatim https://github.com/browserify/pbkdf2/blob/master/lib/sync-browser.js with a few minor changes (e.g. Buffer to Uint8Array).

I'm wondering if Typescript is doing something funky because we're doing a bitwise op. If you have any ideas, please let me know.

[bitjson]

Ok, looks like the error is just that U[k] is possibly 'undefined'. as k isn't guaranteed to be in the range of Uint8Array U. We're sure in this case, so you can make that line:

T[k] ^= U[k]!; // eslint-disable-line @typescript-eslint/no-non-null-assertion

(Breaking rules is fine in sensible places, the linting just forces us to be explicit and deliberate about it. 👍)

[jimtendo]

@bitjson If we include these checks, we break a linter "complexity" rule (max is 5, we end up on 10).

I'm not sure if these are maybe overkill? Or maybe should be split out into a separate verifyPbkdf2Parameters function?

[jimtendo]

NOTE: If we keep these in, will try to add fail test cases for completeness.

[bitjson]

These checks are great! They make sense here, and I don't really see a reason to break them out into a separate function (it would be quite the stretch to use just this validation in some other context). So this is a good place to break our complexity rule (right above the relevant function):

// eslint-disable-next-line complexity

[jimtendo]

Have placed this one in.

[jimtendo]

@bitjson probably ready for another look now. Note the long list of ignored linter rules and which ones we should try to fix up.

Original algo is taken from https://github.com/browserify/pbkdf2/blob/master/lib/sync-browser.js

[bitjson]

Looking great so far! For the BIP39 implementation, maybe a reasonable strategy is including a bip39.ts under src/lib/key, and importing the const string bip39WordListEnglish from bip39.english.ts? (Similar to what we do with ripemd160.base64.ts currently.)

[bitjson]

Just pushed my initial recommendation, but feel free to experiment with other options 👍

[jimtendo]

I think this is finally ready for review (sorry about how long this took!)

A few notes:

1. It was hard to find suitable vectors - those referenced in BIP39 all used the passphrase "TREZOR" and did not provide the seed value without the passphrase. In the end, I decided to just generate using the following script so that I could have both a) without passphrase and b) with passphrase. If preferred, I can do a conversion of the Trezor vectors to derive seed value without passphrase, but I thought we might want to tinker with the number of vectors to reduce overall test time (I've done four per wordlist in the below).

import bip39 from 'bip39';

const wordlists = ['english', 'chinese_simplified', 'japanese', 'spanish', 'italian', 'french', 'korean', 'czech', 'portuguese', 'chinese_traditional'];

let validData = [];

// Create tests for each wordlist.
for(let wordList of wordlists) {
	// Set the wordlist to use.
	bip39.setDefaultWordlist(wordList)

	// Generate four mnemonics using the given wordlist.
	for (let i = 0; i < 4; i++) {
		const passphrase = 'libauth';
		const mnemonic = bip39.generateMnemonic();
		const seed = bip39.mnemonicToSeedSync(mnemonic).toString('hex');
		const seedUsingPassphrase = bip39.mnemonicToSeedSync(mnemonic, passphrase).toString('hex');
		const entropy = bip39.mnemonicToEntropy(mnemonic);

		const test = {
			entropy,
			mnemonic,
			seed,
			wordList,
			passphrase,
			seedUsingPassphrase,
		}

		validData.push(test)
	}
}

console.log(JSON.stringify(validData, null, 2));

2. I've cheated in some places with eslint ignore where I couldn't find an otherwise elegant solution (or name).

3. Japanese is exceptional - it uses a \u3000 separator as opposed to a space character. Detection of Japanese wordlist is dirty - but it seems most other implementations use a similar approach.

4. There is almost 100% test coverage. The one exception is with an entropy check function which I think is redundant - but is in the bip39 npm package I based this on. I've left it there just in case there's some undocumented edge-case that I'm unaware of but please let me know if you think I can safely remove it.

[bitjson]

Wow, fantastic work @jimtendo! Thanks for the attention to detail, this looks great. I'm going to dedicate some more time to reviewing it, and I'll pull it in with the next release. 🚀

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (7c7e74d) 0.00% compared to head (4aefb8a) 84.26%.
Report is 2 commits behind head on master.

Files	Patch %	Lines
src/lib/key/bip39.ts	98.86%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #109       +/-   ##
===========================================
+ Coverage        0   84.26%   +84.26%     
===========================================
  Files           0      144      +144     
  Lines           0    51448    +51448     
  Branches        0     2136     +2136     
===========================================
+ Hits            0    43355    +43355     
- Misses          0     8076     +8076     
- Partials        0       17       +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[bitjson]

Thanks again for all your work here @jimtendo!

Merged manually in 8e032c2. I added a few more functions, expanded the docs, and added a few more tests (including fuzzing/property-based testing) in 4709c17.

Now released in @bitauth/[email protected] 🚀