RFR: 8329251: Print custom truststore/ keystore name [v11]

[Sean Mullan]

On Thu, 7 Nov 2024 04:44:25 GMT, Prasadrao Koppula <pkoppula at openjdk.org> wrote:

>> Using SharedSecrets, I attempted to expose FileInputStream::path information. After implementing the fix, I validated the startup performance tests. Observed no consistent pattern of performance drops or gains, can disregard the occasional performance drop observed in 1 or 2 runs.
>
> Prasadrao Koppula has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 11 commits:
> 
>  - Merge master
>  - initialized storeName with empty string
>  - Replaced Paths.get with Path.of
>  - Removed unnecessary code
>  - Removed unnecessary code
>  - Handled nested wrappers around FileInputStream
>  - Handled BIS case as well
>  - JDK-8329251
>  - JDK-8329251
>  - JDK-8329251
>  - ... and 1 more: https://git.openjdk.org/jdk/compare/f2316f68...c90b4f30

Some initial comments, still reviewing.

src/java.base/share/classes/java/security/KeyStore.java line 216:

> 214:     private static final String KEYSTORE_TYPE = "keystore.type";
> 215: 
> 216:     //The keystore full path

Nit, add space after `//`

src/java.base/share/classes/java/security/KeyStore.java line 217:

> 215: 
> 216:     //The keystore full path
> 217:     private static String keystorePath = null;

No need to initialize to null.

src/java.base/share/classes/java/security/KeyStore.java line 811:

> 809:     }
> 810: 
> 811:     // Set up JavaIOInputStreamAccess in SharedSecrets

s/JavaIOInputStreamAccess/JavaSecurityKeyStoreAccess/

src/java.base/share/classes/java/security/KeyStore.java line 1515:

> 1513:         throws IOException, NoSuchAlgorithmException, CertificateException
> 1514:     {
> 1515:         if (kdebug != null) {

See my other comment in `TrustStoreManagerFactory`.

src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 1:

> 1: /*

Update copyright date.

src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 283:

> 281:         if (ks != null && SSLLogger.isOn && SSLLogger.isOn("trustmanager")) {
> 282:             String keystorePath = SharedSecrets
> 283:                     .getJavaSecurityKeyStoreAccess()

This code only works if `java.security.debug=keystore` is also enabled, otherwise it will always return `null`. I think that dependency is not intuitive, and will be hard for users to remember. I think you should change `KeyStore.java` to always cache the filename, whether debug is on or not.

src/java.base/share/classes/javax/net/ssl/TrustManagerFactory.java line 286:

> 284:                     .getPath(ks);
> 285:             if (keystorePath != null) {
> 286:                 SSLLogger.fine(provider.getName() + ": using \"" + Path.of(

Suggest rewording as "Initializing with keystore: keystore.p12 in PKCS12 format from SUN provider"

Do you really need to log the provider name?

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

> 1960:         macAlgorithm = null;
> 1961:         macIterationCount = -1;
> 1962:         String storeName = "";

I think it is cleaner to not initialize this (leave it as null) and check for null inside debug statements. Same comment in `PKCS12KeyStore.java`.

src/java.base/share/classes/sun/security/provider/JavaKeyStore.java line 1:

> 1: /*

Update copyright date.

src/java.base/share/classes/sun/security/ssl/TrustStoreManager.java line 1:

> 1: /*

Update copyright date.

src/java.base/share/classes/sun/security/util/KeyStoreDelegator.java line 1:

> 1: /*

Update copyright date.

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

PR Review: https://git.openjdk.org/jdk/pull/20414#pullrequestreview-2421303647
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833180471
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833180807
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833192049
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833192647
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833125524
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833191501
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833129736
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833138466
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1833135436
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1832866804
PR Review Comment: https://git.openjdk.org/jdk/pull/20414#discussion_r1832873896