[9] RFR: 8141039: Test Task: Develop new tests for JEP 273: DRBG-Based SecureRandom Implementations

Sibabrata Sahoo sibabrata.sahoo at oracle.com
Tue May 17 08:13:13 UTC 2016


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 at 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.sahoo at 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 at 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.sahoo at 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
> 
> 




More information about the security-dev mailing list