Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Close connection when a TLS close_notify is received and acknowledged. #2518

Merged
merged 4 commits into from Sep 30, 2022

Conversation

pderop
Copy link
Member

@pderop pderop commented Sep 29, 2022

When a TLS close_notify is received and when the close_notify ack is replied, then the Netty SslHandler just does not close the channel. Instead of that, in that case it fires a SslCloseCompletionEvent.SUCCESS event down the pipeline, and the closing of the channel is left to other handlers.

And since the TLS RFC states that the party receiving a close_notify MUST respond with a close_notify alert of its own and close down the connection immediately, let's do this in Reactor-Netty.

This patch won't avoid AbortedExceptions or PrematureCloseException exceptions in case a close_notify is being received while a request is being written, or while a response is waited for, but it may address some of the pain highlighted by #2498, #2499, and #2509; and may avoid SslClosedEngineException: SSLEngine closed already " exceptions.

When a close_notify is received and acknowledged, a log is now displayed just before closing the connection (in DEBUG):

09:09:16.948 [reactor-http-nio-3] DEBUG r.n.channel.ChannelOperationsHandler - [3a3ece28-1, L:/x.x.x.x:49624 - R:/0:0:0:0:0:0:0:0:49623] Received a TLS close_notify, closing the channel now.

There are three tests, which are trying to reproduce some corner cases, for example, like in #2498, where a server sends a close_notify to the client but does not close the connection right after (maybe later).

  • test2498_close_notify_after_response_two_clients(): the test simulates a particular situation where a connection is idle and available in the client connection pool, and then the client receives a close_notify, but the server has not yet closed the connection (and may close it later). In this case, the connection is now closed immediately and removed from the pool, in order to avoid any SslClosedEngineException: SSLEngine closed already exception the next time the connection will be acquired and written. So, in the test, a client sends a first request to a server, which replies correctly. But once the last response is flushed, then the server sends a close_notify without closing the connection. In this case, on the client side, when the close_notify is received and acknowledged, then the connection is immediately closed and removed from the pool. Then a second request is sent, using a fresh new connection (because the previous one has been closed when the close_notify was received. The patch in this particular case avoids the SslClosedEngineException: SSLEngine closed already exception

  • test2498_close_notify_on_connect(): the test simulates a situation where the client is receiving a close_notify while writing request body parts to the connection. To simulate this, the server immediately sends a close_notify when the client is connecting. The request is not read and the client connection is not closed. The client is now closing the connection when receiving the close_notify and the request will be aborted with a PrematureCloseException or an AbortedException while writing the request body parts (but won't SslClosedEngineException: SSLEngine closed already exceptions anymore).

  • test2498_close_notify_on_request(): the test simulates a situation where the client has fully written its request to a connection, but while waiting for the response, then a close_notify is received, and the server has not yet closed the connection (it may close it later). So in the test, the server fully reads the client request, but does not respond and sends a close_notify without closing the socket. The client, when receiving the close_notify, will send the close_notify ack and will close the channel, and the request will be aborted immediately with a PrematureCloseException.

Fixes #2498,#2499,#2509

@pderop pderop self-assigned this Sep 29, 2022
@pderop pderop added this to the 1.0.24 milestone Sep 29, 2022
@pderop pderop marked this pull request as draft September 30, 2022 06:59
@pderop pderop changed the title Close connection when a TLS close_notify is received and acknowledge. Close connection when a TLS close_notify is received and acknowledged. Sep 30, 2022
@pderop pderop marked this pull request as ready for review September 30, 2022 07:35
@pderop
Copy link
Member Author

pderop commented Sep 30, 2022

@violetagg , can you please have a look ? thanks.

@pderop pderop added the type/enhancement A general enhancement label Sep 30, 2022
…od does not need to declare a thrown exception.
@pderop
Copy link
Member Author

pderop commented Sep 30, 2022

applied feedbacks, can you recheck please ?

@violetagg
Copy link
Member

ship it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment