AlinsRan commented on code in PR #13939:
URL: https://github.com/apache/apisix/pull/13939#discussion_r4013893671
##########
apisix/init.lua:
##########
@@ -988,6 +1051,196 @@ function _M.grpc_access_phase()
end
+-- call ws_x_frame hook
+function _M.websocket_content_phase()
+ ngx.ctx = fetch_ctx()
+ local api_ctx = ngx.ctx.api_ctx
+ local up_conf = api_ctx.upstream_conf
+ -- a Route's own `timeout` overrides upstream.timeout, same as
+ -- set_balancer_opts() does for the plain proxy_pass path
+ local route = api_ctx.matched_route
+ local up_timeout = (route and route.value and route.value.timeout) or
up_conf.timeout
+ local connect_timeout_ms = up_timeout and up_timeout.connect and
up_timeout.connect * 1000
+ local recv_timeout_ms = up_timeout and up_timeout.read and up_timeout.read
* 1000
+ -- upstream.timeout.send is silently ignored for ws/wss
+
+ local ws_headers = build_ws_forward_headers(api_ctx)
+ local ws_protocols = core.request.header(api_ctx, "Sec-WebSocket-Protocol")
+ local ws_origin = core.request.header(api_ctx, "Origin")
+
+ -- resolve upstream.tls once, same as https/grpcs in apisix/upstream.lua
+ local ssl_verify, client_cert, client_priv_key
+ if api_ctx.matched_upstream.scheme == "wss" and up_conf.tls then
+ ssl_verify = up_conf.tls.verify
+
+ if up_conf.tls.client_cert or up_conf.tls.client_cert_id then
+ local cert_pem, key_pem
+ if up_conf.tls.client_cert_id then
+ cert_pem = api_ctx.upstream_ssl and api_ctx.upstream_ssl.cert
+ key_pem = api_ctx.upstream_ssl and api_ctx.upstream_ssl.key
+ else
+ cert_pem = up_conf.tls.client_cert
+ key_pem = up_conf.tls.client_key
+ end
+
+ local cert_err, key_err
+ client_cert, cert_err =
apisix_ssl.fetch_cert(api_ctx.var.upstream_host, cert_pem)
+ if not client_cert then
+ ngx.log(ngx.ERR, "failed to fetch websocket upstream client
cert: ", cert_err)
+ return core.response.exit(503)
+ end
+
+ client_priv_key, key_err =
apisix_ssl.fetch_pkey(api_ctx.var.upstream_host, key_pem)
+ if not client_priv_key then
+ ngx.log(ngx.ERR, "failed to fetch websocket upstream client
key: ", key_err)
+ return core.response.exit(503)
+ end
+ end
+ end
+
+ local ok, proxy, err = pcall(ws_proxy.new, {
Review Comment:
Single frames larger than 65535 bytes kill the connection on this path.
`ws_proxy.new()` builds `resty.websocket.client` and
`resty.websocket.server` without options (see the TODO at
`resty/websocket/proxy.lua:110`), so both sides use the library default
`max_payload_len = 65535`. `protocol.lua` checks that per frame in
`recv_frame()`, before `aggregate_fragments` joins anything, so leaving
`client_max_frame_size` & co. unset does not avoid it. An oversized frame marks
the socket fatal and the forwarder tears down the whole connection.
`enable_websocket` has no such limit: after the 101, nginx relays raw bytes
and never parses frames. Verified locally against the same upstream (client ->
APISIX -> echo server that reports the received length):
| frame size | `scheme: ws` | `enable_websocket` |
|---|---|---|
| 65535 | echoed | echoed |
| 65536 | connection closed (`failed to receive the first 2 bytes: closed`)
| echoed |
| 1 MiB | - | echoed |
Most client libraries do not fragment by default, so an app sending a large
JSON message or a file chunk breaks after switching to `ws`/`wss`.
`resty.websocket.proxy` needs to pass `max_payload_len` (or
`max_recv_len`/`max_send_len`) through to both constructors, with a default
that does not cap ordinary traffic. A test with a >64K frame in each direction
would cover it.
##########
apisix/init.lua:
##########
@@ -330,6 +346,46 @@ local function set_upstream_headers(api_ctx, picked_server)
end
+-- hop-by-hop headers, plus handshake headers connect() already sets itself
+-- (host/protocols/origin opts, or generated Sec-WebSocket-Key/-Version).
+local ws_skip_forward_headers = {
+ ["host"] = true,
+ ["connection"] = true,
+ ["upgrade"] = true,
+ ["keep-alive"] = true,
+ ["te"] = true,
+ ["trailers"] = true,
+ ["proxy-authenticate"] = true,
+ ["proxy-authorization"] = true,
+ ["content-length"] = true,
+ ["transfer-encoding"] = true,
+ ["sec-websocket-key"] = true,
+ ["sec-websocket-version"] = true,
+ ["sec-websocket-extensions"] = true,
+ ["sec-websocket-protocol"] = true,
+ ["origin"] = true,
+}
+
+
+-- forwards the client's other headers (Cookie, Authorization, ...) upstream.
+local function build_ws_forward_headers(api_ctx)
Review Comment:
Client-supplied `X-Real-IP` and `X-Forwarded-For` are forwarded upstream
verbatim here.
On the `proxy_pass` path these two never come from the request:
`ngx_tpl.lua` sets them with `proxy_set_header X-Real-IP $remote_addr` and
`proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for`, and those only
apply to the proxy module, which `@websocket_pass` does not use.
`X-Forwarded-Proto/Host/Port` come out right because `more_set_input_headers`
rewrites `r->headers_in` itself, so `core.request.headers()` already sees the
sanitized values. `X-Real-IP`/`X-Forwarded-For` in `headers_in` are still
whatever the client sent, they are not in `ws_skip_forward_headers`, and
nothing here adds the connection address.
Verified locally, client sends `X-Real-IP: 1.2.3.4` and `X-Forwarded-For:
5.6.7.8`, upstream receives:
- `scheme: ws`: `X-Real-IP: 1.2.3.4`, `X-Forwarded-For: 5.6.7.8`
- `enable_websocket`: `X-Real-IP: 127.0.0.1`, `X-Forwarded-For: 5.6.7.8,
127.0.0.1`
An upstream that trusts `X-Real-IP` for the client address (audit logs, IP
allowlists, rate limiting) can be spoofed on a `ws`/`wss` route, and when the
client sends neither header the upstream gets no client address at all. Suggest
adding both names to `ws_skip_forward_headers` and appending `X-Real-IP:
<var.remote_addr>` and `X-Forwarded-For: <var.proxy_add_x_forwarded_for>`
explicitly, with a test asserting the upstream values.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
