Handle early disconnects before SSL handshake by pvlugter · Pull Request #596 · pgjdbc/r2dbc-postgresql

pvlugter · 2023-06-13T04:05:10Z

Handle the inbound connection closing before SSL negotiation and always complete the SSL handshake future.

At first, only added the channel inactive lifecycle event in SSLSessionHandlerAdapter. But with the channel pipeline being updated after connect, found that the inbound could already be closed before the handler was added (could check if it's active) but the channel could also be unregistered and outside the event loop and then the handler added event would be pending and never called. So reworked the pipeline changes so that the SSL adapter is added on channel init, so all lifecycle events are received. Pass the handshake Mono via a channel attribute — not sure if there's a better way, and the SSL adapter may already have swapped itself for the underlying SSL handler.

Simple integration test for checking that disconnects are completed exceptionally, simulating what we've seen with Google Cloud SQL. Before these changes, this test will hang. No test specifically for the races between the disconnects and the handler lifecycle events, but have tested these changes manually under load.

pvlugter · 2023-06-13T04:07:18Z

src/main/java/io/r2dbc/postgresql/client/ReactorNettyClient.java


-        connection.addHandlerFirst(new EnsureSubscribersCompleteChannelHandler(this.requestSink));
+        connection.addHandlerLast(new EnsureSubscribersCompleteChannelHandler(this.requestSink));
        connection.addHandlerLast(new LengthFieldBasedFrameDecoder(Integer.MAX_VALUE - 5, 1, 4, -4, 0));


Keeps the same handler order. The SSL adapter is already added first.

pvlugter · 2023-06-13T04:07:50Z

src/main/java/io/r2dbc/postgresql/client/ReactorNettyClient.java

            InternalLogger logger = InternalLoggerFactory.getInstance(ReactorNettyClient.class);
            if (logger.isTraceEnabled()) {
                pipeline.addFirst(LoggingHandler.class.getSimpleName(),
                    new LoggingHandler(ReactorNettyClient.class, LogLevel.TRACE));
            }


Trace logging handler also added on channel init, so it logs all lifecycle events.

pvlugter · 2023-06-13T04:09:30Z

src/main/java/io/r2dbc/postgresql/client/SSLSessionHandlerAdapter.java

            completeHandshakeExceptionally(e);
        } else {
            completeHandshake();
+            ctx.channel().pipeline().remove(this);


Noticed this path as well, which could be reached if the server is ssl=off and the client is sslmode=prefer. Probably not tested currently, but the adapter would still be in the pipeline and fail subsequent reads.

mp911de

Thanks a lot. Closing the potential gaps that can leave a connection lingering makes sense. I left a few comments to provide guidance toward some changes.

mp911de · 2023-06-16T08:54:50Z

src/main/java/io/r2dbc/postgresql/client/SSLSessionHandlerAdapter.java

-    public void handlerAdded(ChannelHandlerContext ctx) {
+    public void channelActive(ChannelHandlerContext ctx) {
        Mono.from(SSLRequest.INSTANCE.encode(this.alloc)).subscribe(ctx::writeAndFlush);
+        ctx.fireChannelActive();


We can resort to calling super.channelActive().

Yes, wasn't sure what would be preferred. Updated in d0bbd9d.

mp911de · 2023-06-16T08:55:07Z

src/main/java/io/r2dbc/postgresql/client/SSLSessionHandlerAdapter.java

+        // If we receive channel inactive before removing this handler, then the inbound has closed early.
+        PostgresqlSslException e = new PostgresqlSslException("Connection closed during SSL negotiation");
+        completeHandshakeExceptionally(e);
+        ctx.fireChannelInactive();


let's call super.channelInactive()

mp911de · 2023-06-16T08:56:24Z

src/main/java/io/r2dbc/postgresql/client/SSLSessionHandlerAdapter.java


    @Override
-    public void handlerAdded(ChannelHandlerContext ctx) {
+    public void channelActive(ChannelHandlerContext ctx) {


Do we require the same changes in SSLTunnelHandlerAdapter?

SSLTunnelHandlerAdapter immediately swaps itself for the underlying SslHandler, which already handles the disconnects, so shouldn't need any changes. Added a similar integration test for tunnel mode: 6de187d

mp911de · 2023-06-16T08:57:47Z

src/main/java/io/r2dbc/postgresql/client/ReactorNettyClient.java

-        return tcpClient.connect().flatMap(it -> {
-
-            ChannelPipeline pipeline = it.channel().pipeline();
+        return tcpClient.doOnChannelInit((observer, channel, remoteAddress) -> {


Using doOnChannelInit does make sense to avoid the gap of having a closed channel.

mp911de · 2023-06-16T08:59:14Z

src/main/java/io/r2dbc/postgresql/client/ReactorNettyClient.java


-        return Mono.empty();
+    private static Mono<Void> getSslHandshake(Channel channel) {
+        Mono<Void> sslHandshake = channel.attr(SSL_HANDSHAKE_KEY).getAndSet(null);


Instead of an attribute we could walk the handler pipeline and obtain the handshake mono from the AbstractPostgresSSLHandlerAdapter object.

As mentioned in original comment, since the handshake mono is under connect.flatMap and the SSL adapter swaps itself for the underlying SslHandler, the SSL adapter can already be removed from the pipeline at this point. Definitely for SSLTunnelHandlerAdapter, which swaps on handler added. Assuming it's possible for SSLSessionHandlerAdapter as well, based on timing. Or am I missing something? Maybe if the SSL tunnel adapter doesn't swap for the SSL handler until after connection, and it's clear that the adapters will still be in the pipeline.

This is an indication that the idea of removing the SSL handler isn't ideal. The SSLSessionHandlerAdapter performs one-time-only ops by sending the SSL request upon connect and reading the result afterwards. If we introduce state to the handler to remember that we've sent the request and that we've consumed its result, then we could skip the respective activity in channelActive and channelRead and keep the handler within the pipeline.

Right, not removing the handlers is the alternative. So if leaving them in the pipeline, as noop handlers after the negotiation, is preferred, then I'll update to that.

Updated in ~~0ba6b07~~ d40226f

pvlugter · 2023-07-08T02:05:45Z

@mp911de do you have time for another review here? Would be great to have this fix in a release to use.

[#595][#596] Signed-off-by: Mark Paluch <mpaluch@vmware.com>

[resolves #595][#596]

[#595][#596] Signed-off-by: Mark Paluch <mpaluch@vmware.com>

mp911de · 2023-07-11T11:50:43Z

Thank you for your contribution. That's merged, polished, and backported now.

Handle early disconnects before SSL handshake

d05f93e

pvlugter commented Jun 13, 2023

View reviewed changes

mp911de added this to the 1.0.2.RELEASE milestone Jun 15, 2023

mp911de added the type: bug A general bug label Jun 15, 2023

mp911de linked an issue Jun 15, 2023 that may be closed by this pull request

Creating connections can hang during server downtime #595

Closed

mp911de requested changes Jun 16, 2023

View reviewed changes

pvlugter added 2 commits June 17, 2023 13:20

update: super calls rather than ctx fire directly

d0bbd9d

update: add downtime integration test for ssl tunnel mode

6de187d

pvlugter force-pushed the gh-595-ssl-handler branch from 0ba6b07 to ba6b2f4 Compare June 26, 2023 22:37

update: keep SSL handler adapters in the pipeline

d40226f

pvlugter force-pushed the gh-595-ssl-handler branch from ba6b2f4 to d40226f Compare June 26, 2023 22:41

mp911de merged commit 70afe5f into pgjdbc:main Jul 11, 2023

mp911de added a commit that referenced this pull request Jul 11, 2023

Polishing.

ceea9e1

[#595][#596] Signed-off-by: Mark Paluch <mpaluch@vmware.com>

mp911de pushed a commit that referenced this pull request Jul 11, 2023

Handle early disconnects before SSL handshake.

b402400

[resolves #595][#596]

mp911de added a commit that referenced this pull request Jul 11, 2023

Polishing.

0d612f8

[#595][#596] Signed-off-by: Mark Paluch <mpaluch@vmware.com>

pvlugter deleted the gh-595-ssl-handler branch July 11, 2023 22:20

Conversation

pvlugter commented Jun 13, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mp911de left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvlugter Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pvlugter commented Jul 8, 2023

Uh oh!

mp911de commented Jul 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pvlugter Jun 26, 2023 •

edited

Loading