RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

> On Apr 19, 2016, at 9:09 PM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> 
> On 4/15/2016 9:
>> http://cr.openjdk.java.net/~weijun/8051408/webrev.10/
> 
> Please update copyright dates.
> 
> src/java.base/share/classes/java/security/Provider.java
> -------------------------------------------------------
> 145-151:  Looks like the comment are not correct.  There are
> getInstance(alg,params) since JDK 1.4 (See CertStore)

Yes, but in those cases, there is either getInstance(alg) or getInstance(alg,params), but not both. In SecureRandom, we are now supporting both, and a fallback is needed for those implementations that does not override new SecureRandomSpi(SRP).

> 
> 156-157:  Fallback change the behavior significantly.  Construct with or
> without parameter can be very differently.  For example, LDAP cert store
> get requested, but the CertStore.getInstane(String,
> LDAPCertStoreParameters) may not return an LDAP cert store.  Can you
> make more comment why the fallback is OK?

For CertStore (and Configuration and Policy), all of its implementations only support the with-arg constructor, so NoSuchMethodException will never be thrown. Again, this fallback is only for SecureRandom now.


> 
> src/java.base/share/classes/java/security/SecureRandom.java
> -----------------------------------------------------------
> 456    SecureRandomSpi spi = (SecureRandomSpi) instance.impl;
> 457    SecureRandom r = new SecureRandom(spi, instance.provider,
> algorithm);
> 458    return r;
> 
> 456, there is a whitespace of the casting.

Will remove.

> 
> The compiler may optimize the code, but I may use the old style as line
> 401-402.
> 
> Similar comment for 507-510, 553-556.

You mean inline and remove the spi variable? Sure I can.

> 
> 477 -       secureRandomSpi.engineNextBytes(bytes);
> 667 +       secureRandomSpi.engineNextBytes(
> 668 +               Objects.requireNonNull(bytes));
> Some provider may tolerant null bytes input (for example SunPKCS11).
> Please consider this update more for compatibility.

Oh, then this will be unspecified. I'll think about it.

> 
> 
> src/java.base/share/classes/java/security/SecureRandomSpi.java
> --------------------------------------------------------------
> 124         throw new UnsupportedOperationException("not supported");
> 156         throw new UnsupportedOperationException("not supported");
> 
> I would like to use no-parameter exception, or more specific message.

No params.

> 
> 174     @Override
> 175     public String toString() {
> 176         return getClass().getSimpleName();
> 177     }
> What's the usage of this new toString() method?

Testing and debugging. For DRBG, it returns a text like the "drbg" security property.

Thanks
Max

> 
> 
> Review the DRBG impl later.
> 
> Thanks,
> Xuelei
> 
> 
>