Re: relayd patch for websocket upgrade
commited, Thanks for reporting and this and the patches, and sorry for the delay. /Benno Sebastian Benoit(be...@openbsd.org) on 2021.10.23 22:22:10 +0200: > Jonathon Fletcher(jonathon.fletc...@gmail.com) 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=15833439123 > > > > > > > > > > especially the two comments by sthen@: > > > > > > > > > > https://marc.info/?m=161349608614743 > > > > > https://marc.info/?m=16135019371 > > > > > > > > > > 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 >
Re: relayd patch for websocket upgrade
Jonathon Fletcher(jonathon.fletc...@gmail.com) 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=15833439123 > > > > > > > > especially the two comments by sthen@: > > > > > > > > https://marc.info/?m=161349608614743 > > > > https://marc.info/?m=16135019371 > > > > > > > > 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 > > > > > > > > > This is for my use cases (tls accelerator) and relayd is adept at > > > handling them. I really appreciate the functionality of relayd in base. > > > &
Re: relayd patch for websocket upgrade
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=15833439123 > > > > > > especially the two comments by sthen@: > > > > > > https://marc.info/?m=161349608614743 > > > https://marc.info/?m=16135019371 > > > > > > 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 > > > > > > 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
Re: relayd patch for websocket upgrade
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=15833439123 > > > > especially the two comments by sthen@: > > > > https://marc.info/?m=161349608614743 > > https://marc.info/?m=16135019371 > > > > 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 > > > 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.c24 Mar 2021 20:59:54 - 1.81 +++ usr.sbin/relayd/relay_http.c2 May 2021 17:45:09 - @@ -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
Re: relayd patch for websocket upgrade
On Sun, Mar 07, 2021 at 06:46:33PM +0100, Marcus MERIGHI wrote: > Hello Jonathon! > > welcome to the party: > > https://marc.info/?t=15833439123 > > especially the two comments by sthen@: > > https://marc.info/?m=161349608614743 > https://marc.info/?m=16135019371 > > 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.c4 Sep 2020 13:09:14 - 1.79 > > +++ usr.sbin/relayd/relay_http.c6 Mar 2021 19:46:56 - > > @@ -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(>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 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.c4 Sep 2020 13:09:14 - 1.79 +++ usr.sbin/relayd/relay_http.c8 Mar 2021 00:03:31 - @@ -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(>se_tv_last);
Re: relayd patch for websocket upgrade
Hello Jonathon! welcome to the party: https://marc.info/?t=15833439123 especially the two comments by sthen@: https://marc.info/?m=161349608614743 https://marc.info/?m=16135019371 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 - 1.79 > +++ usr.sbin/relayd/relay_http.c 6 Mar 2021 19:46:56 - > @@ -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(>http_headers, "Connection", > "close", 0) == NULL) > goto fail; >
relayd patch for websocket upgrade
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.c4 Sep 2020 13:09:14 - 1.79 +++ usr.sbin/relayd/relay_http.c6 Mar 2021 19:46:56 - @@ -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(>http_headers, "Connection", "close", 0) == NULL) goto fail;
