On Sat, Mar 02, 2019 at 12:13:20AM +0100, Sebastian Benoit wrote:
> --- usr.sbin/relayd/parse.y
> +++ usr.sbin/relayd/parse.y
> @@ -176,6 +176,7 @@ typedef struct {
> %token TO ROUTER RTLABEL TRANSPARENT TRAP UPDATES URL VIRTUAL WITH TTL
> RTABLE
> %token MATCH PARAMS RANDOM LEASTSTATES SRCHASH KEY CERTIFICATE
> PASSWORD ECDHE
> %token EDH TICKETS CONNECTION CONNECTIONS ERRORS STATE CHANGES CHECKS
> +%token WEBSOCKETS
> %token <v.string> STRING
> %token <v.number> NUMBER
> %type <v.string> hostname interface table value optstring
Here are some tab vs space incosistencies.
> + if (strcasecmp("Connection", key) == 0 &&
> + strcasecmp("Upgrade", value) == 0)
> + priv->http_upgrade_req |=
> + HTTP_UPGRADE_CONN;
Would the name HTTP_CONNECTION_UPGRADE be better? Perhaps we want
to do something spcific with connection keep-alive or close some
day.
> @@ -422,6 +445,28 @@ relay_read_http(struct bufferevent *bev, void *arg)
> return;
> }
>
> + /* HTTP 101 Switching Protocols */
> + if (cre->dir == RELAY_DIR_REQUEST) {
> + if (!(proto->httpflags & HTTPFLAG_WEBSOCKETS) &&
> + (priv->http_upgrade_req &
> + HTTP_UPGRADE_WEBSOCKET)) {
Should we check for both upgrade and websocket?
> + relay_abort_http(con, 403,
> + "Forbidden", 0);
A more specific error message like "Websocket Forbidden" would make
debugging much easier.
The RFC says it must be a GET request. We should check at least
this. If we check more, an attacker can create less dubious states.
> + return;
> + }
> + } else if (cre->dir == RELAY_DIR_RESPONSE &&
> + desc->http_status == 101) {
> + if ((priv->http_upgrade_req &
> + (HTTP_UPGRADE_CONN | HTTP_UPGRADE_WEBSOCKET)) &&
Both flags must be present.
> + proto->httpflags & HTTPFLAG_WEBSOCKETS) {
> + cre->dst->toread = TOREAD_UNLIMITED;
> + cre->dst->bev->readcb = relay_read;
> + } else {
> + relay_abort_http(con, 502, "Bad Gateway", 0);
Could we have a more specific message like "Bad Websocket Gateway"?
> + return;
> + }
> + }
> +
> switch (desc->http_method) {
> case HTTP_METHOD_CONNECT:
> /* Data stream */
bluhm
