crypto: refactor argument validation for pbkdf2 #15746
Conversation
nodejs-github-bot
added
c++
jasnell
force-pushed
the
pbkdf2_internal_errors
branch
from
443042f to
7afbaa2
Compare
jasnell
added
errors
lib/internal/crypto/pbkdf2.js
Outdated
There was a problem hiding this comment.
Number.isFinite already checks if it is a NaN
test/parallel/test-crypto-pbkdf2.js
Outdated
There was a problem hiding this comment.
DEFAULT_ENCODING would not affect the digest, right? Do we really need the following tests?
There was a problem hiding this comment.
It does. DEFAULT_ENCODING changes the encoding of the derived key returned.
There was a problem hiding this comment.
It concerns me a bit that the way it is put in documentation right now suggest use of DEFAULT_ENCODING. Can we discourage it?
There was a problem hiding this comment.
Yeah, that's a good point. Document it's possible but strongly discourage it.
There was a problem hiding this comment.
It affects the derived key, but these tests are validating only the digest. So these tests look superfluous to me.
There was a problem hiding this comment.
ha! actually I think I was intending these to be something else but got distracted and didn't finish editing the test! If you notice, the items below are identical to the items above ;-) I'll take another pass at that lol
test/parallel/test-crypto-pbkdf2.js
Outdated
There was a problem hiding this comment.
Just a dumb question: Till today my understanding was that INT_MAX would be the size of the word of the given processor architecture. But today, even on my 64-bit machine, the value is defined as 2147483647. I am a little confused here. C++ Spec is not very clear in this regard. Do you know where I can get a clear answer?
There was a problem hiding this comment.
@thefourtheye INT_MAX is the maximal value of the signed int type. int can be anything larger than or equal to 16 bits, although most compilers implement it as 32 bits. The number you are seeing is 2^31-1.
There was a problem hiding this comment.
@tniessen That is exactly the problem. There is no guarantee that it is going to be always 2^31-1, right? Otherwise this test will fail.
There was a problem hiding this comment.
@thefourtheye I believe all compilers we officially support use 32 bit ints on all supported platforms, though I might be mistaken. If I remember correctly, there is also INT32_MAX, but I am not sure it is more appropriate to use. INT_MAX seems fine as long as we use int internally. If INT_MAX is greater on some platforms, e.g. 2^63-1, it will likely be bigger than MAX_SAFE_INTEGER (which I believe is around 2^54).
There was a problem hiding this comment.
Ah, "all compilers we officially support"- that is the key here. Let me start a CI to confirm this.
There was a problem hiding this comment.
If just to be safe, we can use process.binding('constants').crypto.INT_MAX + 1 here
jasnell
force-pushed
the
pbkdf2_internal_errors
branch
2 times, most recently
from
05c3eb3 to
812d575
Compare
doc/api/errors.md
Outdated
There was a problem hiding this comment.
Maybe a link to crypto.getHashes() here?
Move input argument validation to js, using internal/errors. Also update docs * `password` and `salt` may be Buffers or any TypedArrays * `crypto.DEFAULT_ENCODING` changes the returned derivedKey type
jasnell
force-pushed
the
pbkdf2_internal_errors
branch
from
dce9707 to
572b090
Compare
Move input argument validation to js, using internal/errors. Also update docs * `password` and `salt` may be Buffers or any TypedArrays * `crypto.DEFAULT_ENCODING` changes the returned derivedKey type PR-URL: #15746 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
|
Landed in 7124b46 |
Move input argument validation to js, using internal/errors. Also update docs * `password` and `salt` may be Buffers or any TypedArrays * `crypto.DEFAULT_ENCODING` changes the returned derivedKey type PR-URL: nodejs/node#15746 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Move input argument validation to js, using internal/errors. Also update docs * `password` and `salt` may be Buffers or any TypedArrays * `crypto.DEFAULT_ENCODING` changes the returned derivedKey type PR-URL: nodejs/node#15746 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
7 participants
Move input argument validation to js, using internal/errors.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
crypto