RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

> On Apr 20, 2016, at 9:35 AM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> 
> On 4/20/2016 9:17 AM, Wang Weijun wrote:
>> 
>>> On Apr 20, 2016, at 7:41 AM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
>>> 
>>> On 4/19/2016 11:41 PM, Wang Weijun wrote:
>>>>>>>> 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,
>>> I think the constructor is not of CertStore, but of CertStoreSpi
>>> implementation.  
>> 
>> Yes.
>> 
>>> The provider implementation may support any kind of
>>> constructor (implicit or explicit, intended or not).  The constructor
>>> should be unknown to Provider.
>> 
>> Not sure what you mean, but Provider uses reflection to get the constructor and create new instances. By convention, when something is created via getInstance(alg), the SPI class would provide the arg-less constructor; when via getInstance(alg,params), it would provide the with-arg constructor.
>> 
> Not sure what you mean.
> 
> public MyCertStore extends CertStoreSpi {
> 
>    public MyCertStore() {
>        // whatever
>        // ;-) Don't ask me why this construct is necessary.
>    }
> 
>    public MyCertStore(XXX params) {
>        // throws NoSuchMethodException
>        // ;-) Don't ask me why throw this exception.
>    }
> }
> 
> newInstanceUtil(MyCertStore, ...)
> 
> The MyCertStore() would get called, unexpectly.  Am I missing something?

Probably not, unless you call getInstance(arg, null). I am not sure this null will trigger some other exception along the way.

OK, I admit there is a side effect here: If you design getInstance(alg,params) but params is always null, then you can only implement a constructor with no params.

This is stupid and useless, but not really harmful.

> 
>>> 
>>>> so NoSuchMethodException will never be thrown. Again, this fallback is only for SecureRandom now.
>>>> 
>>>> 
>>> Please add more comment and control so that this only apply to SecureRandom.
>> 
>> It already had "This is to support the enhanced SecureRandom".
>> 
> My understanding is:  this is to support the enhanced SecureRandom, and
> it now applies to non-SecureRandom too.
> 
> If it does not apply to non-SecureRandom, please add comment and update
> the code accordingly.

Comment added. As described above, there should not practical impact on non-SecureRandom primitives.

I would revert the following line

159                     throw new IllegalArgumentException(nsme);

back to a simple "throw nsme".

Thanks
Max

> 
> Xuelei