RE: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
Likewise, planning for the future, should a new JDK property such as
"jdk.securerandom.disabledAlgorithms" be implemented, which could impact the
value returned from getDefaultSecureRandomAlgorithm() ?
Thanks,
François
-Original Message-
From: security-dev On Behalf Of
Prasadrao Koppula
Sent: Friday, June 5, 2020 1:44 AM
To: Valerie Peng; security-dev@openjdk.java.net
Subject: RE: [15] RFR JDK-8246613: Choose the default SecureRandom algo based
on registration ordering
[EXTERNAL EMAIL]
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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
>>>>>>>>> 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
>>>>>>>>>>>> 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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
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
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
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@openjdk.java.net
Subject: Re: [15] RFR JDK-8246613: Choose the default
SecureRandom algo
based on r
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
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
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
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@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 P
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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 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 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
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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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 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 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
>>>>>>> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> Looks good to me, one question
>>>>>>>
>>>>>>> If first registered SecureRandom
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
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
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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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 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
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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
Looks like this should work, but still find it complicated.
1. Do we need to care about thread safety when managing legacyStrings?
2. Does implReplaceAll() need to set legacyChanged = true?
3. How about using prngAlgorithms.iterator().next() below?
1416 return prngAlgorithms.toArray(new String[0])[0];
--Max
> On Jun 6, 2020, at 11:54 AM, Valerie Peng 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
>>>> 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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
I don't know how complex this is. Will it become another map (set) that needs
to be thread-safe and has an order?
If this is not a documented spec and just added for compatibility. How about we
just keep the current design but inside getDefaultSecureRandomAlgorithm() if
firstPrng is not in the map we return another one?
--Max
> On Jun 6, 2020, at 2: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
>>> 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@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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
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@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
RE: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
>-Original Message-
>From: Weijun Wang
>Sent: Friday, June 5, 2020 8:24 PM
>To: Prasadrao Koppula
>Cc: Valerie Peng ; security-dev@openjdk.java.net
>Subject: Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo
>based on registration ordering
>
>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".
I too agree, users may not do this. But technically it causes exception in the
flow.
>
>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.
Yes Max, I'm fine with current approach, if refresh at remove costs performance
hit.
Thanks,
Prasad.K
>
>--Max
>
>> On Jun 5, 2020, at 1:44 PM, Prasadrao Koppula
> 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@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
>
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
> 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@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
RE: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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@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
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
Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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
[15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering
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. Bug: https://bugs.openjdk.java.net/browse/JDK-8246613 Webrev: http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/ Thanks, Valerie
