On Thu, 18 Dec 2025 12:44:30 GMT, Sean Coffey <[email protected]> wrote:
>> Introduce lazy static initialization logic to SSLContextImpl via use of the
>> new LazyConstant API and improve logging output
>>
>> As per JBS comments:
>>
>> * Each subclass of AbstractTLSContext (TLSv10. TLSv11 etc) is being
>> initialization at framework initialization time due to the
>> getApplicableSupportedCipherSuites(..) calls made in static block. Such
>> calls are unnecessary if the subclass isn't required. This is especially
>> true for the default JDK configuration where TLSv10, TLSv11 protocols are
>> disabled. I've moved logic to lazy initialization of these fields via
>> LazyConstant
>>
>> * The debug prints output never made clear what protocol version each cipher
>> suite was being disabled for. Improved logging there
>> * The debug prints never printed out the resulting set of enabled/allowed
>> cipher suites
>>
>> There's efficiency gain also in having one less call to the
>> getApplicableEnabledCipherSuites method in the scenario where customized
>> cipher suites are not in use.
>>
>> example of new debug output:
>>
>>
>> javax.net.ssl|TRACE|30|main|2025-11-26 14:31:31.997
>> GMT|SSLContextImpl.java:425|Ignore disabled cipher suites for
>> protocols:[TLSv1.3, TLSv1.2]
>> [TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256
>> TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA384,TLS_ECDH_RSA_WITH_AES_256_CBC_SHA384
>> TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA256,TLS_ECDH_RSA_WITH_AES_128_CBC_SHA256,TLS_ECDH_ECDSA_WITH_AES_256_CBC_SHA
>> TLS_ECDH_RSA_WITH_AES_256_CBC_SHA,TLS_ECDH_ECDSA_WITH_AES_128_CBC_SHA,TLS_ECDH_RSA_WITH_AES_128_CBC_SHA,TLS_RSA_WITH_AES_256_GCM_SHA384
>> TLS_RSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_AES_256_CBC_SHA256,TLS_RSA_WITH_AES_128_CBC_SHA256,TLS_RSA_WITH_AES_256_CBC_SHA
>> TLS_RSA_WITH_AES_128_CBC_SHA]
>> javax.net.ssl|TRACE|30|main|2025-11-26 14:31:31.997
>> GMT|SSLContextImpl.java:425|Available cipher suites for protocols:[TLSv1.3,
>> TLSv1.2]
>> [TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256,TLS_CHACHA20_POLY1305_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
>> TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
>> TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384
>> TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256,TLS_DHE_DSS_WITH_AES_256_GCM_SHA384,TLS_DHE_RSA_WITH_AES_128_GCM_SHA256
>> TLS_DHE_DSS_WITH_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SH...
>
> Sean Coffey has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - Merge branch 'master' into 8371333-ssl-debug
> - Move wrapText method to Utilities
> - Merge branch 'master' into 8371333-ssl-debug
> - use LINE_SEP
> - 8371333
src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java line 369:
> 367: List<ProtocolVersion> protocols) {
> 368: LinkedHashSet<CipherSuite> suites = new LinkedHashSet<>();
> 369: List<String> disabledSuites = new ArrayList<>();
Do you need to perform the instantiation here? I ask only because it looks
like you're only using these two lists when debugging is turned on, so maybe
instantiation can happen only in that circumstance?
src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java line 400:
> 398: }
> 399:
> 400: if(SSLLogger.isOn() && SSLLogger.isOn("ssl,sslctx")) {
Nit: space between if and parenthesis.
src/java.base/share/classes/sun/security/ssl/SSLContextImpl.java line 401:
> 399:
> 400: if(SSLLogger.isOn() && SSLLogger.isOn("ssl,sslctx")) {
> 401: logSuites("Ignore disabled cipher suites for protocols:",
Nit: just a personal preference - I wouldn't mind seeing a space after the
colons. But no strong feelings on this one.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/28511#discussion_r2638717667
PR Review Comment: https://git.openjdk.org/jdk/pull/28511#discussion_r2638718651
PR Review Comment: https://git.openjdk.org/jdk/pull/28511#discussion_r2638720100
