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

Allow key to be a TypedArray #9

Closed
wants to merge 1 commit into from

Conversation

gabegorelick
Copy link
Contributor

Earlier work allowed passphrase to be a TypedArray. This extends that to allow for key to be too.

See #5

Earlier work allowed `passphrase` to be a TypedArray. This extends
that to allow for `key` to be too.

See chrisveness#5
@chrisveness
Copy link
Owner

If Scrypt.key() returns a Buffer, which it is up to the user to convert to a string, it doesn't seem right to assume that if a string is passed to Scrypt.verify() that it will be base64 – it could be hex, for example (though I cannot see why a user would prefer to store a key as hex in preference to base64!).

Perhaps it would make most sense for Scrypt.verify() to accept the key as a Buffer only, and the user would be required to convert the string to a buffer, as in

const key = await Scrypt.kdf('my secret password', { logN: 15 }).toString('base64');
const ok = await Scrypt.verify(Buffer.from(key, 'base64'), 'my secret password');

@demurgos
Copy link
Contributor

demurgos commented Jan 8, 2019

@chrisveness It makes sense, by accepting only buffers you reduce the assumptions made by the lib.

@gabegorelick
Copy link
Contributor Author

I agree with all the comments here. I wrote this PR to be backwards compatible. But assuming that's not something we care about (because it's being broken to return Buffers instead of strings), then I think only accepting Buffers (or TypedArrays) makes sense.

We could merge this and then remove String support though. But up to you how you want to manage the changes.

@chrisveness
Copy link
Owner

Thanks for the PR, but I decided it would be easier to base the new Buffer version off the current version.

@chrisveness chrisveness closed this Jan 9, 2019
@gabegorelick gabegorelick deleted the buffer-key branch January 9, 2019 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants