Improve websockets server error reporting

PATCH Superseded Daniel P. Berrange 2 participants 6 messages Sep 6, 2017 – Sep 7, 2017

lore

Share & tools

lore

Daniel P. Berrange

author

2017-09-06 10:40 UTC #0 lore raw

cover Improve websockets server error reporting

The websockets server gives no useful error response to the client nor provides any way to debug errors no the server

Daniel P. Berrange (2):
io: send proper HTTP response for websocket errors
io: include full error message in websocket handshake trace

 io/channel-websock.c | 184 ++++++++++++++++++++++++++++++++++++++-------------
 io/trace-events      |   2 +-
 2 files changed, 138 insertions(+), 48 deletions(-)

Signature · 1 line

2.13.5

Daniel P. Berrange

author

2017-09-06 10:40 UTC #1 lore raw

PATCH 1/2 io: send proper HTTP response for websocket errors

When any error occurs while processing the websockets handshake, QEMU just terminates the connection abruptly. This is in violation of the HTTP specs and does not help the client understand what they did wrong. This is particularly bad when the client gives the wrong path, as a "404 Not Found" would be very helpful.

Refactor the handshake code so that it always sends a response to the client unless there was an I/O error.

Fixes bug: #1715186

Signed-off-by Daniel P. Berrange <[email protected]>

---
 io/channel-websock.c | 179 ++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 134 insertions(+), 45 deletions(-)

io/channel-websock.c +134 ~~-45~~

index 5a3badbec2..b9cc5a1371 100644
@@ -44,13 +44,39 @@
 #define QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE "Upgrade"
 #define QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET "websocket"
 
-#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE  \
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
+    "Server: QEMU VNC\r\n"                       \
+    "Date: %s\r\n"
+
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK    \
     "HTTP/1.1 101 Switching Protocols\r\n"      \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON    \
     "Upgrade: websocket\r\n"                    \
     "Connection: Upgrade\r\n"                   \
     "Sec-WebSocket-Accept: %s\r\n"              \
     "Sec-WebSocket-Protocol: binary\r\n"        \
     "\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND \
+    "HTTP/1.1 404 Not Found\r\n"                    \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON        \
+    "Connection: close\r\n"                         \
+    "\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \
+    "HTTP/1.1 400 Bad Request\r\n"                    \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON          \
+    "Connection: close\r\n"                           \
+    "Sec-WebSocket-Version: 13\r\n"                   \
+    "\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR \
+    "HTTP/1.1 500 Internal Server Error\r\n"         \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON         \
+    "Connection: close\r\n"                          \
+    "\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE  \
+    "HTTP/1.1 403 Request Entity Too Large\r\n"      \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON         \
+    "Connection: close\r\n"                          \
+    "\r\n"
 #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM "\r\n"
 #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END "\r\n\r\n"
 #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "13"

@@ -123,8 +149,43 @@ enum {
     QIO_CHANNEL_WEBSOCK_OPCODE_PONG = 0xA
 };
 
+static void qio_channel_websock_handshake_send_res(QIOChannelWebsock *ioc,
+                                                   const char *resmsg,
+                                                   ...)
+{
+    va_list vargs;
+    char *response = NULL;
+    size_t responselen;
+
+    va_start(vargs, resmsg);
+    response = g_strdup_vprintf(resmsg, vargs);
+    responselen = strlen(response);
+    buffer_reserve(&ioc->encoutput, responselen);
+    buffer_append(&ioc->encoutput, response, responselen);
+    va_end(vargs);
+}
+
+static gchar *qio_channel_websock_date_str(void)
+{
+    GTimeZone *utc = g_time_zone_new_utc();
+    GDateTime *now = g_date_time_new_now(utc);
+    gchar *ret = g_date_time_format(now, "%a, %d %b %Y %H:%M:%S GMT");
+    g_date_time_unref(now);
+    g_time_zone_unref(utc);
+    return ret;
+}
+
+static void qio_channel_websock_handshake_send_res_err(QIOChannelWebsock *ioc,
+                                                       const char *resdata)
+{
+    char *date = qio_channel_websock_date_str();
+    qio_channel_websock_handshake_send_res(ioc, resdata, date);
+    g_free(date);
+}
+
 static size_t
-qio_channel_websock_extract_headers(char *buffer,
+qio_channel_websock_extract_headers(QIOChannelWebsock *ioc,
+                                    char *buffer,
                                     QIOChannelWebsockHTTPHeader *hdrs,
                                     size_t nhdrsalloc,
                                     Error **errp)

@@ -145,7 +206,7 @@ qio_channel_websock_extract_headers(char *buffer,
     nl = strstr(buffer, QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM);
     if (!nl) {
         error_setg(errp, "Missing HTTP header delimiter");
-        return 0;
+        goto bad_request;
     }
     *nl = '\0';

@@ -158,18 +219,20 @@ qio_channel_websock_extract_headers(char *buffer,
 
     if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_METHOD)) {
         error_setg(errp, "Unsupported HTTP method %s", buffer);
-        return 0;
+        goto bad_request;
     }
 
     buffer = tmp + 1;
     tmp = strchr(buffer, ' ');
     if (!tmp) {
         error_setg(errp, "Missing HTTP version delimiter");
-        return 0;
+        goto bad_request;
     }
     *tmp = '\0';
 
     if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_PATH)) {
+        qio_channel_websock_handshake_send_res_err(
+            ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND);
         error_setg(errp, "Unexpected HTTP path %s", buffer);
         return 0;
     }

@@ -178,7 +241,7 @@ qio_channel_websock_extract_headers(char *buffer,
 
     if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_VERSION)) {
         error_setg(errp, "Unsupported HTTP version %s", buffer);
-        return 0;
+        goto bad_request;
     }
 
     buffer = nl + strlen(QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM);

@@ -203,7 +266,7 @@ qio_channel_websock_extract_headers(char *buffer,
         sep = strchr(buffer, ':');
         if (!sep) {
             error_setg(errp, "Malformed HTTP header");
-            return 0;
+            goto bad_request;
         }
         *sep = '\0';
         sep++;

@@ -213,7 +276,7 @@ qio_channel_websock_extract_headers(char *buffer,
 
         if (nhdrs >= nhdrsalloc) {
             error_setg(errp, "Too many HTTP headers");
-            return 0;
+            goto bad_request;
         }
 
         hdr = &hdrs[nhdrs++];

@@ -231,6 +294,11 @@ qio_channel_websock_extract_headers(char *buffer,
     } while (nl != NULL);
 
     return nhdrs;
+
+ bad_request:
+    qio_channel_websock_handshake_send_res_err(
+        ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST);
+    return 0;
 }
 
 static const char *

@@ -250,14 +318,14 @@ qio_channel_websock_find_header(QIOChannelWebsockHTTPHeader *hdrs,
 }
 
 
-static int qio_channel_websock_handshake_send_response(QIOChannelWebsock *ioc,
-                                                       const char *key,
-                                                       Error **errp)
+static void qio_channel_websock_handshake_send_res_ok(QIOChannelWebsock *ioc,
+                                                      const char *key,
+                                                      Error **errp)
 {
     char combined_key[QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN +
                       QIO_CHANNEL_WEBSOCK_GUID_LEN + 1];
-    char *accept = NULL, *response = NULL;
-    size_t responselen;
+    char *accept = NULL;
+    char *date = qio_channel_websock_date_str();
 
     g_strlcpy(combined_key, key, QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN + 1);
     g_strlcat(combined_key, QIO_CHANNEL_WEBSOCK_GUID,

@@ -271,105 +339,110 @@ static int qio_channel_websock_handshake_send_response(QIOChannelWebsock *ioc,
                             QIO_CHANNEL_WEBSOCK_GUID_LEN,
                             &accept,
                             errp) < 0) {
-        return -1;
+        qio_channel_websock_handshake_send_res_err(
+            ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR);
+        return;
     }
 
-    response = g_strdup_printf(QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE, accept);
-    responselen = strlen(response);
-    buffer_reserve(&ioc->encoutput, responselen);
-    buffer_append(&ioc->encoutput, response, responselen);
+    qio_channel_websock_handshake_send_res(
+        ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK, date, accept);
 
+    g_free(date);
     g_free(accept);
-    g_free(response);
 
-    return 0;
+    return;
 }
 
-static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
-                                                 char *buffer,
-                                                 Error **errp)
+static void qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
+                                                  char *buffer,
+                                                  Error **errp)
 {
     QIOChannelWebsockHTTPHeader hdrs[32];
     size_t nhdrs = G_N_ELEMENTS(hdrs);
     const char *protocols = NULL, *version = NULL, *key = NULL,
         *host = NULL, *connection = NULL, *upgrade = NULL;
 
-    nhdrs = qio_channel_websock_extract_headers(buffer, hdrs, nhdrs, errp);
+    nhdrs = qio_channel_websock_extract_headers(ioc, buffer, hdrs, nhdrs, errp);
     if (!nhdrs) {
-        return -1;
+        return;
     }
 
     protocols = qio_channel_websock_find_header(
         hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL);
     if (!protocols) {
         error_setg(errp, "Missing websocket protocol header data");
-        return -1;
+        goto bad_request;
     }
 
     version = qio_channel_websock_find_header(
         hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_VERSION);
     if (!version) {
         error_setg(errp, "Missing websocket version header data");
-        return -1;
+        goto bad_request;
     }
 
     key = qio_channel_websock_find_header(
         hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_KEY);
     if (!key) {
         error_setg(errp, "Missing websocket key header data");
-        return -1;
+        goto bad_request;
     }
 
     host = qio_channel_websock_find_header(
         hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_HOST);
     if (!host) {
         error_setg(errp, "Missing websocket host header data");
-        return -1;
+        goto bad_request;
     }
 
     connection = qio_channel_websock_find_header(
         hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_CONNECTION);
     if (!connection) {
         error_setg(errp, "Missing websocket connection header data");
-        return -1;
+        goto bad_request;
     }
 
     upgrade = qio_channel_websock_find_header(
         hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_UPGRADE);
     if (!upgrade) {
         error_setg(errp, "Missing websocket upgrade header data");
-        return -1;
+        goto bad_request;
     }
 
     if (!g_strrstr(protocols, QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY)) {
         error_setg(errp, "No '%s' protocol is supported by client '%s'",
                    QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY, protocols);
-        return -1;
+        goto bad_request;
     }
 
     if (!g_str_equal(version, QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION)) {
         error_setg(errp, "Version '%s' is not supported by client '%s'",
                    QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION, version);
-        return -1;
+        goto bad_request;
     }
 
     if (strlen(key) != QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN) {
         error_setg(errp, "Key length '%zu' was not as expected '%d'",
                    strlen(key), QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN);
-        return -1;
+        goto bad_request;
     }
 
     if (!g_strrstr(connection, QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE)) {
         error_setg(errp, "No connection upgrade requested '%s'", connection);
-        return -1;
+        goto bad_request;
     }
 
     if (!g_str_equal(upgrade, QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET)) {
         error_setg(errp, "Incorrect upgrade method '%s'", upgrade);
-        return -1;
+        goto bad_request;
     }
 
-    return qio_channel_websock_handshake_send_response(ioc, key, errp);
+    qio_channel_websock_handshake_send_res_ok(ioc, key, errp);
+    return;
+
+ bad_request:
+    qio_channel_websock_handshake_send_res_err(
+        ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST);
 }
 
 static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,

@@ -393,20 +466,20 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,
                                  QIO_CHANNEL_WEBSOCK_HANDSHAKE_END);
     if (!handshake_end) {
         if (ioc->encinput.offset >= 4096) {
+            qio_channel_websock_handshake_send_res_err(
+                ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE);
             error_setg(errp,
                        "End of headers not found in first 4096 bytes");
-            return -1;
+            return 1;
         } else {
             return 0;
         }
     }
     *handshake_end = '\0';
 
-    if (qio_channel_websock_handshake_process(ioc,
-                                              (char *)ioc->encinput.buffer,
-                                              errp) < 0) {
-        return -1;
-    }
+    qio_channel_websock_handshake_process(ioc,
+                                          (char *)ioc->encinput.buffer,
+                                          errp);
 
     buffer_advance(&ioc->encinput,
                    handshake_end - (char *)ioc->encinput.buffer +

@@ -438,8 +511,15 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
 
     buffer_advance(&wioc->encoutput, ret);
     if (wioc->encoutput.offset == 0) {
-        trace_qio_channel_websock_handshake_complete(ioc);
-        qio_task_complete(task);
+        if (wioc->io_err) {
+            trace_qio_channel_websock_handshake_fail(ioc);
+            qio_task_set_error(task, wioc->io_err);
+            wioc->io_err = NULL;
+            qio_task_complete(task);
+        } else {
+            trace_qio_channel_websock_handshake_complete(ioc);
+            qio_task_complete(task);
+        }
         return FALSE;
     }
     trace_qio_channel_websock_handshake_pending(ioc, G_IO_OUT);

@@ -458,6 +538,11 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
 
     ret = qio_channel_websock_handshake_read(wioc, &err);
     if (ret < 0) {
+        /*
+         * We only take this path on a fatal I/O error reading from
+         * client connection, as most of the time we have an
+         * HTTP 4xx err response to send instead
+         */
         trace_qio_channel_websock_handshake_fail(ioc);
         qio_task_set_error(task, err);
         qio_task_complete(task);

@@ -469,6 +554,10 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
         return TRUE;
     }
 
+    if (err) {
+        error_propagate(&wioc->io_err, err);
+    }
+
     trace_qio_channel_websock_handshake_reply(ioc);
     qio_channel_add_watch(
         wioc->master,

Signature · 1 line

2.13.5

Philippe Mathieu-Daudé

2017-09-06 18:06 UTC #2 lore raw

Re: [Qemu-devel] [PATCH v1 1/2] io: send proper HTTP response for websocket errors

Hi Daniel,

Quoted 21 lines · Daniel P. Berrange

When any error occurs while processing the websockets handshake, QEMU just terminates the connection abruptly. This is in violation of the HTTP specs and does not help the client understand what they did wrong. This is particularly bad when the client gives the wrong path, as a "404 Not Found" would be very helpful.
Refactor the handshake code so that it always sends a response to the client unless there was an I/O error.
Fixes bug: #1715186
Signed-off-by Daniel P. Berrange <[email protected]>
---
  io/channel-websock.c | 179 ++++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 134 insertions(+), 45 deletions(-)
io/channel-websock.c +0 -0
index 5a3badbec2..b9cc5a1371 100644

io/channel-websock.c line 49

Quoted 7 lines

io/channel-websock.c +3 -1

@@ -44,13 +44,39 @@
  #define QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE "Upgrade"
  #define QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET "websocket"
 
-#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE  \
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
+    "Server: QEMU VNC\r\n"                       \
+    "Date: %s\r\n"

and

        "Sec-WebSocket-Version: " \
          QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "\r\n" \

or what about:

#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON(conn)   \
          "Server: QEMU VNC\r\n"                         \
          "Date: %s\r\n"                                 \
          "Connection: " conn "\r\n"                     \
          "Sec-WebSocket-Version: "                      \

QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "\r\n"

Quoted 9 lines

io/channel-websock.c +3 -0

+
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK    \
      "HTTP/1.1 101 Switching Protocols\r\n"      \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON    \
      "Upgrade: websocket\r\n"                    \
      "Connection: Upgrade\r\n"                   \
      "Sec-WebSocket-Accept: %s\r\n"              \
      "Sec-WebSocket-Protocol: binary\r\n"        \
      "\r\n"

io/channel-websock.c line 68

Quoted 10 lines

io/channel-websock.c +10 -0

+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND \
+    "HTTP/1.1 404 Not Found\r\n"                    \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON        \
+    "Connection: close\r\n"                         \
+    "\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \
+    "HTTP/1.1 400 Bad Request\r\n"                    \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON          \
+    "Connection: close\r\n"                           \
+    "Sec-WebSocket-Version: 13\r\n"                   \

drop

io/channel-websock.c line 69

Quoted 1 line

io/channel-websock.c +1 -0
+    "\r\n"

or with previous macro:

#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \
    "HTTP/1.1 400 Bad Request\r\n"                    \
    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON("close") \
    "\r\n"

Quoted 14 lines

io/channel-websock.c +10 -0

+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR \
+    "HTTP/1.1 500 Internal Server Error\r\n"         \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON         \
+    "Connection: close\r\n"                          \
+    "\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE  \
+    "HTTP/1.1 403 Request Entity Too Large\r\n"      \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON         \
+    "Connection: close\r\n"                          \
+    "\r\n"
  #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM "\r\n"
  #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END "\r\n\r\n"
  #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "13"

io/channel-websock.c enum { · line 157

Quoted 9 lines

io/channel-websock.c +6 -0

@@ -123,8 +149,43 @@ enum {
      QIO_CHANNEL_WEBSOCK_OPCODE_PONG = 0xA
  };
 
+static void qio_channel_websock_handshake_send_res(QIOChannelWebsock *ioc,
+                                                   const char *resmsg,
+                                                   ...)
+{
+    va_list vargs;
+    char *response = NULL;

NULL not needed

Quoted 5 lines

io/channel-websock.c +5 -0

+    size_t responselen;
+
+    va_start(vargs, resmsg);
+    response = g_strdup_vprintf(resmsg, vargs);
+    responselen = strlen(response);

io/channel-websock.c enum { · line 172

Quoted 10 lines

io/channel-websock.c +10 -0

+    buffer_reserve(&ioc->encoutput, responselen);
+    buffer_append(&ioc->encoutput, response, responselen);
+    va_end(vargs);
+}
+
+static gchar *qio_channel_websock_date_str(void)
+{
+    GTimeZone *utc = g_time_zone_new_utc();
+    GDateTime *now = g_date_time_new_now(utc);
+    gchar *ret = g_date_time_format(now, "%a, %d %b %Y %H:%M:%S GMT");

assert(ret);

Quoted 124 lines

io/channel-websock.c +36 ~~-16~~

+    g_date_time_unref(now);
+    g_time_zone_unref(utc);
+    return ret;
+}
+
+static void qio_channel_websock_handshake_send_res_err(QIOChannelWebsock *ioc,
+                                                       const char *resdata)
+{
+    char *date = qio_channel_websock_date_str();
+    qio_channel_websock_handshake_send_res(ioc, resdata, date);
+    g_free(date);
+}
+
  static size_t
-qio_channel_websock_extract_headers(char *buffer,
+qio_channel_websock_extract_headers(QIOChannelWebsock *ioc,
+                                    char *buffer,
                                      QIOChannelWebsockHTTPHeader *hdrs,
                                      size_t nhdrsalloc,
                                      Error **errp)

@@ -145,7 +206,7 @@ qio_channel_websock_extract_headers(char *buffer,
      nl = strstr(buffer, QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM);
      if (!nl) {
          error_setg(errp, "Missing HTTP header delimiter");
-        return 0;
+        goto bad_request;
      }
      *nl = '\0';

@@ -158,18 +219,20 @@ qio_channel_websock_extract_headers(char *buffer,
 
      if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_METHOD)) {
          error_setg(errp, "Unsupported HTTP method %s", buffer);
-        return 0;
+        goto bad_request;
      }
 
      buffer = tmp + 1;
      tmp = strchr(buffer, ' ');
      if (!tmp) {
          error_setg(errp, "Missing HTTP version delimiter");
-        return 0;
+        goto bad_request;
      }
      *tmp = '\0';
 
      if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_PATH)) {
+        qio_channel_websock_handshake_send_res_err(
+            ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND);
          error_setg(errp, "Unexpected HTTP path %s", buffer);
          return 0;
      }

@@ -178,7 +241,7 @@ qio_channel_websock_extract_headers(char *buffer,
 
      if (!g_str_equal(buffer, QIO_CHANNEL_WEBSOCK_HTTP_VERSION)) {
          error_setg(errp, "Unsupported HTTP version %s", buffer);
-        return 0;
+        goto bad_request;
      }
 
      buffer = nl + strlen(QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM);

@@ -203,7 +266,7 @@ qio_channel_websock_extract_headers(char *buffer,
          sep = strchr(buffer, ':');
          if (!sep) {
              error_setg(errp, "Malformed HTTP header");
-            return 0;
+            goto bad_request;
          }
          *sep = '\0';
          sep++;

@@ -213,7 +276,7 @@ qio_channel_websock_extract_headers(char *buffer,
 
          if (nhdrs >= nhdrsalloc) {
              error_setg(errp, "Too many HTTP headers");
-            return 0;
+            goto bad_request;
          }
 
          hdr = &hdrs[nhdrs++];

@@ -231,6 +294,11 @@ qio_channel_websock_extract_headers(char *buffer,
      } while (nl != NULL);
 
      return nhdrs;
+
+ bad_request:
+    qio_channel_websock_handshake_send_res_err(
+        ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST);
+    return 0;
  }
 
  static const char *

@@ -250,14 +318,14 @@ qio_channel_websock_find_header(QIOChannelWebsockHTTPHeader *hdrs,
  }
 
 
-static int qio_channel_websock_handshake_send_response(QIOChannelWebsock *ioc,
-                                                       const char *key,
-                                                       Error **errp)
+static void qio_channel_websock_handshake_send_res_ok(QIOChannelWebsock *ioc,
+                                                      const char *key,
+                                                      Error **errp)
  {
      char combined_key[QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN +
                        QIO_CHANNEL_WEBSOCK_GUID_LEN + 1];
-    char *accept = NULL, *response = NULL;
-    size_t responselen;
+    char *accept = NULL;
+    char *date = qio_channel_websock_date_str();
 
      g_strlcpy(combined_key, key, QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN + 1);
      g_strlcat(combined_key, QIO_CHANNEL_WEBSOCK_GUID,

@@ -271,105 +339,110 @@ static int qio_channel_websock_handshake_send_response(QIOChannelWebsock *ioc,
                              QIO_CHANNEL_WEBSOCK_GUID_LEN,
                              &accept,
                              errp) < 0) {
-        return -1;
+        qio_channel_websock_handshake_send_res_err(
+            ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR);
+        return;
      }
 
-    response = g_strdup_printf(QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE, accept);
-    responselen = strlen(response);
-    buffer_reserve(&ioc->encoutput, responselen);

io/channel-websock.c static int qio_channel_websock_handshake_send_response(QIOChannelWebsock *ioc, · line 353

Quoted 10 lines

io/channel-websock.c +4 -3

-    buffer_append(&ioc->encoutput, response, responselen);
+    qio_channel_websock_handshake_send_res(
+        ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK, date, accept);
 
+    g_free(date);
      g_free(accept);
-    g_free(response);
 
-    return 0;
+    return;

drop

Quoted 171 lines

io/channel-websock.c +42 ~~-25~~

  }
 
-static int qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
-                                                 char *buffer,
-                                                 Error **errp)
+static void qio_channel_websock_handshake_process(QIOChannelWebsock *ioc,
+                                                  char *buffer,
+                                                  Error **errp)
  {
      QIOChannelWebsockHTTPHeader hdrs[32];
      size_t nhdrs = G_N_ELEMENTS(hdrs);
      const char *protocols = NULL, *version = NULL, *key = NULL,
          *host = NULL, *connection = NULL, *upgrade = NULL;
 
-    nhdrs = qio_channel_websock_extract_headers(buffer, hdrs, nhdrs, errp);
+    nhdrs = qio_channel_websock_extract_headers(ioc, buffer, hdrs, nhdrs, errp);
      if (!nhdrs) {
-        return -1;
+        return;
      }
 
      protocols = qio_channel_websock_find_header(
          hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_PROTOCOL);
      if (!protocols) {
          error_setg(errp, "Missing websocket protocol header data");
-        return -1;
+        goto bad_request;
      }
 
      version = qio_channel_websock_find_header(
          hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_VERSION);
      if (!version) {
          error_setg(errp, "Missing websocket version header data");
-        return -1;
+        goto bad_request;
      }
 
      key = qio_channel_websock_find_header(
          hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_KEY);
      if (!key) {
          error_setg(errp, "Missing websocket key header data");
-        return -1;
+        goto bad_request;
      }
 
      host = qio_channel_websock_find_header(
          hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_HOST);
      if (!host) {
          error_setg(errp, "Missing websocket host header data");
-        return -1;
+        goto bad_request;
      }
 
      connection = qio_channel_websock_find_header(
          hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_CONNECTION);
      if (!connection) {
          error_setg(errp, "Missing websocket connection header data");
-        return -1;
+        goto bad_request;
      }
 
      upgrade = qio_channel_websock_find_header(
          hdrs, nhdrs, QIO_CHANNEL_WEBSOCK_HEADER_UPGRADE);
      if (!upgrade) {
          error_setg(errp, "Missing websocket upgrade header data");
-        return -1;
+        goto bad_request;
      }
 
      if (!g_strrstr(protocols, QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY)) {
          error_setg(errp, "No '%s' protocol is supported by client '%s'",
                     QIO_CHANNEL_WEBSOCK_PROTOCOL_BINARY, protocols);
-        return -1;
+        goto bad_request;
      }
 
      if (!g_str_equal(version, QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION)) {
          error_setg(errp, "Version '%s' is not supported by client '%s'",
                     QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION, version);
-        return -1;
+        goto bad_request;
      }
 
      if (strlen(key) != QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN) {
          error_setg(errp, "Key length '%zu' was not as expected '%d'",
                     strlen(key), QIO_CHANNEL_WEBSOCK_CLIENT_KEY_LEN);
-        return -1;
+        goto bad_request;
      }
 
      if (!g_strrstr(connection, QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE)) {
          error_setg(errp, "No connection upgrade requested '%s'", connection);
-        return -1;
+        goto bad_request;
      }
 
      if (!g_str_equal(upgrade, QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET)) {
          error_setg(errp, "Incorrect upgrade method '%s'", upgrade);
-        return -1;
+        goto bad_request;
      }
 
-    return qio_channel_websock_handshake_send_response(ioc, key, errp);
+    qio_channel_websock_handshake_send_res_ok(ioc, key, errp);
+    return;
+
+ bad_request:
+    qio_channel_websock_handshake_send_res_err(
+        ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST);
  }
 
  static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,

@@ -393,20 +466,20 @@ static int qio_channel_websock_handshake_read(QIOChannelWebsock *ioc,
                                   QIO_CHANNEL_WEBSOCK_HANDSHAKE_END);
      if (!handshake_end) {
          if (ioc->encinput.offset >= 4096) {
+            qio_channel_websock_handshake_send_res_err(
+                ioc, QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE);
              error_setg(errp,
                         "End of headers not found in first 4096 bytes");
-            return -1;
+            return 1;
          } else {
              return 0;
          }
      }
      *handshake_end = '\0';
 
-    if (qio_channel_websock_handshake_process(ioc,
-                                              (char *)ioc->encinput.buffer,
-                                              errp) < 0) {
-        return -1;
-    }
+    qio_channel_websock_handshake_process(ioc,
+                                          (char *)ioc->encinput.buffer,
+                                          errp);
 
      buffer_advance(&ioc->encinput,
                     handshake_end - (char *)ioc->encinput.buffer +

@@ -438,8 +511,15 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
 
      buffer_advance(&wioc->encoutput, ret);
      if (wioc->encoutput.offset == 0) {
-        trace_qio_channel_websock_handshake_complete(ioc);
-        qio_task_complete(task);
+        if (wioc->io_err) {
+            trace_qio_channel_websock_handshake_fail(ioc);
+            qio_task_set_error(task, wioc->io_err);
+            wioc->io_err = NULL;
+            qio_task_complete(task);
+        } else {
+            trace_qio_channel_websock_handshake_complete(ioc);
+            qio_task_complete(task);
+        }
          return FALSE;
      }
      trace_qio_channel_websock_handshake_pending(ioc, G_IO_OUT);

@@ -458,6 +538,11 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
 
      ret = qio_channel_websock_handshake_read(wioc, &err);
      if (ret < 0) {
+        /*
+         * We only take this path on a fatal I/O error reading from
+         * client connection, as most of the time we have an
+         * HTTP 4xx err response to send instead
+         */
          trace_qio_channel_websock_handshake_fail(ioc);
          qio_task_set_error(task, err);
          qio_task_complete(task);

io/channel-websock.c static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc, · line 563

Quoted 10 lines

io/channel-websock.c +4 -0

@@ -469,6 +554,10 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
          return TRUE;
      }
 
+    if (err) {
+        error_propagate(&wioc->io_err, err);
+    }
+
      trace_qio_channel_websock_handshake_reply(ioc);
      qio_channel_add_watch(
          wioc->master,

looks good :)

Regards,

Phil.

Daniel P. Berrange

author

2017-09-07 09:13 UTC #3 lore raw

Re: [Qemu-devel] [PATCH v1 1/2] io: send proper HTTP response for websocket errors

Quoted 32 lines · Philippe Mathieu-Daudé

Hi Daniel,
Quoted 21 lines · Daniel P. Berrange
When any error occurs while processing the websockets handshake, QEMU just terminates the connection abruptly. This is in violation of the HTTP specs and does not help the client understand what they did wrong. This is particularly bad when the client gives the wrong path, as a "404 Not Found" would be very helpful.
Refactor the handshake code so that it always sends a response to the client unless there was an I/O error.
Fixes bug: #1715186
Signed-off-by Daniel P. Berrange <[email protected]>
---
  io/channel-websock.c | 179 ++++++++++++++++++++++++++++++++++++++-------------
  1 file changed, 134 insertions(+), 45 deletions(-)
io/channel-websock.c +0 -0
index 5a3badbec2..b9cc5a1371 100644
io/channel-websock.c line 48
Quoted 6 lines
io/channel-websock.c +3 -1
@@ -44,13 +44,39 @@
  #define QIO_CHANNEL_WEBSOCK_CONNECTION_UPGRADE "Upgrade"
  #define QIO_CHANNEL_WEBSOCK_UPGRADE_WEBSOCKET "websocket"
-#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RESPONSE  \
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON \
+    "Server: QEMU VNC\r\n"                       \
+    "Date: %s\r\n"
and

Quoted 2 lines

        "Sec-WebSocket-Version: " \
          QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "\r\n" \

This header is only supposed to be set in error responses, not in the 101 response.

Quoted 8 lines

or what about:

#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON(conn)   \
          "Server: QEMU VNC\r\n"                         \
          "Date: %s\r\n"                                 \
          "Connection: " conn "\r\n"                     \
          "Sec-WebSocket-Version: "                      \

QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "\r\n"

I'm not a fan of parameterizing this - I think it is clearer to see the full connection header inline below.

Quoted 31 lines

Quoted 9 lines

io/channel-websock.c +3 -0

+
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_OK    \
      "HTTP/1.1 101 Switching Protocols\r\n"      \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON    \
      "Upgrade: websocket\r\n"                    \
      "Connection: Upgrade\r\n"                   \
      "Sec-WebSocket-Accept: %s\r\n"              \
      "Sec-WebSocket-Protocol: binary\r\n"        \
      "\r\n"

io/channel-websock.c line 68

Quoted 10 lines

io/channel-websock.c +10 -0

+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_NOT_FOUND \
+    "HTTP/1.1 404 Not Found\r\n"                    \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON        \
+    "Connection: close\r\n"                         \
+    "\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \
+    "HTTP/1.1 400 Bad Request\r\n"                    \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON          \
+    "Connection: close\r\n"                           \
+    "Sec-WebSocket-Version: 13\r\n"                   \

drop

io/channel-websock.c line 68

Quoted 1 line

io/channel-websock.c +1 -0
+    "\r\n"

or with previous macro:

#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_BAD_REQUEST \
    "HTTP/1.1 400 Bad Request\r\n"                    \
    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON("close") \
    "\r\n"

Quoted 13 lines

io/channel-websock.c line 82

Quoted 13 lines

io/channel-websock.c +10 -0

+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_SERVER_ERR \
+    "HTTP/1.1 500 Internal Server Error\r\n"         \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON         \
+    "Connection: close\r\n"                          \
+    "\r\n"
+#define QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_TOO_LARGE  \
+    "HTTP/1.1 403 Request Entity Too Large\r\n"      \
+    QIO_CHANNEL_WEBSOCK_HANDSHAKE_RES_COMMON         \
+    "Connection: close\r\n"                          \
+    "\r\n"
  #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_DELIM "\r\n"
  #define QIO_CHANNEL_WEBSOCK_HANDSHAKE_END "\r\n\r\n"
  #define QIO_CHANNEL_WEBSOCK_SUPPORTED_VERSION "13"

Regards,
Daniel

Signature · 3 lines

|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Daniel P. Berrange

author

2017-09-06 10:40 UTC #4 lore raw

PATCH 2/2 io: include full error message in websocket handshake trace

In reply to Daniel P. Berrange

When the websocket handshake fails it is useful to log the real error message via the trace points for debugging purposes.

Fixes bug: #1715186

Signed-off-by Daniel P. Berrange <[email protected]>

---
 io/channel-websock.c | 7 ++++---
 io/trace-events      | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

io/channel-websock.c +4 -3

index b9cc5a1371..463c04b0aa 100644
@@ -503,7 +503,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
                             &err);
 
     if (ret < 0) {
-        trace_qio_channel_websock_handshake_fail(ioc);
+        trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
         qio_task_set_error(task, err);
         qio_task_complete(task);
         return FALSE;

@@ -512,7 +512,8 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
     buffer_advance(&wioc->encoutput, ret);
     if (wioc->encoutput.offset == 0) {
         if (wioc->io_err) {
-            trace_qio_channel_websock_handshake_fail(ioc);
+            trace_qio_channel_websock_handshake_fail(
+                ioc, error_get_pretty(wioc->io_err));
             qio_task_set_error(task, wioc->io_err);
             wioc->io_err = NULL;
             qio_task_complete(task);

@@ -543,7 +544,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
          * client connection, as most of the time we have an
          * HTTP 4xx err response to send instead
          */
-        trace_qio_channel_websock_handshake_fail(ioc);
+        trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
         qio_task_set_error(task, err);
         qio_task_complete(task);
         return FALSE;

io/trace-events +1 -1

index 3d233698d0..6459f71f5b 100644
@@ -46,7 +46,7 @@ qio_channel_websock_new_server(void *ioc, void *master) "Websock new client ioc=
 qio_channel_websock_handshake_start(void *ioc) "Websock handshake start ioc=%p"
 qio_channel_websock_handshake_pending(void *ioc, int status) "Websock handshake pending ioc=%p status=%d"
 qio_channel_websock_handshake_reply(void *ioc) "Websock handshake reply ioc=%p"
-qio_channel_websock_handshake_fail(void *ioc) "Websock handshake fail ioc=%p"
+qio_channel_websock_handshake_fail(void *ioc, const char *msg) "Websock handshake fail ioc=%p err=%s"
 qio_channel_websock_handshake_complete(void *ioc) "Websock handshake complete ioc=%p"
 
 # io/channel-command.c

Signature · 1 line

2.13.5

Philippe Mathieu-Daudé

Reviewed-by on v1 2/2

2017-09-06 18:07 UTC #5 lore raw

Re: [Qemu-devel] [PATCH v1 2/2] io: include full error message in websocket handshake trace

Quoted 6 lines · Daniel P. Berrange

When the websocket handshake fails it is useful to log the real error message via the trace points for debugging purposes.
Fixes bug: #1715186
Signed-off-by Daniel P. Berrange <[email protected]>

Reviewed-by Philippe Mathieu-Daudé <[email protected]>

Quoted 51 lines

---
  io/channel-websock.c | 7 ++++---
  io/trace-events      | 2 +-
  2 files changed, 5 insertions(+), 4 deletions(-)

io/channel-websock.c +4 -3

index b9cc5a1371..463c04b0aa 100644
@@ -503,7 +503,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
                              &err);
 
      if (ret < 0) {
-        trace_qio_channel_websock_handshake_fail(ioc);
+        trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
          qio_task_set_error(task, err);
          qio_task_complete(task);
          return FALSE;

@@ -512,7 +512,8 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc,
      buffer_advance(&wioc->encoutput, ret);
      if (wioc->encoutput.offset == 0) {
          if (wioc->io_err) {
-            trace_qio_channel_websock_handshake_fail(ioc);
+            trace_qio_channel_websock_handshake_fail(
+                ioc, error_get_pretty(wioc->io_err));
              qio_task_set_error(task, wioc->io_err);
              wioc->io_err = NULL;
              qio_task_complete(task);

@@ -543,7 +544,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc,
           * client connection, as most of the time we have an
           * HTTP 4xx err response to send instead
           */
-        trace_qio_channel_websock_handshake_fail(ioc);
+        trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err));
          qio_task_set_error(task, err);
          qio_task_complete(task);
          return FALSE;

io/trace-events +1 -1

index 3d233698d0..6459f71f5b 100644
@@ -46,7 +46,7 @@ qio_channel_websock_new_server(void *ioc, void *master) "Websock new client ioc=
  qio_channel_websock_handshake_start(void *ioc) "Websock handshake start ioc=%p"
  qio_channel_websock_handshake_pending(void *ioc, int status) "Websock handshake pending ioc=%p status=%d"
  qio_channel_websock_handshake_reply(void *ioc) "Websock handshake reply ioc=%p"
-qio_channel_websock_handshake_fail(void *ioc) "Websock handshake fail ioc=%p"
+qio_channel_websock_handshake_fail(void *ioc, const char *msg) "Websock handshake fail ioc=%p err=%s"
  qio_channel_websock_handshake_complete(void *ioc) "Websock handshake complete ioc=%p"
 
  # io/channel-command.c