8312428: PKCS11 tests fail with NSS 3.91 #15217

valeriepeng · 2023-08-10T00:56:56Z

Starting NSS v3.91, SHA-3 support is added for MessageDigest but not for PSS Signature. This breaks existing test assumptions made by PSS regression tests. In addition, the NSS SHA-3 message digests do not support cloning which causes the failure of TestCloning.java.

This PR adds a PSSUtil.java class which provides utility method for detecting/guessing whether a digest algorithm is valid for PSS signature or not.

The changes are verified with NSS v3.46, v3.57 and v3.91 (on local Linux machine).

Thanks in advance for review~

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-8312428: PKCS11 tests fail with NSS 3.91 (Bug - P3)

Reviewers

Sibabrata Sahoo (@sisahoo - Committer) ⚠️ Review applies to ca1a3b92
Rajan Halade (@rhalade - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15217

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-08-10T00:58:19Z

👋 Welcome back valeriep! 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-08-10T01:00:01Z

@valeriepeng 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-08-10T01:19:59Z

Webrevs

sisahoo · 2023-08-10T05:23:38Z

test/jdk/sun/security/pkcs11/PSSUtil.java

+
+    public static boolean isSignatureSupported(Provider p) {
+        try {
+            Signature.getInstance("RSASSA-PSS", p);


It can use the SIGALG constant name.

rhalade · 2023-08-10T17:51:41Z

test/jdk/sun/security/pkcs11/MessageDigest/TestCloning.java

 /*
 * @test
- * @bug 6414899 8242332
+ * @bug 6414899 8242332 8312428


No need to add bug id for test only updates.

test/jdk/sun/security/pkcs11/MessageDigest/TestCloning.java

rhalade · 2023-08-10T17:55:14Z

test/jdk/sun/security/pkcs11/Signature/KeyAndParamCheckForPSS.java

-            sig = Signature.getInstance(SIGALG, p);
-        } catch (NoSuchAlgorithmException e) {
+        if (!PSSUtil.isSignatureSupported(p)) {
            System.out.println("Skip testing RSASSA-PSS" +


Update to throw SkippedException.

test/jdk/sun/security/pkcs11/Signature/KeyAndParamCheckForPSS.java

rhalade · 2023-08-10T17:58:31Z

test/jdk/sun/security/pkcs11/Signature/KeyAndParamCheckForPSS.java

-        sig.initSign(priv);
-        System.out.println("test#1: pass");
+        sig.initVerify(pub);
+        System.out.println("test#2: good params pass");


Either add a test#1 message after initSign test or update this to remove #2

test#1 message matches with test#2 ones. Anyway, I removed the test output messages with your comments.

rhalade · 2023-08-10T18:00:38Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

-            sig = Signature.getInstance(SIGALG, p);
-        } catch (NoSuchAlgorithmException e) {
+        if (!PSSUtil.isSignatureSupported(p)) {
            System.out.println("Skip testing RSASSA-PSS" +


throw SkippedException here.

Ok. I initially thought that the SkippedException only applies to library not found and/or un-configured OS. With this extension of the meaning of SkippedException, many of the existing tests have to be updated to match.

Yes, I did #1 update with JDK-8313206 and plan to do update more scenarios with JDK-8313575

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

test/jdk/sun/security/pkcs11/PSSUtil.java

rhalade · 2023-08-10T18:16:26Z

test/jdk/sun/security/pkcs11/PSSUtil.java

+            String sigAlg = (h.startsWith("SHA3-")?
+                    h : h.replace("-","")) + "withRSASSA-PSS";


Suggested change

String sigAlg = (h.startsWith("SHA3-")?

h : h.replace("-","")) + "withRSASSA-PSS";

String sigAlg = (h.startsWith("SHA3-") ?

h : h.replace("-", "")) + "withRSASSA-PSS";

rhalade · 2023-08-10T18:16:57Z

Can you please also update https://github.com/openjdk/jdk/blob/master/test/jdk/sun/security/pkcs11/PKCS11Test.java#L1002 to use 3.91?

rhalade · 2023-08-10T19:48:05Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

-            sig = Signature.getInstance(SIGALG, p);
-        } catch (NoSuchAlgorithmException e) {
+        if (!PSSUtil.isSignatureSupported(p)) {
            System.out.println("Skip testing RSASSA-PSS" +


Yes, I did #1 update with JDK-8313206 and plan to do update more scenarios with JDK-8313575

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

valeriepeng · 2023-08-10T20:02:41Z

Can you please also update https://github.com/openjdk/jdk/blob/master/test/jdk/sun/security/pkcs11/PKCS11Test.java#L1002 to use 3.91?

Are you suggesting to use 3.91 for all the OS configurations?
Based on my experiment, many of the mach5 linux x64 machines miss the required library for the NSS v3.91. I ended up using an older version of sqlite3 and test the changes locally. Thus, I have not changed the NSS version in the PKCS11Test.java.

rhalade · 2023-08-10T20:05:21Z

Ok, lets address this update in a separate bug after validating all platforms.

valeriepeng · 2023-08-15T00:13:27Z

Ok, lets address this update in a separate bug after validating all platforms.

Sounds good to me.
Do you have any comment that I have yet address?

rhalade · 2023-08-23T18:21:28Z

test/jdk/sun/security/pkcs11/Signature/KeyAndParamCheckForPSS.java

+                // confirmed to be unsupported; skip the rest of the test
+                System.out.println("=> Skip; no PSS support");
+                return;
+            }


else...Should the test be marked as failed?

Sure, should not happen but good to check to be sure.

rhalade · 2023-08-23T18:26:45Z

test/jdk/sun/security/pkcs11/Signature/SignatureTestPSS.java

+                // confirmed to be unsupported; skip the rest of the test
+                System.out.println("    => Skip; no PSS support");
+                return;
            }


Same here, should test fail here?

test/jdk/sun/security/pkcs11/PSSUtil.java

openjdk · 2023-08-29T16:46:14Z

@valeriepeng 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:

8312428: PKCS11 tests fail with NSS 3.91

Reviewed-by: ssahoo, rhalade

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 221 new commits pushed to the master branch:

e22762c: 8314932: G1: Fix -Wconversion warnings for simple cases inside g1 folder
762b652: 8314573: G1: Heap resizing at Remark does not take existing eden regions into account
a4e97aa: 8314753: Remove support for @beaninfo, @todo, @since.unbundled, and @note
5cc64cc: 8309463: IGV: Dynamic graph layout algorithm
1d02507: 8314476: TestJstatdPortAndServer.java failed with "java.rmi.NoSuchObjectException: no such object in table"
93188bd: 8314569: (fs) Improve normalization of UnixPath for input with trailing slashes
8b8182d: 8315087: G1: Use uint for G1 flags indicating percentage
3dc266c: 8315070: RISC-V: Clean up platform dependent inline headers
25f5df2: 8315073: Zero build on macOS fails after JDK-8303852
a03954e: 8309697: [TESTBUG] Remove "@requires vm.flagless" from jtreg vectorization tests
... and 211 more: https://git.openjdk.org/jdk/compare/c307391ab1f071b1473cd5f4c12437b8d5e0ca93...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this 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.

valeriepeng · 2023-08-29T22:07:42Z

/integrate

openjdk · 2023-08-29T22:08:38Z

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

b2728cb: 8189685: need PerfMemory class update and a volatile_static_field support in VMStructs
e22762c: 8314932: G1: Fix -Wconversion warnings for simple cases inside g1 folder
762b652: 8314573: G1: Heap resizing at Remark does not take existing eden regions into account
a4e97aa: 8314753: Remove support for @beaninfo, @todo, @since.unbundled, and @note
5cc64cc: 8309463: IGV: Dynamic graph layout algorithm
1d02507: 8314476: TestJstatdPortAndServer.java failed with "java.rmi.NoSuchObjectException: no such object in table"
93188bd: 8314569: (fs) Improve normalization of UnixPath for input with trailing slashes
8b8182d: 8315087: G1: Use uint for G1 flags indicating percentage
3dc266c: 8315070: RISC-V: Clean up platform dependent inline headers
25f5df2: 8315073: Zero build on macOS fails after JDK-8303852
... and 212 more: https://git.openjdk.org/jdk/compare/c307391ab1f071b1473cd5f4c12437b8d5e0ca93...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-29T22:08:51Z

@valeriepeng Pushed as commit 1c598c2.

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

8312428: PKCS11 tests fail with NSS 3.91

ca1a3b9

openjdk bot added the rfr Pull request is ready for review label Aug 10, 2023

openjdk bot added the security security-dev@openjdk.org label Aug 10, 2023

sisahoo approved these changes Aug 10, 2023

View reviewed changes

rhalade reviewed Aug 10, 2023

View reviewed changes

address review feedbacks

4306fa1

rhalade suggested changes Aug 10, 2023

View reviewed changes

More review feedbacks

499e963

rhalade suggested changes Aug 24, 2023

View reviewed changes

mpdonova mentioned this pull request Aug 25, 2023

8314283: Support for NSS tests on aarch64 platforms #15429

Closed

3 tasks

address more review feedbacks

03a194d

rhalade approved these changes Aug 29, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 29, 2023

openjdk bot added the integrated Pull request has been integrated label Aug 29, 2023

openjdk bot closed this Aug 29, 2023

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

valeriepeng deleted the JDK-8312428 branch August 29, 2023 22:08

		String sigAlg = (h.startsWith("SHA3-")?
		h : h.replace("-","")) + "withRSASSA-PSS";

8312428: PKCS11 tests fail with NSS 3.91 #15217

8312428: PKCS11 tests fail with NSS 3.91 #15217

Uh oh!

Conversation

valeriepeng commented Aug 10, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 10, 2023

Uh oh!

openjdk bot commented Aug 10, 2023

Uh oh!

mlbridge bot commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhalade commented Aug 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

valeriepeng commented Aug 10, 2023

Uh oh!

rhalade commented Aug 10, 2023

Uh oh!

valeriepeng commented Aug 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

openjdk bot commented Aug 29, 2023

Uh oh!

valeriepeng commented Aug 29, 2023

Uh oh!

openjdk bot commented Aug 29, 2023

Uh oh!

openjdk bot commented Aug 29, 2023

Uh oh!

Reviewers

valeriepeng commented Aug 10, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 10, 2023 •

edited

Loading