[8u] RFR 8202343: Disable TLS 1.0 and 1.1

[Severin Gehwolf]

Hi Martin,

Thanks for doing this patch and the detailed write-up. Review below.

On Thu, 2021-01-21 at 11:15 -0300, Martin Balao wrote:
> Hi,
> 
> I'd like to propose an 8u backport of JDK-8202343 [1].
> 
> Webrev.00:
> 
>  *
> http://cr.openjdk.java.net/~mbalao/webrevs/8202343/8202343.webrev.8u.jdk.00/
> 
> The 11u patch does not apply cleanly because of the following
> conflicts:
> 
>  * src/share/conf/security/java.security
>   * In 8u, there is one java.security file per supported operating
> system. Manually added 'TLSv1' and 'TLSv1.1' to the
> 'jdk.tls.disabledAlgorithms' property for each file.

OK.

>  * test/javax/net/ssl/TLS/TLSClientPropertyTest.java
>   * 8u has JDK-8251478 which introduced a change to this test consistent
> with the decision of having TLSv1.2 as the default choice for the TLS
> client. This conflict affects the context only; so I manually added the
> change.
>   * It's important to notice that HUNK #3 succeeded when applying the
> patch but the change was not correct and should have been a conflict.
> Removed "TLSv1" and "TLSv1.1" in the 'NoProperty' case.

OK.

>  * test/javax/net/ssl/TLSCommon/interop/JdkProcClient.java
>   * 8u does not have JDK-8243029 which introduces this test file.
> JDK-8243029 is a massive patch which does not apply cleanly. If
> JDK-8243029 is backported to 8u, tests using this class will fail until
> the change in JDK-8202343 (re-enabling TLSv1 and TLSv1.1) is applied.
> Because of the previous, and the fact that JDK-8243029 is not
> fundamental to JDK-8202343, I propose to skip the changes on this file
> at this time.

Agreed.

>  * test/javax/net/ssl/TLSv11/GenericBlockCipher.java
>   * 8u does not have JDK-8166032, which changes the copyright date to
> 2016 and adds the '@modules' jtreg tag. JDK-8166032 does not apply to
> 8u, so I manually update the copyright date and the '@library /test/lib'
> line. See more on '@library /test/lib' below.

OK.

>  *
> test/javax/net/ssl/sanity/ciphersuites/SystemPropCipherSuitesOrder.ja
> va
>   * 8u does not have JDK-8234728 which introduces this test file. This
> case is similar to the JdkProcClient.java one, so I propose the same
> action now.

I tend to disagree. JDK-8234728 seems like a good patch to have for
OpenJDK 8u. It verifies proper ciphers are available for certain
algorithms. With the TLS 1.3 backport in 8u, it would be good to have
associated tests too. Oracle backported it to 8 as well. I'd suggest to
first backport JDK-8234728 and rebase this one on top.

If you prefer to integrate this one first, I'd urge you to follow up
with JDK-8234728 for 8u with relevant hunks from this patch included.
Your call.

>  * test/javax/net/ssl/sanity/ciphersuites/TLSCipherSuitesOrder.java
>   * 8u does not have JDK-8234728 which introduces this test file. This
> case is similar to the JdkProcClient.java one, so I propose the same
> action now.

Same comment as for SystemPropCipherSuitesOrder.java. I think we should
get it in first.

> 
>  * test/sun/security/ssl/CipherSuite/DisabledCurve.java
>   * 8u does not have JDK-8246330 which introduces this test file.
> JDK-8246330 is not as large as JDK-8243029, but it does not apply
> cleanly and is not fundamental to JDK-8202343; so I propose the same
> action now.

OK to post-pone this one before integrating this patch, but please add
a comment to JDK-8246330 that relevant hunks from 8202343 need to get
included when it gets backported to JDK 8.

>  * test/sun/security/ssl/CipherSuite/NamedGroupsWithCipherSuite.java
>   * 8u does not have JDK-8224650 which introduces this test file.
> JDK-8224650 does not apply to 8u, unless the JDK-8171279 enhancement is
> backported first. This is not fundamental to JDK-8202343 so I propose
> this not to be a dependency.

OK.

>  * test/sun/security/ssl/ClientHandshaker/LengthCheckTest.java
>   * 8u has JDK-8251478 which removes the '@modules' jtreg tag. The
> conflict is because of the context. Manually added the new '@library'
> jtreg tag. See more on '@library' below.

OK.

>  *
> test/sun/security/ssl/HandshakeHash/HandshakeHashCloneExhaustion.java
>   * 8u does not have JDK-8234728 which changes the copyright date to
> 2019. Manually adjusted the copyright date.

OK.

>  * test/sun/security/util/HostnameMatcher/NullHostnameCheck.java
>   * JDK-8228967 is not in 8u so there is a context conflict while adding
> the import. JDK-8228967 also causes a context conflict when adding the
> line that re-enables TLS 1.0 and 1.1. However, these changes are not
> needed in 8u as the test works for TLS 1.2 only (so there is no need to
> re-enable TLS 1.0 and 1.1). If JDK-8228967 is ever backported to 8u,
> this test will fail until TLS 1.0 and 1.1 are enabled.

OK. Please add a comment to that effect to JDK-8228967.

>  * test/lib/security/SecurityUtils.java
>   * For an unknown reason, the 8u backport of 8207258 has copyright date
> 2019 (instead of 2018, as the original file). This has been this way
> from the first proposed 8u backport webrev
> (           http://cr.openjdk.java.net/~phh/8207258/webrev.8u.00/raw_files/new/test/lib/security/SecurityUtils.java).
> I've manually update the copyright date to go between 2018 and 2020, so
> we hopefully avoid a future conflict here.

OK.

> In addition to static file-patching conflicts, I had to make changes for
> this patch to run in 8u:
> 
>  * test/sun/security/ssl/EngineArgs/DebugReportsOneExtraByte.java
>   * '@library /test/lib' replaced with '@library /lib/security'
>   * 'import jdk.test.lib.security.SecurityUtils;' removed
>   * OutputAnalyzer and ProcessTools imports changed to 8u's locations
>   * Added /lib to @library for OutputAnalyzer and ProcessTools
> imports

OK.

>  * test/lib/security/SecurityUtils.java
>   * 'List.<String>of' is not available in 8u. Found a replacement.

OK.

>  * test/sun/security/ssl/HandshakeHash/HandshakeHashCloneExhaustion.java
>  * test/sun/security/ssl/SSLContextImpl/IllegalProtocolProperty.java
>  * test/sun/security/ssl/SSLContextImpl/SSLContextVersion.java
>  * test/sun/security/ssl/SSLEngineImpl/EmptyExtensionData.java
>  * test/sun/security/ssl/SSLEngineImpl/SSLEngineBadBufferArrayAccess.java
>  * test/sun/security/ssl/ClientHandshaker/LengthCheckTest.java
>  * test/javax/net/ssl/TLSv11/GenericBlockCipher.java
>  * test/javax/net/ssl/SSLEngine/Arrays.java
>   * '@library /test/lib' replaced with '@library /lib/security'
>   * 'import jdk.test.lib.security.SecurityUtils;' removed

OK.

>  * sun/security/ssl/SSLContextImpl/SSLContextDefault.java
>   * 'List.<String>of' is not available in 8u. Found replacements.


  41 public class SSLContextDefault {
  42 
  43     private final static String[] protocols = {
  44         "", "SSL", "TLS", "SSLv3", "TLSv1", "TLSv1.1", "TLSv1.2", "TLSv1.3"
  45     };
  46 
  47     private final static List<String> disabledProtocols;
  48 
  49     static {
  50         List<String> protocols = Arrays.asList("SSLv3", "TLSv1", "TLSv1.1");
  51         protocols = Collections.unmodifiableList(protocols);
  52         disabledProtocols = protocols;
  53     }

Local variable 'protocols' shadows static variable 'protocols'. Please
use a different name in the static initializer instead. Suggestion:
'disProts' or some such. I wonder if we should just simplify this to
and get rid of the static initializer:

private final static List<String> disabledProtocols =
   Collections.unmodifiableList(Arrays.asList("SSLv3", "TLSv1", "TLSv1.1"));

Your call.

 131             List<String> protocolsList = Arrays.asList(protocols);
 132             protocolsList = Collections.unmodifiableList(protocolsList);
 133             for (String disabledProtocol : disabledProtocols) {
 134                 if (!protocolsList.contains(disabledProtocol)) {

Same here. Perhaps use:

List<String> protocolsList = Collections.unmodifiableList(Arrays.asList(protocols));

Thanks,
Severin