RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

> On Apr 1, 2016, at 3:24 AM, Sean Mullan <sean.mullan at oracle.com> wrote:
> 
> Just a few comments:
> 
> - SunJCE
> 
> 707                     // TODO: aliases with OIDs
> 
> leftover TODO.

Every other HMAC algorithm has an OID that can be used as an Alg.Alias.***, but the 2 new algorithms do not have. I expect one day they will have.

> 
> - SecureRandom
> 
> 604      * @implSpec The default implementation returns {@code null}.
> 
> Technically, I don't think that is correct, since it is really dependent on what the underlying Spi is doing. The same comment applies to the other @implSpec sections in this class.

I see. So @implSpec should only appear in SecureRandomSpi.

> 
> 
> 683      * @throws UnsupportedOperationException if the implementation
> 684      *         has not overridden this method.
> 
> Would it be more accurate to say "if the underlying provider implementation (SecureRandomSpi) has not overridden this method". Same comment applies to other UOEs in this class.

Yes.

> - SecureRandomSpi
> 
>  86     protected SecureRandomSpi(SecureRandomInstantiateParameters params) {
>  87         // ignored
>  88     }
> 
> If you changed this to:
> 
> protected SecureRandomSpi(SecureRandomInstantiateParameters params) {
>    this();
> }
> 
> couldn't you avoid the code which catchs a NoSuchMethodExc and retries, etc? It would be nice to not have these extra rules about calling this constructor or that constructor, and instead you could just always call the constructor above and it would do the right thing. Just thinking out loud here, not sure if it is the right thing to do.

I'll think about it. I assume you are talking about my change in Provider.java. The problem is that old and new SecureRandom implementations do not have the same constructors and I need them both working.

> 
> - java.security
> 
> what happens if you have parsing/syntax errors in the drbg property?

Well, there is no parsing error now since I treat everything non-parseable as algorithm and it will be caught later.

Said that, maybe I should at least check if one aspect is provided 2 times and if there is an empty aspect, and if an integer is non-positive.

> Also, does the order of the aspects matter?

No. The only non-literals are algorithm name and strength. The strength should always be an integer and the algorithm name should not be the same as other aspects.

> 
> - DrbgParameters
> 
> 249          * @return If used in {@code getInstance}, returns the minimum strength
> 
> s/If/if/
> 
> 253          * strengh requested.
> 
> s/strengh/strength/
> 
> 290          * @return If used in {@code getInstance}, returns the minimum capability
> 301          * @return If used in {@code getInstance}, returns the requested
> 
> a/If/if/
> 
> 428     public static Instantiate instantiate(int strength,
> 429                                           Capability capability,
> 430                                           byte[] personalizationString) {
> 
> Should this throw NPE if capability is null?

Yes.

> Should it throw IllegalArgExc if strength < -1?

I can, but necessary?

> 
> - EntropySource
> 
> Is this interface used anywhere?

In AbstractDrbg.SeedHolder, as the default entropy sources for pr_true and pr_false are different.

And in tests.

> 
> Should getEntropy throw IllegalArgumentExceptions if int params are less than a certain value or if maxLength < minLength? Does it return a new byte array each time it is invoked?

Yes and yes, but I haven't checked/documented it since it's not a public API yet.

Thanks
Max

> 
> --Sean
> 
> On 03/29/2016 04:47 AM, Wang Weijun wrote:
>> Ping again. No comment?
>> 
>> --Max
>> 
>>> On Mar 21, 2016, at 1:15 PM, Wang Weijun <weijun.wang at oracle.com> wrote:
>>> 
>>> Hi All
>>> 
>>> Please take a review at the design and implementation of DRBG at:
>>> 
>>> http://cr.openjdk.java.net/~weijun/8051408/webrev.07
>>> http://cr.openjdk.java.net/~weijun/8051408/webrev.07/spec
>>> http://cr.openjdk.java.net/~weijun/8051408/webrev.07/specdiff/overview-summary.html
>>> 
>>> An example:
>>> 
>>> SecureRandom drbg;
>>> byte[] buffer = new byte[32];
>>> 
>>> drbg = SecureRandom.getInstance("DRBG",
>>>        DrbgParameters.instantiate(256, PR_ONLY, "hello".getBytes()));
>>> 
>>> drbg.nextBytes(buffer,
>>>        DrbgParameters.nextBytes(-1, false, "more".getBytes()));
>>> 
>>> SecureRandomInstantiateParameters params = drbg.getParameters();
>>> if (params instanceof DrbgParameters.Instantiate) {
>>>    DrbgParameters.Instantiate ins = (DrbgParameters.Instantiate) params;
>>>    if (ins.getCapability() != NONE) {
>>>        drbg.reseed(DrbgParameters.reseed(false, "extra".getBytes()));
>>>    }
>>> }
>>> 
>>> Thanks
>>> Max
>>> 
>>