8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec

[phohensee]

The backport from 11u was not clean because:

1. the change to PKCS11Test.java affects code that does not exist in 8u, so is not included
2. the new test, TestP11KeyFactoryGetRSAKeySpec.java, uses /lib/testlibrary instead of /test/lib

The new test and the modified existing tests pass, and the jdk/test/sun/security/pkcs11 tests pass as well as they did before this change.

---

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-8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec

Reviewers

• Volker Simonis (@simonis - Reviewer) ⚠️ Review applies to d12508f8
• Martin Balao (@martinuy - Reviewer) ⚠️ Review applies to d12508f8
• Andrew John Hughes (@gnu-andrew - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk8u-dev.git pull/302/head:pull/302
$ git checkout pull/302

Update a local copy of the PR:
$ git checkout pull/302
$ git pull https://git.openjdk.org/jdk8u-dev.git pull/302/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 302

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk8u-dev/pull/302.diff

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back phh! 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]

This backport pull request has now been updated with issue from the original commit.

[mlbridge]

Webrevs

• 02: Full - Incremental (5df0d7d0)
• 01: Full - Incremental (d12508f8)
• 00: Full (f10540bc)

[jerboaa]

@martinuy @franferrax Could you please help review this? Thanks!

[simonis]

Looks good except for a small cosmetic issue in p11-nss-sensitive.txt.

[simonis]

These *_SHA3_* entries here and below have been removed from the 11u-dev downport because SHA3 support was only introduced in JDK 16 (see the 11u review thread).

I suggest to remove them here as well to be on par with the 11u downport.

[phohensee]

Done.

[gnu-andrew]

They should be kept. The definitions are added by JDK-8244154, not JDK-8242332. You said the tests passed before they were removed, right?

[phohensee]

I've added them back.

[simonis]

Looks good now. Thanks!

[openjdk]

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

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

8263404: RsaPrivateKeySpec is always recognized as RSAPrivateCrtKeySpec in RSAKeyFactory.engineGetKeySpec

Reviewed-by: simonis, mbalao, andrew

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.

[phohensee]

This is a critical fix for 8u372, so replaced this PR with openjdk/jdk8u#41.

[martinuy]

Hi @phohensee ,

Thanks for contributing this backport. Looks good to me.

[gnu-andrew]

This is a critical fix for 8u372, so replaced this PR with openjdk/jdk8u#41.

8u is frozen as of 2023-03-24. This PR should be used, and then we can consider separately whether this should be included with the security fixes.

[phohensee]

Thanks for the reviews, Volker and Martin. Andrew, I've additionally tagged the issue with jdk8u-fix-request. I'll push this PR as soon as it's approved.

[gnu-andrew]

Backport looks correct, apart from the removal of the SHA3 definitions. Those values are defined in 8u by JDK-8244154, even if they are not used by the PKCS11 provider, so I see no reason to diverge the configuration, unless it causes a test failure.

[martinuy]

I'm okay either way, but I wonder why specifying a list of disabled PKCS #11 mechanisms would be necessary for the test. This is a question for the original fix, though.

[phohensee]

The 8u JDK-8244154 backport https://hg.openjdk.org/jdk8u/jdk8u/hotspot/rev/803cbdf0f152 includes just the readme comment update, while the 11u backport https://hg.openjdk.org/jdk-updates/jdk11u/rev/87e85e5123dd includes much else. 8u and 11u both define the SHA3 mechanisms in share/native/sun/security/pkcs11/wrapper/pkcs11t.h, but the 11u JDK-8263404 backport doesn't include them in p11-nss-sensitive.txt.

In light of the above, shall we stay with 11u backport compatibility or add the SHA3 mechanisms back to p11-nss-sensitive.txt?

[gnu-andrew]

The 8u JDK-8244154 backport https://hg.openjdk.org/jdk8u/jdk8u/hotspot/rev/803cbdf0f152 includes just the readme comment update, while the 11u backport https://hg.openjdk.org/jdk-updates/jdk11u/rev/87e85e5123dd includes much else. 8u and 11u both define the SHA3 mechanisms in share/native/sun/security/pkcs11/wrapper/pkcs11t.h, but the 11u JDK-8263404 backport doesn't include them in p11-nss-sensitive.txt.

In light of the above, shall we stay with 11u backport compatibility or add the SHA3 mechanisms back to p11-nss-sensitive.txt?

There are eight 8u changesets for this change, in light of the forest of repository used at the time and the need to update THIRD_PARTY_README across all of them.

The meat of the change is c52caa9#diff-a616f7793085d46b7c847c0b4a5b81f3f7c7b7d66fd3d7212c1dd3aa49385b00R665 which includes the SHA3 definitions.

I tend towards keeping the trunk version of p11-nss-sensitive.txt because it avoids any surprises in the (unlikely) case that a provider is used with that test that has one of the SHA3 mechanisms enabled. I'm with Martin in that it isn't clear to me why non-RSA mechanisms are being disabled at all to begin with, but I'd prefer to keep the original configuration if it doesn't break the test on 8u.

Incidentally, the @modules jdk.crypto.cryptoki line in TestP11KeyFactoryGetRSAKeySpec.java could go. No-one is ever going to backport the entire module system to 8u. The lack of it is one of the main reasons people stick with 8u.

[phohensee]

I've added the SHA3 mechanisms back to p11-nss-sensitive.txt and removed @modules from TestP11KeyFactoryGetRSAKeySpec.java.

[gnu-andrew]

Thanks Paul. Approved for 8u-dev.

[phohensee]

Thanks, Andrew.

[phohensee]

/integrate

[openjdk]

Going to push as commit 4445e5c.

[openjdk]

@phohensee Pushed as commit 4445e5c.

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