On Fri, 31 Oct 2025 15:02:42 GMT, Matthew Donovan <[email protected]> wrote:
>> This PR updates tests that were using MD5 certificates. For most of the
>> tests, I added test cases for TLSv1.2/MD5withRSA and TLSv1.3/SHA256withRSA.
>
> Matthew Donovan has updated the pull request incrementally with one
> additional commit since the last revision:
>
> changed line wrapping
Can you convert or remove the `IPAddressDNSIdentities` test as well?
test/jdk/javax/net/ssl/HttpsURLConnection/CriticalSubjectAltName.java line 36:
> 34: * @library /test/lib
> 35: * @modules java.base/sun.security.x509 java.base/sun.security.util
> 36: * @run main/othervm CriticalSubjectAltName TLSv1.2 MD5withRSA
as far as I could tell, this test doesn't verify any functionality that would
require a specific key type, it's simply using MD5 because that was the popular
choice in 2008. Do we need to keep using MD5, or can we make it use whatever
key type is the default?
test/jdk/javax/net/ssl/HttpsURLConnection/CriticalSubjectAltName.java line 39:
> 37:
> 38: /*
> 39: * This test depends on binary keystore, crisubn.jks and trusted.jks.
> Because
Peease remove the crisubn.jks and trusted.jks files from this directory, they
aren't used anywhere else.
test/jdk/javax/net/ssl/HttpsURLConnection/CriticalSubjectAltName.java line 157:
> 155: ks.setCertificateEntry("Trusted Cert", trustedCert);
> 156:
> 157: Certificate[] chain = new Certificate[] {serverCert,
> trustedCert};
(probably preexisting) you can remove the trusted cert from the chain; real
servers usually don't send it.
test/jdk/javax/net/ssl/HttpsURLConnection/CriticalSubjectAltName.java line 190:
> 188: void doClientSide() throws Exception {
> 189:
> 190: if (!serverReady.await(SERVER_WAIT_SECS, TimeUnit.SECONDS)) {
I'd remove the time limit, it only causes trouble when people increase test
concurrency and timeout factors.
test/jdk/sun/net/www/protocol/https/HttpsURLConnection/IPIdentities.java line 1:
> 1: /*
This might be preexisting, but this file is identical to
`test/jdk/sun/net/www/protocol/https/HttpsURLConnection/IPAddressIPIdentities.java`
now. Can we remove one?
test/jdk/sun/net/www/protocol/https/HttpsURLConnection/IdentitiesBase.java line
104:
> 102: CertificateBuilder.KeyUsage.KEY_ENCIPHERMENT)
> 103: .addBasicConstraintsExt(false, false, -1)
> 104:
> .addExtension(CertificateBuilder.createIPSubjectAltNameExt(true, "127.0.0.1"))
I assume you verified that the DNSIdentities customization overwrites the SAN
configured here, but I'd feel more confident if this line were moved to
customizeServerCert in IPIdentities
test/jdk/sun/net/www/protocol/https/HttpsURLConnection/IdentitiesBase.java line
119:
> 117: CertificateBuilder.KeyUsage.NONREPUDIATION,
> 118: CertificateBuilder.KeyUsage.KEY_ENCIPHERMENT)
> 119:
> .addExtension(CertificateBuilder.createIPSubjectAltNameExt(true, "127.0.0.1"))
Same here.
-------------
PR Review: https://git.openjdk.org/jdk/pull/27342#pullrequestreview-3459883168
PR Review Comment: https://git.openjdk.org/jdk/pull/27342#discussion_r2523546617
PR Review Comment: https://git.openjdk.org/jdk/pull/27342#discussion_r2523539070
PR Review Comment: https://git.openjdk.org/jdk/pull/27342#discussion_r2523561169
PR Review Comment: https://git.openjdk.org/jdk/pull/27342#discussion_r2523555451
PR Review Comment: https://git.openjdk.org/jdk/pull/27342#discussion_r2523677717
PR Review Comment: https://git.openjdk.org/jdk/pull/27342#discussion_r2523705701
PR Review Comment: https://git.openjdk.org/jdk/pull/27342#discussion_r2523706426
