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

[Alexey Bakhtin]

Hello Martin,

Thank you very much for your great work on this reviews.

Please see my comments below and find updated webrev at :
http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245469/webrev.v1/

Regards
Alexey

> On 27 May 2020, at 04:06, Martin Balao <mbalao at redhat.com> wrote:
> 
> 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,
> 
> A few comments below:
> 
> ------------------------------------
> 
> * OutputRecord.java, introduced by 8196584
>  * d13Encrypt -> Should probably be removed.
>  * d10Encrypt -> Should probably be removed.
> 
Thank you. You are right. This methods no make sense any more - removed.

> ------------------------------------
> 
> * OutputRecord.java / InputRecord.java
> 
>  * Not a big deal but I wonder if we should keep hook methods (such as
> initHandshaker, finishHandshaker, launchRetransmission, isEmpty,
> expectingFinishFlight) and their calls. Even though there would be no
> action because DTLSInputRecord and DTLSOutputRecord were removed, we can
> minimize code changes and perhaps avoid work in the future if used by
> other OutputRecord/InputRecord subclasses. On the other hand, it would
> be 'nop' code currently. I'm not sure, just raised this point to think
> loudly. Opinions here are welcomed.
> 

I would suggest to remove these methods as it makes 8u code more cleaner and readable.

> ------------------------------------
> 
> * SSLContextImpl.java
>  * I understand that the comment "// Used for DTLS in server mode
> only." is probably wrong because this is defined in TLS 1.3 (section
> 4.2.2) and is not DTLS 1.3 specific -even though it makes more sense for
> UPD connections than TCP-. Looking into the code, seems to me that it is
> not only used in SunJSSE's DTLS 1.3 but could be used in TLS 1.3.
> However, that comment should be fixed in main line and then backported.
> Otherwise, we would be introducing an improvement that applies to main
> line in the update release first. This is for the same reason that we
> will keep the "// DTLS cookie exchange manager" related comment.

OK. I’ve reverted the comment and created a bug:
https://bugs.openjdk.java.net/browse/JDK-8245937

> 
> ------------------------------------
> 
> * SSLEngineImpl.java
> 
>             deltaDsts -= dsts[i].remaining();
>         }
> 
> -        return new SSLEngineResult(status, hsStatus, deltaSrcs, deltaDsts,
> -                ciphertext != null ? ciphertext.recordSN : -1L);
> +        return new SSLEngineResult(status, hsStatus, deltaSrcs, deltaDsts);
>     }
> 
>     private Ciphertext encode(
> 
> You probably decided this change because 8043758 (not in JDK-8) is the
> one that introduces the SSLEngineResult public constructor that takes
> the sequenceNumber as the last argument. However, TLS 1.3 uses this
> constructor so it's not DTLS specific. This is obviously a problem
> because a public API is being modified.
> 
> I need to understand the consequences of not providing this API (and the
> corresponding SSLEngineResult::sequenceNumber function counterpart).
> Please let me know of your thoughts on this point.

In my understanding, DTLS messages over UDP could be reordered in case of
received using SSLEngine unwrap() method. The sequence number in SSLEngineResult
helps to restore the real sequence.

> 
> ------------------------------------
> 
> * BaseSSLSocketImpl.java
> 
> Why are those methods (introduced by 8040062) removed?

These methods are implementation of java.net.Socket public API methods not available in the JDK8
You are right. These methods are not related to removal of DTLS protocol.
These changes should go to the next patch JDK-8245470
Fixed
The patch for JDK-8245470 will be updated later

> 
> ------------------------------------
> 
> * SSLConfiguration.java
> 
> I'd only replace enableRetransmissions with 'false' and avoid other
> inferences -even if they look correct-.
> 

It is similar to changes in the OutputRecord.java / InputRecord.java classes.
If we leave enableRetransmissions in the code it could lead to misleading that retransmission
could be enabled or used.
I'd suggest to remove it.

> ------------------------------------
> 
> Thanks,
> Martin.-
>