Re: [EXTERNAL]Re: NullPointer in JceSecurity.getVerificationResult - Affects JDK 11.07, and JDK 12 and later.

[Valerie Peng]

Hi, John,

Thanks for the reply.

Yes, your understanding is correct. I expect the fix for JDK-8246613 
will be backported into other releases.


BTW, if you have spare time for the test provider, it'd help speeding up 
the fix for JDK-8246383. If not, it's ok as well. Just may take longer 
to get to it.


Regards,

Valerie
On 6/10/2020 1:31 PM, John Gray wrote:


Thanks Valerie!

If I understand you correctly, I think you are saying the fix for 
https://bugs.openjdk.java.net/browse/JDK-8246613  will mean the first 
SecureRandom in the provider will be used as the default (which will 
make it equivalent to JDK 11.06 and previous versions).    If that is 
the case, I agree it should mitigate the issue and would be ok with 
lowering the priority of 8246383.


Will this new fix be back-ported into 11 LTS versions as well?

Thanks so much for your amazing fast response.   I have been trying to 
set aside some time to put together a simple sample you can use to 
verify 8246383 (that doesn’t require our full toolkit), but haven’t 
been able to find the time to do that yet.


Cheers,

John Gray

*From:*Valerie Peng [mailto:valerie.p...@oracle.com]
*Sent:* Wednesday, June 10, 2020 4:14 PM
*To:* John Gray ; 
security-dev@openjdk.java.net
*Cc:* Muthu Kannappan ; Raj Arora 

*Subject:* [EXTERNAL]Re: NullPointer in 
JceSecurity.getVerificationResult - Affects JDK 11.07, and JDK 12 and 
later.


*WARNING:* This email originated outside of Entrust Datacard.
*DO NOT CLICK* links or attachments unless you trust the sender and 
know the content is safe.




Hi John,

As you may have noticed, we are progressing a fix for 
https://bugs.openjdk.java.net/browse/JDK-8246613 
 for returning the 
same default SecureRandom algo for 3rd party providers as in 
pre-JDK7092821 releases. Thus, the chance of observing JDK-8246613 
should be lowered significantly. Given this, I plan to lower the 
priority of JDK-8246383 and it may not be fixed in JDK 15 as earlier 
communicated.


If this will be an issue, please let me know.

Thanks,
Valerie

On 6/2/2020 4:37 PM, Valerie Peng wrote:

Thanks for reporting the bug and the detailed analysis.

I have filed https://bugs.openjdk.java.net/browse/JDK-8246383 to
keep track of this. Will aim to fix this for 15 and have it
backported accordingly.

Is it possible to get hold of an test provider to reproduce and
verifying the fix?

Regards,

Valerie

On 6/2/2020 1:18 PM, John Gray wrote:

Hello,

At Entrust Datacard, we produce a Java based toolkit that
contains our own Security Provider.   This toolkit and
provider  has been around for about 19 years.

In JDK version 11.07 (and I also think Java 12 and beyond),
our toolkit reports the following error:

java.lang.RuntimeException:
java.security.NoSuchAlgorithmException: Error constructing
implementation (algorithm: X9_31usingAES256, provider:
Entrust, class:
com.entrust.toolkit.security.crypto.random.X9_31usingAES256)
at

java.base/java.security.SecureRandom.getDefaultPRNG(SecureRandom.java:281)
at
java.base/java.security.SecureRandom.(SecureRandom.java:219)
at
java.base/javax.crypto.JceSecurity.(JceSecurity.java:80)
... 41 more
Caused by: java.security.NoSuchAlgorithmException: Error
constructing implementation (algorithm: X9_31usingAES256,
provider: Entrust, class:
com.entrust.toolkit.security.crypto.random.X9_31usingAES256)
at
java.base/java.security.Provider$Service.newInstance(Provider.java:1825)
at
java.base/sun.security.jca.GetInstance.getInstance(GetInstance.java:236)
at
java.base/sun.security.jca.GetInstance.getInstance(GetInstance.java:164)
at
java.base/java.security.SecureRandom.getInstance(SecureRandom.java:365)
at

java.base/java.security.SecureRandom.getDefaultPRNG(SecureRandom.java:273)
... 43 more
Caused by: java.lang.NullPointerException
at

java.base/javax.crypto.JceSecurity.getVerificationResult(JceSecurity.java:203)
at java.base/javax.crypto.Cipher.getInstance(Cipher.java:690)
at java.base/javax.crypto.Cipher.getInstance(Cipher.java:625)
at

com.entrust.toolkit.security.crypto.random.X9_31usingAES256.initialize(X9_31usingAES256.java:524)
at

com.entrust.toolkit.security.crypto.random.X9_31usingAES256.(X9_31usingAES256.java:102)
at

java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native
Method)
at

java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
at

Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering

[Valerie Peng]

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  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  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. 

RE: [15] RFR JDK-8246031: Hang observed with coherence SSLNIOServer test

[Prasadrao Koppula]

Thanks Xuelei, updated the code accordingly.

Thanks,
Prasad.K

>-Original Message-
>From: Xuelei Fan
>Sent: Thursday, June 11, 2020 9:03 PM
>To: Prasadrao Koppula ; security-
>d...@openjdk.java.net
>Subject: Re: [15] RFR JDK-8246031: Hang observed with coherence
>SSLNIOServer test
>
>On 6/11/2020 1:50 AM, Prasadrao Koppula wrote:
>> http://cr.openjdk.java.net/~pkoppula/8246031/webrev.01/
>I may have 8 more white spaces indent at line 449, and have 446 and 447 in
>one line.
>
>Otherwise, looks good to me.  Need no more review from me.
>
>Thanks,
>Xuelei

Re: [15] RFR JDK-8246031: Hang observed with coherence SSLNIOServer test

[Xuelei Fan]

On 6/11/2020 1:50 AM, Prasadrao Koppula wrote:
http://cr.openjdk.java.net/~pkoppula/8246031/webrev.01/
I may have 8 more white spaces indent at line 449, and have 446 and 447 
in one line.


Otherwise, looks good to me.  Need no more review from me.

Thanks,
Xuelei

Re: Thread leak by LdapLoginModule

[Seán Coffey]

If 8217606 is your issue, then it's fixed in JDK 11.0.8 which is due for 
release in mid July.


regards,
Sean.

On 09/06/2020 22:15, Mkrtchyan, Tigran wrote:

Hi all,

with Java-11 we have notice a thread leak with ldap module.
We use LDAP to authenticate users with username+pasword by
directly calling LdapLoginModule. This was ok with java 7 and
java 8. With java 11 we see threads getting accumulated. here is a
test case that demonstrates it:

```

 private static final String USERNAME_KEY = 
"javax.security.auth.login.name";
 private static final String PASSWORD_KEY = 
"javax.security.auth.login.password";

 String ldapUrl = "ldap://;;
 String peopleOU = "ou= ... o= ... c=...");

String user = ...;
String pass = ...;


 @Test
 public void threadLeakTest() throws AuthenticationException, 
NoSuchPrincipalException, LoginException {

 Map threadsBefore = 
Thread.getAllStackTraces();

 Map  globalLoginOptions = Map.of(
 "userProvider", ldapUrl + "/" + peopleOU,
 "useSSL", "false",
 "userFilter", "(uid={USERNAME})",
 "useFirstPass", "true"
 );

 for (int i = 0; i < 10; i++) {

 Map loginOptions = Map.of(
 USERNAME_KEY, user,
 PASSWORD_KEY, pass.toCharArray());

 Subject subject = new Subject();

 LdapLoginModule loginModule = new LdapLoginModule();
 loginModule.initialize(subject, null, loginOptions, 
globalLoginOptions);
 loginModule.login();
 loginModule.commit();
 loginModule.logout();
 }

 Map threadsAfter = 
Thread.getAllStackTraces();

 assertEquals("Thread leak detected",  threadsBefore.size() + 1, 
threadsAfter.size());
 }

```

The thread count difference is always equals to the number of iterations in the 
loop, e.g. on each call a
thread is created and stays around. Eventually our server crashes with:

[19497.011s][warning][os,thread] Attempt to protect stack guard pages failed 
(0x7fcc4c65c000-0x7fcc4c66).
OpenJDK 64-Bit Server VM warning: INFO: os::commit_memory(0x7fcc4c55b000, 
16384, 0) failed; error='Not enough space' (errno=12)

The issue is not observed with java-14, thus I assume that the fix is related 
to commit

http://hg.openjdk.java.net/jdk/jdk/rev/6717d7e59db4

As java-11 is LTS, what is the procedure to get it fix back-ported?

Regards,
Tigran.

RE: [15] RFR JDK-8246031: Hang observed with coherence SSLNIOServer test

[Prasadrao Koppula]

Thanks for review Xuelei,

Removed handleException changes. Please review the change from latest webrev[1]

>+} else {
>+handleException(iioe);

handleException throws exception for InterruptedIOException without closing the 
connection. A direct conContext.fatal call helps to close the connection. 
Updated the code accordingly.
  
>+}

Thanks,
Prasad.K

[1] http://cr.openjdk.java.net/~pkoppula/8246031/webrev.01/   

>-Original Message-
>From: Xuelei Fan
>Sent: Wednesday, June 10, 2020 10:13 PM
>To: security-dev@openjdk.java.net
>Subject: Re: [15] RFR JDK-8246031: Hang observed with coherence
>SSLNIOServer test
>
>It may be not needed to update the handleException() method.
>
>- 440handleException(iioe);
>+ 440if (resumable) {
>+throw iioe;
>+} else {
>+handleException(iioe);

handleException throws exception for InterruptedIOException without closing the 
connection. A direct conContext.fatal call helps to close the connection. 
Update the code accordingly.
  

>+}
>
>Otherwise, looks good to me.
>
>Xuelei
>
>On 6/4/2020 8:13 AM, Prasadrao Koppula wrote:
>> Hi,
>>
>> Could you please review this patch. For timeout/interrupts, JDK11u+
>> releases, SSLSocket:getSession behavior is different, compare to JDK8u.
>> i.e, connection is in open state for timeout/interrupts exception. For
>> comparability reasons, this fix will close connection for getSession
>> timeout/ interrupts.
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8246031
>>
>> Webrev: http://cr.openjdk.java.net/~pkoppula/8246031/webrev.00/
>>
>> Thanks,
>>
>> Prasad.K
>>

Re: RFR 8244148: keytool -printcert and -printcrl should support the -trustcacerts and -keystore options

[sha . jiang]

Hi Hai-May,

On 2020/6/8 04:01, Hai-May Chao wrote:

Updated webrev -

https://cr.openjdk.java.net/~hchao/8244148/webrev.02/

-- src/java.base/share/classes/sun/security/util/FilePaths.java
Would it be better to use String.join() or even java.nio.file.Path to
build the file path?

-- src/java.base/share/classes/sun/security/util/AnchorCertificates.java
55 File f = new File(FilePaths.cacerts());
...
59 try (FileInputStream fis = new FileInputStream(f)) {
f is used only once, so it may be unnecessary.

56 KeyStore cacerts;
Though it's not in your change, would you mind to declare this variable
in the try block? I just want to narrow the variable scope.

-- test/lib/jdk/test/lib/SecurityTools.java
Could move method createCacerts() to 
test/lib/jdk/test/lib/security/KeyStoreUtils.java?

This class contains a set of methods for creating trust/key stores.
And getCertFromFile() in test/lib/jdk/test/lib/security/CertUtils.java
can load certificate from a file.

289 int pos = 0;
...
294 for (String crt : crts) {
If not using for-each style, pos can be declared in the for statement.

--
test/jdk/sun/security/tools/keytool/fakecacerts/java.base/sun/security/util/FilePaths.java
This patch can be used by other tests, so could you please move it to a
common path? Like 
test/jdk/sun/security/util/module_patch/java.base/sun/security/util/FilePaths.java


32 public static String cacerts() {
33 return "mycacerts";
34 }
Could it be flexible for returning an alternative path?
For example, accept a system property, say test.cacerts, for specifying
an alternative path. mycacerts is the default value of this property.

Best regards,
John Jiang



Thanks,
Hai-May


On Jun 5, 2020, at 11:04 PM, Weijun Wang > wrote:


I still think duplicated commands in TrustedCert.java are useless. 
Line 104 and line 133 are exactly the same, line 109 and line 138 are 
exactly the same, and you haven't made any change to these 2 files in 
between.


Same for line 80 and line 96 of TrustedCRL.java.

Everything else is fine.

Thanks,
Max


On Jun 6, 2020, at 2:25 AM, Hai-May Chao > wrote:


Updated webrev -

https://cr.openjdk.java.net/~hchao/8244148/webrev.01/

Added one command line -importcert in TrustCert.java.
Added createCacerts() in test/lib SecurityTools.java.

Thanks,
Hai-May




On Jun 4, 2020, at 5:57 AM, Weijun Wang  wrote:



On Jun 4, 2020, at 7:29 PM, Hai-May Chao  
wrote:


Hi Max,

On Jun 3, 2020, at 12:59 AM, Weijun Wang  
wrote:


The source change looks fine to me.

In TrustedCert.java:

- You can use FileOutputStream and Files.copy(Path,OutputStream) 
in cat().


This cat() is taken from WealAlg.java.



- There is no need to recreate root.jks and root.pem.


The sequences of the commands used in this test scenario allows me 
to test -printcert for the -trustcacerts and -keytsore options. We 
had discussion offline about it. The test uses trusted 
certificates and checks no warnings on the weak algorithms to 
address the requirement described in the bug. I believe it does 
serve that purpose, and looks legitimate to me. There could be 
different ways of testing a functionality, and please let me know 
if there is a problem with the current approach.


I just meant that the keytool commands generating root.jks and 
root.pem are exactly the same and there is no need to recreate it.




Please also elaborate your comment about no need to recreate 
root.jks and root.pem.




- Why not use -trustcacerts below?

160 kt("-importcert -file server.pem -noprompt", 
"server.jks”);



Because here is to import the server (end-entity) cert, and it 
will not make a difference for the test result whether to use the 
-trustcacerts or not. It’s the ca (intermediate) cert needs to 
have it in this test scenario. I intended to leave it out in #160 
to distinguish between server and ca certs.


OK.

Then how about we add a new command before line 155?

  kt("-importcert -file ca.pem", "ca.jks").shouldNotHaveExitValue(0);

This would prove the "-trustcacerts" on line 155 is really useful.





- It's probably better to add a " " between cmd and options in 
patchcmd(). Same in TrustedCRL.java.


Ok, will change it.



In TrustedCRL.java:

- No need to recreate ks and ca.crl. Just call "-printcrl" with 
different options.


Same reply as above.


Same question as above.





- Why create using MD5withRSA? Do you meant to warn about the 
weak algorithm?


Yes, exactly, and it differentiates from the weak algorithm 
SHA1withRSA used in root CA where no warning will be emitted. 
There is another -gencrl in #119 without using MD5withRSA so I’d 
have two test cases.




Also I would suggest you create a dedicate method (maybe in 
SecurityTools.java) to create your own cacerts. There is no need 
to copy over the system cacerts, just make sure the file is 
created with the JKS storetype. We are 

Re: RFR 8245520: Provide a way to add CA certificate to cacerts during test run

[sha . jiang]

Hi Max,
Thanks for your info!

It looks that patch for java.base:sun.security.util.FilePaths.java is used
for specifying an alternative cacerts file. But my requirement is updating
the existing cacerts with given CAs.

Both of the functions are useful. I'll submit a new webrev after the fix
for JDK-8244148 is pushed. I also have some comments about Hai-May's
webrev.

Best regards,
John Jiang

On 2020/6/11 11:25, Weijun Wang wrote:

Hai-May has a similar function as a by-product in

https://cr.openjdk.java.net/~hchao/8244148/webrev.02/

See FilePaths.java and MyOwnCacerts.java for the detail. Her code change is in 
code review now.

Please coordinate with her on how to get this done nicely.

Thanks,
Max


On Jun 11, 2020, at 9:39 AM, sha.ji...@oracle.com wrote:

Hi,
This patch contains a patched version for class 
sun.security.util.AnchorCertificates.
It allows to add CAs to cacerts at runtime for testing purpose.

Webrev: http://cr.openjdk.java.net/~jjiang/8245520/webrev.00/
Issue: https://bugs.openjdk.java.net/browse/JDK-8245520

Best regards,
John Jiang

Re: [15] RFR JDK-8246613: Choose the default SecureRandom algo based on registration ordering

[Weijun Wang]

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  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  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,