On Sun, Mar 07, 2021 at 06:22:04PM -0800, Jonathon Fletcher wrote: > 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 > > > > [email protected] (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 > > > > > >
snip > 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 > Here is the same relayd websocket upgrade patch as above, but against OPENBSD_6_9. Thanks, Jonathon Index: usr.sbin/relayd/relay_http.c =================================================================== RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v retrieving revision 1.81 diff -u -p -u -b -r1.81 relay_http.c --- usr.sbin/relayd/relay_http.c 24 Mar 2021 20:59:54 -0000 1.81 +++ usr.sbin/relayd/relay_http.c 2 May 2021 17:45:09 -0000 @@ -180,6 +180,8 @@ relay_read_http(struct bufferevent *bev, struct http_method_node *hmn; struct http_session *hs; enum httpmethod request_method; + struct kv *connection_close = NULL; + int ws_response = 0; getmonotime(&con->se_tv_last); cre->timedout = 0; @@ -493,6 +495,7 @@ relay_read_http(struct bufferevent *bev, "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, @@ -511,6 +514,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 { @@ -520,6 +524,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 */ @@ -586,9 +593,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;
