Skip to content

Commit

Permalink
fix(proxy) hop-by-hop headers to not clear upgrade header on upgrade (#…
Browse files Browse the repository at this point in the history
…5495)

### Summary

`HTTP` `Connection` header usually carries a list of `headers` meant
for receiver (in this case proxy) that are not meant to be proxied
further (so called hop-by-hop headers). Our gateway correctly cleans
these headers, but it was a bit too aggressive.

It turns out as reported on #5465 that websockets upgrade mechanism
is broken with Firefox, but works on Chrome. The difference being that
Chrome sends following headers:

```http
Connection: Upgrade
Upgrade: websocket
```

while Firefox sends the following:

```
Connection: keep-alive, Upgrade
Upgrade: websocket
```

As the `Upgrade` was listed on `Connection` header by Firefox, the Kong
incorrecly removed the `Upgrade` header from the upstream headers, and
thus it made the websockets upgrade to fail.

This PR fixed it so that we do not remove `Upgrade` header on such case.

This PR also adds `keep-alive` to upstream request `Connection` header
when proxying the upgrade request (just like Firefox does).

### Issues Resolved

Fix #5465
  • Loading branch information
bungle authored Jan 23, 2020
1 parent 74559e2 commit f1f1c61
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 4 deletions.
13 changes: 9 additions & 4 deletions kong/runloop/handler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -1117,7 +1117,7 @@ return {

-- Keep-Alive and WebSocket Protocol Upgrade Headers
if var.http_upgrade and lower(var.http_upgrade) == "websocket" then
var.upstream_connection = "upgrade"
var.upstream_connection = "keep-alive, Upgrade"
var.upstream_upgrade = "websocket"

else
Expand Down Expand Up @@ -1198,9 +1198,14 @@ return {
-- clear hop-by-hop request headers:
for _, header_name in csv(var.http_connection) do
-- some of these are already handled by the proxy module,
-- proxy-authorization being an exception that is handled
-- below with special semantics.
if header_name ~= "proxy-authorization" then
-- proxy-authorization and upgrade being an exception that
-- is handled below with special semantics.
if header_name == "upgrade" then
if var.upstream_connection == "keep-alive" then
clear_header(header_name)
end

elseif header_name ~= "proxy-authorization" then
clear_header(header_name)
end
end
Expand Down
16 changes: 16 additions & 0 deletions spec/02-integration/05-proxy/03-upstream_headers_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,22 @@ for _, strategy in helpers.each_strategy() do

assert.equal("Expires", headers["Trailer"])
end)

it("keeps upgrade when upgrading", function()
local res = assert(proxy_client:send {
method = "GET",
headers = {
["Host"] = "headers-inspect.com",
["Connection"] = "keep-alive, Upgrade",
["Upgrade"] = "websocket"
},
path = "/get",
})

local json = cjson.decode(assert.res_status(200, res))
assert.equal("keep-alive, Upgrade", json.headers.connection)
assert.equal("websocket", json.headers.upgrade)
end)
end)

describe("(using the default configuration values)", function()
Expand Down

0 comments on commit f1f1c61

Please sign in to comment.