Skip to content

RejectedSetupException on auth failure results in ClosedChannelException #1092

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

Closed
mdindoffer opened this issue Jun 9, 2023 · 6 comments · Fixed by #1117
Closed

RejectedSetupException on auth failure results in ClosedChannelException #1092

mdindoffer opened this issue Jun 9, 2023 · 6 comments · Fixed by #1117
Labels
bug regression superseded Issue is superseded by another

Comments

@mdindoffer
Copy link

RSocket server can do connection-level authentication checks via socketAcceptor interceptors. An interceptor can check for valid authentication in the setup payload and decide whether to forward the accept the connection, or reply with a Mono#error of io.rsocket.exceptions.RejectedSetupException with rejection details in the exception message.

This authentication approach has worked flawlessly on 1.1.3, but on 1.1.4 the clients don't receive the RejectedSetupException anymore. Rather the connection is abruptly closed with ClosedChannelException.

Expected Behavior

Clients should receive the error with RejectedSetupException throwable containing the details when authentication fails.

Actual Behavior

The TCP connection is eagerly shut down and clients receive java.nio.channels.ClosedChannelException.

I tried bisecting the commits since the 1.1.3 git tag and it seems it works fine until at least 00d8311a7436fd2421dbcefb13f744bc4973184e, and then it gets weird, but I'm not sure if those next commits are self-contained changes...

Steps to Reproduce

The following example runs fine with 1.1.3, but fails with 1.1.4:

public class AuthenticationTest {

    private static final Logger LOG = LoggerFactory.getLogger(AuthenticationTest.class);
    private static final int PORT = 23200;

    @Test
    void authTest() {
        createServer().block();
        RSocket rsocketClient = createClient().block();

        StepVerifier.create(
                        rsocketClient.requestResponse(DefaultPayload.create("Client: Hello"))
                )
                .expectError(RejectedSetupException.class)
                .verify();
    }

    private static Mono<CloseableChannel> createServer() {
        LOG.info("Starting server at port {}", PORT);
        RSocketServer rSocketServer = RSocketServer.create((connectionSetupPayload, rSocket) -> Mono.just(new MyServerRsocket()));

        TcpServer tcpServer = TcpServer.create()
                .host("localhost")
                .port(PORT);

        return rSocketServer
                .interceptors(interceptorRegistry -> interceptorRegistry.forSocketAcceptor(socketAcceptor -> (setup, sendingSocket) -> {
                    if (true) {//TODO here would be an authentication check based on the setup payload
                        return Mono.error(new RejectedSetupException("ACCESS_DENIED"));
                    } else {
                        return socketAcceptor.accept(setup, sendingSocket);
                    }
                }))
                .bind(TcpServerTransport.create(tcpServer))
                .doOnNext(closeableChannel -> LOG.info("RSocket server started."));
    }

    private static Mono<RSocket> createClient() {
        LOG.info("Connecting....");
        return RSocketConnector.create().connect(TcpClientTransport.create(TcpClient.create()
                        .host("localhost")
                        .port(PORT)))
                .doOnNext(rSocket -> LOG.info("Successfully connected to server"))
                .doOnError(throwable -> LOG.error("Failed to connect to server"));
    }

    public static class MyServerRsocket implements RSocket {
        private static final Logger LOG = LoggerFactory.getLogger(MyServerRsocket.class);

        @Override
        public Mono<Payload> requestResponse(Payload payload) {
            LOG.info("Got a request with payload: {}", payload.getDataUtf8());
            return Mono.just("Response data blah blah blah")
                    .map(DefaultPayload::create);
        }
    }
}

Your Environment

  • RSocket 1.1.4 (via rsocket-bom)
  • JDK 17
@ianbrandt
Copy link

Just FYI, I filed what seems like a very similar regression between 1.1.2 and 1.1.3: #1087.

@mdindoffer
Copy link
Author

I just want to add here that this is blocking us (and probably others) from upgrading to 1.1.4.
What's worse is that due to 1.1.3 depending on old netty, we are also stuck on Reactor 2020.0.24 which is now 14 months old.

@OlegDokuka Do you know of a suitable workaround for this bug?

@OlegDokuka
Copy link
Member

@mdindoffer I'm back to work. Let me fix it quickly!

Thank you for being patient all that time!

@mdindoffer
Copy link
Author

@OlegDokuka I understand this is not a priority for you, and that's fine. But we'd really love to upgrade the rest of the reactive stack at least (i.e. bumping the whole reactor-bom).

So please forgive me for asking again - is there a way to decouple RSocket from the rest of the Reactor stack (core, reactor-netty, netty) in a way that enables safe upgrade path with RSocket 1.1.3? See #1082 (comment) . I imagine locking individual netty artifacts is a bad idea.

Alternatively, can you think of a workaround for the premature closing of the channel?

@rstoyanchev
Copy link
Contributor

After debugging with @OlegDokuka we've narrowed this.

Commit c65683e intended to change how DuplexConnection#sendErrorAndClose works. Instead of calling sendObject on the Reactor Netty connection to write immediately, it passes it to UnboundedProcessor to give allow it to drain the queue with any other messages first.

This works okay, and Reactor Netty writes the error frame, but it is not flushed immediately. As the setup is rejected from a doOnNext callback, the error signal continues to propagate unhandled until it reaches Reactor Netty in ChannelOperations#onError, which closes the connection and removes the channel handlers before the error frame is flushed.

rstoyanchev added a commit to rstoyanchev/rsocket-java that referenced this issue Jan 24, 2025

Verified

This commit was signed with the committer’s verified signature. The key has expired.
6543 6543
@rstoyanchev rstoyanchev changed the title 1.1.4 Regression - RejectedSetupException on auth failure results in ClosedChannelException RejectedSetupException on auth failure results in ClosedChannelException Jan 24, 2025
rstoyanchev added a commit that referenced this issue Jan 24, 2025
Closes gh-1092

Signed-off-by: rstoyanchev <rossen.stoyanchev@broadcom.com>
@rstoyanchev rstoyanchev linked a pull request Jan 24, 2025 that will close this issue
@rstoyanchev
Copy link
Contributor

Superseded by #1117.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Jan 24, 2025
@rstoyanchev rstoyanchev added the superseded Issue is superseded by another label Jan 24, 2025
OlegDokuka pushed a commit that referenced this issue Jan 24, 2025
Closes gh-1092
rstoyanchev added a commit to rstoyanchev/rsocket-java that referenced this issue Jan 31, 2025
Fixes rsocketgh-1092

Signed-off-by: rstoyanchev <rossen.stoyanchev@broadcom.com>
@rstoyanchev rstoyanchev removed this from the 1.1.5 milestone Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug regression superseded Issue is superseded by another
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants