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

[Valerie Peng]

Webrev updated: http://cr.openjdk.java.net/~valeriep/8246613/webrev.02/

Besides addressing Max's comments, I also made 
updateSecureRandomEntries(...) method private and removed the 
synchronized keyword as all of its accesses are synchronized.

Thanks,
Valerie
On 6/8/2020 2:33 PM, Valerie Peng wrote:
> Hi Max,
>
> Please find comments in line.
>
> On 6/8/2020 2:34 AM, Weijun Wang wrote:
>> Looks like this should work, but still find it complicated.
>>
>> 1. Do we need to care about thread safety when managing legacyStrings?
>
> Right, it's more complicated than I like as well.
>
> As for thread safety, the legacy relevant data are all synchronized 
> under the current provider object, i.e. this. Is there a particular 
> call path not doing this? This is the same as the pre-7092821 code.
>
>> 2. Does implReplaceAll() need to set legacyChanged = true?
> Correct, the removal is by accident. Thanks for catching this.
>> 3. How about using prngAlgorithms.iterator().next() below?
>>
>>    1416     return prngAlgorithms.toArray(new String[0])[0];
>
> Sure, changed.
>
> Valerie
>
>>
>> --Max
>>
>>
>>> On Jun 6, 2020, at 11:54 AM, Valerie Peng <valerie.peng at oracle.com> 
>>> wrote:
>>>
>>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20200608/928c8da3/attachment.htm>