vert-x3 / vertx-mqtt

Vert.x MQTT
Apache License 2.0
184 stars 88 forks source link

Allow Websocket URLs with Query Strings and make query string accessible for validations (extension to #208) #221

Open nanosek opened 2 years ago

nanosek commented 2 years ago

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=

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.

nanosek commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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