Hi Adam, thanks for taking a look at this. Comments are in-line:
On 3/14/19 7:53 AM, Adam Petcher wrote:
The change to PBKDF2KeyImpl.java looks fine. About the test:
*) Is it necessary to put the provider in a separate jar? It seems
unnecessary because you are adding it with Security.insertProviderAt.
JN: I'm honestly not sure. I thought if I wanted this test to run on
both Open and Oracle JDK I thought I'd need the provider to be signed
(which the jar file is). I can try bringing the provider code into the
test body itself. If it runs fine on Oracle JDK then it would be way
better to do that and get rid of the makefile and all the attendant
source for rebuilding the provider jar.
*) Line 54 of the test compares the result of a constructor to null.
Unless I'm missing something, this reference will always be non-null.
JN: That was left over from an early rev of the test where I was doing
Security.getProvider(String) where it can return null. I'll take that out.
*) At the end of the test, there are some methods that do conversion
between hex strings and bytes. Can you use the methods in Convert (in
the test list) instead? I think Convert.hexStringToByteArray is the
same thing as hex2bin. You may also want to move dumpHexBytes to
Convert, but it's fine either way.
JN: I had meant to fix that before I sent it to review, I'm in the
process of making that change now.
*) It looks the evilprovider source files have the wrong copyright
header.
JN: Easily fixed.
*) There is a commented out line of code on line 16 of EvilProvider.java
JN: Good as gone.
On 3/14/2019 9:34 AM, Jamil Nimeh wrote:
Hello all,
This review will change the SunJCE implementation of PBKDF2 so that
it always uses the SunJCE version of the PRF algorithm internally.
Webrev: http://cr.openjdk.java.net/~jnimeh/reviews/8218723/webrev.01/
JBS: https://bugs.openjdk.java.net/browse/JDK-8218723
CSR: https://bugs.openjdk.java.net/browse/JDK-8220531
