8359956: Support algorithm constraints and certificate checks in SunX509 key manager

[artur-oracle]

SunX509 key manager should support the same certificate checks that are supported by PKIX key manager.

Effectively there should be only 2 differences between 2 key managers:

• PKIX supports multiple key stores through KeyStore.Builder interface while SunX509 supports only a single keystore.
• SunX509 caches its whole key store on initialization thus improving performance. This means that subsequent modifications of the KeyStore have no effect on SunX509 KM, unlike PKIX .

SUNX509 KeyManager performance before the change
Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units
SSLHandshake.doHandshake true TLSv1.2 thrpt 15 19758.012 ± 758.237 ops/s
SSLHandshake.doHandshake true TLS thrpt 15 1861.695 ± 14.681 ops/s
SSLHandshake.doHandshake false TLSv1.2 thrpt 15 1186.962 ± 12.085 ops/s
SSLHandshake.doHandshake false TLS thrpt 15 1056.288 ± 7.197 ops/s

SUNX509 KeyManager performance after the change
Benchmark (resume) (tlsVersion) Mode Cnt Score Error Units
SSLHandshake.doHandshake true TLSv1.2 thrpt 15 20954.399 ± 260.817 ops/s
SSLHandshake.doHandshake true TLS thrpt 15 1813.401 ± 13.917 ops/s
SSLHandshake.doHandshake false TLSv1.2 thrpt 15 1158.190 ± 6.023 ops/s
SSLHandshake.doHandshake false TLS thrpt 15 1012.988 ± 10.943 ops/s

---

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

Issues

• JDK-8359956: Support algorithm constraints and certificate checks in SunX509 key manager (Enhancement - P4)
• JDK-8360289: Support algorithm constraints and certificate checks in SunX509 key manager (CSR)

Reviewers

• Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25016

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

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

8359956: Support algorithm constraints and certificate checks in SunX509 key manager

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

• 559795b: 8362501: Update test/hotspot/jtreg/applications/jcstress/README
• 413ac74: 8357682: sun.security.provider.certpath.Builder#getMatchingPolicies always returns null
• 3bdac53: 8361212: Remove AffirmTrust root CAs
• ... and 104 more: https://git.openjdk.org/jdk/compare/f8c8bcf4fd31509fdb40d32e8e16ba4fba1f987d...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]

@artur-oracle The following labels will be automatically applied to this pull request:

• core-libs
• net
• security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 14: Full - Incremental (c8c4008a)
• 13: Full - Incremental (df1a1fac)
• 12: Full - Incremental (eb972544)
• 11: Full - Incremental (5626eb9d)
• 10: Full - Incremental (3edca307)
• 09: Full - Incremental (c5e381ec)
• 08: Full - Incremental (a83ea8bd)
• 07: Full - Incremental (bcef7346)
• 06: Full - Incremental (45a4a48e)
• 05: Full - Incremental (eaf343ad)
• 04: Full - Incremental (448442e9)
• 03: Full - Incremental (451e1efd)
• 02: Full - Incremental (5c137c14)
• 01: Full - Incremental (88cd4016)
• 00: Full (75528109)

[haimaychao]

It is nice to refactor the common code for algorithm constraints checking into a new class, X509KeyManagerConstraints.java, used by both SunX509KeyManagerImpl and X509KeyManagerImpl. However, it looks like a new system property, "jdk.tls.keymanager.disableConstraintsChecking", is introduced, and it will affect both SunX509KeyManagerImpl and X509KeyManagerImpl. Should the property be a switch for SunX509 KeyManager, not a general toggle for all KeyManager? Avoiding its misuse for X509KeyManagerImpl that may lead to disable the existing RFC compliant algorithm constraints checking? It might be preferable to keep the property logic in SunX509KeyManagerImpl (not in the common code).

[artur-oracle]

It is nice to refactor the common code for algorithm constraints checking into a new class, X509KeyManagerConstraints.java, used by both SunX509KeyManagerImpl and X509KeyManagerImpl. However, it looks like a new system property, "jdk.tls.keymanager.disableConstraintsChecking", is introduced, and it will affect both SunX509KeyManagerImpl and X509KeyManagerImpl. Should the property be a switch for SunX509 KeyManager, not a general toggle for all KeyManager? Avoiding its misuse for X509KeyManagerImpl that may lead to disable the existing RFC compliant algorithm constraints checking? It might be preferable to keep the property logic in SunX509KeyManagerImpl (not in the common code).

@haimaychao Thanks for looking into it! Yes, it will disable constraints checking for both key managers and I did it this way on purpose. I think it will be simpler and less confusing to the end users. This system property is off by default and my assumption is that if end users want to disable KM algorithm constraints checking they would expect it to be disabled system-wide.
Making this toggle SunX509-specific is a trivial change if we have a consensus on this.

@seanjmullan What do you think?

[artur-oracle]

/issue add JDK-8170706

[openjdk]

@artur-oracle
Adding additional issue to issue list: 8170706: Support algorithm constraints in SunX509 key manager.

[seanjmullan]

What if you instead just removed "RSA keySize < 1024" from the jdk.certpath.disabledAlgorithms security property - would this test still pass? This way you could still test the other parts of the cert selection code.

This same comment applies to other tests where you have set the jdk.tls.SunX509keymanager.certSelectionChecking property to false.

[artur-oracle]

Done, good point! It works for this particular test but the same approach doesn't work for other tests because they either rely on TrustManager do the constraints checks or MD5 algorithm being blocks by TLSv1.3 spec.

[seanjmullan]

/csr

[seanjmullan]

Please create a CSR for the new system property.

[openjdk]

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

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

[bridgekeeper]

@artur-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[artur-oracle]

/integrate

[openjdk]

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

• 458f033: 8362533: Tests sun/management/jmxremote/bootstrap/* duplicate VM flags
• 3f21c8b: 8361897: gc/z/TestUncommit.java fails with Uncommitted too slow
• ddb6483: 8364409: [BACKOUT] Consolidate Identity of self-inverse operations
• ... and 111 more: https://git.openjdk.org/jdk/compare/f8c8bcf4fd31509fdb40d32e8e16ba4fba1f987d...master

Your commit was automatically rebased without conflicts.

[openjdk]

@artur-oracle Pushed as commit e544cd9.

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

[shqking]

Hi I found that test/jdk/javax/management/security/SecurityTest.java failed after this patch on both AArch64 and x86_64. Filed in https://bugs.openjdk.org/browse/JDK-8364495. I was wondering if you could help take a look at this issue? Thanks.

[artur-oracle]

Hi I found that test/jdk/javax/management/security/SecurityTest.java failed after this patch on both AArch64 and x86_64. Filed in https://bugs.openjdk.org/browse/JDK-8364495. I was wondering if you could help take a look at this issue? Thanks.

Hi! It's already fixed.