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)

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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[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

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

[openjdk]

@AlanBateman
The core-libs label was successfully removed.

[seanjmullan]

I am trying to figure out when the algorithm constraints are enabled, why the key isn't being selected. I don't see anywhere that you are setting the algorithm constraints property.

Please add some more comments explaining how the exception case occurs.

[artur-oracle]

Hi @seanjmullan! This PR fixes both JDK-8353113 and JDK-8170706. So we have 2 new unit tests, one for each issue:

1. AlgorithmConstraintsCheck: tests JDK-8170706. BTW, I'm going to update the @bug tag in this test to 8170706
2. PeerConstraintsCheck: tests JDK-8353113. No need to set any algorithm constraints because we test against the peer supported certificate signatures sent to us in "signature_algorithms"/"signature_algorithms_cert" extensions. I'll add a comment to this test with the explanation.

[seanjmullan]

I see. You also have a 3rd: JDK-8359069. It's rare to see one PR fix multiple issues, even though skara supports it. I'm not sure I see specific advantages of having three separate issues instead of just one. Is it primarily because you see these as separate issues? In that case, does it make sense to fix this as 3 different issues in case one or more them needs to be selectively backported?

[artur-oracle]

Done: all 3 issues have been combined into one.

[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).

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

Need to think about it some more, but I am kind of leaning towards it only affecting SunX509. The main benefit of the property is to workaround any compatibility issues where current code is not ready for the change. Any application already using the PKIX TrustManager already has this checking enabled/enforced.

I'd agree. As I mentioned in my earlier comment, if the new system property ends up toggling behavior in both SunX509KeyManager and X509KeyManagerImpl, we could run into an unintended side effect. While we're adding compliant algorithm constraints checking to SunX509KeyManager, turning on the property to disable it for compatibility reasons would also disable the already-existing checking in X509KeyManagerImpl. The applications already relying on the stricter checks in X509KeyManagerImpl might lose that enforcement unintentionally.

[artur-oracle]

/issue add JDK-8359069

[openjdk]

@artur-oracle
Adding additional issue to issue list: 8359069: Support certificate checks in SunX509 key manager.

[artur-oracle]

/issue remove JDK-8170706

[openjdk]

@artur-oracle
Removing additional issue from issue list: 8170706.

[artur-oracle]

/issue remove JDK-8359069

[openjdk]

@artur-oracle
Removing additional issue from issue list: 8359069.

[artur-oracle]

/issue add JDK-8359956

[openjdk]

@artur-oracle This issue is referenced in the PR title - it will now be updated.

[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?

Need to think about it some more, but I am kind of leaning towards it only affecting SunX509. The main benefit of the property is to workaround any compatibility issues where current code is not ready for the change. Any application already using the PKIX TrustManager already has this checking enabled/enforced.

I'd agree. As I mentioned in my earlier comment, if the new system property ends up toggling behavior in both SunX509KeyManager and X509KeyManagerImpl, we could run into an unintended side effect. While we're adding compliant algorithm constraints checking to SunX509KeyManager, turning on the property to disable it for compatibility reasons would also disable the already-existing checking in X509KeyManagerImpl. The applications already relying on the stricter checks in X509KeyManagerImpl might lose that enforcement unintentionally.

I made the toggle specific to SunX509 Key Manager as suggested.

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