8279066: entries.remove(entry) is useless in PKCS12KeyStore

[wangweij]

Before password-less PKCS12 keystores are supported, certificates in a PKCS12 file are always encrypted. Therefore if one loads the keystore with a null pass, it contains PrivateKeyEntrys without certificates. This has always been awkward (and most likely useless) so when JDK-8076190 introduced the password-less feature I also added a line to remove such an entry.

jdk/src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java

Line 2272 in a729a70

entries.remove(entry);

Unfortunately, the line is not coded correctly, it should have been remove(key) but here it's remove(value).

This code change correctly removes the entry.

That said, this behavior, although weird, has been there from the beginning since PKCS12 keystore was introduced. If you can find out a usage of a private key entry without any certificate and think it's worth kept that way, I can simply remove the remove call and leave the entry there.

---

Progress

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

Issue

• JDK-8279066: entries.remove(entry) is useless in PKCS12KeyStore

Reviewers

• Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6910

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

Using diff file

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

[bridgekeeper]

👋 Welcome back weijun! 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.

[gerry1888]

Hi @gerry1888, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user gerry1888 for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

• I agree to the OpenJDK Terms of Use for all comments I make in a project in the OpenJDK GitHub organization.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

[openjdk]

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

• 01: Full - Incremental (844bf487)
• 00: Full (ed6ee075)

[seanjmullan]

I still think it's useful even if I can't see the certificate chain. I'd rather see the entry if it actually exists in the keystore and I think removing it is odd because it still exists in the keystore. Also, sometimes I use keytool without a storepass just to see what is in it, and then if I see the certificates are not showing up, I can try again with the password.

[wangweij]

OK, I've updated the change to simply removing that remove all.

[openjdk]

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

8279066: entries.remove(entry) is useless in PKCS12KeyStore

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

• 997b1ee: 8279060: Parallel: Remove unused PSVirtualSpace constructors
• 6aeb40c: 8278396: G1: Initialize the BOT threshold to be region bottom
• f31dead: 8279043: Some Security Exception Messages Miss Spaces
• f730906: 8278793: Interpreter(x64) intrinsify Thread.currentThread()
• 8c0bb53: 8278044: ObjectInputStream methods invoking the OIF.CFG.getSerialFilterFactory() silent about error cases.
• f90425a: 8278087: Deserialization filter and filter factory property error reporting under specified
• f4f2f32: 8278917: Use Prev Bitmap for recording evac failed objects
• 29bd736: 8277893: Arraycopy stress tests
• ff5d417: 8278893: Parallel: Remove GCWorkerDelayMillis
• 5179672: 8278953: Clarify Class.getDeclaredConstructor specification
• ... and 47 more: https://git.openjdk.java.net/jdk/compare/04dbdd36dd04bf40737cb8c2f13d5b75303d2b1a...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.

[wangweij]

/integrate

[openjdk]

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

• 803cb8a: Merge
• 1128674: 8278627: Shenandoah: TestHeapDump test failed
• 54517fa: 8279074: ProblemList compiler/codecache/jmx/PoolsIndependenceTest.java on macosx-aarch64
• ac7430c: 8278044: ObjectInputStream methods invoking the OIF.CFG.getSerialFilterFactory() silent about error cases.
• db3d6d7: 8278087: Deserialization filter and filter factory property error reporting under specified
• 467f654: 8279011: JFR: JfrChunkWriter incorrectly handles int64_t chunk size as size_t
• 819f9bd: 8274323: compiler/codegen/aes/TestAESMain.java failed with "Error: invalid offset: -1434443640" after 8273297
• ad12828: 8278609: [macos] accessibility frame is misplaced on a secondary monitor on macOS
• deaf75a: 8278413: C2 crash when allocating array of size too large
• 36676db: 8278970: [macos] SigningPackageTest is failed with runtime exception
• ... and 66 more: https://git.openjdk.java.net/jdk/compare/04dbdd36dd04bf40737cb8c2f13d5b75303d2b1a...master

Your commit was automatically rebased without conflicts.

[openjdk]

@wangweij Pushed as commit fb623f1.

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

[mlbridge]

Mailing list message from Michael StJohns on security-dev:

On 12/21/2021 1:26 PM, Sean Mullan wrote:

I got curious and took a look at P11KeyStore.java -
https://github.com/openjdk/jdk/blob/master/src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11KeyStore.java
- and it appears to do exactly the opposite of what you've got here.?
E.g. if there's no certificate that matches up with the private key,
then it returns a null - that's in engineGetEntry() around line 963.

Speaking personally, I've always found it a bit annoying that a
KeyStore.PrivateKeyEntry required a certificate(chain).? It actually
made dealing with PKCS11 painful as the Java conventions didn't always
conform to the actual structure of the PKCS11 contents.

Not suggesting that a change necessarily needs to be made, but perhaps
you might want to have a consistent behavior between the two?

Mike