RFR: 8331682: Slow networks/Impatient clients can potentially send unencrypted TLSv1.3 alerts that won't parse on the server [v17]

[Artur Barashev]

On Wed, 9 Oct 2024 07:47:38 GMT, Daniel Jeliński <djelinski at openjdk.org> wrote:

>> Artur Barashev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Return null if there is no record we attempted to decode
>
> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line 122:
> 
>> 120:                     SSLEngineResult clientResult;
>> 121:                     // Client-side plain TCP socket.
>> 122:                     clientSocket = new Socket("localhost", port);
> 
> if you use a SocketChannel instead, you'll be able to deal with ByteBuffers directly instead of going through byte arrays.

Good idea, done!

> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line 134:
> 
>> 132: 
>> 133:                     // Send client_hello, read and store all available messages from the server.
>> 134:                     while (delayed.size() < 6) {
> 
> Could you drop the delayed queue, and instead send user_canceled/close_notify before you start reading from the socket? It would simplify the code and improve the coverage.

Indeed, done!

> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line 162:
> 
>> 160:                     // Send user_canceled and close_notify alerts to server. Server should process
>> 161:                     // 2 unencrypted alerts and send its own close_notify alert back to the client.
>> 162:                     ByteBuffer serverCloseNotify = clientWriteRead();
> 
> Make sure to read until EOF

Done

> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line 196:
> 
>> 194:     }
>> 195: 
>> 196:     protected ByteBuffer clientWriteRead() throws IOException {
> 
> could you split the write and read methods?

Done

> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line 204:
> 
>> 202:         inspectTlsBuffer(cTOs);
>> 203: 
>> 204:         byte[] outbound = new byte[cTOs.limit() - cTOs.position()];
> 
> limit() - position() is equivalent to remaining()

Indeed, but this code is gone now.

> test/jdk/javax/net/ssl/TLSv13/SSLSocketNoServerHelloClientShutdown.java line 215:
> 
>> 213: 
>> 214:         try {
>> 215:             len = is.read(inbound);
> 
> this method might return a partial TLS record or multiple records. SSLEngine expects to receive the entire record in one buffer.

Reading until the EOF now

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794153103
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794153406
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794153565
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794153746
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794154338
PR Review Comment: https://git.openjdk.org/jdk/pull/21043#discussion_r1794155144