Hi, Max,
Sure, I've reverted to that old code so it should be bullet proof now.
Webrev updated: http://cr.openjdk.java.net/~valeriep/8246613/webrev.04/
Will proceed with integration once pre-integration Mach5 tests finishes.
Thanks for reviewing~
Valerie
On 6/10/2020 11:15 PM, Weijun Wang wrote:
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 <valerie.p...@oracle.com> 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 <valerie.p...@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.p...@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.kopp...@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@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
