commited, Thanks for reporting and this and the patches, and sorry for the delay.
/Benno Sebastian Benoit([email protected]) on 2021.10.23 22:22:10 +0200: > Jonathon Fletcher([email protected]) on 2021.10.19 14:26:51 -0700: > > 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 > > > > > > > > > > [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 > > > > 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. > > Hi Jonathon, > > your diff is ok benno@, i'd like to have an ok from claudio@ too. > > rediffed with one whitespace change: > > diff --git usr.sbin/relayd/relay_http.c usr.sbin/relayd/relay_http.c > index b1cfafd9718..3d6d65f18e7 100644 > --- usr.sbin/relayd/relay_http.c > +++ usr.sbin/relayd/relay_http.c > @@ -177,6 +177,8 @@ relay_read_http(struct bufferevent *bev, void *arg) > 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, void *arg) > /* > * 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, void *arg) > 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, void *arg) > } > } > > + 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, void *arg) > > /* > * 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; > >
