On Wed, 14 May 2025 18:16:13 GMT, Artur Barashev <abaras...@openjdk.org> wrote:
>> When the deafult SunX509KeyManagerImpl is being used we are in violation of >> TLSv1.3 RFC spec because we ignore peer supported certificate signatures >> sent to us in "signature_algorithms"/"signature_algorithms_cert" extensions: >> https://datatracker.ietf.org/doc/html/rfc8446#section-4.4.2.2 >> https://datatracker.ietf.org/doc/html/rfc8446#section-4.4.2.3 >> >> X509KeyManagerImpl on the other hand includes the algorithms sent by the >> peer in "signature_algorithms_cert" extension (or in "signature_algorithms" >> extension when "signature_algorithms_cert" extension isn't present) in the >> algorithm constraints being checked. > > Artur Barashev has updated the pull request incrementally with one additional > commit since the last revision: > > Make the test run on TLSv1.3 src/java.base/share/classes/sun/security/ssl/SunX509KeyManagerImpl.java line 401: > 399: continue; > 400: } > 401: I think we should also call `CertType.check` here, like in `X509KeyManagerImpl`. Since this change is now only selecting certificates with algorithms that are not disabled, I think it also makes sense to select certificates that have the proper extensions, etc and will not cause subsequent certificate chain validation failures. This means we would have to change the name of the property so that it isn't only about disabling constraints checking. Perhaps: `jdk.tls.keymanager.disableCertSelectionChecking`. src/java.base/share/classes/sun/security/ssl/X509KeyManagerConstraints.java line 170: > 168: } > 169: > 170: protected boolean isConstraintsDisabled() { Make this private? Not sure why it would need to be overridden. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2132394682 PR Review Comment: https://git.openjdk.org/jdk/pull/25016#discussion_r2132187663
