Hi Max,
Please find the updated webrev:
http://cr.openjdk.java.net/~ssahoo/8141039/webrev.01/
Changes includes:
- Removed otherVM option and default "securerandom.drbg.config" will get reset
after each DRBG test run. This change affected to all test files.
- Addressed all the following comments for GetAlgorithm.java,
EnoughSeedTest.java, MultiThreadTest.java.
- Moved ApiTest.java, into "sun/security/provider/SecureRandom" because it uses
MoreDrbgParameters.
- remove SHA1 and TDEA, 3KEYTDEA" ("3 KEY TDEA", "DESEDE")
- corrected the logic for checkException() reported bellow.
- Moved GetInstanceTest.java into "sun/security/provider/SecureRandom" because
it uses MoreDrbgParameters.
- I am using default as well as custom entropy source just to make sure the
extension works for getInstance() method.
- Again, I use MoreDrbgParameter, just to make sure the instance of it can
be acceptable by corresponding getInstance() method.
- About SerializedSeedTest.java, I think the SHA1PRNG bug fix will get pushed
to repo very soon. So the test is expected to work as the fix pushed into repo.
But if you wish, I can change it accordingly.
Thanks,
Siba
-----Original Message-----
From: Wang Weijun
Sent: Tuesday, May 10, 2016 1:14 PM
To: Sibabrata Sahoo
Cc: security-dev@openjdk.java.net
Subject: Re: [9] RFR: 8141039: Test Task: Develop new tests for JEP 273:
DRBG-Based SecureRandom Implementations
For all:
- If you can remember the old "securerandom.drbg.config" value to reset it in a
finally clause, there is no need to othervm.
- All DRBG mechanism names contain "_DRBG", therefore I'd rather change
NON_DRBG.contains(mech) to !mech.contains("_DRBG"). The same technique can be
applied everywhere. In fact, you can also add "DRBG" as a test algorithm, i.e.
using the default "securerandom.drbg.config". If so, you have to reset the
value after every call.
GetAlgorithm.java:
- You can but check on line 60 into lines 51 and 54, something like check(name,
SecureRandom.getInstance(...))
ApiTest.java:
- Please remove tests for SHA-1 and "3KEYTDEA" ("3 KEY TDEA", "DESEDE"), we are
going to remove them soon.
- I don't fully understand what lines 144-147 mean. If an NSAE is thrown,
checkException() only fails if alg is invalid. Maybe you should also fail if
strength check inside it is false? For example, line 192, it's OK for SHA-1 to
fail if strength > 128, but not if it's less.
- MoreDrbgParameters is internal. I suggest you stuff info about alg and df
inside "securerandom.drbg.config". Probably no need to test nonce and es then.
Or, if you still want to test on MoreDrbgParameters, move the test inside
sun/security/provider/SecureRandom.
EnoughSeedTest.java:
- What if SUCCESS has been false once, but turns true in the last loop? Is that
a success? Maybe you should "SUCCESS &= ..."?
- same with "success" inside forEachSeedBytes?
- What's the theory behind this test? Who said seed cannot ends with one or
more zero? Zero is just as normal as any other byte which has its own 1/256 of
probability to appear.
GetInstanceTest.java:
- Why name it DRBG_SEC_PROP?
- You use MoreDrbgParameters again. Is this necessary? I don't see how the 2
EntropySource are different.
MultiThreadTest.java:
- Lines 135-144: If it's out System.out.printf, then it's useless. There are
theories on quality of RNGs. For example: https://www.random.org/analysis/.
SerializedSeedTest.java:
- The SHA1PRNG bug is pending. At the moment, can you just generate 32 bytes of
data with nextBytes() inside check()?
Thanks
Max
> On May 8, 2016, at 9:57 PM, Sibabrata Sahoo <sibabrata.sa...@oracle.com>
> wrote:
>
> Hello,
>
> Please review the tests for “JDK-8141039:Test Task: Develop new tests for JEP
> 273: DRBG-Based SecureRandom Implementations”.
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8141039
> Webrev: http://cr.openjdk.java.net/~ssahoo/8141039/webrev.00/
>
> The tests covers the new DRBG API with different set of parameters. It also
> covers the behavior of all SecureRandom mechanism supported in Java in
> Serialized as well as in multithread context.
>
> Thanks,
> Siba
