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

WebSockets Next: add convenient way to handle the subprotocol header #39465

Closed
mkouba opened this issue Mar 15, 2024 · 5 comments · Fixed by #39624
Closed

WebSockets Next: add convenient way to handle the subprotocol header #39465

mkouba opened this issue Mar 15, 2024 · 5 comments · Fixed by #39624
Labels
Milestone

Comments

@mkouba
Copy link
Contributor

mkouba commented Mar 15, 2024

Description

https://datatracker.ietf.org/doc/html/rfc6455#section-11.3.4

Currently, the only thing a user can do is obtain the header value with connection.handshakeRequest().header("Sec-WebSocket-Protocol").

Implementation ideas

No response

@mkouba
Copy link
Contributor Author

mkouba commented Mar 20, 2024

For the record, it seems that Vert.x only makes it possible to define supported subprotocols globally with HttpServerOptions.setWebSocketSubProtocols(List<String>). In other words, I don't think we would be able to support per endpoint subprotocol handling, i.e. something like jakarta.websocket.server.ServerEndpoint.subprotocols(), with the current Vert.x API.

That said, we might add the WebSocketConnection#subProtocol() to obtain the subprotocol selected during handshake 🤷.

CC @cescoffier @vietj

mkouba added a commit to mkouba/quarkus that referenced this issue Mar 21, 2024
- add WebSocketConnection#subprotocol() that can be used to obtain the
subprotocol selected by the handshake
- the values defined with quarkus.websockets-next.supported-subprotocols
contribute to the set of subprotocols passed to the HTTP server
configuration
- also add constants for handshake headers defined by the RFC
- resolves quarkusio#39465
mkouba added a commit to mkouba/quarkus that referenced this issue Mar 21, 2024
- add WebSocketConnection#subprotocol() that can be used to obtain the
subprotocol selected by the handshake
- the values defined with quarkus.websockets-next.supported-subprotocols
contribute to the set of subprotocols passed to the HTTP server
configuration
- also add constants for handshake headers defined by the RFC
- resolves quarkusio#39465
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Mar 22, 2024
ketola pushed a commit to ketola/quarkus that referenced this issue Mar 23, 2024
- add WebSocketConnection#subprotocol() that can be used to obtain the
subprotocol selected by the handshake
- the values defined with quarkus.websockets-next.supported-subprotocols
contribute to the set of subprotocols passed to the HTTP server
configuration
- also add constants for handshake headers defined by the RFC
- resolves quarkusio#39465
@emattheis
Copy link
Contributor

@mkouba I stumbled across this while reviewing the latest WebSockets Next work (nice job, by the way, it's looking good!). I wanted to pass along an interesting thing I discovered while playing around with the low-level Vert.x websockets support. As it turns out, Netty has a magic value you can configure so that it will accept any subprotocol:

https://github.com/netty/netty/blob/151dfa083d28e995a18f7d2c73d4a7d3b7ab73b2/codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketServerHandshaker.java#L69-L72

I was able to configure this using the normal Quarkus mechanism:

import io.quarkus.vertx.http.HttpServerOptionsCustomizer;
import io.vertx.core.http.HttpServerOptions;
import jakarta.enterprise.context.ApplicationScoped;

import java.util.List;

import static io.netty.handler.codec.http.websocketx.WebSocketServerHandshaker.SUB_PROTOCOL_WILDCARD;

@ApplicationScoped
public class VertxHttpConfiguration implements HttpServerOptionsCustomizer {

    @Override
    public void customizeHttpServer(HttpServerOptions options) {
        allowAnySubProtocol(options);
    }

    @Override
    public void customizeHttpsServer(HttpServerOptions options) {
        allowAnySubProtocol(options);
    }

    @Override
    public void customizeDomainSocketServer(HttpServerOptions options) {
        allowAnySubProtocol(options);
    }

    // configure the wildcard protocol so that Vert.x/Netty will use anything we rewrite in the request
    private void allowAnySubProtocol(HttpServerOptions options) {
        options.setWebSocketSubProtocols(List.of(SUB_PROTOCOL_WILDCARD));
    }
}

Then, in a reactive route I can negotiate the sub-protocol myself and whatever I write into the header will be returned by Netty:

    @Route(path = "/my/socket", methods = GET)
    public void openSocket(HttpServerRequest request) {
        var requestedProtocols = request.headers().get(SEC_WEBSOCKET_PROTOCOL);
        var negotiatedProtocol = negotiateProtocol(requestedProtocols);
        // rewrite the protocol header to control what Vert.x/Netty uses in the response
        request.headers().set(SEC_WEBSOCKET_PROTOCOL, negotiatedProtocol);
        request.toWebSocket()
                .onItem().invoke(this::connectSocket)
                .onFailure().invoke(failure -> log.error("failed to upgrade to websocket", failure))
                .subscribe().with(IGNORE);
    }

A bit of a hack, but could be used to enable programmatic sub-protocol negotiation in WebSockets Next.

@mkouba
Copy link
Contributor Author

mkouba commented Jun 19, 2024

@emattheis Thanks for your feedback!

First of all, I don't think that you need to use the HttpServerOptionsCustomizer to set the wildcard - quarkus.websockets-next.server.supported-subprotocols=* should work the same way.

As for the negotiation - recently (3.12.0.CR1) we added the HttpUpgradeCheck feature that could be used for similar stuff, i.e. you can inspect the headers and also set the negotiated protocol using HttpUpgradeCheck.HttpUpgradeContext#httpRequest().headers().

@emattheis
Copy link
Contributor

@mkouba I did this without the new extension, so the customizer seemed like the my only option. I’ll be glad to have the config property instead 😛

I’ll see if I can make the same trick work with the upgrade check, although perhaps it should be renamed as an interceptor if the intent is to allow altering the upgrade behavior instead of just inspecting/rejecting?

@mkouba
Copy link
Contributor Author

mkouba commented Jun 19, 2024

...although perhaps it should be renamed as an interceptor if the intent is to allow altering the upgrade behavior instead of just inspecting/rejecting?

We wanted to avoid confusion with CDI interceptors. The name is not set in stone yet but I can't think of a better name 🤔.

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

Successfully merging a pull request may close this issue.

2 participants