8360463: Ambiguity in Cipher.getInstance() specification between NoSuchAlgorithmException and NoSuchPaddingException #26489

valeriepeng · 2025-07-25T23:39:54Z

This PR is for clarifying the NoSuchAlgorithmException and NoSuchPaddingException for the Cipher.getInstance(String transformation, Provider provider) and Cipher.getInstance(String transformation, String provider) methods.

As stated in javax.crypto.CipherSpi class, provider has the flexibility to register their implementations through various sub-transformations. As a result, depending on how the providers register the implementation, it may lead to NoSuchAlgorithmException or NoSuchPaddingException. For example, the provider A registers to support "AES/CBC/PKCS5Padding" vs provider B registers to support "AES" (but would only accept "CBC" and "PKCS5Padding" as the valid input for setting mode and padding). Calling Cipher.getInstance(...) with "AES/CBC/NoPadding" against provider A and B would lead to NoSuchAlgorithmException and NoSuchPaddingException. This javadoc update hope to make it clear.

Thanks in advance for the review~
Valerie

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-8364134 to be approved

Issues

JDK-8360463: Ambiguity in Cipher.getInstance() specification between NoSuchAlgorithmException and NoSuchPaddingException (Bug - P3)
JDK-8364134: Ambiguity in Cipher.getInstance() specification between NoSuchAlgorithmException and NoSuchPaddingException (CSR)

Reviewers

Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26489

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…chAlgorithmException and NoSuchPaddingException

bridgekeeper · 2025-07-25T23:40:56Z

👋 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 · 2025-07-25T23:41:59Z

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

8360463: Ambiguity in Cipher.getInstance() specification between NoSuchAlgorithmException and NoSuchPaddingException

Reviewed-by: mullan

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

82bdef1: 8367913: LIBDL dependency seems to be not needed for some jdk libs
4df04a2: 8366809: JFR: Use factory for aggregator functions
fd30ae9: 8350550: Preload classes from AOT cache during VM bootstrap
... and 791 more: https://git.openjdk.org/jdk/compare/e756c0dbbb7d99df0751d71726b173e4eabcc903...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 · 2025-07-25T23:42:30Z

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

valeriepeng · 2025-07-25T23:43:34Z

/csr

openjdk · 2025-07-25T23:43:48Z

@valeriepeng has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@valeriepeng please create a CSR request for issue JDK-8360463 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2025-07-25T23:48:33Z

Webrevs

haimaychao · 2025-07-31T03:45:28Z

Changes look good with minor suggestions.

haimaychao · 2025-07-31T03:44:08Z

src/java.base/share/classes/javax/crypto/Cipher.java

     *
-     * @throws NoSuchPaddingException if {@code transformation}
-     *         contains a padding scheme that is not available
+     * @throws NoSuchPaddingException if a {@code CipherSpi} object


suggest to s/{@code CipherSpi} object/{@code CipherSpi} implementation, consistent with other places.

Sure, will change~

haimaychao · 2025-07-31T03:44:27Z

src/java.base/share/classes/javax/crypto/Cipher.java

-     * @throws NoSuchPaddingException if {@code transformation}
-     *         contains a padding scheme that is not available
+     * @throws NoSuchPaddingException if a {@code CipherSpi} object
+     *         from the {@code provider} is found using the algorithm


Suggest to s/from the/from the specified, and to make the same changes in this method at lines #567 and #568, s/the specified provider/the specified {@code provider}.

Sure, will change~

haimaychao · 2025-07-31T03:44:34Z

src/java.base/share/classes/javax/crypto/Cipher.java

     *
-     * @throws NoSuchPaddingException if {@code transformation}
-     *         contains a padding scheme that is not available
+     * @throws NoSuchPaddingException if the {@code CipherSpi} object


same suggestion as above.

haimaychao · 2025-07-31T03:44:37Z

src/java.base/share/classes/javax/crypto/Cipher.java

-     * @throws NoSuchPaddingException if {@code transformation}
-     *         contains a padding scheme that is not available
+     * @throws NoSuchPaddingException if the {@code CipherSpi} object
+     *         from the {@code provider} is found using the algorithm


same suggestion as above.

wangweij · 2025-08-13T00:09:42Z

Is it worth demonstrating the different behaviors with a test?

valeriepeng · 2025-08-13T00:21:21Z

Is it worth demonstrating the different behaviors with a test?

This is covered by existing TCK test and thus the request for clarification. This is provider-specific, i.e. depending on how provider registers its implementation, it may leads to different exceptions. Given that the difference is only the type of exception, e.g. NoSuchAlgorithmException vs NoSuchPaddingException, I don't feel it's that critical to add a regression test in addition to the existing TCK test.

wangweij · 2025-08-19T23:59:09Z

I wonder if we should list NSPE before NSAE, since NSAE refers to it.

Or, can we just say NSAE is thrown when the specified algorithm and mode are not supported?

wangweij · 2025-08-20T00:00:51Z

Also, is this consistent among other security providers?

valeriepeng · 2025-08-20T05:26:01Z

I wonder if we should list NSPE before NSAE, since NSAE refers to it.

Or, can we just say NSAE is thrown when the specified algorithm and mode are not supported?

I see your point to list NSPE before NSAE. However, it seems that the exceptions are currently ordered alphabetically... Given the NSAE under getInstance(String) does not refer to NSPE, I am inclined to keep the current ordering, otherwise it looks inconsistent?

As for the NSAE vs NSPE, it's somewhat complicated due to the various provider registrations documented in the javax.crypto.CipherSpi class. There could be situations which NSAE is thrown besides the unsupported algorithm and/or mode. E.g. provider supports the algorithm and mode and a different padding scheme and registers the support through full transformation instead of only the algorithm and mode. The former case leads to NSAE vs the other one leads to NSPE.

valeriepeng · 2025-08-20T05:31:13Z

Also, is this consistent among other security providers?

Whether it's NSAE or NSPE depends on how security provider registers the support. The updated wording in the PR reflects the current JCA behavior, i.e. Cipher lookup, and should be consistent across providers.

in NoSuchAlgorithmException

wangweij · 2025-08-21T20:38:22Z

src/java.base/share/classes/javax/crypto/Cipher.java

-     *         contains a padding scheme that is not available
+     * @throws NoSuchPaddingException if a {@code CipherSpi} implementation
+     *         from the specified {@code provider} is found but it does not
+     *         support the padding scheme


The wording is correct now. Small nit: the NSAE one uses "but does not" and the NSPE one uses "but it does not". You might want to make them the same.

Yes, will do.

Krushna948 · 2025-08-22T05:30:43Z

src/java.base/share/classes/javax/crypto/Cipher.java

-     *         or if a {@code CipherSpi} implementation for the
-     *         specified algorithm is not available from the specified
-     *         provider
+     *         or if a {@code CipherSpi} implementation from the specified


IMO,
NoSuchAlgorithmException - if transformation is null, empty, in an invalid format, or if a CipherSpi implementation for the specified transformation is not available from the specified provider

The transformation can be linked to the class level mention

A transformation is of the form:
"algorithm/mode/padding" or
"algorithm"

It's important to point out that when a matched (either full or partial) transform is found, not able to set mode throws an NSAE, while not able to set padding throws an NSPE. So NSAE is related on mode availability and NSPE to padding availability. This is why the current PR need to mention mode here.

Thanks for clarifying this.

to match the javadoc

seanjmullan · 2025-08-28T12:52:37Z

test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/unittest/ChaCha20CipherUnitTest.java

                System.out.println("Expected transformation: " + transformation);
            }
-        } catch (NoSuchAlgorithmException e) {
+        } catch (NoSuchAlgorithmException | NoSuchPaddingException e) {


I think it would be good to enhance this test to distinguish between the expected exceptions for each transformation.

Well, which exception is thrown depends on which provider is used and how it registers its implementations. Pinpointing the exact Exception would require running the test against a specific provider which we know how the implementations are registered. Without tying the test to a specific provider, existing transformations in this test may lead to either NSAE or NSPE, thus I just add NSPE to the catch clause. I can add more transformations which would lead to only NSAE assuming this is what you want to see?

I think it is ok to assume the JDK provider configuration has not been changed.

Wouldn't "ChaCha20/None/PKCS5Padding" and ""ChaCha20-Poly1305/None/PKCS5Padding"" always throw NoSuchPaddingException, assuming the current JDK configuration is not changed? Maybe instead of passing a boolean expected parameter, you could pass the expected Exception, or null if none expected.

Yes, I have adopted your suggestion.

seanjmullan · 2025-08-28T13:02:51Z

src/java.base/share/classes/javax/crypto/Cipher.java

-            ("Cannot find any provider supporting " + transformation, failure);
+        if (failure instanceof NoSuchPaddingException nspe) {
+            throw nspe;
+        } else {


Small nit, you don't need the else block here.

seanjmullan · 2025-09-04T13:59:28Z

test/jdk/com/sun/crypto/provider/Cipher/ChaCha20/unittest/ChaCha20CipherUnitTest.java

-        checkTransformation("ChaCha20-Poly1305/ECB/NoPadding", false);
-        checkTransformation("ChaCha20-Poly1305/None/PKCS5Padding", false);
+        checkTransformation("ChaCha20/ECB/NoPadding", Expected.NSAE_OR_NSPE);
+        checkTransformation("ChaCha20/None/PKCS5Padding", Expected.NSAE_OR_NSPE);


Isn't this always going to throw NSPE given the current provider configuration? Also, same comment on line 88.

I can explicit specify SunJCE provider in the getInstance(...) call and then match the specific exception based on its impl.

exception per Sean's suggestion

valeriepeng · 2025-09-18T01:29:28Z

Release Note is filed @ https://bugs.openjdk.org/browse/JDK-8367577
Thanks in advance for review~

seanjmullan · 2025-09-18T19:33:27Z

Release Note is filed @ https://bugs.openjdk.org/browse/JDK-8367577 Thanks in advance for review~

Made a couple of edits but otherwise looks fine. Also, you can use markdown instead of html, if you like.

wangweij · 2025-09-18T19:59:36Z

Release Note is filed @ https://bugs.openjdk.org/browse/JDK-8367577 Thanks in advance for review~

Made a couple of edits but otherwise looks fine. Also, you can use markdown instead of html, if you like.

Is it common to use italics instead of fixed width for exception types?

wangweij · 2025-09-18T20:20:25Z

src/java.base/share/classes/javax/crypto/Cipher.java

     * transformation
     *
     * @throws NoSuchAlgorithmException if {@code transformation}
     *         is {@code null}, empty, in an invalid format,


Not related to this change, but do we need an "or" before "in an valid format"?

Hmm, how about this?

@throws NoSuchAlgorithmException if {@code transformation} is {@code null}, empty or in an invalid format, or if a {@code CipherSpi} implementation is not found or is found but does not support the mode

A semi-colon (instead of a comma) after "format" might also help separate the 2 main error conditions:

@throws NoSuchAlgorithmException if {@code transformation} is {@code null}, empty or in an invalid format; or if a {@code CipherSpi} implementation is not found or is found but does not support the mode

Sure, will use this. Thanks for the suggestions @wangweij @seanjmullan

Now that we are using semi-colon, do we dare add back the commas?

Don't think it makes a significant difference.

valeriepeng · 2025-09-25T18:10:19Z

/integrate

openjdk · 2025-09-25T18:11:02Z

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

32ab0db: 8368674: Incremental builds keep rebuilding interim jmod
de1f5a3: 8368468: Split out everything but configure results from spec.gmk
7412219: 8368261: Serial: Use more precise nmethod scope during Full GC marking
... and 844 more: https://git.openjdk.org/jdk/compare/e756c0dbbb7d99df0751d71726b173e4eabcc903...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-09-25T18:11:12Z

@valeriepeng Pushed as commit 5c596e2.

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

8360463: Ambiguity in Cipher.getInstance() specification between NoSu…

2bedc41

…chAlgorithmException and NoSuchPaddingException

openjdk bot added the rfr Pull request is ready for review label Jul 25, 2025

openjdk bot added the security security-dev@openjdk.org label Jul 25, 2025

openjdk bot added the csr Pull request needs approved CSR before integration label Jul 25, 2025

haimaychao reviewed Jul 31, 2025

View reviewed changes

valeriepeng added 2 commits August 12, 2025 23:59

Address review comments from Haimay.

aa99296

Added a missing "specified" for consistency.

92ae5bc

Revised the wording again to avoid referring to NoSuchPaddingException

6f19aed

in NoSuchAlgorithmException

wangweij reviewed Aug 21, 2025

View reviewed changes

Update to address Weijun's comment.

e057f88

Krushna948 reviewed Aug 22, 2025

View reviewed changes

Applied javadoc changes to getInstance(String) method and made the impl

9b2a09f

to match the javadoc

seanjmullan reviewed Aug 28, 2025

View reviewed changes

Address review comments from Sean.

4d4638c

seanjmullan reviewed Sep 4, 2025

View reviewed changes

openjdk bot removed the csr Pull request needs approved CSR before integration label Sep 15, 2025

updated the checkTransformation() method w/ specific provider and

0ce7a49

exception per Sean's suggestion

seanjmullan approved these changes Sep 18, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 18, 2025

wangweij reviewed Sep 18, 2025

View reviewed changes

Minor syntax change for the NoSuchAlgorithmException description.

caac725

openjdk bot removed the ready Pull request is ready to be integrated label Sep 22, 2025

seanjmullan approved these changes Sep 23, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 23, 2025

openjdk bot added the integrated Pull request has been integrated label Sep 25, 2025

openjdk bot closed this Sep 25, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 25, 2025

valeriepeng deleted the JDK-8360463 branch September 25, 2025 18:18

8360463: Ambiguity in Cipher.getInstance() specification between NoSuchAlgorithmException and NoSuchPaddingException #26489

8360463: Ambiguity in Cipher.getInstance() specification between NoSuchAlgorithmException and NoSuchPaddingException #26489

Uh oh!

Conversation

valeriepeng commented Jul 25, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Jul 25, 2025

Uh oh!

openjdk bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valeriepeng commented Jul 25, 2025

Uh oh!

openjdk bot commented Jul 25, 2025

Uh oh!

mlbridge bot commented Jul 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

haimaychao commented Jul 31, 2025

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!

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!

wangweij commented Aug 13, 2025

Uh oh!

valeriepeng commented Aug 13, 2025

Uh oh!

wangweij commented Aug 19, 2025

Uh oh!

wangweij commented Aug 20, 2025

Uh oh!

valeriepeng commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

valeriepeng commented Aug 20, 2025

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!

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!

seanjmullan Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

valeriepeng commented Jul 25, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jul 25, 2025 •

edited

Loading

openjdk bot commented Jul 25, 2025 •

edited

Loading

mlbridge bot commented Jul 25, 2025 •

edited

Loading

valeriepeng commented Aug 20, 2025 •

edited

Loading

seanjmullan Sep 4, 2025 •

edited

Loading

seanjmullan Sep 18, 2025 •

edited

Loading