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

[Valerie Peng]

On Mon, 15 Mar 2021 22:11:51 GMT, SalusaSecondus <github.com+829871+SalusaSecondus at openjdk.org> wrote:

>> Cipher and KeyPairGenerator could be used a lot in TLS context.  The synchronization may impact in some circumstances.  Supporting dynamic configuration could be error-prone.  For example, one thread to set the providers, and another thread get the providers by calling Cipher.init().  Unless the set and get use the same lock, the return value of the get method is not predictable.  The lock here is on CachedSecureRandomHolder for providers configuration getting, and no lock (or different lock) for providers configuration setting.  So applications may be able to get the expected behaviors by introducing this synchronization.
>> 
>> 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.
>> 
>> If you want go ahead, maybe the performance impacted could be mitigated by locking less processes.  For example:
>> 
>> if (checkConfig) { 
>>     Provider[] currConfig = Security.getProviders();
>>     if (!Arrays.equals(cachedConfig, currConfig)) {
>>        synchronized (CachedSecureRandomHolder.class) {
>>            // double check the currConfig
>>            currConfig = Security.getProviders();
>>           if (!Arrays.equals(cachedConfig, currConfig)) {
>>               instance = ...
>>               cachedConfig = ...
>>            }
>>        }
>>     }
>> }
>> return instance;
>> 
>> However, I still not very sure of the performance of Security.getProviders() because it is synchronized as well.  Further, I may want to evaluate if the update of Security provider (for example Providers.setProviderList) could trigger an even to update the default instances.
>
> I would definitely not call `new SecureRandom()` each time as that can have real performance problems (especially on systems which pull from `/dev/random` and may not have hardware entropy). The synchronization worries me too.
> 
> Could `java.security.Security` contain a "provider version" `int` (volatile, atomic, or similar) which is incremented every time the provider list is changed? Then this class (clearly using some non-public API) would check that value and if it is the same, just return the cached instance. If it has changed since the last time, then it would update `instance` and its remembered value of the index. I think that we might be able to do it without taking any locks in this class because even if the providers change out from under us, we returned a reasonable value (subject to multi-threading) and will fix things up when we're called the next time.
> 
> // using this and name collision to highlight what's a field and what's local
> int providerListVersion = Security.nonPublicGetProviderListVersion();
> if (this.providerListVersion != providerListVersion) {
>    this.providerListVersion = providerListVersion;
>    this.instance = new SecureRandom();
> }
> return this.instance;

Or, I can also separate out the instance returned by getSecureRandom() so that code path should be unaffected, e.g.:
    // 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();
    }

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

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