JDK-8290532: Adjust PKCS11Exception and handle more PKCS11 error codes

[MBaesken]

The issue https://bugs.openjdk.org/browse/JDK-8282538 gave an example of the following PKCS11 exception (see attached jtr files of that bug) :

.... Caused by: sun.security.pkcs11.wrapper.PKCS11Exception: 0xCE534351

Unfortunately the error code 0xCE534351 is currently not in the RV/errorMap table of PKCS11Exception, That's why we get this
hex code and no more descriptive output, this could be improved.

---

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-8290532: Adjust PKCS11Exception and handle more PKCS11 error codes

Reviewers

• Valerie Peng (@valeriepeng - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9555

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

Using diff file

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

[bridgekeeper]

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

@MBaesken 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]

Webrevs

• 02: Full - Incremental (85e2648d)
• 01: Full - Incremental (f0fc6d73)
• 00: Full (a25101d5)

[MBaesken]

Btw. there are for most (all?) pkcs11 error/return codes good error descriptions available here http://docs.oasis-open.org/pkcs11/pkcs11-base/v3.0/os/pkcs11-base-v3.0-os.html (5.1 Function return values) ; should we maybe extend the exception message with a short text

[valeriepeng]

None of the 3 proposed error codes in this PR is in the standard PKCS#11 header files - two of them are vendor specific. For vendor specific error code, such direct mapping may be incorrect. As for CKR_COPY_PROHIBITED, I can't find any reference in your cited PKCS#11 spec above. Do you have other standard source for it?

[MBaesken]

None of the 3 proposed error codes in this PR is in the standard PKCS#11 header files - two of them are vendor specific. For vendor specific error code, such direct mapping may be incorrect. As for CKR_COPY_PROHIBITED, I can't find any reference in your cited PKCS#11 spec above. Do you have other standard source for it?

Hi Valerie, yes 0xCE534351L and 0xCE534352L are vendor specific but I added them because we got those (well the first one) in our jtreg test (please see JDK-8282538 for a detailled description, we got the same on RHEL9 instead of CentOS). So they are rather common errors on Linux I think and it would be nice to have them.
The CKR_COPY_PROHIBITED seems to be deprecated, see this discussion https://lists.oasis-open.org/archives/pkcs11/201306/msg00073.html ; so maybe we do not need it any more these days.
The older documentation still mentions it http://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/cs01/pkcs11-base-v2.40-cs01.html .

[valeriepeng]

I don't think it's worthwhile to add deprecated error codes. As for vendor specific error codes, I think they belong in a separate collection based on which vendor/PKCS#11 library is used.

[MBaesken]

I don't think it's worthwhile to add deprecated error codes. As for vendor specific error codes, I think they belong in a separate collection based on which vendor/PKCS#11 library is used.

Okay makes sense then we leave the deprecated error code out. Do you have an idea how to easily get the vendor info of the lib ? A separate per vendor table sounds like a good idea.

[valeriepeng]

PKCS11Exception objects are constructed by PKCS#11 JNI code and vendor info is not readily available there. One easy compromise is to keep the hex error code value but append the string form when there is a match., i.e.

+    public static enum RV_VENDOR {
+        // NSS
+        CKR_NSS_CERTDB_FAILED(0xCE534351L),
+        CKR_NSS_KEYDB_FAILED(0xCE534352L);
+
+        private final long value;
+
+        RV_VENDOR(long value) {
+            this.value = value;
+        }
+    };
+
     private static String lookup(long errorCode) {
         for (RV r : RV.values()) {
             if (r.value == errorCode) {
                 return r.name();
             }
         }

-        // for unknown PKCS11 return values, just use hex as its string
-        return "0x" + Functions.toFullHexString((int)errorCode);
+        // for unknown PKCS11 return values, use hex as its string
+        String res = "0x" + Functions.toFullHexString((int)errorCode);
+        // for vendor-defined values, check the enum for vendors and include
+        // potential matches
+        if ((errorCode & 0x80000000L) != 0) {
+            // for unknown PKCS11 return values, just use hex as its string
+            for (RV_VENDOR r : RV_VENDOR.values()) {
+                if (r.value == errorCode) {
+                    res += ("(" + r.name() + ")");
+                    break;
+                }
+            }
+        }
+        return res;

This way, even if the vendor is not NSS, but the original return value is still available for callers.
Just my .02.

[MBaesken]

Hi Valerie that sounds like a good idea, I adjusted the PR .

[valeriepeng]

nit: dup with line 201; can be removed.

[MBaesken]

Hi Valerie I removed the comment line, also removed the '(' ')' at one place where they seem to be not needed.
Regarding "adresses the new output my needs" - ideally I would like to see some error text like the ones we find at http://docs.oasis-open.org/pkcs11/pkcs11-base/v2.40/os/pkcs11-base-v2.40-os.html where the eror codes are explained.
But adding the (I think rather common) vendor errors is an improvement.

[valeriepeng]

For standard errors (except the deprecated one), the output is their name (e.g. CKR_xxx) instead of the value (0xABxxx hex). Vendor error code is vendor specific. It'd be nice if PKCS#11 API would provide an method for retrieving a String info based on error code, then callers won't have to refer to vendor's doc for the meaning.

Changes look good to me. Test result looks clean too.

[valeriepeng]

Hi Valerie that sounds like a good idea, I adjusted the PR .

Have you verified the new output and see if it addresses your need? I will also run the regression tests just to be safe. Thanks~

[openjdk]

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

8290532: Adjust PKCS11Exception and handle more PKCS11 error codes

Reviewed-by: valeriep

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

• dc74ea2: 8291048: x86: compiler/c2/irTests/TestAutoVectorization2DArray.java fails with lower SSE
• c104089: 8290848: LoadLibraryUnload.java still fails with "Too few cleared WeakReferences"
• 9234624: Merge
• 36c00fd: 8291006: java/foreign/TestUnsupportedPlatform fails after JDK-8290455
• 8c9d5ad: 8290455: jck test api/java_lang/foreign/VaList/Empty.html fails on some platforms
• c29242e: 8290460: Alpine: disable some panama tests that rely on std::thread
• 2f3e494: 8290074: Remove implicit arguments for RegisterMap constructor
• e804236: 8291289: gc/TestPLABAdaptToMinTLABSize fails after JDK-8289137
• adaf3b9: 8291056: Remove unused Generation::par_promote*()
• 48b77a6: 8285792: Posix signal handler modification checking issues.
• ... and 100 more: https://git.openjdk.org/jdk/compare/a41b12f430b8d6ebbb634c0a6a077ed13c68bcb7...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.

[MBaesken]

/integrate

[openjdk]

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

• 93f96d8: 8290399: [macos] Aqua LAF does not fire an action event if combo box menu is displayed
• 5d82d67: 8290034: Auto vectorize reverse bit operations.
• 348a052: 8290846: sun/tools/jstatd/JstatdTest* tests should use VM options
• bc6a3c7: 8290739: Simplify storage of dump-time class information
• 16a1275: 8290943: Fix several IR test issues on SVE after JDK-8289801
• c1a3347: 8291358: Fix the "overridding" typo
• 37b08c7: 8237913: G1CollectedHeap::heap_region_containing shouldn't be a template
• dc74ea2: 8291048: x86: compiler/c2/irTests/TestAutoVectorization2DArray.java fails with lower SSE
• c104089: 8290848: LoadLibraryUnload.java still fails with "Too few cleared WeakReferences"
• 9234624: Merge
• ... and 107 more: https://git.openjdk.org/jdk/compare/a41b12f430b8d6ebbb634c0a6a077ed13c68bcb7...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit 07f0612.

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