kaazing / gateway

Kaazing Gateway
Apache License 2.0
141 stars 84 forks source link

Gateway sends response with invalid Sec-WebSocket-Protocol header #472

Open robinzimmermann opened 8 years ago

robinzimmermann commented 8 years ago

Gateway with version:

5.0

Can the bug be reproduced in a Kaazing demo out-of-the-box?

No

Steps to reproduce:

Install ActiveMQ, a recent version that supports MQTT. You can tell, because ACTIVEMQ_HOME/conf/activemq.xml contains an MQTT <transportConnector>:

<transportConnectors>
    ...
    <transportConnector name="mqtt" uri="mqtt://0.0.0.0:1883?maximumConnections=1000&amp;wireFormat.maxFrameSize=104857600"/>
    ...
</transportConnectors>

Install Gateway 5.0.

Configure the gateway using the gateway-config.xml file at the end of this post.

Place the mqtt.html file at the end of this post into GATEWAY_HOME/web/base.

Download the Paho MQTT Javascript library and place it in GATEWAY_HOME/web/base.

Open mqtt.html in a browser: http://localhost:8000/mqtt.html. This will connect, send a message, receive an echoed response, and write it to the screen.

However in this case it will fail, and nothing will be written to the screen. The Javascript console will have an error like this:

WebSocket connection to 'ws://localhost:8080/mqtt' failed: Error during WebSocket handshake: Sent non-empty 'Sec-WebSocket-Protocol' header but no response was received

Using the Chrome browser, open the Developer Tools, and click the Network tab. Refresh the page. Find the row for the WebSocket connection (where the Type column says "websocket". Click the row to see the headers.

Notice the request headers have this:

Sec-WebSocket-Protocol:mqtt

and the response headers do not contain the Sec-WebSocket-Protocol: header. This is in violation of the RFC 6455 spec, which says:

The client can request that the server use a specific subprotocol by including the |Sec-WebSocket-Protocol| field in its handshake. If it is specified, the server needs to include the same field and one of the selected subprotocol values in its response for the connection to be established.

The expected behavior is that either the Sec-WebSocket-Protocol: header says "mqtt", or that the connection is closed.

Back-End System with version

ActiveMQ 5.13.2

Client Technology:

Javascript

Browser with Version:

Chrome, latest.

gateway-config.xml

<?xml version="1.0" encoding="UTF-8" ?>
<gateway-config xmlns="http://xmlns.kaazing.org/2014/09/gateway">

  <!-- =================================================================== -->

  <properties>
    <property>
      <name>gateway.host</name>
      <value>localhost</value>
    </property>
    <property>
      <name>gateway.port</name>
      <value>8000</value>
    </property>
  </properties>

  <!-- =================================================================== -->

  <service>
    <name>base-directory</name>
    <description>Directory service for base files</description>
    <accept>http://${gateway.host}:${gateway.port}/</accept>

    <type>directory</type>

    <properties>
      <directory>/base</directory>
      <welcome-file>index.html</welcome-file>
      <error-pages-directory>/error-pages</error-pages-directory>
      <options>indexes</options>
    </properties>

    <accept-options>
        <tcp.bind>${gateway.port}</tcp.bind>
    </accept-options>
  </service>

  <!-- =================================================================== -->

  <service>
    <name>mqtt-proxy</name>
    <accept>ws://${gateway.host}:8080/mqtt</accept>
    <connect>tcp://${gateway.host}:1883</connect>

    <type>proxy</type>

    <cross-site-constraint>
      <allow-origin>*</allow-origin>
    </cross-site-constraint>

  </service>

</gateway-config>

mqtt.html

<!DOCTYPE html>

<head>

  <title>MQTT Test</title>

  <script src="mqttws31.js"></script>

  <script src="//code.jquery.com/jquery-1.12.0.min.js"></script>

</head>

<body>

  <h1>MQTT Test</h1>

  <div id="output"></div>

</body>

  <script>

var output = $("#output");

// Create a client instance
client = new Paho.MQTT.Client("localhost", 8080, "robin");

// set callback handlers
client.onConnectionLost = onConnectionLost;
client.onMessageArrived = onMessageArrived;

// connect the client
client.connect({onSuccess:onConnect});

// called when the client connects
function onConnect() {
  // Once a connection has been made, make a subscription and send a message.
  console.log("onConnect");
  output.append("onConnect<br/>");
  client.subscribe("/World");
  message = new Paho.MQTT.Message("Hello");
  message.destinationName = "/World";
  client.send(message);
}

// called when the client loses its connection
function onConnectionLost(responseObject) {
  if (responseObject.errorCode !== 0) {
    console.log("onConnectionLost:"+responseObject.errorMessage);
    output.append("onConnectionLost:"+responseObject.errorMessage+"<br/>");
 }
}

// called when a message arrives
function onMessageArrived(message) {
  console.log("onMessageArrived:"+message.payloadString);
  output.append("onMessageArrived:"+message.payloadString+"<br/>");
}

</script>

</html>
dpwspoon commented 8 years ago

For the ws to tcp proxy, there is currently no feature in the gateway that allows you to configure the websocket to tcp proxy to know what subprotocols it should accept, or deny, or perhaps even route differently… The subprotocol the websocket has selected can’t really be sent out to the backend because the backend is speakng tcp. So as the gateway is currently implemented, for any practical application with the gateway configured as a ws to tcp proxy there probably isn’t a need for a subprotocol to be sent…To fix your bug we probably require some thought to the configuration on what we want to implement, and then we could default it in the case of ws to tcp proxy so it works out of the box with out people tripping on it, i.e. probably always send back all the protocols.

dpwspoon commented 8 years ago

Perhaps we could configure it via an accept option, i.e.

<service>
    <name>mqtt-proxy</name>
    <accept>ws://${gateway.host}:8080/messagingservice</accept>
    <connect>tcp://${gateway.host}:1883</connect>

    <type>proxy</type>

    <cross-site-constraint>
      <allow-origin>*</allow-origin>
    </cross-site-constraint>

   <accept-options>
      <ws.sec-websocket-protocol>mqtt<ws.sec-websocket-protocol>
   </accept-otpions>

  </service>
<service>
    <name>mqtt-proxy</name>
    <accept>ws://${gateway.host}:8080/messagingservice</accept>
    <connect>tcp://${gateway.host}:1884</connect>

    <type>proxy</type>

    <cross-site-constraint>
      <allow-origin>*</allow-origin>
    </cross-site-constraint>

   <accept-options>
      <ws.sec-websocket-protocol>amqp<ws.sec-websocket-protocol>
   </accept-otpions>

  </service>

Where an accept to ws://${gateway.host}:8080/messagingservice gets routed based on the ws.sec-websocket-protocol, or gets dropped (not sure what the right response code is), if it isn't listed.

If ws.sec-websocket-protocol is not configured then it should default to empty, or it could have the behavior to bind to all, and return yes for all responses. The first is probably more correct, the latter is easier to configure.

dpwspoon commented 8 years ago

The behavior for the server when if finds no Sec-Websocket-Protocol that matches the client is underspecified, on whether it should return a 404 (or other appropriate http response code) or return a 101 with out the Sec-Websocket-Protocol header. Today, judging from our code we return a 404 when we have services only bound to Sec-Websocket-Protocol. And we return the empty header when there is one Websocket bound with out a next protocol.

As such, from a practical point of view its unclear whether the 101 without the header is off spec, but perhaps it is better to send a 404. The trade off is that clients can accurately report the issue with the 101, where a 404 may appear to be a url problem and not a next protocol problem. However, the 404 more accurately reflects that the client (if written to spec) can not actually use the websocket, and it seems to better align with how we do bindings. Regardless, modifying the server to send a 404 for all the cases above with out the corresponding user story to expose next protocol would break unspec compliant websocket clients (like our current 4.x clients), and would make it impossible for people to use our proxy for anything that requires next protocol. Likewise, making the client spec compliant with out the feature would invalidate that use case. As such, the order of operations on this should be: 1) Add the new feature the gateway (add demos to gateway-config with this) 2) Make 5.x client spec compliant, with added spec test for when the header is empty, 3) When 4.x clients are phased out for this use case, change the gateway to send a 404 or other response code for this

dpwspoon commented 8 years ago

Note, this code can got back in when this is complete: https://github.com/kaazing/gateway/pull/509