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

Tue Jun 9 19:21:21 UTC 2020

Looks good, just a couple of minor comments on the test:

* test/jdk/java/security/SecureRandom/DefaultAlgo.java

75         Objects.requireNonNull(p);

Not sure why you need this line, since the test never passes null.

90                 validate(new SecureRandom(), pName, algos[0]);

Is there a reason why you don't call removeService for each algorithm 
when testing the non-legacy provider?

--Sean

On 6/9/20 12:52 PM, Valerie Peng wrote:
> Thanks for review~
> 
> As for the isProviderInfo() name, since I reverted the code for its impl 
> to pre-7092821, I changed it back to the old name as you noticed. Sean 
> mentioned that he also wants to take a look at this updated webrev, so I 
> will wait for him to do that...
> 
> Valerie
> 
> On 6/8/2020 6:11 PM, Weijun Wang wrote:
>> Code change looks fine to me.
>>
>> I re-look at every place where legacyStrings and prngAlgorithms are 
>> used and they are all synchronized. Last time I thought some were not. 
>> Sorry.
>>
>> Only one comment: I like the isProviderInfo() name better, but I 
>> notice it was the old name pre-7092821.
>>
>> Thanks,
>> Max
>>
>>
>>> On Jun 9, 2020, at 6:31 AM, Valerie Peng <valerie.peng at oracle.com> 
>>> wrote:
>>>
>>> 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