RFR: 8260274: Cipher.init(int, key) does not use highest priority provider for random bytes

[Valerie Peng]

On Tue, 16 Mar 2021 20:53:27 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> I did not get the point that the code path should be unaffected". As there are a few update like:
>>          init(keysize, JCAUtil.getSecureRandom());
>>          init(keysize, JCAUtil.getDefSecureRandom());
>> 
>> I think there are some impact on the existing application which use the default sec
>> 
>>> Or, I can also separate out the instance returned by getSecureRandom() so that code path should be unaffected, e.g.:
>> 
>> I did not get the point that the "code path should be unaffected"? JCAUtil.getDefSecureRandom() is used in updates like:
>>          init(keysize, JCAUtil.getSecureRandom());
>>          init(keysize, JCAUtil.getDefSecureRandom());
>> 
>>> 
>>> ```
>>>     // cached SecureRandom instance
>>>     private static class CachedSecureRandomHolder {
>>>         public static SecureRandom instance = new SecureRandom();
>>>         private static Provider[] cachedConfig = Security.getProviders();
>>>         private static SecureRandom def = instance;
>>> 
>>>         public static SecureRandom getDefault() {
>>>             synchronized (CachedSecureRandomHolder.class) {
>>>                 Provider[] currConfig = Security.getProviders();
>>>                 if (!Arrays.equals(cachedConfig, currConfig)) {
>>>                     def = new SecureRandom();
>>>                     cachedConfig = currConfig;
>>>                 }
>>>             }
>>>             return def;
>>>         }
>>>     }
>>> 
>>>     /**
>>>      * Get a SecureRandom instance. This method should be used by JDK
>>>      * internal code in favor of calling "new SecureRandom()". That needs to
>>>      * iterate through the provider table to find the default SecureRandom
>>>      * implementation, which is fairly inefficient.
>>>      */
>>>     public static SecureRandom getSecureRandom() {
>>>         return CachedSecureRandomHolder.instance;
>>>     }
>>> 
>>>     /**
>>>      * Get the default SecureRandom instance. This method is the
>>>      * optimized version of "new SecureRandom()" which re-uses the default
>>>      * SecureRandom impl if the provider table is the same.
>>>      */
>>>     public static SecureRandom getDefSecureRandom() {
>>>         return CachedSecureRandomHolder.getDefault();
>>>     }
>>> ```
>
>> As this behavior has been a while, I may not update the implementation. Instead, I would like to add an "implNote" tag to state that once the default provider is loaded, an implementation may be changed it any longer. Applications could use provider parameter if there is a preference.
> 
> Well, javadoc of the affected init methods clearly specifies that "get them using the SecureRandom implementation of the highest-priority installed provider as the source of randomness. ", thus I am not sure if we can just get around this with an _ at implNote_.

> I did not get the point that the code path should be unaffected". As there are a few update like:
> init(keysize, JCAUtil.getSecureRandom());
> init(keysize, JCAUtil.getDefSecureRandom());

What I mean is that the new suggestion will not impact callers of JCAUtil.getSecureRandom() which I thought what your performance regression means. To enforce that the most preferred SecureRandom impl is used, this for sure will incur some cost especially when the existing impl just returns a cached obj w/o any checking.

-------------

PR: https://git.openjdk.java.net/jdk/pull/3018