[8u] TLSv1.3 RFR: 8245469: Remove DTLS protocol implementation

Martin Balao mbalao at redhat.com
Fri May 29 03:51:27 UTC 2020 

On 5/21/20 10:23 AM, Alexey Bakhtin wrote:
> Please review changes required to backport TLSv1.3 protocol from JDK11.0.7 to JDK8u
> 

Hi,

The reason why we need 8245469 (Step 2) is because TLS files added to
sun/security/ssl as part of 8245468 (Step 1) include changes from "JEP
219: Datagram Transport Layer Security (DTLS)" 8043758 [1]. JEP 219
(pushed to main line [2] previous to 8196584 [3]) involves specification
changes and will not be backported to JDK-8 at this time.

8245469 (Step 2) should be a revert of the changes introduced by 8043758
(and follow up patches, including 8196584) but limited to
sun/security/ssl directory (because 8245468 did not add files in other
directories).

A few easy checks first:

 * No files should be added
  * Ok

 * No files out of sun/security/ssl should be added, deleted or modified
  * Ok

 * Files deleted by 8043758 should not be present after 8245469 (Step 2)
  * classes/sun/security/ssl/EngineArgs.java
  * classes/sun/security/ssl/EngineInputRecord.java
  * classes/sun/security/ssl/EngineOutputRecord.java
  * classes/sun/security/ssl/EngineWriter.java
  * Ok

 * There shouldn't be any "dtls" (case insensitive) words in
sun/security/ssl
  * Ok, exceptions are correct.

 * There shouldn't be any file named with "dtls" (case insensitive) in
sun/security/ssl
  * Ok

The strategy to review this change is as follows:

 1) Go through the list of files added or modified by 8043758 and
manually verify how changes on those files are reverted by 8245469 (Step
2). Analyzing how a change is reverted is an involved task because there
are changes introduced by 8043758 which are essential for 8196584 and
should not be reverted. On the other hand, 8196584 (or any other patch
after 8043758) may refactor code and "re-introduce" DTLS changes in
places different than where 8043758 originally affected. This is how
I'll proceed: have a look at the change in 8043758, see if it's reverted
by 8196584 or refactored and moved to a different place, see how 8245469
deals with it, grep in the actual file for "DTLS", "(D)TLS" and
"HELLO_VERIFY" words (case insensitive and all possible variations) as
the last check. It's not feasible to manually analyze all other patches
manually in the time constraints we have: that's a risk I'll take here.
The other risk is that going through these changes without considering
the whole TLS engine context may me loose sight of what's going on
semantically. There are two factors that work as a mitigation to some
extent: 1) code compilation (if there were a symbol reference not
satisfied, the compiler would catch it), 2) DTLS-specific code is
usually protected under "if" clauses (so even if a trace is left, may be
dead code not compromising functionality, CPU performance or security).

 2) Verify that 8245469 (Step 2) includes nothing else or that any other
inclusions are justified.

Here we go with #1:

 * classes/sun/security/ssl/AppInputStream.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/AppOutputStream.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/Authenticator.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/CipherBox.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/CipherSuite.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/Ciphertext.java (new)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/ClientAuthType.java (new)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/ClientHandshaker.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/DTLSInputRecord.java (new)
  * File deleted. Deletion makes sense as all code here is DTLS specific.
  * Ok
 * classes/sun/security/ssl/DTLSOutputRecord.java (new)
  * File deleted. Deletion makes sense as all code here is DTLS specific.
  * Ok
 * classes/sun/security/ssl/DTLSRecord.java (new)
  * File deleted. Deletion makes sense as all code here is DTLS specific.
  * Ok
 * classes/sun/security/ssl/Debug.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/HandshakeHash.java (modified)
  * Change by change manual analysis.
   * I'm okay with removing the reference to
SSLHandshake.HELLO_VERIFY_REQUEST.id. Good catch!
  * Ok
 * classes/sun/security/ssl/HandshakeInStream.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/HandshakeMessage.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/HandshakeOutStream.java (modified)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/HandshakeStateManager.java (new)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/Handshaker.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/HelloCookieManager.java (new)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/HelloExtensions.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/InputRecord.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/MAC.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/MaxFragmentLengthExtension.java (new)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/OutputRecord.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/Plaintext.java (new)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/ProtocolList.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/ProtocolVersion.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/RSAClientKeyExchange.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/RandomCookie.java (modified)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/Record.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/RecordType.java (new)
  * Added by 8043758 but removed by 8180856, so 8245469 did not have the
necessity of removing it.
  * Ok
 * classes/sun/security/ssl/SSLContextImpl.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/SSLEngineImpl.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/SSLEngineInputRecord.java (new)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/SSLEngineOutputRecord.java (new)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/SSLRecord.java (new)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/SSLServerSocketImpl.java (modified)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/SSLSessionImpl.java (modified)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/SSLSocketImpl.java (modified)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/SSLSocketInputRecord.java (new)
  * Change by change manual analysis.
  * Ok
 * classes/sun/security/ssl/SSLSocketOutputRecord.java (new)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/ServerHandshaker.java (modified)
  * Changes introduced by 8043758. Completely deleted by 8196584. No
traces in 8245469. File does not exist after 8245469.
  * Ok
 * classes/sun/security/ssl/SunJSSE.java (modified)
  * DTLS changes not added by 8245468.
  * Ok
 * classes/sun/security/ssl/X509KeyManagerImpl.java (modified)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok
 * classes/sun/security/ssl/X509TrustManagerImpl.java (modified)
  * Change by change manual analysis. No changes required for 8245469.
  * Ok

Here we go with #2:

 * classes/sun/security/ssl/Alert.java (modified)
  * Ok
 * classes/sun/security/ssl/BaseSSLSocketImpl.java (modified)
  * Ok, after deleted methods were re-introduced for v1.
 * classes/sun/security/ssl/CipherType.java (modified)
  * Ok
 * classes/sun/security/ssl/ClientHello.java (modified)
  * Ok
 * classes/sun/security/ssl/ConnectionContext.java (modified)
  * Ok
 * classes/sun/security/ssl/ContentType.java (modified)
  * Ok
 * classes/sun/security/ssl/Finished.java (modified)
  * Ok
 * classes/sun/security/ssl/HelloVerifyRequest.java (deleted)
  * Ok
 * classes/sun/security/ssl/MaxFragExtension.java (modified)
  * Ok
 * classes/sun/security/ssl/SSLCipher.java (modified)
  * Ok
 * classes/sun/security/ssl/SSLConfiguration.java (modified)
  * Ok
 * classes/sun/security/ssl/SSLExtensions.java (modified)
  * Ok
 * classes/sun/security/ssl/SSLHandshake.java (modified)
  * Ok
 * classes/sun/security/ssl/SSLKeyExchange.java (modified)
  * Ok
 * classes/sun/security/ssl/SSLMasterKeyDerivation.java (modified)
  * Ok
 * classes/sun/security/ssl/SSLTrafficKeyDerivation.java (modified)
  * Ok
 * classes/sun/security/ssl/SSLTransport.java (modified)
  * Ok
 * classes/sun/security/ssl/ServerHello.java (modified)
  * Ok
 * classes/sun/security/ssl/StatusResponseManager.java (modified)
  * Ok
 * classes/sun/security/ssl/TransportContext.java (modified)
  * Ok

Note: there are previous comments -part of this review- here [4] [5].

With that said, 8245469 (Step 2) v1 looks good to me. Please hold the
push until the whole series is review-approved and maintainer-approved.

Thanks,
Martin.-

--
[1] - https://bugs.openjdk.java.net/browse/JDK-8043758
[2] - https://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/6721ff11d592
[3] - https://bugs.openjdk.java.net/browse/JDK-8196584
[4] - https://mail.openjdk.java.net/pipermail/jdk8u-dev/2020-May/011797.html
[5] - https://mail.openjdk.java.net/pipermail/jdk8u-dev/2020-May/011803.html

More information about the jdk8u-dev mailing list