[15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
Valerie Peng
valerie.peng at oracle.com
Thu Jun 11 00:16:51 UTC 2020
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 <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
More information about the security-dev
mailing list