The old code
265 prng = "SHA1PRNG";
266 this.secureRandomSpi = new
sun.security.provider.SecureRandom();
267 this.provider = Providers.getSunProvider();
always works since that class is always there.
Now, it's
282 prngService =
Providers.getSunProvider().getService("SecureRandom",
283 "SHA1PRNG");
If someone remove that service the above would return null. Can we simply reuse
the old lines?
Everything else looks fine.
Thanks,
Max
> On Jun 11, 2020, at 8:16 AM, Valerie Peng <[email protected]> wrote:
>
> Updated webrev after incorporating Sean's feedbacks:
>
> http://cr.openjdk.java.net/~valeriep/8246613/webrev.03/
>
> Main changes are code refactoring in SecureRandom class and changed Provider
> class to store SecureRandom services in their order of registration instead
> of only the algorithm name.
>
> Thanks,
> Valerie
> On 6/9/2020 4:53 PM, Valerie Peng wrote:
>> 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 <[email protected]> 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 <[email protected]>
>>>>>>>>> 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
>>>>>>>>>>>> <[email protected]> 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: [email protected]
>>>>>>>>>>>>> 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
