RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

> On Apr 20, 2016, at 11:34 AM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> 
> On 4/19/2016 9:09 PM, Xuelei Fan wrote:
>> On 4/15/2016 9:
>>> http://cr.openjdk.java.net/~weijun/8051408/webrev.10/ 
> 
> src/java.base/share/classes/sun/security/provider/AbstractDrbg.java
> ===================================================================
> line 66-68: My understanding is that ...
> 
> I would suggest rewords or remove this sentence.  "Not used much" does
> not mean needing no synchronization.  

Of course.

Precisely engineNextBytes() should synchronize on both states and configuration, and engineGenerateSeed() should synchronize only on configuration. But since engineGenerateSeed() is not used a lot, I don't think it's not worth coding it with a special synchronize(configuration) so both now just synchronize on "this".

> As you have add synchronized
> keyword for engineGenerateSeed, I may suggest you remove lines 63-68,
> and move 57-61 to class description.

I'll move 57-61 to class description, and would like to keep 63-68 there, and update the words "does not need to be synchronized" to "does not need to be synchronized on the internal states".

> 
> I think comment are mainly used for code readers.  You used a lot "my"
> and "I", and questions to yourself in the comments.  Would you mind
> change to use neutral tones and remove unnecessary comments in the final
> code?

OK.

> 
> ------
> 662     // 6/7. Get entropy. TODO: 1st arg security strength?
> 
> Do you want to complete the TODO task.   Similar comments to other TODO
> tasks.

Will remove TODO word and add a clarification.

> 
> ------
> 670     nonce = longToByteArray(cc.incrementAndGet());
> 685     private static byte[] longToByteArray(long l)
> 
> The nonce is leading with "sun.drbg", and following by a 8-bytes integer
> value.  This scheme only provider 8-bytes randomness.  Looks like the
> quality does not meet the nonce requirements (8.7.6 NIST SP-800-90Ar1)
> for 256-bit strength.

8.6.7 allows it to be a "monotonically increased sequence number".

> 
> ------
> 598     protected final synchronized void engineConfigure(
> 
> engineConfigure is not part of SPI.  Would you mind change the name and
> remove the leading "engine" in case of miss-understanding?

s/engineConfigure/configure/

> 
> It would be nice grant as less accessibility as possible.  For the
> "protected" keyword, should it be package accessibility at present?
> Similar comment for other protected methods other than the override
> ones, and some public classes in the package.

Will go through them.

I created AbstractDrbg so that it can be the base for any DRBG not necessarily in this package. For example, the AbstractDrbgSpec test also contains an implementation.

> 
> ------
> Section 7.2 of NIST SP 800-90Ar1 says: "The personalization string
> should be unique for all instantiations of the same DRBG mechanism type".
> 
> Please check the unique for the personalization string in the
> implementation.

"Should" is not "shall" (section 4, terms). Two other reasons:

1. Checking for uniqueness needs to save all strings in memory.

2. The default is null, and all nulls are the same. Why bother checking for those non-nulls for uniqueness?

Thanks
Max

> 
> Looking forward to your next webrev.
> 
> Xuelei
> 
>