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 URLs with Query Strings and make query string accessible for validations (extension to #208) #221

Open
nanosek opened this issue Jun 10, 2022 · 3 comments

Comments

@nanosek
Copy link

nanosek commented Jun 10, 2022

Describe the feature

With #208 it is now possible to access the http headers and the URI that was used by MQTT clients to connect to the MQTT Broker. But unfortunately it seems access to the URI is not of much use for the endpoint handler because the only allowed base address of the URI that the Server will accept correctly is "/mqtt" (without any query parameters).

Would it be possible to allow at least /mqtt?key=value style URLs to end up correctly in the EndpointHandler so that authentication parameters can be given with the URI to decide inside the endpoint handler if a connection attempt is legitimate?

I was unsure if this should already work (and this ticket should be a bug) or if it is a new feature.

Use cases

Authentication of clients based on a URL query parameter (concret mqtt url used by the client: ws://[hostname]:[port]/mqtt?auth=<AUTHENTICATION_TOKEN>

The reason why I would like to be able to access the authentication token inside the MQTT Endpoint Handler is that I want to be sure that the authentication token in the URL (which potentially is validated upstream by a reverse proxy) matches the given mqtt username/clientID/pw combination which I can only validate inside the MQTT Endpoint Handler - so that I can be sure that MQTT topic permissions match the identity of the WS connection.

@vietj vietj added this to the 4.3.2 milestone Jun 10, 2022
@nanosek
Copy link
Author

nanosek commented Jun 10, 2022

I am testing with 4.2.4 - and there it seems that the websocket handshake is not completed as soon as you add a query parameter - probably (but not yet confirmed) due to "io.netty.handler.codec.http.websocketx.WebSocketServerProtocolConfig#checkStartsWith set to false because it is configured like that in io.vertx.mqtt.impl.MqttServerImpl#initChannel

The current behaviour is:
11:12:18.437 [vert.x-eventloop-thread-2] WARN io.netty.channel.DefaultChannelPipeline - An exceptionCaught() event was fired, and it reached at the tail of the pipeline. It usually means the last handler in the pipeline did not handle the exception.
java.lang.Exception: Wrong message type io.netty.handler.codec.http.HttpObjectAggregator$AggregatedFullHttpRequest
at io.vertx.mqtt.impl.MqttServerConnection.handleMessage(MqttServerConnection.java:213)
at io.vertx.mqtt.impl.MqttServerImpl.lambda$null$1(MqttServerImpl.java:102)
at io.vertx.core.impl.EventLoopContext.emit(EventLoopContext.java:50)

@nanosek
Copy link
Author

nanosek commented Jun 13, 2022

@vietj : I am willing to help - but it seems to be indeed only a very small change - so if you have anyway something open in the area you can probably just sneak it it in.
The WebSocketServerProtocolConfig#checkStartsWith parameter set to "true" seems to solve it.

Inside io.vertx.mqtt.impl.MqttServerImpl#initChannel

`

//version with configurable startsWith parameter
if(options.isUseWebSocket()) {

  int maxFrameSize = options.getWebSocketMaxFrameSize();
  boolean allowStartsWithURLs= options.isWebSocketAcceptURLsThatStartsWithBaseUrl();

  pipeline.addBefore("mqttEncoder", "httpServerCodec", new HttpServerCodec());
  pipeline.addAfter("httpServerCodec", "aggregator", new HttpObjectAggregator(maxFrameSize));

  pipeline.addAfter("aggregator", "webSocketHandler",
    new WebSocketServerProtocolHandler("/mqtt", MQTT_SUBPROTOCOL_CSV_LIST, false, maxFrameSize, false, allowStartsWithURLs,
      10000L));

  pipeline.addAfter("webSocketHandler", "bytebuf2wsEncoder", new ByteBufToWebSocketFrameEncoder());
  pipeline.addAfter("bytebuf2wsEncoder", "ws2bytebufDecoder", new WebSocketFrameToByteBufDecoder());
}

`

The alternative would be to give the user a chance to replace the complete WebSocketServerProtocolHandler with one that he creates via a callback or overload - this would also solve various other configuration demands in a one shot
(but you did not go that route for the maxFrameSize recently - so there are probably reasons).

Greetings!

@vietj
Copy link
Contributor

vietj commented Jun 13, 2022

so I think there are two things here

1/ we should correctly handle a WebSocket handshake failure
2/ we should also allow such URI to be valid I think

so you can provide a PR for that with a test

@vietj vietj modified the milestones: 4.3.4, 4.3.5 Oct 1, 2022
@vietj vietj modified the milestones: 4.3.5, 4.4.0 Nov 18, 2022
@vietj vietj modified the milestones: 4.4.0, 4.4.1 Mar 2, 2023
@vietj vietj modified the milestones: 4.4.1, 4.4.2 Mar 31, 2023
@vietj vietj modified the milestones: 4.4.2, 4.4.3 May 12, 2023
@vietj vietj modified the milestones: 4.4.3, 4.4.4-SNAPSHOT, 4.4.4 Jun 7, 2023
@vietj vietj modified the milestones: 4.4.4, 4.4.5 Jun 22, 2023
@vietj vietj modified the milestones: 4.4.5, 4.4.6 Aug 30, 2023
@vietj vietj modified the milestones: 4.4.6, 4.5.0 Sep 12, 2023
@vietj vietj modified the milestones: 4.5.0, 4.5.1 Nov 15, 2023
@vietj vietj modified the milestones: 4.5.1, 4.5.2 Dec 13, 2023
@vietj vietj modified the milestones: 4.5.2, 4.5.3 Jan 30, 2024
@vietj vietj modified the milestones: 4.5.3, 4.5.4 Feb 6, 2024
@vietj vietj modified the milestones: 4.5.4, 4.5.5 Feb 22, 2024
@vietj vietj modified the milestones: 4.5.5, 4.5.6 Mar 14, 2024
@vietj vietj modified the milestones: 4.5.6, 4.5.7, 4.5.8 Mar 21, 2024
@vietj vietj modified the milestones: 4.5.8, 4.5.9 May 24, 2024
@vietj vietj modified the milestones: 4.5.9, 4.5.10 Jul 17, 2024
@vietj vietj modified the milestones: 4.5.10, 4.5.11 Sep 4, 2024
@vietj vietj modified the milestones: 4.5.11, 4.5.12 Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants