RFR: 8327461: KeyStore getEntry is not thread-safe [v6]

[Weijun Wang]

On Fri, 8 Mar 2024 21:15:45 GMT, Hai-May Chao <hchao at openjdk.org> wrote:

>> Change was made to engineGetEntry() in PKCS12KeyStore to extract the key and certificate chain from Entry only once. This is because the entry may get updated between engineGetKey() and engineGetCertificateChain() which causes inconsistent result. A new test was added to assess and manipulate PKCS12KeyStore with read and write operations concurrently from multiple threads. Thanks!
>
> Hai-May Chao has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
> 
>  - Merge
>  - remove unneeded checks in engineGetEntry
>  - Update engineDeleteEntry
>  - Update engineIsKeyEntry and engineIsCertificateEntry
>  - Update bug number in the test
>  - 8327461: KeyStore getEntry is not thread-safe

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 301:

> 299:         Entry entry = entries.get(alias.toLowerCase(Locale.ENGLISH));
> 300:         Key entryKey = internalGetKey(entry, password);
> 301:         return entryKey;

Combine the 2 lines above.

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 480:

> 478:         Entry entry = entries.get(alias.toLowerCase(Locale.ENGLISH));
> 479:         Certificate[] certChain = internalGetCertificateChain(entry);
> 480:         return certChain;

Combine the two lines above.

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1027:

> 1025: 
> 1026:         Entry entry = entries.remove(alias.toLowerCase(Locale.ENGLISH));
> 1027:         if (entry != null) {

No need to check `entry != null`.

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1082:

> 1080:     }
> 1081: 
> 1082:     private boolean internalEngineIsKeyEntry(Entry entry) {

No need to have both `internal` and `Engine` in the method name. Use `internal` only to follow the other method names. Same with `internalEngineIsCertificateEntry` below.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518348960
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518349769
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518350934
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518355653