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

crypto: add scrypt() and scryptSync() methods #20816

Merged
merged 8 commits into from Jun 13, 2018

Conversation

@bnoordhuis
Member

bnoordhuis commented May 18, 2018

Scrypt is a password-based key derivation function that is designed to
be expensive both computationally and memory-wise in order to make
brute-force attacks unrewarding.

OpenSSL has had support for the scrypt algorithm since v1.1.0. Add a
Node.js API modeled after crypto.pbkdf2() and crypto.pbkdf2Sync().

Changes:

  • Introduce helpers for copying buffers, collecting openssl errors, etc.

  • Add new infrastructure for offloading crypto to a worker thread.

  • Add a AsyncWrap JS class to simplify pbkdf2(), randomBytes() and
    scrypt().

CI: https://ci.nodejs.org/job/node-test-pull-request/14953/
CI: https://ci.nodejs.org/job/node-test-pull-request/14956/

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented May 18, 2018

Interesting how GH messes up the order of the commits...

salt = checkIsArrayBufferView('salt', salt);
// FIXME(bnoordhuis) The error message is in fact wrong since |iterations|
// cannot be > INT_MAX. Adjust in the next major release.
iterations = checkIsUint('iterations', iterations, 'a non-negative number');

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

I’m not sure we’d actually have to consider this semver-major anymore.

@@ -1465,7 +1465,7 @@ rapidly.
In line with OpenSSL's recommendation to use PBKDF2 instead of
[`EVP_BytesToKey`][] it is recommended that developers derive a key and IV on
their own using [`crypto.pbkdf2()`][] and to use [`crypto.createDecipheriv()`][]
their own using [`crypto.scrypt()`][] and to use [`crypto.createDecipheriv()`][]

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

2 lines above this change and the previous one there there’s still recommendation to use PBKDF2, I assume we’d want to change that too?

This comment has been minimized.

@bnoordhuis

bnoordhuis May 18, 2018

Member

Oh, probably a rebase that went wrong. Good catch, I'll fix that.

- `N` {number} CPU/memory cost parameter. Must be a power of two greater
than one. **Default:** `16384`
- `r` {number} Block size parameter. **Default:** `8`
- `p` {number} Parallelization parameter. **Default:** `1`

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

Could we spell these names out in the options object? E.g. cost, blockSize, parallelization?

This comment has been minimized.

@bnoordhuis

bnoordhuis May 18, 2018

Member

For better or worse, they're the "official" names of the algorithm's parameters. I'd say straying from the established nomenclature is not a good thing.

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

@bnoordhuis I don’t think the typical consumer of these APIs would be familiar with the ‘official’ nomenclature, though – it would be good to mention the names N, r, p in the docs, but I do think that our API should have names that make sense to most developers rather than crypto people?

This comment has been minimized.

@bnoordhuis

bnoordhuis May 18, 2018

Member

I did consider more expressive names but if you survey scrypt bindings in other languages (java, python, php, etc.), they all use N, r and p. We'd be the outlier.

To be fair, the perl binding doesn't call them that but it uses CPU time as the bound, not algorithmic complexity.

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

We'd be the outlier.

I don’t think that’s necessarily a bad thing. If we mention the formal “names” in the documentation, it’s still easy enough to look up what corresponds to what – reading/writing JS code directly is probably more common that comparing to code in other languages or porting code over.

This comment has been minimized.

@tniessen

tniessen May 18, 2018

Member

To be honest, I am doing the same thing in my PQC implementations in C, but I feel like node's crypto API could be more accessible if we use more intuitive names. On the other hand, I can understand @bnoordhuis opinion, I have barely ever seen other names for these parameters.

errors_array->Set(env->context(), i, errors[i]).FromJust();
for (auto const& string : *this) {
auto index = &string - &front();
auto value =

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

Tbh, I’m not a huge fan of using auto when it comes to types that are not too complex to fully write out – that too often makes adds an extra round-trip of “what type was $x again?” for my taste…

@@ -4556,6 +4575,43 @@ bool ECDH::IsKeyPairValid() {
}
struct CryptoJob : public ThreadPoolWork {
Environment* const env;

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

There’s already env_ on the ThreadPoolWork class, could we just expose that instead?

This comment has been minimized.

@bnoordhuis

bnoordhuis May 18, 2018

Member

Not as a method, I tried. Diamond problem: it conflicts with env() from AsyncWrap / BaseObject.

inline void AfterThreadPoolWork(int status) final;
virtual void AfterThreadPoolWork() = 0;
static inline void Run(std::unique_ptr<CryptoJob> job, Local<Value> wrap);
};

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

Is there any specific advantage to having the separate AsyncWrap JS class as opposed to what we currently do, making the individual request objects (in this case CryptoJob) inherit from AsyncWrap?

This comment has been minimized.

@bnoordhuis

bnoordhuis May 18, 2018

Member

Separation of concerns, mainly. I initially mashed them together like you suggest but I didn't like how it looked and worked.

(There's a lot I don't like about how AsyncWrap currently works but I'll save that for another issue.)

std::unique_ptr<RandomBytesJob> job(new RandomBytesJob(env));
job->data = reinterpret_cast<unsigned char*>(Buffer::Data(args[0])) + offset;
job->size = size;
if (args[3]->IsObject()) return RandomBytesJob::Run(std::move(job), args[3]);

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

Also not a huge fan of same-line returns – if nothing else, having separate lines makes line coverage data a bit more accurate :)

This comment has been minimized.

@bnoordhuis

bnoordhuis May 18, 2018

Member

The reason I like it is that it makes code grepping a bit easier.

Apropos code coverage, I thought gcov works in terms of 'spans' (can't think of the right word) rather than 'lines'?

This comment has been minimized.

@addaleax

addaleax May 18, 2018

Member

At least the way we have coverage set up it does track line coverage + branch coverage for conditionals. I don’t feel too strongly about this and the point about grepping is valid. :)

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented May 18, 2018

I'm reasonably sure the test failures are all flakes that have happened before (http2, tls verify) but here is another run just in case: https://ci.nodejs.org/job/node-test-pull-request/14956/

const {
scrypt,
scryptSync
} = require('internal/crypto/scrypt');

This comment has been minimized.

@BridgeAR

BridgeAR May 18, 2018

Member

It would be great to actually lazy load this as it is definitely not always required when loading crypto.

This comment has been minimized.

@bnoordhuis

bnoordhuis May 18, 2018

Member

You could say the same thing for pbkdf2 and others. Comment noted but I'd like to save that for another PR.

throw new ERR_OUT_OF_RANGE(name, errmsg, value);
return value;
}

This comment has been minimized.

@BridgeAR

BridgeAR May 18, 2018

Member

There is a validators file that already contains a check like this.

This comment has been minimized.

@bnoordhuis

bnoordhuis May 30, 2018

Member

validateInt32 doesn't do quite the same thing but take a look at the second-to-last commit. This is with #20816 (comment) in mind.

- `r` {number} Block size parameter. **Default:** `8`
- `p` {number} Parallelization parameter. **Default:** `1`
- `maxmem` {number} Memory upper bound. It is an error when (approximately)
`128*N*p*r > maxmem` **Default:** `32 * 1024 * 1024`

This comment has been minimized.

@bnoordhuis

bnoordhuis May 18, 2018

Member

Note to self: typo, should be 128*N*r. Happens in another place in this file and in the test.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt May 19, 2018

Member

Nit: missing period after `128*N*p*r > maxmem`.

- `N` {number} CPU/memory cost parameter. Must be a power of two greater
than one. **Default:** `16384`
- `r` {number} Block size parameter. **Default:** `8`
- `p` {number} Parallelization parameter. **Default:** `1`

This comment has been minimized.

@tniessen

tniessen May 18, 2018

Member

To be honest, I am doing the same thing in my PQC implementations in C, but I feel like node's crypto API could be more accessible if we use more intuitive names. On the other hand, I can understand @bnoordhuis opinion, I have barely ever seen other names for these parameters.

<a id="ERR_CRYPTO_SCRYPT_NOT_SUPPORTED"></a>
### ERR_CRYPTO_SCRYPT_NOT_SUPPORTED
Node.js was compiled without `scrypt` support. Not possible with the official

This comment has been minimized.

@tniessen

tniessen May 18, 2018

Member

Nit: Double space.

- `keylen` {number}
- `options` {Object}
- `N` {number} CPU/memory cost parameter. Must be a power of two greater
than one. **Default:** `16384`

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt May 19, 2018

Member

Nit (here and in other lines in added stuff): it seems we usually use periods after **Default:** phrase if it follows a full sentence.

This comment has been minimized.

@bnoordhuis

bnoordhuis May 22, 2018

Member

I knew you were going to bring that up but I copied the style from the surrounding documentation. Local consistency > global consistency.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt May 22, 2018

Member

But all previous cases in this doc differ: **Default:** phrases do not follow full sentences.

- `r` {number} Block size parameter. **Default:** `8`
- `p` {number} Parallelization parameter. **Default:** `1`
- `maxmem` {number} Memory upper bound. It is an error when (approximately)
`128*N*p*r > maxmem` **Default:** `32 * 1024 * 1024`

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt May 19, 2018

Member

Nit: missing period after `128*N*p*r > maxmem`.

An exception is thrown when any of the input arguments specify invalid values
or types.
Example:

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt May 19, 2018

Member

Nit: IIRC, we eliminate such explicit intro for examples lately.

- `r` {number} Block size parameter. **Default:** `8`
- `p` {number} Parallelization parameter. **Default:** `1`
- `maxmem` {number} Memory upper bound. It is an error when (approximately)
`128*N*p*r > maxmem` **Default:** `32 * 1024 * 1024`

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt May 19, 2018

Member

Nit: missing period after `128*N*p*r > maxmem`.

An exception is thrown when any of the input arguments specify invalid values
or types.
Example:

This comment has been minimized.

@vsemozhetbyt
release binaries but can happen with custom builds, including distro builds.
<a id="ERR_CRYPTO_SCRYPT_INVALID_PARAMETER"></a>
### ERR_CRYPTO_SCRYPT_INVALID_PARAMETER

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt May 19, 2018

Member

Nit: should go before ERR_CRYPTO_SCRYPT_NOT_SUPPORTED ABC-wise?

@@ -1363,7 +1363,7 @@ rapidly.
In line with OpenSSL's recommendation to use PBKDF2 instead of
[`EVP_BytesToKey`][] it is recommended that developers derive a key and IV on
their own using [`crypto.pbkdf2()`][] and to use [`crypto.createCipheriv()`][]

This comment has been minimized.

@benjamingr

benjamingr May 20, 2018

Member

Why are we preferring scrypt over pbkdf2? This is the first I've heard of the recommendation

This comment has been minimized.

@tniessen

tniessen May 21, 2018

Member

Personally, I'd prefer "scrypt or pbkdf2". Scrypt kind of includes PBKDF2 by design, and is supposed to also increase the memory requirements to make brute-forcing harder, but that might not be desirable in all cases.

This comment has been minimized.

@bnoordhuis

bnoordhuis May 22, 2018

Member

It needs to a simple recommendation - just do this, not do this OR do that - because otherwise you need to explain the trade-offs to readers that aren't cryptographers and probably only glance at the documentation anyway.

When it's between scrypt and pbkdf2, I hope we can agree scrypt > pbkdf2.

This comment has been minimized.

@tniessen

tniessen May 22, 2018

Member

When it's between scrypt and pbkdf2, I hope we can agree scrypt > pbkdf2.

I think we can agree that scrypt > pbkdf2 for people who don't have highly specific requirements.

This comment has been minimized.

@benjamingr

benjamingr May 22, 2018

Member

I hope we can agree scrypt > pbkdf2.

I'm literally asking why because I don't know, so why?

This comment has been minimized.

@tniessen

tniessen May 22, 2018

Member

@benjamingr As scrypt involves a specific (non-customizable) instance of PBKDF2 which is believed to be secure, we can assume that it is at least as computationally secure as PBKDF2 itself, with the "benefit" of preventing any security-violating modifications by the user (e.g. using SHA1 and the like). One "problem" with PBKDF2 is that it can in theory be cracked using enough parallelization (e.g. using GPUs which tend to have many parallel cores), whereas scrypt requires a non-trivial amount of memory to derive a key which heavily limits parallel computation on such devices.

@targos

This comment has been minimized.

Member

targos commented May 22, 2018

I reopened #8417. It will be properly closed by this PR :)

@targos targos referenced this pull request May 22, 2018

Closed

crypto.scrypt() #8417

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented May 28, 2018

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented May 30, 2018

Rebased and incorporated feedback. New CI: https://ci.nodejs.org/job/node-test-pull-request/15166/

I kept the short variable names, though.

@tniessen

LGTM either way.

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Jun 11, 2018

bnoordhuis added some commits May 18, 2018

crypto: DRY type checking
Factor out some common code.  The `checkUint()` function will also be
used in a follow-up commit that adds scrypt support to core.

PR-URL: #20816
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
crypto: add scrypt() and scryptSync() methods
Scrypt is a password-based key derivation function that is designed to
be expensive both computationally and memory-wise in order to make
brute-force attacks unrewarding.

OpenSSL has had support for the scrypt algorithm since v1.1.0.  Add a
Node.js API modeled after `crypto.pbkdf2()` and `crypto.pbkdf2Sync()`.

Changes:

* Introduce helpers for copying buffers, collecting openssl errors, etc.

* Add new infrastructure for offloading crypto to a worker thread.

* Add a `AsyncWrap` JS class to simplify pbkdf2(), randomBytes() and
  scrypt().

Fixes: #8417
PR-URL: #20816
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
crypto: refactor pbkdf2() and pbkdf2Sync() methods
Use the scrypt() infrastructure to reimplement pbkdf2() in a simpler
manner.

PR-URL: #20816
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
crypto: refactor randomBytes()
Use the scrypt() infrastructure to reimplement randomBytes() and
randomFill() in a simpler manner.

PR-URL: #20816
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
crypto: drop Math.pow(), use static exponentation
PR-URL: #20816
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
build: expose openssl scrypt functions to addons
Add scrypt functions to the list of exported openssl symbols.

PR-URL: #20816
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
lib: replace checkUint() with validateInt32()
PR-URL: #20816
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
lib: rename checkIsArrayBufferView()
Rename it to validateArrayBufferView() to align with validateInt32()
and friends.

Swap the name and the value in the argument list for consistency,
although any reasonable person will agree it's a crime against
humanity to put the value before the name.

PR-URL: #20816
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>

@bnoordhuis bnoordhuis closed this Jun 13, 2018

@bnoordhuis bnoordhuis deleted the bnoordhuis:scrypt branch Jun 13, 2018

@bnoordhuis bnoordhuis merged commit 59ace57 into nodejs:master Jun 13, 2018

@fanatid fanatid referenced this pull request Jun 21, 2018

Open

Comparison with scryptsy #1

addaleax added a commit to addaleax/node that referenced this pull request Jun 25, 2018

crypto: add better scrypt option aliases
Make parameter names available in a human-readable way, for
more accessible/self-documenting usage of the `scrypt` functions.

This implements a review comment from the original PR that has
not been addressed.

Refs: nodejs#20816 (comment)

@addaleax addaleax referenced this pull request Jun 25, 2018

Closed

crypto: add better scrypt option aliases #21525

4 of 4 tasks complete

addaleax added a commit to addaleax/node that referenced this pull request Jun 29, 2018

crypto: add better scrypt option aliases
Make parameter names available in a human-readable way, for
more accessible/self-documenting usage of the `scrypt` functions.

This implements a review comment from the original PR that has
not been addressed.

Refs: nodejs#20816 (comment)

addaleax added a commit to addaleax/node that referenced this pull request Jul 9, 2018

crypto: add better scrypt option aliases
Make parameter names available in a human-readable way, for
more accessible/self-documenting usage of the `scrypt` functions.

This implements a review comment from the original PR that has
not been addressed.

Refs: nodejs#20816 (comment)
@joepie91

This comment has been minimized.

Contributor

joepie91 commented Jul 11, 2018

I'm... not really sure how this got merged like this. This is an incredibly dangerous API design that is going to cause a ton of security issues.

The specific issues here:

  1. The API design expects the user to supply the salt. Any modern password hashing implementation (which, realistically, is what this function will be primarily used for) should handle this automatically behind the scenes, because there is no valid reason for a developer to specify a custom salt; it should just always be initialized to random data, and included in a composite output format.

    As it stands, this API will lead to developers hard-coding certain values or using poor random sources because they do not understand the requirements for a salt. That is going to effectively break the security of scrypt.

  2. There is no safe verification API; the developer is seemingly expected to manually juggle the various components of the hash generation process (settings, salt, etc.) and then reproduce the same hash and use timingSafeEqual to verify the hashes.

    Realistically, no developer is going to do that. They're all going to use === and introduce timing attacks, because they are not aware of the existence of timing attacks (and why should they, given that they're not cryptographers?).

A safe API would look something like what I've done in scrypt-for-humans: a single hash method, a single verifyHash method, no manual salt specification, and a composite output format. If you use it in the obvious way, it is safe to use.

If the intention is to provide scrypt as a more customizable primitive for other cryptographic applications, then it's fine to have an additional low-level API; but the default API really should be safe-by-default and optimized for the most common case (password hashing).

I would strongly recommend to remove this API again until a safer API has been hashed out, now that its addition is still fairly recent and there's very little chance that it has made it into production systems. Leaving the current API in place will produce a mess of security issues that will take years to clean up even if it does eventually get replaced by a different API.


Aside from the above specific issues, there doesn't seem to have been any review of the API whatsoever, despite this being security-critical code where a safe API is absolutely crucial, to prevent reoccurrence of incidents like the Buffer incident (and the subsequent fallout that I and others in the ecosystem are still cleaning up on a regular basis).

Is there not an API review process for changes to the crypto module and other security-sensitive code? If not, why not?

@paragonie-scott

This comment has been minimized.

paragonie-scott commented Jul 11, 2018

This is how PHP does password hashing:

// All in one go:
$hash = password_hash($userProvidedPassword, PASSWORD_DEFAULT);

// Later:
if (password_verify($userProvidedPassword, $hash)) {
    // Everything works out. Optionally, do this too:
    if (password_needs_rehash($userProvidedPassword, PASSWORD_DEFAULT)) {
        // Someone upgraded PHP and there's a new default algorithm. We should rehash the password.
    }
}

This is almost an optimal UX for PHP developers for password storage: Salts are explicitly generated by the kernel's CSPRNG, encoded, and included in the password hash.

A step further would be making PASSWORD_DEFAULT--, well, the default.

You should only need one input for generating a new password hash: The password.
You should only need two inputs for verification: The challenge and the attempt (or the hash and password, respectively).

All other parameters should be optional.

Note: Key derivation is a separate concern, where you don't want the salt stored alongside the password hash, but that's a smaller corner of the cryptography usability market than password hashing.

Let's look at how another library handles both corner cases:

Key Derivation

#define PASSWORD "Correct Horse Battery Staple"
#define KEY_LEN crypto_box_SEEDBYTES

unsigned char salt[crypto_pwhash_SALTBYTES];
unsigned char key[KEY_LEN];

randombytes_buf(salt, sizeof salt);

if (crypto_pwhash
    (key, sizeof key, PASSWORD, strlen(PASSWORD), salt,
     crypto_pwhash_OPSLIMIT_INTERACTIVE, crypto_pwhash_MEMLIMIT_INTERACTIVE,
     crypto_pwhash_ALG_DEFAULT) != 0) {
    /* out of memory */
}

Password Hashing

#define PASSWORD "Correct Horse Battery Staple"

char hashed_password[crypto_pwhash_STRBYTES];

if (crypto_pwhash_str
    (hashed_password, PASSWORD, strlen(PASSWORD),
     crypto_pwhash_OPSLIMIT_SENSITIVE, crypto_pwhash_MEMLIMIT_SENSITIVE) != 0) {
    /* out of memory */
}

if (crypto_pwhash_str_verify
    (hashed_password, PASSWORD, strlen(PASSWORD)) != 0) {
    /* wrong password */
}

Usable cryptography API design is a nontrivial undertaking, and getting it wrong will mean years (or even decades) of clean-up.

I would propose, at minimum, an alternative API that has a similar user experience to PHP's password_hash() that transparently wraps scrypt as part of the Node.js core, and then recommend that API for users who want to store password hashes (which is roughly 98% of the use case when someone reaches for bcrypt, scrypt, or Argon2).

Or you could always just incorporate @joepie91's scrypt-for-humans into the Node.js core. Designing cryptography APIs for humans is a good idea.

@joepie91

This comment has been minimized.

Contributor

joepie91 commented Jul 11, 2018

Or you could always just incorporate @joepie91's scrypt-for-humans into the Node.js core. Designing cryptography APIs for humans is a good idea.

To be clear, scrypt-for-humans is a thin wrapper to resolve the API design issues in node-scrypt (which are nevertheless less severe than the ones here). It wouldn't make sense to merge the implementation of either scrypt-for-humans or node-scrypt into Node core; but reusing the API design would.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jul 11, 2018

@joepie91 This can not be removed from v10.x due to stability concerns (putting aside everything else).

It could, nevertheless, have documentation updated, noting that this is a low-level API aimed at people who are absolutely sure what they are doing with it, and that direct usage for password hashing could be dangereous, noting common mistakes.

Probably the example code should be updated to use real salt, not a hard-coded one.

I suggest to file another issue about this instead of discussing it in a closed PR.

@paragonie-scott

This comment has been minimized.

paragonie-scott commented Jul 11, 2018

Issue filed: #21766

@joepie91

This comment has been minimized.

Contributor

joepie91 commented Jul 11, 2018

I'll be moving over to that issue thread as well.

@drifkin drifkin referenced this pull request Jul 11, 2018

Open

The crypto module #339

addaleax added a commit to addaleax/node that referenced this pull request Jul 16, 2018

crypto: add better scrypt option aliases
Make parameter names available in a human-readable way, for
more accessible/self-documenting usage of the `scrypt` functions.

This implements a review comment from the original PR that has
not been addressed.

Refs: nodejs#20816 (comment)

addaleax added a commit that referenced this pull request Jul 18, 2018

crypto: add better scrypt option aliases
Make parameter names available in a human-readable way, for
more accessible/self-documenting usage of the `scrypt` functions.

This implements a review comment from the original PR that has
not been addressed.

Refs: #20816 (comment)

PR-URL: #21525
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jul 18, 2018

Labelling as dont-land on 8.x and 6.x due to the ongoging discussion of security concerns.

@Trott Trott referenced this pull request Aug 1, 2018

Open

doc: add req for sec wg review in some cases #21896

3 of 3 tasks complete

targos added a commit that referenced this pull request Aug 7, 2018

crypto: add better scrypt option aliases
Make parameter names available in a human-readable way, for
more accessible/self-documenting usage of the `scrypt` functions.

This implements a review comment from the original PR that has
not been addressed.

Refs: #20816 (comment)

PR-URL: #21525
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

rvagg added a commit that referenced this pull request Aug 13, 2018

crypto: add better scrypt option aliases
Make parameter names available in a human-readable way, for
more accessible/self-documenting usage of the `scrypt` functions.

This implements a review comment from the original PR that has
not been addressed.

Refs: #20816 (comment)

PR-URL: #21525
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

tniessen added a commit to tniessen/node that referenced this pull request Sep 4, 2018

@tniessen tniessen referenced this pull request Sep 4, 2018

Closed

errors: decapitalize PBKDF2 error #22687

2 of 2 tasks complete

danbev added a commit that referenced this pull request Sep 7, 2018

errors: decapitalize PBKDF2 error
PR-URL: #22687
Refs: #20816
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

targos added a commit that referenced this pull request Sep 7, 2018

errors: decapitalize PBKDF2 error
PR-URL: #22687
Refs: #20816
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018

errors: decapitalize PBKDF2 error
PR-URL: nodejs#22687
Refs: nodejs#20816
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment