On Sun, Mar 07, 2021 at 06:46:33PM +0100, Marcus MERIGHI wrote:
> Hello Jonathon!
>
> welcome to the party:
>
> https://marc.info/?t=158334391200003
>
> especially the two comments by sthen@:
>
> https://marc.info/?m=161349608614743
> https://marc.info/?m=161350666619371
>
> reyk@ removed from CC: on purpose:
> https://twitter.com/reykfloeter/status/1284868070901776384
>
> Marcus
>
> jonathon.fletc...@gmail.com (Jonathon Fletcher), 2021.03.06 (Sat) 21:02 (CET):
> > When relayd relays a connection upgrade to a websocket, it relays
> > the outbound "Connection: Upgrade" header from the interal server.
> >
> > It also tags on a "Connection: close" header to the outbound
> > response - ie the response goes out with two "Connection"
> > header lines.
> >
> > Chrome and Netscape work despite the double upgrade/close connection
> > headers. Safari fails.
> >
> > Small patch below against 6.8 to only send the "Connection: close"
> > header if we are not handling a http_status 101.
> >
> > Thanks,
> > Jonathon
> >
> >
> > cvs -q -d /cvs diff -ub -rOPENBSD_6_8 usr.sbin/relayd/relay_http.c
> >
> >
> > Index: usr.sbin/relayd/relay_http.c
> > ===================================================================
> > RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v
> > retrieving revision 1.79
> > diff -u -p -u -b -r1.79 relay_http.c
> > --- usr.sbin/relayd/relay_http.c 4 Sep 2020 13:09:14 -0000 1.79
> > +++ usr.sbin/relayd/relay_http.c 6 Mar 2021 19:46:56 -0000
> > @@ -526,7 +526,7 @@ relay_read_http(struct bufferevent *bev,
> > * Ask the server to close the connection after this request
> > * since we don't read any further request headers.
> > */
> > - if (cre->toread == TOREAD_UNLIMITED)
> > + if (cre->toread == TOREAD_UNLIMITED && desc->http_status != 101)
> > if (kv_add(&desc->http_headers, "Connection",
> > "close", 0) == NULL)
> > goto fail;
> >
Marcus,
I did not realize that there was already a party. Apologies for my
previous, duplicate, patch.
Reading through the thread, I came to the conclusion that the comments
worried that the previous patch(es) were not specific enough.
The current relayd behaviour is that outbound http responses have a
"Connection: close" header/value attached to them by relayd.
This can result in multiple "Connection" headers in the response
which is .. not good.
The current behaviour is because relayd does not handle repeated http
request/response sequences after the first one and prefers to force the
http session to close. Fortunately for websockets, the protocol after
the websocket upgrade is not http and so there is no need for relayd
to look for or process http headers after the upgrade.
Here is an updated patch. This avoids the incorrect current in-tree
behaviour in the following specific sitations:
1: The headers for an outbound (internal -> external) response already
include "Connection: Upgrade", "Upgrade: websocket" and the config
permits websocket upgrades, or
2: The headers for an outbound response already include a Connection
header with the value "close" - so do not send a duplicate as the
in-tree code currently does.
I think this is specfic enough for now. In order for a websocket upgrade
to work the external client has to request it and the internal server
has to respond in agreement.
I am explicit about websocket upgrades in my configs: I require the
"Connection" and "Upgrade" headers in the rule that directs traffic
to the websocket pool:
pass request quick \
header "Host" value "ws.example.com" \
header "Connection" value "Upgrade" \
header "Upgrade" value "websocket" \
forward to <websocket_pool>
This is for my use cases (tls accelerator) and relayd is adept at
handling them. I really appreciate the functionality of relayd in base.
Let me know if there are specific concerns about the patch below or
if there are specific preferred ways to accomplish better compliance
with the RFC within the context of relayd.
Thanks,
Jonathon
The Connection header is covered in:
https://tools.ietf.org/html/rfc7230#section-6
Index: usr.sbin/relayd/relay_http.c
===================================================================
RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v
retrieving revision 1.79
diff -u -p -u -b -r1.79 relay_http.c
--- usr.sbin/relayd/relay_http.c 4 Sep 2020 13:09:14 -0000 1.79
+++ usr.sbin/relayd/relay_http.c 8 Mar 2021 00:03:31 -0000
@@ -161,6 +161,8 @@ relay_read_http(struct bufferevent *bev,
size_t size, linelen;
struct kv *hdr = NULL;
struct kv *upgrade = NULL, *upgrade_ws = NULL;
+ struct kv *connection_close = NULL;
+ int ws_response = 0;
getmonotime(&con->se_tv_last);
cre->timedout = 0;
@@ -428,10 +430,12 @@ relay_read_http(struct bufferevent *bev,
/*
* HTTP 101 Switching Protocols
*/
+
upgrade = kv_find_value(&desc->http_headers,
"Connection", "upgrade", ",");
upgrade_ws = kv_find_value(&desc->http_headers,
"Upgrade", "websocket", ",");
+ ws_response = 0;
if (cre->dir == RELAY_DIR_REQUEST && upgrade_ws != NULL) {
if ((proto->httpflags & HTTPFLAG_WEBSOCKETS) == 0) {
relay_abort_http(con, 403,
@@ -450,6 +454,7 @@ relay_read_http(struct bufferevent *bev,
desc->http_status == 101) {
if (upgrade_ws != NULL && upgrade != NULL &&
(proto->httpflags & HTTPFLAG_WEBSOCKETS)) {
+ ws_response = 1;
cre->dst->toread = TOREAD_UNLIMITED;
cre->dst->bev->readcb = relay_read;
} else {
@@ -459,6 +464,9 @@ relay_read_http(struct bufferevent *bev,
}
}
+ connection_close = kv_find_value(&desc->http_headers,
+ "Connection", "close", ",");
+
switch (desc->http_method) {
case HTTP_METHOD_CONNECT:
/* Data stream */
@@ -524,9 +532,12 @@ relay_read_http(struct bufferevent *bev,
/*
* Ask the server to close the connection after this request
- * since we don't read any further request headers.
+ * since we don't read any further request headers. Only add
+ * this header if it does not already exist or if this is a
+ * outbound websocket upgrade response.
*/
- if (cre->toread == TOREAD_UNLIMITED)
+ if (cre->toread == TOREAD_UNLIMITED &&
+ connection_close == NULL && !ws_response)
if (kv_add(&desc->http_headers, "Connection",
"close", 0) == NULL)
goto fail;
