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.
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;
