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

Allow WebSocket's ServerEndpointConfig.Configurator.modifyHandShake to control the outcome of the upgrade request #7953

Closed
dmutreja opened this issue Nov 6, 2023 · 10 comments
Assignees
Labels
3.x Issues for 3.x version branch bug Something isn't working P2 SE TOCI websocket WebSocket in Helidon
Milestone

Comments

@dmutreja
Copy link

dmutreja commented Nov 6, 2023

Our use case is to intercept a "upgrade to WebSocket" request, run some custom logic to inspect Authorization header in the HTTP request and control the outcome of that upgrade request.

The suggested interception point from Helidon team is a custom jakarta.websocket.server.ServerEndpointConfig.Configurator added to the WebSocket routing that overrides the modiyHandshake method. Custom implementation can fail the handshake (as desired) but there are couple of issues with the approach:

  • jakarta.websocket.server.HandshakeResponse supplied to modiyHandshake doesn't allow response code to be set. One can workaround this by casting down to the response TyrusUpgradeResponse
  • Response code and reason set in TyrusUpgradeResponse isn't propagated back to client. Clients see a generic handshake failure error instead.

Environment Details

  • Helidon Version: 3.2.2
  • Helidon SE
  • JDK version: "17.0.7" 2023-04-18 LTS
  • OS: MacOS
  • Docker version (if applicable): N/A

Problem Description

See above

Steps to reproduce

  • Write a custom ServerConfigurator, like so
package io.helidon.webserver.examples.websocket;

import jakarta.websocket.HandshakeResponse;
import jakarta.websocket.server.HandshakeRequest;
import jakarta.websocket.server.ServerEndpointConfig;
import org.glassfish.tyrus.core.TyrusUpgradeResponse;

import java.util.Optional;
import java.util.logging.Level;

public class FailingAuthConfigurator  extends ServerEndpointConfig.Configurator {
    private static final String WS_ACCEPT_HEADER = "Sec-WebSocket-Accept";

    @Override
    public void modifyHandshake(ServerEndpointConfig sec, HandshakeRequest request, HandshakeResponse response) {
        if (!this.isAuthenticated(sec, request)) {
            failUpgrade(response);
        }
    }

    private boolean isAuthenticated(ServerEndpointConfig sec, HandshakeRequest request) {
        // Forcefully fail to demonstrate the issue
        return false;
    }

    private void failUpgrade(HandshakeResponse response) {

        // HandShakeResponse doesn't allow setting the status
        if (response instanceof TyrusUpgradeResponse) {
            ((TyrusUpgradeResponse) response).setStatus(401);
            ((TyrusUpgradeResponse) response).setReasonPhrase("Failed to authenticate");
        }
        // response.getHeaders().computeIfPresent(WS_ACCEPT_HEADER, (k,v)->{return null;});
        // Or
        // response.getHeaders().clear()?
        // Neither choice returns a 401 to the client. In both cases, client sees a Handshake Error
        // which can be confusing for clients.
        // Tyrus client engine has the ability to send an Upgrade request with an Authentication header
        // but that's not exposed in Jakarta WebSocket API.
        response.getHeaders().clear();
    }
}
  • Add the configurator to a WebSocket routing end point
 WebServer server = WebServer.builder()
                .port(8080)
                .addRouting(WebSocketRouting.builder()
                        .endpoint("/websocket", ServerEndpointConfig.Builder.create(MessageBoardEndpoint.class, "/board")
                                .configurator(new FailingAuthConfigurator())
                                .encoders(encoders)
                                .build())
                        .build()
                )
                .build()
                .start()
                .await();
  • Start the server and connect to it using a WebSocket client. The handshake will fail but the client will see "Handshake Failure" response instead of a 401.
@spericas
Copy link
Member

spericas commented Nov 7, 2023

The problem is that Tyrus is only honoring the header updates in the modifyHandshake call. Any other changes to the response are ignored. Tyrus also supports throwing a HandshakeException, but it is a checked exception, so it cannot be thrown from modifyHandshake. The spec is not very clear about this use case either.

@spericas
Copy link
Member

spericas commented Nov 7, 2023

@m0mus m0mus added P2 bug Something isn't working TOCI 3.x Issues for 3.x version branch SE labels Nov 13, 2023
@spericas
Copy link
Member

eclipse-ee4j/tyrus#864

@spericas
Copy link
Member

@jansupol Please let us know when a new release of Tyrus with this fix becomes available

@jansupol
Copy link
Contributor

Waiting for resolution of #7954

@spericas
Copy link
Member

Tyrus was upgraded to 2.1.5. See #8277.

@spericas spericas added this to the 3.2.6 milestone Jan 25, 2024
@dmutreja
Copy link
Author

We picked up version 3.2.6 but the issue persists from a Helidon client perspective. Status set in the updated Tyrus version is still not propagated to clients.

@dmutreja
Copy link
Author

@spericas I don't see an option to re-open this issue here. Could you please help with that? Thank you.

@spericas spericas reopened this Mar 25, 2024
@spericas
Copy link
Member

@dmutreja Could you share your WS client code that is looking for the status code?

@spericas
Copy link
Member

spericas commented Mar 26, 2024

So I went back and analyze the HTTP upgrade flow through Netty -> Helidon -> Tyrus (I didn't get this far before due the issue in Tyrus):

  1. I can confirm that Tyrus is now doing the right thing and returning the updated response from the upgrade handler

  2. Helidon has access to any status code or error returned by the handler

  3. However, the only thing that Netty supports and is able to propagate back are any updated headers and only if the upgrade request succeeds. If Helidon fails the upgrade (by looking at a status code different from 101) then Netty passes the request to the next handler in the chain, which is not useful in this case.

In summary, I think with the current integration in Helidon 3.x, we cannot support this use case as it is. It may be different in Helidon 4.x where we have full control over the WS stack (no Netty there). Have you considered any other ways to implement your auth use case?

For more information on what Netty does, you can check io.netty.handler.codec.http.HttpServerUpgradeHandler:376.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x Issues for 3.x version branch bug Something isn't working P2 SE TOCI websocket WebSocket in Helidon
Projects
Archived in project
Development

No branches or pull requests

5 participants