Add TLS close_notify handler to http server #109899

mhl-b · 2024-06-19T05:11:54Z

"close_notify" alert is used to indicate orderly closure of one
direction of the connection. Upon receiving such an alert, the TLS
implementation SHOULD indicate end-of-data to the application.

This PR addresses issue when HTTP client sends TLS close_notify alert and wait for response from our HTTP server.
But we dont handle close_notify alerts today, only full connection termination. Some clients might hang for a while until other timeouts fire up.

In this change I introduce an event listener for SslCloseCompletionEvent in the Netty4HttpPipeliningHandler, that handles close_notify alerts. When we receive alert we will close connection immediately. It might introduce server response truncation, but we rely on client to send close_notify when no more data is expected from the server.

Added warning logging, can be spotted now in unit and integ tests

[2024-06-25T21:08:49,310][WARN ][o.e.h.n.Netty4HttpPipeliningHandler] [node_t1] received TLS close_notify, closing connection [id: 0xfb96648b, L:/[0:0:0:0:0:0:0:1]:13484 - R:/[0:0:0:0:0:0:0:1]:63937]

Fixes #76642

elasticsearchmachine · 2024-06-19T05:13:27Z

Pinging @elastic/es-distributed (Team:Distributed)

DaveCTurner · 2024-06-19T05:30:25Z

...earch/xpack/security/transport/netty4/SecurityNetty4HttpServerTransportCloseNotifyTests.java

+ var cert = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt");
+ var pk = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.pem");


Aha nice find! Although it seems hardly ideal to be using a cert which exposed in 2019...

I tried to use ssc = new SelfSignedCertificate(); from netty, it relies on bouncy-castle, but we dont have it in dependencies. Should I update deps or static cert?

I think we have good reasons not to rely on bouncy castle in prod code but it seems like it should be ok for tests IMO. If it's easy to add then please do so and we can raise it for discussion as the PR nears completion.

Made netty's self-signed-cert work with few security suppressions and bounce-castle test dependnecy

@ESTestCase.WithoutSecurityManager @SuppressForbidden(reason = "requires java.io.File for netty self-signed certificate")

DaveCTurner · 2024-06-19T05:32:16Z

...earch/xpack/security/transport/netty4/SecurityNetty4HttpServerTransportCloseNotifyTests.java

+
+import javax.net.ssl.SSLException;
+
+public class SecurityNetty4HttpServerTransportCloseNotifyTests extends AbstractHttpServerTransportTestCase {


Tests make sense, but I think we'll also need some integ tests showing how this interacts with the rest of the system, particularly the task cancellation mechanism (which it doesn't today, but it'll need to).

I'm still thinking should we cancel or not. If we serve all requests while channel is writable then we dont need additional handling for cancelation. If channel is not writable, client decided not to wait, it will go through exception path, which exists today.

Although it is certainly possible to keep serving responses as you propose, it's also legitimate to cancel tasks and send the resulting exception responses, or indeed to just tear down the connection without sending any of the remaining responses, so this is more of a design question than a technical absolute.

I couldn't find any clients which might send a close_notify while awaiting responses - AFAICT this only happens on the abort path or when dropping an idle connection from a pool.

You are right, looking further into HTTP and TLS RFC's closing connection on close_notify make sense. Serving all responses and closing connection should be handled with Connection: close header in HTTP1.

In TLS prior to v1.3 close_notify should discard all pending writes and close connection. TLS v1.3 tries to address truncation attack by letting application layer to figure out how to close connection, we probably want to handle logout requests (oidc, saml) fully without truncation. I think in scope of this PR closing connection should be a good step forward.

I don't think truncation attacks are a possibility here, we always send either a Content-length header or else a properly-terminated chunked-encoded message so clients can tell if a response is incomplete at the HTTP layer. But this isn't the right place to discuss this more deeply - if you think there's a security issue in this area then please mail [email protected] with details.

Since there is no cancellation now,

I think task cancellation should be hooked-up by default with this implementation. Since you close the channel, that will propagate through the listener framework and cancel tasks (I think).

Netty4HttpChannel#addCloseListener.

Nice! Is it worth to test cancellation in integ test? If yes, what rest operation would be a good example?

Nice! Is it worth to test cancellation in integ test?

I will take another look at this PR later today.

But IMO, if "HTTP Close triggers Task cancellation" triggers task cancellation is already tested, then this PR only needs to validate the "SSL client close_notify" leads to channel close.

However, I don't know if that test exists. My initial perusal of the code is that we do not have this test at an HTTP level (which we should). I would say this PR should implement it if it is missing.

I would look in RestActionCancellationIT and other classes in that qa package (SearchRestCancellationIT, ClusterHealthRestCancellationIT, etc).

It looks like these are testing task cancellation through the http layer. I would do a bit of investigation and look at how the "cancellation" is implemented. I think it might be through closing the http connection. But that appears to be a bit deep in the apache code and we should confirm it.

If those tests are sufficient then I think we are covered at the moment.

I added action cancellation test.

RestActionCancellationIT and SearchRestCancellationIT call request.abort() which indeed close connection.
With help of neat plugin CancellableActionTestPlugin I found it's pretty easy to add integ test here for action cancellation - testSendCloseNotifyCancelAction

...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

Tim-Brooks · 2024-06-27T04:33:33Z

...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java

+ if (evt instanceof SslCloseCompletionEvent closeEvent) {
+ if (closeEvent.isSuccess() && ctx.channel().isActive()) {
+ logger.trace("received TLS close_notify, closing connection {}", ctx.channel());
+ ctx.close();


I have a minor preference to use:

ctx.channel().close();

to ensure that the close initiates from the tail of the pipeline (should we added handlers after this in the future).

Tim-Brooks · 2024-06-27T04:38:40Z

...earch/xpack/security/transport/netty4/SecurityNetty4HttpServerTransportCloseNotifyTests.java

+
+ @Override
+ public void dispatchRequest(RestRequest request, RestChannel channel, ThreadContext threadContext) {
+ reqQueue.add(new ReqCtx(request, channel, threadContext));


I have not investigated the test failure much. But I'm pretty sure these are going to need to be released at the end of the test.

Or else the leak detector will fire.

Tim-Brooks

Looks pretty good. Just one small comment and a note about a failing test.

mhl-b · 2024-06-28T05:07:10Z

Looks pretty good. Just one small comment and a note about a failing test.

Thanks @Tim-Brooks for the feedback! I fixed leaking buffers.

Tim-Brooks

LGTM

Some suggestions from a post-merge review of elastic#109899: - Reduce scope of `SuppressForbidden` to a single method. - Reinstate security manager. - Remove hard-coded BouncyCastle version. - Sometimes run on multi-node cluster. - Use `SAFE_AWAIT_TIMEOUT` for waits. - Remove unnecessary `assertBusy()`

Relates: elastic#109899 Resolves: elastic#110356

Relates: #109899 Resolves: #110356

Some suggestions from a post-merge review of #109899: - Sometimes run on multi-node cluster. - Use `SAFE_AWAIT_TIMEOUT` for waits. - Remove unnecessary `assertBusy()`

add tls close_notify handler to http server

dfeae2d

mhl-b added v8.15.0 :Distributed/Network Http and internode communication implementations Team:Distributed Meta label for distributed team >bug labels Jun 19, 2024

mhl-b requested review from DaveCTurner and Tim-Brooks June 19, 2024 05:13

DaveCTurner reviewed Jun 19, 2024

View reviewed changes

mhl-b added 3 commits June 18, 2024 23:05

Merge remote-tracking branch 'upstream/main' into close-notify

1922740

Merge remote-tracking branch 'upstream/main' into close-notify

1203e05

close connection immediately on close_notify

724fd5b

mhl-b requested a review from a team as a code owner June 21, 2024 23:43

mhl-b added 3 commits June 21, 2024 16:58

cleanup

eff01c9

cleanup

be335a9

naming

0b29ca0

mhl-b removed the request for review from a team June 22, 2024 00:14

mhl-b added 2 commits June 25, 2024 12:12

Merge remote-tracking branch 'upstream/main' into close-notify

a5ba5e5

add integ test

9824649

Tim-Brooks reviewed Jun 26, 2024

View reviewed changes

...ransport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpPipeliningHandler.java Outdated Show resolved Hide resolved

change logging from warn to trace

aa27872

mhl-b requested review from Tim-Brooks and DaveCTurner June 26, 2024 17:20

mhl-b added 4 commits June 26, 2024 14:46

add action cancellation integ test

1411fb3

Merge remote-tracking branch 'upstream/main' into close-notify

2dc80fa

add all tasks finished assertion

2d1a70c

fix unit test

716eec1

Tim-Brooks reviewed Jun 27, 2024

View reviewed changes

mhl-b added 2 commits June 27, 2024 11:15

fix leaking buffers in tests

a239605

spotless

fd7e37c

Tim-Brooks approved these changes Jun 28, 2024

View reviewed changes

mhl-b added 3 commits June 28, 2024 10:53

Merge remote-tracking branch 'upstream/main' into close-notify

f55b163

Merge remote-tracking branch 'upstream/main' into close-notify

ff4907f

Merge remote-tracking branch 'upstream/main' into close-notify

75a5fbc

mhl-b added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jun 28, 2024

elasticsearchmachine merged commit 9b6cca1 into elastic:main Jun 28, 2024
15 checks passed

DaveCTurner mentioned this pull request Jun 30, 2024

Tidy up SecurityNetty4TransportCloseNotifyIT #110311

Merged

brianseeders mentioned this pull request Jul 1, 2024

[CI] Many tests failing when testing with fips #110356

Closed

ywangd added a commit to ywangd/elasticsearch that referenced this pull request Jul 2, 2024

[Test] Reuse existing TLS setup infrastructure in tests

50d4662

Relates: elastic#109899 Resolves: elastic#110356

ywangd mentioned this pull request Jul 2, 2024

[Test] Reuse existing TLS setup infrastructure in tests #110358

Merged

ywangd added a commit that referenced this pull request Jul 2, 2024

[Test] Reuse existing TLS setup infrastructure in tests (#110358)

94fe32a

Relates: #109899 Resolves: #110356

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS close_notify handler to http server #109899

Add TLS close_notify handler to http server #109899

mhl-b commented Jun 19, 2024 •

edited

Loading

elasticsearchmachine commented Jun 19, 2024

DaveCTurner Jun 19, 2024

mhl-b Jun 19, 2024

DaveCTurner Jun 19, 2024

mhl-b Jun 21, 2024 •

edited

Loading

DaveCTurner Jun 19, 2024

mhl-b Jun 19, 2024

DaveCTurner Jun 19, 2024

mhl-b Jun 20, 2024 •

edited

Loading

DaveCTurner Jun 20, 2024

Tim-Brooks Jun 26, 2024

mhl-b Jun 26, 2024

Tim-Brooks Jun 26, 2024

Tim-Brooks Jun 26, 2024

mhl-b Jun 26, 2024

Tim-Brooks Jun 27, 2024

Tim-Brooks Jun 27, 2024

Tim-Brooks Jun 27, 2024

Tim-Brooks left a comment

mhl-b commented Jun 28, 2024 •

edited

Loading

Tim-Brooks left a comment

		var cert = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.crt");
		var pk = getDataPath("/org/elasticsearch/xpack/security/transport/ssl/certs/simple/testnode.pem");


		import javax.net.ssl.SSLException;

		public class SecurityNetty4HttpServerTransportCloseNotifyTests extends AbstractHttpServerTransportTestCase {

Add TLS close_notify handler to http server #109899

Add TLS close_notify handler to http server #109899

Conversation

mhl-b commented Jun 19, 2024 • edited Loading

elasticsearchmachine commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhl-b Jun 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhl-b Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Brooks left a comment

Choose a reason for hiding this comment

mhl-b commented Jun 28, 2024 • edited Loading

Tim-Brooks left a comment

Choose a reason for hiding this comment

mhl-b commented Jun 19, 2024 •

edited

Loading

mhl-b Jun 21, 2024 •

edited

Loading

mhl-b Jun 20, 2024 •

edited

Loading

mhl-b commented Jun 28, 2024 •

edited

Loading