Hi Max,
Here is the updated webrev:
http://cr.openjdk.java.net/~ssahoo/8141039/webrev.03/
I misinterpreted your previous comment that the following change is only
applicable to getInstanceTest.java and not applicable to ApiTest.java.
The change includes,
- ApiTest.java moved to " java/security/SecureRandom ".
- Removed reference to MoreDrbgParameters from ApiTest.java
Thanks,
Siba
-----Original Message-----
From: Wang Weijun
Sent: Monday, May 16, 2016 6:36 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
Sorry, I might be not clear enough about the usage of MoreDrbgParameters and
securerandom.drbg.config. You are still using MoreDrbgParameters in
ApiTest.java. For example,
131 SecureRandomParameters mParam = new MoreDrbgParameters(
132 null, mech, alg, nonce, df, (Instantiation)
param);
133 Security.setProperty(DRBG_CONFIG, mech);
134 SecureRandom sr = SecureRandom.getInstance("DRBG",
mParam);
What I wish to see is something like
Security.setProperty(DRBG_CONFIG, mech + "," + alg + ","
+ (df ? "use_df" : "no_df"));
SecureRandom sr = SecureRandom.getInstance("DRBG", param);
So you are still able to iterate through different combinations of mech + alg +
df using a publicly supported API. (Nonce will not be tested, it's not a part
of the API.)
Thanks
Max
> On May 16, 2016, at 6:40 PM, Sibabrata Sahoo <sibabrata.sa...@oracle.com>
> wrote:
>
> Hi Max,
>
> Please find the updated webrev:
> http://cr.openjdk.java.net/~ssahoo/8141039/webrev.02/
>
> The changes includes,
> - ApiTest.java : Removed printing unnecessary message inside checkException()
> method.
> - GetInstanceTest.java
> - Moved the test back into " java/security/SecureRandom/ ".
> - Removed all reference to MoreDrbgParameters and EntropySource.
>
>
> Thanks,
> Siba
>
> -----Original Message-----
> From: Wang Weijun
> Sent: Thursday, May 12, 2016 9:14 AM
> 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
>
>
>> On May 12, 2016, at 1:46 AM, Sibabrata Sahoo <sibabrata.sa...@oracle.com>
>> wrote:
>>
>> 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.
>
> I don't see a difference. You print out messages in those "if (strength > n)"
> blocks but there is no other action. What are the messages for? Information
> or warning? In my opinion, a test should print out as little as possible
> unless an error occurs. (Sometimes "Testing XYZ..." is OK).
>
>> - 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.
>
> Well, I have a different opinion.
>
> I think most of your tests should be categorized as functional (or
> conformance) tests, so they should be focused on public APIs. In this case,
> the test should set "securerandom.drbg.config" to Hash_DRBG/SHA-512 instead
> of using MoreDrbgParameters to set algorithm to SHA-512.
>
> In fact, there is no requirement that MoreDrbgParameters must be accepted by
> getInstance().
>
> MoreDrbgParameters and EntropySource are implementation details. Sometimes
> they are necessary (For example, CAVP tests), but you'd better avoid touching
> them. I did wrote some tests on them (DRBGAlg, AbstractDrbgSpec) in an
> implementor's perspective but you can ignore them.
>
>> - 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.
>
> Both the SHA1PRNG setSeed bug and the DRBG synchronization bug are already
> fixed and pushed.
>
> Thanks
> Max
>
>
