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

Tue Jun 9 23:53:00 UTC 2020

Hi, Sean,

On 6/9/2020 12:21 PM, Sean Mullan wrote:
> 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.

True, the test never passes null, this line is just to make it clear 
that the provider argument should not be null as the test is not 
prepared to handle null provider. It's not essential, so I removed it 
per your comment.

>
> 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?

The Provider.removeService(...) call is protected. So, it's not a public 
API for users of Provider objects.

Thanks,
Valerie
>
> --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