8293345: SunPKCS11 provider checks on PKCS11 Mechanism are problematic #18387

valeriepeng · 2024-03-20T02:44:19Z

Existing legacy mechanism check disables mechanism(s) when the support is partial, e.g. supports decryption but not encryption, or supports verification but not signing. Some mechanisms can be used for both encryption/decryption and sign/verify such as RSA related ones. If the particular mechanism supports sign/verify/decryption but not encryption, it'd be disabled as a result. Fine tune the legacy mechanism check with the service type, i.e. supports encryption for Cipher, sign for Signature, so the mechanism is disabled based on the service type.
For completeness sake, I also added a PKCS11 provider configuration option to control this. If not set, SunPKCS11 provider will disable legacy mechanisms by default.

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
Change requires CSR request JDK-8329300 to be approved

Issues

JDK-8293345: SunPKCS11 provider checks on PKCS11 Mechanism are problematic (Enhancement - P3)
JDK-8329300: SunPKCS11 provider checks on PKCS11 Mechanism are problematic (CSR)

Reviewers

Daniel Jeliński (@djelinski - Reviewer)
Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18387

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-03-20T02:44:52Z

👋 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 · 2024-03-20T02:45:32Z

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

8293345: SunPKCS11 provider checks on PKCS11 Mechanism are problematic

Reviewed-by: djelinski, 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 734 new commits pushed to the master branch:

d47a4e9: 8332008: Enable issuestitle check
0bf7282: 8331231: containers/docker/TestContainerInfo.java fails
ffbdfff: 8331999: BasicDirectoryModel/LoaderThreadCount.java frequently fails on Windows in CI
c7d98df: 8329691: Support nonlikelyScript parent locale inheritance
aa4cddd: 8331950: Remove MemoryPoolMBean/isCollectionUsageThresholdExceeded from ZGC ProblemLists
c4ff58b: 8330146: assert(!_thread->is_in_any_VTMS_transition()) failed
aaa90b3: 8327476: Upgrade JLine to 3.26.1
0a4eeea: 8331033: EA fails with "EA unexpected CallLeaf unsafe_setmemory" after JDK-8329331
ad0b54d: 8314225: SIGSEGV in JavaThread::is_lock_owned
ac86f59: 8330532: Improve line-oriented text parsing in HotSpot
... and 724 more: https://git.openjdk.org/jdk/compare/9bc1b065db238b7c9d0562f9bd55d2f338c6ff3d...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.

openjdk · 2024-03-20T02:45:55Z

@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 · 2024-03-20T02:48:32Z

Webrevs

01: Full - Incremental (1fa05b2f)
00: Full (f0cc775e)

djelinski

LGTM

seanjmullan · 2024-03-26T19:35:29Z

Is "disableLegacy" a standard PKCS11 attribute we are introducing support for? If so, I think a CSR is probably needed as it is kind of like a system property.

valeriepeng · 2024-03-26T23:17:53Z

Is "disableLegacy" a standard PKCS11 attribute we are introducing support for? If so, I think a CSR is probably needed as it is kind of like a system property.

It's a configuration file option. Just for the uncommon case if users would like to disable the check entirely. I can file a CSR on this. Thanks for the comment.

valeriepeng · 2024-03-26T23:18:13Z

LGTM

Thanks for the review~

valeriepeng · 2024-04-03T03:41:42Z

/csr

openjdk · 2024-04-03T03:42:36Z

@valeriepeng an approved CSR request is already required for this pull request.

mcpowers

What about testing?

mcpowers · 2024-04-09T18:34:52Z

src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/SunPKCS11.java

+                    if (!allowLegacy && mechInfo != null) {
+                        if ((d.type == CIP &&
+                                (mechInfo.flags & CKF_ENCRYPT) == 0) ||
+                                (d.type == SIG &&
+                                (mechInfo.flags & CKF_SIGN) == 0)) {
+                            if (showInfo) {
+                                System.out.println("DISABLED " +  d.type +
+                                        " " + d.algorithm +
+                                        " due to partial support");
+                            }
+                            continue;
+                        }
+                    }


I'm curious why this check was moved down in the file. The only advantage I see is that you have access to d.type.

Yes, this is the purpose, e.g. perform the check based on the type of registered service. The earlier check does not take into account of the registered service and disables a mechanism completely even when it can still be used for some.
For example, a mechanism supports decryption, signing, and verification will be disabled completely with the earlier check, but will still be enabled for Signature with this proposed change.

valeriepeng · 2024-04-09T22:15:23Z

What about testing?

I tested the change manually by changing some behavior to simulate the case. However, this cannot be done by regression test since NSS does not have legacy mechanisms.

dems54 · 2024-04-15T13:50:40Z

@valeriepeng can this feature be backported to Java 17 ?

valeriepeng · 2024-04-16T16:54:30Z

@valeriepeng can this feature be backported to Java 17 ?

I suppose so, the PKCS11 configuration attribute part which requires CSR can be separated out if needed.

valeriepeng · 2024-04-16T22:30:57Z

@mcpowers I am about to leave for vacation. Will wait for your review and resume on this PR after I return. Thanks!

mcpowers · 2024-05-09T21:43:58Z

Your changes look fine to me.

valeriepeng · 2024-05-10T16:53:05Z

/integrate

openjdk · 2024-05-10T16:53:28Z

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

1c5f150: 8331734: Atomic MemorySegment VarHandle operations fails for element layouts
65abf24: 8331866: Add warnings for locale data dependence
d215bc4: 8332066: AArch64: Math test failures since JDK-8331558
d11e70a: 8331646: Add specific regression leap year tests
f95c937: 8331577: RISC-V: C2 CountLeadingZerosV
675fbe6: 8331993: Add counting leading/trailing zero tests for Integer
242446b: 8331931: JFR: Avoid loading regex classes during startup
45792c5: 8331352: error: template-id not allowed for constructor/destructor in C++20
1547a69: 8327696: [TESTBUG] "javax/swing/JTable/KeyBoardNavigation/KeyBoardNavigation.java" test instruction needs to be corrected
784b8fc: 8331744: java.lang.classfile.TypeKind improvements
... and 744 more: https://git.openjdk.org/jdk/compare/9bc1b065db238b7c9d0562f9bd55d2f338c6ff3d...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-05-10T16:53:34Z

@valeriepeng Pushed as commit 1b476f5.

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

dems54 · 2024-05-10T20:55:41Z

Many thanks @valeriepeng for this feature.
Possible to backport on JDK 17 or 21 ?

dems54 · 2024-05-29T08:21:57Z

UP @valeriepeng possible to backport PKCS11 configuration attribute part on JDK 17 or 21 ?

valeriepeng · 2024-06-14T18:56:12Z

UP @valeriepeng possible to backport PKCS11 configuration attribute part on JDK 17 or 21 ?

I am not sure, let me check and get back to you.

valeriepeng · 2024-06-27T21:32:53Z

UP @valeriepeng possible to backport PKCS11 configuration attribute part on JDK 17 or 21 ?

Based on the feedback that I got, it should be ok to backport the PKCS11 configuration attribute, separate CSRs would be needed for that though.

dems54 · 2024-07-24T09:59:56Z

Many thanks @valeriepeng !
When could the backport to JDK17 and 21 be done ?
We are very excited to be able to update our clients with a maintained java version

valeriepeng · 2024-07-29T18:48:22Z

Many thanks @valeriepeng ! When could the backport to JDK17 and 21 be done ? We are very excited to be able to update our clients with a maintained java version

This would be subject to the 21u and 17u Maintainer. I've opened the backport request against 17-pool and 21-pool but someone needs to file the CSR and do the actual backport.

8293345: SunPKCS11 provider checks on PKCS11 Mechanism are problematic

f0cc775

openjdk bot added the rfr Pull request is ready for review label Mar 20, 2024

openjdk bot added the security security-dev@openjdk.org label Mar 20, 2024

djelinski approved these changes Mar 26, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 26, 2024

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Mar 29, 2024

Update to match CSR for disableLegacy -> allowLegacy name change

1fa05b2

djelinski approved these changes Apr 9, 2024

View reviewed changes

mcpowers reviewed Apr 9, 2024

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Apr 16, 2024

wangweij approved these changes May 1, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 10, 2024

openjdk bot closed this May 10, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 10, 2024

valeriepeng deleted the JDK-8293345 branch May 10, 2024 16:53

8293345: SunPKCS11 provider checks on PKCS11 Mechanism are problematic #18387

8293345: SunPKCS11 provider checks on PKCS11 Mechanism are problematic #18387

Uh oh!

Conversation

valeriepeng commented Mar 20, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Mar 20, 2024

Uh oh!

openjdk bot commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Mar 20, 2024

Uh oh!

mlbridge bot commented Mar 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

djelinski left a comment

Choose a reason for hiding this comment

Uh oh!

seanjmullan commented Mar 26, 2024

Uh oh!

valeriepeng commented Mar 26, 2024

Uh oh!

valeriepeng commented Mar 26, 2024

Uh oh!

valeriepeng commented Apr 3, 2024

Uh oh!

openjdk bot commented Apr 3, 2024

Uh oh!

mcpowers left a comment

Choose a reason for hiding this comment

Uh oh!

mcpowers Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

valeriepeng Apr 9, 2024

Choose a reason for hiding this comment

Uh oh!

valeriepeng commented Apr 9, 2024

Uh oh!

dems54 commented Apr 15, 2024

Uh oh!

valeriepeng commented Apr 16, 2024

Uh oh!

valeriepeng commented Apr 16, 2024

Uh oh!

mcpowers commented May 9, 2024

Uh oh!

valeriepeng commented May 10, 2024

Uh oh!

openjdk bot commented May 10, 2024

Uh oh!

openjdk bot commented May 10, 2024

Uh oh!

dems54 commented May 10, 2024

Uh oh!

dems54 commented May 29, 2024

Uh oh!

valeriepeng commented Jun 14, 2024

Uh oh!

valeriepeng commented Jun 27, 2024

Uh oh!

dems54 commented Jul 24, 2024

Uh oh!

valeriepeng commented Jul 29, 2024

Uh oh!

Uh oh!

valeriepeng commented Mar 20, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Mar 20, 2024 •

edited

Loading

mlbridge bot commented Mar 20, 2024 •

edited

Loading