On Sun, May 02, 2021 at 11:05:16AM -0700, Jonathon Fletcher wrote: > 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 > > > > > > 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 > > > > > > > > > > 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
Hi, here is the same relayd websocket upgrade patch as above, but against OPENBSD_7_0. The OPENBSD_6_9 patch is almost the same, there were some minor changes (printing strings with "%s") that move one of the patch hunks a little. Thanks, Jonathon Index: usr.sbin/relayd/relay_http.c =================================================================== RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v retrieving revision 1.82 diff -u -p -r1.82 relay_http.c --- usr.sbin/relayd/relay_http.c 25 Jul 2021 20:31:41 -0000 1.82 +++ usr.sbin/relayd/relay_http.c 7 Oct 2021 15:55:23 -0000 @@ -177,6 +177,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; struct http_method_node *hmn; struct http_session *hs; enum httpmethod request_method; @@ -489,10 +491,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, @@ -511,6 +515,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 +525,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 +594,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;