8308453: Convert JKS test keystores in test/jdk/javax/net/ssl/etc to PKCS12 #16159

driverkt · 2023-10-11T21:24:30Z

JDK-8308453: JSSE regression test keystore migration from JKS to PKCS12

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8308453: Convert JKS test keystores in test/jdk/javax/net/ssl/etc to PKCS12 (Enhancement - P4)

Reviewers

Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/16159/head:pull/16159
$ git checkout pull/16159

Update a local copy of the PR:
$ git checkout pull/16159
$ git pull https://git.openjdk.org/jdk.git pull/16159/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 16159

View PR using the GUI difftool:
$ git pr show -t 16159

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/16159.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-10-11T21:26:11Z

👋 Welcome back kdriver! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-10-11T21:28:05Z

@driverkt The following label will be automatically applied to this pull request:

security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-10-11T21:32:45Z

Webrevs

mcpowers

Looks good to me.

seanjmullan · 2023-10-12T13:06:13Z

The ones you changed to PKCS12 is fine, but you should also convert the binary keystores in the test/jdk/javax/net/ssl/etc directory. You can use the keytool -importkeystore command to convert JKS to PKCS12, ex:

keytool -importkeystore -srckeystore keystore -srcstoretype JKS -deststoretype PKCS12 -destkeystore keystore.p12

driverkt · 2023-10-12T15:55:51Z

The ones you changed to PKCS12 is fine, but you should also convert the binary keystores in the test/jdk/javax/net/ssl/etc directory.

I have done so now. Please re-review.

seanjmullan · 2023-10-13T12:46:57Z

There are other tests under test/jdk/sun/security/ssl that are instantiating the keystores in test/jdk/javax/net/ssl/etc as "JKS". For example, see test/jdk/javax/net/ssl/SSLSession/SSLCtxAccessToSessCtx.java. Although the compatibility mode by default allows JKS keystores to load PKCS12 keystores (and vice-versa), I think it would be better/cleaner to change all of these tests to load them as "PKCS12".

…harness; set it explicitly

reason

wangweij · 2023-10-27T13:52:10Z

I have 2 comments:

For those system properties. If you don't set them explicitly to pkcs12, will there by any failure? In my understanding, the default type is already pkcs12 now.
For those getInstance("JKS") calls. Is it possible to rewrite them with getInstance(File)?

test/jdk/sun/security/ssl/SSLSocketImpl/NotifyHandshakeTest.java

driverkt · 2023-10-31T20:21:22Z

test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA.java

-        ks.load(new FileInputStream(keyStoreFile), passphrase);
-        ts.load(new FileInputStream(keyStoreFile), passphrase);
+        KeyStore ks = KeyStore.getInstance(new File(keyStoreFile), passphrase);
+        KeyStore ts = KeyStore.getInstance(new File(keyStoreFile), passphrase);


driverkt · 2023-10-31T20:21:33Z

test/jdk/sun/security/ssl/SSLEngineImpl/TestBadDNForPeerCA12.java

-        ks.load(new FileInputStream(keyStoreFile), passphrase);
-        ts.load(new FileInputStream(keyStoreFile), passphrase);
+        KeyStore ks = KeyStore.getInstance(new File(keyStoreFile), passphrase);
+        KeyStore ts = KeyStore.getInstance(new File(keyStoreFile), passphrase);


driverkt · 2023-10-31T20:22:05Z

test/jdk/sun/security/ssl/ServerHandshaker/GetPeerHost.java

@@ -41,6 +41,8 @@ public static void main(String[] argv) throws Exception {
        String testRoot = System.getProperty("test.src", ".");
        System.setProperty("javax.net.ssl.trustStore", testRoot
                            + "/../../../../javax/net/ssl/etc/truststore");
+        System.setProperty("javax.net.ssl.trustStoreType", "PKCS12");


driverkt · 2023-10-31T20:22:14Z

test/jdk/sun/security/ssl/ServerHandshaker/GetPeerHost.java

@@ -41,6 +41,8 @@ public static void main(String[] argv) throws Exception {
        String testRoot = System.getProperty("test.src", ".");
        System.setProperty("javax.net.ssl.trustStore", testRoot
                            + "/../../../../javax/net/ssl/etc/truststore");
+        System.setProperty("javax.net.ssl.trustStoreType", "PKCS12");
+        System.setProperty("javax.net.ssl.trustStorePassword", "passphrase");


wangweij · 2023-10-31T20:36:29Z

test/jdk/javax/net/ssl/SSLSession/ResumeTLS13withSNI.java

@@ -438,10 +438,9 @@ private static void checkResumedClientHelloSNI(ByteBuffer resCliHello)
    private static TrustManagerFactory makeTrustManagerFactory(String tsPath,
            char[] pass) throws GeneralSecurityException, IOException {
        TrustManagerFactory tmf;
-        KeyStore ts = KeyStore.getInstance("JKS");

        try (FileInputStream fsIn = new FileInputStream(tsPath)) {


No need for this FileInputStream.

I left this due to the "try." I can remove if you prefer, but it seemed like it was better to leave in this check for the existence of the file due to the declared exception in the method signature.

wangweij · 2023-10-31T20:36:40Z

test/jdk/javax/net/ssl/SSLSession/ResumeTLS13withSNI.java

@@ -463,10 +462,9 @@ private static TrustManagerFactory makeTrustManagerFactory(String tsPath,
    private static KeyManagerFactory makeKeyManagerFactory(String ksPath,
            char[] pass) throws GeneralSecurityException, IOException {
        KeyManagerFactory kmf;
-        KeyStore ks = KeyStore.getInstance("JKS");

        try (FileInputStream fsIn = new FileInputStream(ksPath)) {


No need for this FileInputStream.

I left this due to the "try." I can remove if you prefer, but it seemed like it was better to leave in this check for the existence of the file due to the declared exception in the method signature.

I see what you mean, but in any of these tests, no matter if a FNFE or an IAE is thrown, it's not handled and the test simply fails. I don't think it's worth checking twice on the existence of the file.

wangweij · 2023-10-31T20:37:02Z

test/jdk/javax/net/ssl/SSLSession/SessionCacheSizeTests.java

@@ -315,9 +315,10 @@ public static void main(String[] args) throws Exception {

        sslctx = SSLContext.getInstance("TLS");
        KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
-        KeyStore ks = KeyStore.getInstance("JKS");
+        KeyStore ks;
        try (FileInputStream fis = new FileInputStream(keyFilename)) {


No need for this FileInputStream.

I left this due to the "try." I can remove if you prefer, but it seemed like it was better to leave in this check for the existence of the file due to the declared exception in the method signature.

wangweij · 2023-10-31T20:38:07Z

test/jdk/sun/security/ssl/SSLSocketImpl/SSLSocketSSLEngineCloseInbound.java

-        KeyStore ks = KeyStore.getInstance("JKS");
-        KeyStore ts = KeyStore.getInstance("JKS");
+        KeyStore ks;
+        KeyStore ts;

        char[] passphrase = "passphrase".toCharArray();

        try (FileInputStream keyFile = new FileInputStream(keyFilename);
                FileInputStream trustFile = new FileInputStream(trustFilename)) {


No need for these FileInputStreams.

I left this due to the "try." I can remove if you prefer, but it seemed like it was better to leave in this check for the existence of the file due to the declared exception in the method signature.

wangweij

Looks good to me.

BTW, have you considered making truststore a password-less pkcs12 file? Then there is no need to provide a password for it. You can create one with -J-Dkeystore.pkcs12.certProtectionAlgorithm=NONE -J-Dkeystore.pkcs12.macAlgorithm=NONE when calling the keytool -importkeystore command.

openjdk · 2023-11-02T20:13:35Z

@driverkt This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8308453: Convert JKS test keystores in test/jdk/javax/net/ssl/etc to PKCS12

Reviewed-by: weijun

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

driverkt · 2023-11-06T15:06:14Z

/integrate

openjdk · 2023-11-06T15:07:06Z

Going to push as commit 1696603.
Since your change was applied there have been 12 commits pushed to the master branch:

b3126b6: 8319455: Test compiler/print/CompileCommandMemLimit.java times out
1c2ea1d: 8319153: Fix: Class is a raw type in ProcessTools
96e6e67: 4365952: Cannot disable JFileChooser
2d4bbf4: 8319465: Typos in javadoc of com.sun.management.OperatingSystemMXBean methods
8fb94fd: 8319379: G1: gc/logging/TestUnifiedLoggingSwitchStress.java crashes after JDK-8318894
b5c863b: 8316533: C2 compilation fails with assert(verify(phase)) failed: missing Value() optimization
377138c: 8318959: C2: define MachNode::fill_new_machnode() statically
c146685: 8319165: hsdis binutils: warns on empty string as option string
01c0d5d: 8319412: RISC-V: Simple fix of indent in c2_MacroAssembler_riscv.hpp
df599db: 8295159: DSO created with -ffast-math breaks Java floating-point arithmetic
... and 2 more: https://git.openjdk.org/jdk/compare/ea6a88a0aa4e8a365a94e71078e67a4452f40945...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-11-06T15:07:13Z

@driverkt Pushed as commit 1696603.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

JDK-8308453: JSSE regression test keystore migration from JKS to PKCS12

9ccd25b

openjdk bot added the rfr Pull request is ready for review label Oct 11, 2023

openjdk bot added the security security-dev@openjdk.org label Oct 11, 2023

mcpowers reviewed Oct 11, 2023

View reviewed changes

JDK-8308453: JSSE regression test keystore migration from JKS to PKCS12

06a30c0

JDK-8308453: JSSE regression test keystore migration from JKS to PKCS12

e118be6

driverkt marked this pull request as draft October 12, 2023 18:01

openjdk bot removed the rfr Pull request is ready for review label Oct 12, 2023

driverkt added 8 commits October 16, 2023 11:54

Merge branch 'master' of github.com:openjdk/jdk into 8308453

78c664f

Merge branch 'master' of github.com:openjdk/jdk into 8308453

ee40174

JDK-8308453: JSSE regression test keystore migration from JKS to PKCS12

767d8bb

wip

a206cce

Merge branch 'master' of github.com:openjdk/jdk into 8308453

5921fb1

Merge branch 'master' of github.com:openjdk/jdk into 8308453

f93140d

password for the keystore wasn't being set automatically by the test …

f519300

…harness; set it explicitly

update keystore/truststore type for more dependent tests

8146a69

driverkt marked this pull request as ready for review October 24, 2023 22:01

openjdk bot added the rfr Pull request is ready for review label Oct 24, 2023

driverkt added 3 commits October 25, 2023 14:13

Merge branch 'master' of github.com:openjdk/jdk into 8308453

e104fb9

additional updates to explicitly use PKCS12

3666328

Security Manager won't let you explicitly set the type here for some

206e5b9

reason

seanjmullan reviewed Oct 27, 2023

View reviewed changes

test/jdk/sun/security/ssl/SSLSocketImpl/NotifyHandshakeTest.java Outdated Show resolved Hide resolved

revert files

d309acd

change getInstance to not require keystore type

253ee1e

driverkt commented Oct 31, 2023

View reviewed changes

fix indenting and wildcard imports

a35fa4c

wangweij reviewed Oct 31, 2023

View reviewed changes

code review comments

59aaba7

driverkt requested review from wangweij and seanjmullan November 2, 2023 18:40

wangweij approved these changes Nov 2, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 2, 2023

Merge branch 'master' of github.com:openjdk/jdk into 8308453

a3856ce

openjdk bot added the integrated Pull request has been integrated label Nov 6, 2023

openjdk bot closed this Nov 6, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 6, 2023

driverkt deleted the 8308453 branch November 6, 2023 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8308453: Convert JKS test keystores in test/jdk/javax/net/ssl/etc to PKCS12 #16159

8308453: Convert JKS test keystores in test/jdk/javax/net/ssl/etc to PKCS12 #16159

driverkt commented Oct 11, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 11, 2023

openjdk bot commented Oct 11, 2023

mlbridge bot commented Oct 11, 2023 •

edited

Loading

mcpowers left a comment

seanjmullan commented Oct 12, 2023 •

edited

Loading

driverkt commented Oct 12, 2023

seanjmullan commented Oct 13, 2023

wangweij commented Oct 27, 2023

driverkt Oct 31, 2023

driverkt Oct 31, 2023

driverkt Oct 31, 2023

driverkt Oct 31, 2023

wangweij Oct 31, 2023

driverkt Nov 1, 2023

wangweij Oct 31, 2023

driverkt Nov 1, 2023

wangweij Nov 1, 2023

wangweij Oct 31, 2023

driverkt Nov 1, 2023

wangweij Oct 31, 2023

driverkt Nov 1, 2023

wangweij left a comment

openjdk bot commented Nov 2, 2023 •

edited

Loading

driverkt commented Nov 6, 2023

openjdk bot commented Nov 6, 2023

openjdk bot commented Nov 6, 2023

8308453: Convert JKS test keystores in test/jdk/javax/net/ssl/etc to PKCS12 #16159

8308453: Convert JKS test keystores in test/jdk/javax/net/ssl/etc to PKCS12 #16159

Conversation

driverkt commented Oct 11, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 11, 2023

openjdk bot commented Oct 11, 2023

mlbridge bot commented Oct 11, 2023 • edited Loading

Webrevs

mcpowers left a comment

Choose a reason for hiding this comment

seanjmullan commented Oct 12, 2023 • edited Loading

driverkt commented Oct 12, 2023

seanjmullan commented Oct 13, 2023

wangweij commented Oct 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij left a comment

Choose a reason for hiding this comment

openjdk bot commented Nov 2, 2023 • edited Loading

driverkt commented Nov 6, 2023

openjdk bot commented Nov 6, 2023

openjdk bot commented Nov 6, 2023

driverkt commented Oct 11, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 11, 2023 •

edited

Loading

seanjmullan commented Oct 12, 2023 •

edited

Loading

openjdk bot commented Nov 2, 2023 •

edited

Loading