[15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering

[Valerie Peng]

Thanks for reviewing and sharing the feedbacks on webrev.00.

In order to support all existing calls for legacy registration for 
default secure random, I have to revert some of the JDK-7092821 changes 
and re-introduce the 'legacyStrings' LinkedHashMap. Updated the 
regression test with removal test for provider using legacy 
registrations as well. Although removal is supported, this is still not 
bullet proof as things may not work as expected if a provider registered 
their impl in both ways, i.e. legacy String pair and Service, and then 
remove/replace some entries later. Please comment if you really need 
this scenario to be supported. Although not explicitly documented, I 
think the intention is to use one or the other, never both.

Webrev update: http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/

Thanks,
Valerie
On 6/5/2020 11:00 AM, Valerie Peng wrote:
> Right, I try to keep the impl simple as I am not aware of any property 
> removal usage.
>
> Oh-well, if we have to cater to the removal scenario, then we'd have 
> to add a List and keep track of ALL secure random algos instead of 
> only the FIRST one. Alright, I guess it costs for covering all aspect. 
> But one extra benefit of this is that it should be easy to handle the 
> future JDK property such as "jdk.securerandom.disabledAlgorithms" if 
> it were to be added.
>
> Valerie
>
> On 6/5/2020 7:54 AM, Weijun Wang wrote:
>> I don't know who in this world would want to do that, but Prasad's 
>> concern is technically possible. I tried 'p.remove("SecureRandom.a")' 
>> in the new test, and new SecureRandom() fails with 
>> "java.security.NoSuchAlgorithmException: a SecureRandom not available".
>>
>> And people can also remove one entry and add it back in order to move 
>> it to the end. One can even add new implementations this way.
>>
>> Unfortunately there is no ConcurrentLinkedHashMap.
>>
>> --Max
>>
>>> On Jun 5, 2020, at 1:44 PM, Prasadrao Koppula 
>>> <prasadarao.koppula at oracle.com> wrote:
>>>
>>> Hi,
>>>
>>> Looks good to me, one question
>>>
>>> If first registered SecureRandom algo gets removed, 
>>> getDefaultSecureRandomAlgorithm return stale data, a refresh 
>>> required in remove?
>>>
>>> Thanks,
>>> Prasad.K
>>>
>>>> -----Original Message-----
>>>> From: Valerie Peng
>>>> Sent: Friday, June 5, 2020 2:52 AM
>>>> To: security-dev at openjdk.java.net
>>>> Subject: Re: [15] RFR JDK-8246613: Choose the default SecureRandom 
>>>> algo
>>>> based on registration ordering
>>>>
>>>> Hi, Sean,
>>>>
>>>> Thanks for the review and feedback. Webrev updated:
>>>> http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/
>>>>
>>>> getTypeAndAlgorithm(...) was not static due to an instance variable 
>>>> used by
>>>> debugging output. I have removed it and made both method static.
>>>>
>>>> I will wait for others' review comments also.
>>>>
>>>> Thanks,
>>>> Valerie
>>>> On 6/4/2020 2:01 PM, Sean Mullan wrote:
>>>>> On 6/4/20 3:34 PM, Valerie Peng wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Could someone help reviewing this fix? This change keep tracks of 
>>>>>> the
>>>>>> first registered SecureRandom algorithm and returns it upon the
>>>>>> request of SecureRandom class.
>>>>> This looks good to me. I would recommend that Max or someone else
>>>>> review it as well.
>>>>>
>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8246613
>>>>>>
>>>>>> Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/
>>>>> A couple of minor comments, feel free to fix or ignore.
>>>>>
>>>>> * SecureRandom.java
>>>>>
>>>>> 879             // For SUN provider, we use
>>>>> SunEntries.DEFF_SECURE_RANDOM_ALGO
>>>>>
>>>>> Might as well fix the typo while you are in there: s/DEFF/DEF/
>>>>>
>>>>> * Provider.java
>>>>>
>>>>> 1156     private String parseSecureRandomPut(String name, String
>>>>> value) {
>>>>>
>>>>> Could be static if you also make getTypeAndAlgorithm static, I think.
>>>>>
>>>>> --Sean