ok benno@
Reyk Floeter(r...@openbsd.org) on 2019.05.08 20:35:46 +0200: > On Wed, May 08, 2019 at 07:07:43PM +0200, Reyk Floeter wrote: > > On Wed, May 08, 2019 at 06:26:45PM +0200, Reyk Floeter wrote: > > > On Wed, Mar 06, 2019 at 05:36:32PM +0100, Sebastian Benoit wrote: > > > > Rivo Nurges(rivo.nur...@smit.ee) on 2019.03.05 22:42:13 +0000: > > > > > Hi! > > > > > > > > > > On 3/5/19 10:36 PM, Claudio Jeker wrote: > > > > > > I guess that this would need strcasestr() instead of strcasecmp(), > > > > > > since you > > > > > > are looking for the substring "Upgrade" in value. Maybe more is > > > > > > needed if > > > > > > we want to be sure that 'Connection: Upgrade-maybe' does not match. > > > > > > > > > > You are correct about strcasestr. "Connection: Upgrade-maybe" would > > > > > need > > > > > to have correct "Upgrade: websocket". Anyway, lets be strict. > > > > > > > > > > Does something like this make sense? > > > > > > > > i think the seperator list needs to include '\t' > > > > because https://tools.ietf.org/html/rfc7230#appendix-B includes HTAB. > > > > > > > > And i dont think you can mix "," with " \t" seperators, > > > > because otherwise "Foo Upgrade, Bar" will match. > > > > > > > > Something more is needed to parse elements of a header. > > > > > > > > > > I would reshuffle the websocket handling a bit as I don't think that > > > we need the http priv struct. We can lookup the parsed headers later. > > > > > > The attached diff moves it all into one places and introduces a new > > > function kv_find_value() that is able to to match headers with > > > multiple values (I think we might need it elsewhere. And even if not, > > > I would prefer to have this in a function intead of stuffing it into > > > relay_read_http). > > > > > > A minor question is if the lookup should be done before or after > > > applying the filters (relay_test - my diff does it afterwards, the > > > current code does it before). > > > > > > Tests? Comments? Ok? > > > > > > > I keep on replying to my own diffs... the updated one below uses > > strcasecmp instead of strcasestr in kv_find_value(). There is no need > > for substring- or fn- matching in this function. > > > > Next one... > > benno@ pointed out that the RFC allows whitespace before the comma. > We also don't have to strip \r\n as it is done by relayd's kv parser. > > OK? > > Reyk > > Index: usr.sbin/relayd/http.h > =================================================================== > RCS file: /cvs/src/usr.sbin/relayd/http.h,v > retrieving revision 1.10 > diff -u -p -u -p -r1.10 http.h > --- usr.sbin/relayd/http.h 4 Mar 2019 21:25:03 -0000 1.10 > +++ usr.sbin/relayd/http.h 8 May 2019 18:34:09 -0000 > @@ -251,10 +251,4 @@ struct http_descriptor { > struct kvtree http_headers; > }; > > -struct relay_http_priv { > -#define HTTP_CONNECTION_UPGRADE 0x01 > -#define HTTP_UPGRADE_WEBSOCKET 0x02 > - int http_upgrade_req; > -}; > - > #endif /* HTTP_H */ > Index: usr.sbin/relayd/relay.c > =================================================================== > RCS file: /cvs/src/usr.sbin/relayd/relay.c,v > retrieving revision 1.242 > diff -u -p -u -p -r1.242 relay.c > --- usr.sbin/relayd/relay.c 4 Mar 2019 21:25:03 -0000 1.242 > +++ usr.sbin/relayd/relay.c 8 May 2019 18:34:09 -0000 > @@ -1410,13 +1410,7 @@ relay_session(struct rsession *con) > return; > } > > - if (rlay->rl_proto->type == RELAY_PROTO_HTTP) { > - if (relay_http_priv_init(con) == -1) { > - relay_close(con, > - "failed to allocate http session data", 1); > - return; > - } > - } else { > + if (rlay->rl_proto->type != RELAY_PROTO_HTTP) { > if (rlay->rl_conf.fwdmode == FWD_TRANS) > relay_bindanyreq(con, 0, IPPROTO_TCP); > else if (relay_connect(con) == -1) { > Index: usr.sbin/relayd/relay_http.c > =================================================================== > RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v > retrieving revision 1.72 > diff -u -p -u -p -r1.72 relay_http.c > --- usr.sbin/relayd/relay_http.c 4 Mar 2019 21:25:03 -0000 1.72 > +++ usr.sbin/relayd/relay_http.c 8 May 2019 18:34:09 -0000 > @@ -110,17 +110,6 @@ relay_http_init(struct relay *rlay) > } > > int > -relay_http_priv_init(struct rsession *con) > -{ > - struct relay_http_priv *p; > - if ((p = calloc(1, sizeof(struct relay_http_priv))) == NULL) > - return (-1); > - > - con->se_priv = p; > - return (0); > -} > - > -int > relay_httpdesc_init(struct ctl_relay_event *cre) > { > struct http_descriptor *desc; > @@ -163,13 +152,13 @@ relay_read_http(struct bufferevent *bev, > struct relay *rlay = con->se_relay; > struct protocol *proto = rlay->rl_proto; > struct evbuffer *src = EVBUFFER_INPUT(bev); > - struct relay_http_priv *priv = con->se_priv; > char *line = NULL, *key, *value; > char *urlproto, *host, *path; > int action, unique, ret; > const char *errstr; > size_t size, linelen; > struct kv *hdr = NULL; > + struct kv *upgrade = NULL, *upgrade_ws = NULL; > > getmonotime(&con->se_tv_last); > cre->timedout = 0; > @@ -398,17 +387,6 @@ relay_read_http(struct bufferevent *bev, > unique = 0; > > if (cre->line != 1) { > - if (cre->dir == RELAY_DIR_REQUEST) { > - if (strcasecmp("Connection", key) == 0 && > - strcasecmp("Upgrade", value) == 0) > - priv->http_upgrade_req |= > - HTTP_CONNECTION_UPGRADE; > - if (strcasecmp("Upgrade", key) == 0 && > - strcasecmp("websocket", value) == 0) > - priv->http_upgrade_req |= > - HTTP_UPGRADE_WEBSOCKET; > - } > - > if ((hdr = kv_add(&desc->http_headers, key, > value, unique)) == NULL) { > relay_abort_http(con, 400, > @@ -445,37 +423,34 @@ relay_read_http(struct bufferevent *bev, > return; > } > > - /* HTTP 101 Switching Protocols */ > - if (cre->dir == RELAY_DIR_REQUEST) { > - if ((priv->http_upgrade_req & HTTP_UPGRADE_WEBSOCKET) && > - !(proto->httpflags & HTTPFLAG_WEBSOCKETS)) { > + /* > + * HTTP 101 Switching Protocols > + */ > + upgrade = kv_find_value(&desc->http_headers, > + "Connection", "upgrade", ","); > + upgrade_ws = kv_find_value(&desc->http_headers, > + "Upgrade", "websocket", ","); > + if (cre->dir == RELAY_DIR_REQUEST && upgrade_ws != NULL) { > + if ((proto->httpflags & HTTPFLAG_WEBSOCKETS) == 0) { > relay_abort_http(con, 403, > "Websocket Forbidden", 0); > return; > - } > - if ((priv->http_upgrade_req & HTTP_UPGRADE_WEBSOCKET) && > - !(priv->http_upgrade_req & HTTP_CONNECTION_UPGRADE)) > - { > + } else if (upgrade == NULL) { > relay_abort_http(con, 400, > "Bad Websocket Request", 0); > return; > - } > - if ((priv->http_upgrade_req & HTTP_UPGRADE_WEBSOCKET) && > - (desc->http_method != HTTP_METHOD_GET)) { > + } else if (desc->http_method != HTTP_METHOD_GET) { > relay_abort_http(con, 405, > "Websocket Method Not Allowed", 0); > return; > } > } else if (cre->dir == RELAY_DIR_RESPONSE && > desc->http_status == 101) { > - if (((priv->http_upgrade_req & > - (HTTP_CONNECTION_UPGRADE | HTTP_UPGRADE_WEBSOCKET)) > - == > - (HTTP_CONNECTION_UPGRADE | HTTP_UPGRADE_WEBSOCKET)) > - && proto->httpflags & HTTPFLAG_WEBSOCKETS) { > + if (upgrade_ws != NULL && upgrade != NULL && > + (proto->httpflags & HTTPFLAG_WEBSOCKETS)) { > cre->dst->toread = TOREAD_UNLIMITED; > cre->dst->bev->readcb = relay_read; > - } else { > + } else { > relay_abort_http(con, 502, > "Bad Websocket Gateway", 0); > return; > Index: usr.sbin/relayd/relayd.c > =================================================================== > RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v > retrieving revision 1.175 > diff -u -p -u -p -r1.175 relayd.c > --- usr.sbin/relayd/relayd.c 24 Apr 2019 19:13:49 -0000 1.175 > +++ usr.sbin/relayd/relayd.c 8 May 2019 18:34:09 -0000 > @@ -812,6 +812,48 @@ kv_find(struct kvtree *keys, struct kv * > return (match); > } > > +struct kv * > +kv_find_value(struct kvtree *keys, char *key, const char *value, > + const char *delim) > +{ > + struct kv *match, kv; > + char *val = NULL, *next, *ptr; > + size_t len; > + > + kv.kv_key = key; > + if ((match = RB_FIND(kvtree, keys, &kv)) == NULL) > + return (NULL); > + > + if (match->kv_value == NULL) > + return (NULL); > + > + if (delim == NULL) { > + if (strcasecmp(match->kv_value, value) == 0) > + goto done; > + } else { > + if ((val = strdup(match->kv_value)) == NULL) > + return (NULL); > + for (next = ptr = val; ptr != NULL; > + ptr = strsep(&next, delim)) { > + /* strip whitespace */ > + ptr += strspn(ptr, " \t"); > + len = strcspn(ptr, " \t"); > + if (strncasecmp(ptr, value, len) == 0) > + goto done; > + } > + } > + > + /* not matched */ > + match = NULL; > + done: > +#ifdef DEBUG > + if (match != NULL) > + DPRINTF("%s: matched %s: %s", __func__, key, value); > +#endif > + free(val); > + return (match); > +} > + > int > kv_cmp(struct kv *a, struct kv *b) > { > Index: usr.sbin/relayd/relayd.h > =================================================================== > RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v > retrieving revision 1.252 > diff -u -p -u -p -r1.252 relayd.h > --- usr.sbin/relayd/relayd.h 4 Mar 2019 21:25:03 -0000 1.252 > +++ usr.sbin/relayd/relayd.h 8 May 2019 18:34:10 -0000 > @@ -1324,6 +1324,8 @@ void relay_log(struct rsession *, char > int kv_log(struct rsession *, struct kv *, u_int16_t, > enum direction); > struct kv *kv_find(struct kvtree *, struct kv *); > +struct kv *kv_find_value(struct kvtree *, char *, const char *, > + const char *); > int kv_cmp(struct kv *, struct kv *); > int rule_add(struct protocol *, struct relay_rule *, const char > *); >
