Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8236671: NullPointerException in JKS keystore #3588

Closed
wants to merge 4 commits into from

Conversation

@coffeys
Copy link
Contributor

@coffeys coffeys commented Apr 20, 2021

Trivial enough change. Improved the exception thrown from JceKeyStore also.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3588/head:pull/3588
$ git checkout pull/3588

Update a local copy of the PR:
$ git checkout pull/3588
$ git pull https://git.openjdk.java.net/jdk pull/3588/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3588

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3588.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 20, 2021

👋 Welcome back coffeys! 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
Copy link

@openjdk openjdk bot commented Apr 20, 2021

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

@openjdk openjdk bot added the security label Apr 20, 2021
@openjdk openjdk bot added the rfr label Apr 20, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 20, 2021

Webrevs

@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Apr 20, 2021

It looks like a public behavior change to me. Did you want to file a CSR and update the specification (KeyStore) as well? I think it would be nice if we could keep use the old exception, IllegalArgumentException, as described in the bug.

@coffeys
Copy link
Contributor Author

@coffeys coffeys commented Apr 20, 2021

It looks like a public behavior change to me. Did you want to file a CSR and update the specification (KeyStore) as well? I think it would be nice if we could keep use the old exception, IllegalArgumentException, as described in the bug.

@XueleiFan - The spec in question has been broken for almost 3 years with the throwing of NPE.

One issue here is that Sun provider with JKS keystore will throw IllegalArgumentException in older JDK versions but the SunJCE provider and JCEKS keystore throws KeyStoreException when null password is encountered . There's a mismatch. To me, it looks like KeyStoreException is the correct exception in such scenarios (and according to API spec)

I can file a CSR to have the implementation adhere to spec if that's desired.

@XueleiFan
Copy link
Member

@XueleiFan XueleiFan commented Apr 20, 2021

It looks like a public behavior change to me. Did you want to file a CSR and update the specification (KeyStore) as well? I think it would be nice if we could keep use the old exception, IllegalArgumentException, as described in the bug.

@XueleiFan - The spec in question has been broken for almost 3 years with the throwing of NPE.

One issue here is that Sun provider with JKS keystore will throw IllegalArgumentException in older JDK versions but the SunJCE provider and JCEKS keystore throws KeyStoreException when null password is encountered . There's a mismatch. To me, it looks like KeyStoreException is the correct exception in such scenarios (and according to API spec)

I can file a CSR to have the implementation adhere to spec if that's desired.

It makes sense to me. I think it would be good to have this stated in the spec in case more mismatch introduced in the future.

@@ -287,6 +287,9 @@ public void engineSetKeyEntry(String alias, Key key, char[] password,
entry.date = new Date();

// Protect the encoding of the key
if (password == null) {
Copy link
Member

@seanjmullan seanjmullan Apr 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could move this check earlier, before the try block.

@coffeys
Copy link
Contributor Author

@coffeys coffeys commented Apr 28, 2021

/csr

I'll log a CSR to highlight the behavioural difference/correction being made to the SUN JKS type keystore.

@openjdk openjdk bot added the csr label Apr 28, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 28, 2021

@coffeys has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@coffeys please create a CSR request for issue JDK-8236671. This pull request cannot be integrated until the CSR request is approved.

@coffeys
Copy link
Contributor Author

@coffeys coffeys commented Apr 30, 2021

/csr unneeded

@openjdk openjdk bot removed the csr label Apr 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

@coffeys determined that a CSR request is not needed for this pull request.

@coffeys
Copy link
Contributor Author

@coffeys coffeys commented Apr 30, 2021

KeyStore specification will be tightened up via another bug record: https://bugs.openjdk.java.net/browse/JDK-8266351

@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

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

8236671: NullPointerException in JKS keystore

Reviewed-by: hchao, xuelei

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

  • e9370a1: 8265761: Font with missed font family name is not properly printed on Windows
  • 3554dc2: 8264395: WB_EnqueueInitializerForCompilation fails with "method holder must be initialized" when called for uninitialized class
  • 4d77171: 8249903: jdk/javadoc/doclet/testSerializedForm/TestSerializedForm.java needs to be updated after 8146022 got closed
  • 51b2188: 8266267: Remove unnecessary jumps in Intel Math Library StubRoutines
  • 2c381e0: 8262376: ReplaceCriticalClassesForSubgraphs.java fails if --with-build-jdk is used
  • 5ecef01: 8266217: ZGC: Improve the -Xlog:gc+init output for NUMA
  • 5d8c1cc: 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider
  • 46b4a14: 8266315: Problem list failing test java/awt/font/TextLayout/LigatureCaretTest.java
  • 42af7da: 8265933: Move Java monitor related fields from class Thread to JavaThread
  • 1afbab6: 8263998: Remove mentions of mc region in comments
  • ... and 139 more: https://git.openjdk.java.net/jdk/compare/891f72fe6ea545cdf845d26157a64315a93f9a06...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 openjdk bot added the ready label Apr 30, 2021
@coffeys
Copy link
Contributor Author

@coffeys coffeys commented Apr 30, 2021

/integrate

@openjdk openjdk bot closed this Apr 30, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Apr 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Apr 30, 2021

@coffeys Since your change was applied there have been 149 commits pushed to the master branch:

  • e9370a1: 8265761: Font with missed font family name is not properly printed on Windows
  • 3554dc2: 8264395: WB_EnqueueInitializerForCompilation fails with "method holder must be initialized" when called for uninitialized class
  • 4d77171: 8249903: jdk/javadoc/doclet/testSerializedForm/TestSerializedForm.java needs to be updated after 8146022 got closed
  • 51b2188: 8266267: Remove unnecessary jumps in Intel Math Library StubRoutines
  • 2c381e0: 8262376: ReplaceCriticalClassesForSubgraphs.java fails if --with-build-jdk is used
  • 5ecef01: 8266217: ZGC: Improve the -Xlog:gc+init output for NUMA
  • 5d8c1cc: 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider
  • 46b4a14: 8266315: Problem list failing test java/awt/font/TextLayout/LigatureCaretTest.java
  • 42af7da: 8265933: Move Java monitor related fields from class Thread to JavaThread
  • 1afbab6: 8263998: Remove mentions of mc region in comments
  • ... and 139 more: https://git.openjdk.java.net/jdk/compare/891f72fe6ea545cdf845d26157a64315a93f9a06...master

Your commit was automatically rebased without conflicts.

Pushed as commit 276a1bf.

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 30, 2021

Mailing list message from Will Sargent on security-dev:

KeyStore specification will be tightened up via another bug record

This would be super helpful, as one thing that confuses me is what the
relationship is between a key entry and a key alias -- in particular, the
existence alias doesn't seem to guarantee a valid entry that can be
retrieved.

In JDK 11 it's possible to create a private key with a keystore using pkcs12
.setKeyEntry() (see link below):

https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L135

and then have a null pointer exception when retrieving the entry from the
alias because the certificate chain is null (see commented out "testSystem"
use case):

https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L27

I can write this up into a formal bug if that helps.

On Fri, Apr 30, 2021 at 2:30 AM Sean Coffey <coffeys at openjdk.java.net>
wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210430/0bea5475/attachment.htm>

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 30, 2021

Mailing list message from Will Sargent on security-dev:

KeyStore specification will be tightened up via another bug record

This would be super helpful, as one thing that confuses me is what the
relationship is between a key entry and a key alias -- in particular, the
existence alias doesn't seem to guarantee a valid entry that can be
retrieved.

In JDK 11 it's possible to create a private key with a keystore using pkcs12
.setKeyEntry() (see link below):

https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L135

and then have a null pointer exception when retrieving the entry from the
alias because the certificate chain is null (see commented out "testSystem"
use case):

https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L27

I can write this up into a formal bug if that helps.

On Fri, Apr 30, 2021 at 2:30 AM Sean Coffey <coffeys at openjdk.java.net>
wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210430/0bea5475/attachment.htm>

@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 30, 2021

Mailing list message from Se=c3=a1n Coffey on security-dev:

Thanks for the feedback Will. It would be useful if you can provide a
testcase and/or add comments to JDK-8266351
<https://bugs.openjdk.java.net/browse/JDK-8266351> on your experience.

regards,
Sean.

On 30/04/2021 17:54, Will Sargent wrote:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210430/6acb3641/attachment.htm>

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented Apr 30, 2021

Mailing list message from Se=c3=a1n Coffey on security-dev:

Thanks for the feedback Will. It would be useful if you can provide a
testcase and/or add comments to JDK-8266351
<https://bugs.openjdk.java.net/browse/JDK-8266351> on your experience.

regards,
Sean.

On 30/04/2021 17:54, Will Sargent wrote:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210430/6acb3641/attachment.htm>

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 24, 2021

Mailing list message from Will Sargent on security-dev:

I have tried to sign up to the bug tracking system (through reset password
I think?) but I'm not getting an email out, so I can't add to the bug.

I have created a test case in Github:

https://github.com/wsargent/jca-key-failure/

The stack trace shows the invalid key store entry after saving and loading
it again.

https://github.com/wsargent/jca-key-failure/blob/main/src/main/java/com/tersesystems/jcakeyfailure/JcaKeyFailure.java#L68

On Fri, Apr 30, 2021 at 12:40 PM Se?n Coffey <sean.coffey at oracle.com> wrote:

Thanks for the feedback Will. It would be useful if you can provide a
testcase and/or add comments to JDK-8266351
<https://bugs.openjdk.java.net/browse/JDK-8266351> on your experience.

regards,
Sean.
On 30/04/2021 17:54, Will Sargent wrote:

KeyStore specification will be tightened up via another bug record

This would be super helpful, as one thing that confuses me is what the
relationship is between a key entry and a key alias -- in particular, the
existence alias doesn't seem to guarantee a valid entry that can be
retrieved.

In JDK 11 it's possible to create a private key with a keystore using
pkcs12.setKeyEntry() (see link below):

https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L135

and then have a null pointer exception when retrieving the entry from the
alias because the certificate chain is null (see commented out "testSystem"
use case):

https://github.com/tersesystems/securitybuilder/blob/master/lib/src/test/java/com/tersesystems/securitybuilder/PrivateKeyStoreTest.java#L27

I can write this up into a formal bug if that helps.

On Fri, Apr 30, 2021 at 2:30 AM Sean Coffey <coffeys at openjdk.java.net>
wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210524/48f95de0/attachment.htm>

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 28, 2021

Mailing list message from Se=c3=a1n Coffey on security-dev:

Thanks for the pointers Will.

I've added your details to the JDK-8266351 bug report.
https://bugs.openjdk.java.net/browse/JDK-8266351

regards,
Sean.

On 24/05/2021 18:53, Will Sargent wrote:

I have tried to sign up to the bug tracking system (through reset
password I think?) but I'm not getting an email out, so I can't add to
the bug.

I have created a test case in Github:

https://github.com/wsargent/jca-key-failure/
<https://urldefense.com/v3/__https://github.com/wsargent/jca-key-failure/__;!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEweeXM5lj3g$>

The stack trace shows the invalid key store entry after saving and
loading it again.

https://github.com/wsargent/jca-key-failure/blob/main/src/main/java/com/tersesystems/jcakeyfailure/JcaKeyFailure.java#L68
<https://urldefense.com/v3/__https://github.com/wsargent/jca-key-failure/blob/main/src/main/java/com/tersesystems/jcakeyfailure/JcaKeyFailure.java*L68__;Iw!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEweeC27YT_w$>

On Fri, Apr 30, 2021 at 12:40 PM Se?n Coffey <sean.coffey at oracle.com
<mailto:sean.coffey at oracle.com>> wrote:

Thanks for the feedback Will\. It would be useful if you can
provide a testcase and\/or add comments to JDK\-8266351
\<https\:\/\/bugs\.openjdk\.java\.net\/browse\/JDK\-8266351> on your experience\.

regards\,
Sean\.

On 30\/04\/2021 17\:54\, Will Sargent wrote\:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210528/2b18fc4c/attachment.htm>

1 similar comment
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 28, 2021

Mailing list message from Se=c3=a1n Coffey on security-dev:

Thanks for the pointers Will.

I've added your details to the JDK-8266351 bug report.
https://bugs.openjdk.java.net/browse/JDK-8266351

regards,
Sean.

On 24/05/2021 18:53, Will Sargent wrote:

I have tried to sign up to the bug tracking system (through reset
password I think?) but I'm not getting an email out, so I can't add to
the bug.

I have created a test case in Github:

https://github.com/wsargent/jca-key-failure/
<https://urldefense.com/v3/__https://github.com/wsargent/jca-key-failure/__;!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEweeXM5lj3g$>

The stack trace shows the invalid key store entry after saving and
loading it again.

https://github.com/wsargent/jca-key-failure/blob/main/src/main/java/com/tersesystems/jcakeyfailure/JcaKeyFailure.java#L68
<https://urldefense.com/v3/__https://github.com/wsargent/jca-key-failure/blob/main/src/main/java/com/tersesystems/jcakeyfailure/JcaKeyFailure.java*L68__;Iw!!GqivPVa7Brio!KZTaOe6TkXX8t-ZTaptDzm3RETFWZV4O6xj-7_iS2CF-NV4g7FxSSzYEweeC27YT_w$>

On Fri, Apr 30, 2021 at 12:40 PM Se?n Coffey <sean.coffey at oracle.com
<mailto:sean.coffey at oracle.com>> wrote:

Thanks for the feedback Will\. It would be useful if you can
provide a testcase and\/or add comments to JDK\-8266351
\<https\:\/\/bugs\.openjdk\.java\.net\/browse\/JDK\-8266351> on your experience\.

regards\,
Sean\.

On 30\/04\/2021 17\:54\, Will Sargent wrote\:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20210528/2b18fc4c/attachment.htm>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants