Use buffers instead of strings for the public API

[demurgos]

The current API uses strings: the passphrase is expected to be a string and the key is a base64 string.

The passphrase is passed as-is to the core lib. The core lib also accepts buffers so it should be safe to simply update the checks and documentation to use (string | Buffer). Requiring strings prevents from using binary keys as passphrases due to encoding-related restrictions (not all buffers are valid UTF-8).

The main issue is related to the return type. In the database, it is more efficient to store the hash as bytes array. The current function post-processes the output buffer to encode it to base64. This requires users to decode the result back to a buffer to handle encoding themselves.
I'd argue that it would be more ergonomic to provide a general API returning returning a Buffer and let the user call .toString("base64") if needed instead of forcing the encoding. This behavior would also be closer to the behavior of the standard library.

This would be a semver major breaking change.

[chrisveness]

I hadn't thought of using binary keys as passphrases, but I can see the value.

I'm not sure that saving 32 bytes is worth deviating from Percival's standard file header format.

I shall have to give this some thought.

[demurgos]

I am mostly concerned about supporting Buffer (of even Uint8Array) as a possible input.

I consider that a buffer better convey the semantics of the output (it's just an array of bytes) but it is something I can post-process myself when storing it in the database (BYTEA is the most fitting type in postgres DB IMO). Keeping a string would avoid the semver major change.

[chrisveness]

I've allowed Buffer or any TypeArray as input types for the passphrase, I can't see any downside to that at all, and can see potential value.

I realise Percival's standard file header format doesn't mention base64 (as it's purely a file format!), but I think hashed passwords are conventionally handled as strings, for convenience and portability. If you prefer to store in binary, there's not much overhead to do Buffer.from(key, 'base64'), and as you say returning binary would be a semver major change, in any case.

If you would like to review revisions & confirm, I'll release a new version. Thanks for all your contributions.

[demurgos]

Hi,
Thank you for implementing support for byte inputs. I was able to successfully integrate scrypt-kdf in my project following this change. I'd still like to see buffer outputs in a future major updates, but for now it works fine.

Could you publish the latest version of the package to npm?

[gabegorelick]

I'd still like to see buffer outputs in a future major updates

Any plans to do this? Should a new issue to track this be opened?

[chrisveness]

Perhaps I could add a third options argument to Scrypt.kdf(), which took an object including a format property, which could take values string, Uint8Array, Buffer.

Hence to obtain a Buffer, a call to kdf() might be

const key = await Scrypt.kdf('my secret password', { logN: 15 }, { format: 'Buffer' });

If format defaulted to string, it wouldn’t be a breaking change.

Scrypt.verify() would then have to accept key as string|Uint8Array|Buffer to match the formats generated by kdf(). Again, this wouldn’t be a breaking change.

[demurgos]

For consistency with fs.readFile, I'd prefer format to behave similarly: null would return a Buffer (Buffer already extends Uint8Array) and other string values would simply be passed to Buffer.toString().

const format = options.format !== undefined ? options:format : "base64";
const buffer = Buffer.from(linear);
return format === null ? buffer : buffer.toString(format);

[gabegorelick]

I agree with @demurgos, Buffer should be the default format. But at that point, there isn't that much benefit to having a format option at all. I would prefer the API remain lean and just have callers call buf.toString if they want.

[chrisveness]

Ok, that makes sense, I'll do a semver major with the key returned as a Buffer.

[chrisveness]

New version using Buffers for the key is now available for review before release as 2.0.0.

[demurgos]

Thank you very much for your reactivity @chrisveness.

I was able to test the version 2.0.0 in my project with minimal changes (and could simplify my code). I sent the PR #11 with minor changes following my test.