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

[Martin Balao]

Hi Alexey,

On 5/27/20 6:30 AM, Alexey Bakhtin wrote:
> Please see my comments below and find updated webrev at :
> http://cr.openjdk.java.net/~abakhtin/tls1.3/8245466/8245469/webrev.v1/

Thanks. I'll reply inline below and then send a separate mail with my
review.

>> * 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.
> 

See below.

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

Thanks.

> 
>>
>> ------------------------------------
>>
>> * 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.

Okay, then we can conclude that this API won't be needed/used for TLS
1.3 SSLEngine.

>>
>> * 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.

Hmm.. yes, part of this comment is related to OutputRecord.java /
InputRecord.java. I understand your point about cleaning up the code.
But the other side of the argument is that we now have the burden of
supporting 8u-specifics in future backports. In other words, if we have
a conflict near this code -which is now more likely-, we will probably
need to think about fragmentation again and why the code is different
than in 11u. As said before, I see arguments on both sides and even
though I'm a bit more inclined to keep the code as similar as possible,
I'm okay if you want to go the remove way.

Regards,
Martin.-