<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Webrev updated:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/8246613/webrev.02/">http://cr.openjdk.java.net/~valeriep/8246613/webrev.02/</a></p>
<p>Besides addressing Max's comments, I also made <span class="new">updateSecureRandomEntries(...)
method private and removed the synchronized keyword as all of
its accesses are synchronized.</span></p>
<span class="new">Thanks,</span><br>
<span class="new">Valerie<br>
</span>
<div class="moz-cite-prefix">On 6/8/2020 2:33 PM, Valerie Peng
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:8b3a106b-b018-41c1-5a49-14cadd5bbae7@oracle.com">Hi Max,
<br>
<br>
Please find comments in line.
<br>
<br>
On 6/8/2020 2:34 AM, Weijun Wang wrote:
<br>
<blockquote type="cite">Looks like this should work, but still
find it complicated.
<br>
<br>
1. Do we need to care about thread safety when managing
legacyStrings?
<br>
</blockquote>
<br>
Right, it's more complicated than I like as well.
<br>
<br>
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.
<br>
<br>
<blockquote type="cite">2. Does implReplaceAll() need to set
legacyChanged = true?
<br>
</blockquote>
Correct, the removal is by accident. Thanks for catching this.
<br>
<blockquote type="cite">3. How about using
prngAlgorithms.iterator().next() below?
<br>
<br>
1416 return prngAlgorithms.toArray(new String[0])[0];
<br>
</blockquote>
<br>
Sure, changed.
<br>
<br>
Valerie
<br>
<br>
<blockquote type="cite">
<br>
--Max
<br>
<br>
<br>
<blockquote type="cite">On Jun 6, 2020, at 11:54 AM, Valerie
Peng <a class="moz-txt-link-rfc2396E" href="mailto:valerie.peng@oracle.com"><valerie.peng@oracle.com></a> wrote:
<br>
<br>
Thanks for reviewing and sharing the feedbacks on webrev.00.
<br>
<br>
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.
<br>
<br>
Webrev update:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/">http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/</a>
<br>
<br>
Thanks,
<br>
Valerie
<br>
On 6/5/2020 11:00 AM, Valerie Peng wrote:
<br>
<blockquote type="cite">Right, I try to keep the impl simple
as I am not aware of any property removal usage.
<br>
<br>
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.
<br>
<br>
Valerie
<br>
<br>
On 6/5/2020 7:54 AM, Weijun Wang wrote:
<br>
<blockquote type="cite">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".
<br>
<br>
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.
<br>
<br>
Unfortunately there is no ConcurrentLinkedHashMap.
<br>
<br>
--Max
<br>
<br>
<blockquote type="cite">On Jun 5, 2020, at 1:44 PM,
Prasadrao Koppula <a class="moz-txt-link-rfc2396E" href="mailto:prasadarao.koppula@oracle.com"><prasadarao.koppula@oracle.com></a>
wrote:
<br>
<br>
Hi,
<br>
<br>
Looks good to me, one question
<br>
<br>
If first registered SecureRandom algo gets removed,
getDefaultSecureRandomAlgorithm return stale data, a
refresh required in remove?
<br>
<br>
Thanks,
<br>
Prasad.K
<br>
<br>
<blockquote type="cite">-----Original Message-----
<br>
From: Valerie Peng
<br>
Sent: Friday, June 5, 2020 2:52 AM
<br>
To: <a class="moz-txt-link-abbreviated" href="mailto:security-dev@openjdk.java.net">security-dev@openjdk.java.net</a>
<br>
Subject: Re: [15] RFR JDK-8246613: Choose the default
SecureRandom algo
<br>
based on registration ordering
<br>
<br>
Hi, Sean,
<br>
<br>
Thanks for the review and feedback. Webrev updated:
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/">http://cr.openjdk.java.net/~valeriep/8246613/webrev.01/</a>
<br>
<br>
getTypeAndAlgorithm(...) was not static due to an
instance variable used by
<br>
debugging output. I have removed it and made both
method static.
<br>
<br>
I will wait for others' review comments also.
<br>
<br>
Thanks,
<br>
Valerie
<br>
On 6/4/2020 2:01 PM, Sean Mullan wrote:
<br>
<blockquote type="cite">On 6/4/20 3:34 PM, Valerie
Peng wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
Could someone help reviewing this fix? This change
keep tracks of the
<br>
first registered SecureRandom algorithm and
returns it upon the
<br>
request of SecureRandom class.
<br>
</blockquote>
This looks good to me. I would recommend that Max or
someone else
<br>
review it as well.
<br>
<br>
<blockquote type="cite">Bug:
<a class="moz-txt-link-freetext" href="https://bugs.openjdk.java.net/browse/JDK-8246613">https://bugs.openjdk.java.net/browse/JDK-8246613</a>
<br>
<br>
Webrev:
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/">http://cr.openjdk.java.net/~valeriep/8246613/webrev.00/</a>
<br>
</blockquote>
A couple of minor comments, feel free to fix or
ignore.
<br>
<br>
* SecureRandom.java
<br>
<br>
879 // For SUN provider, we use
<br>
SunEntries.DEFF_SECURE_RANDOM_ALGO
<br>
<br>
Might as well fix the typo while you are in there:
s/DEFF/DEF/
<br>
<br>
* Provider.java
<br>
<br>
1156 private String parseSecureRandomPut(String
name, String
<br>
value) {
<br>
<br>
Could be static if you also make getTypeAndAlgorithm
static, I think.
<br>
<br>
--Sean
<br>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</blockquote>
</body>
</html>