From 3b066dc4cb66feca63d231ec89feb66c6cb1b7f1 Mon Sep 17 00:00:00 2001 From: Chrono Date: Fri, 7 Jun 2024 14:43:18 +0800 Subject: [PATCH] refactor(tools/http): simplify `check_https()` with `ngx.var` (#13167) We used to call `ngx.req.get_headers()` to get all possible header values, but since nginx 1.23.0 or OpenResty 1.25.3.1 `ngx.var` can get a combined value for all header values with identical name (joined by comma), so I think that we could simplify these code. There is one concern: If header `X-Forwarded-Proto` has a comma separated value like `http, https`, This PR will think it is a multiple header request and report an error `Only one X-Forwarded-Proto header allowed`. But I think that it is an unlikely case in the real world. KAG-4673 --- kong/tools/http.lua | 9 +++++--- spec/01-unit/05-utils_spec.lua | 25 ++++++++++------------ t/01-pdk/05-client/08-get_protocol.t | 31 ++++++++++++++++++++++++++++ 3 files changed, 48 insertions(+), 17 deletions(-) diff --git a/kong/tools/http.lua b/kong/tools/http.lua index 34ca72ccdc2b..613661d68d2f 100644 --- a/kong/tools/http.lua +++ b/kong/tools/http.lua @@ -210,16 +210,19 @@ _M.check_https = function(trusted_ip, allow_terminated) -- otherwise, we fall back to relying on the client scheme -- (which was either validated earlier, or we fall through this block) if trusted_ip then - local scheme = ngx.req.get_headers()["x-forwarded-proto"] + local scheme = ngx.var.http_x_forwarded_proto + if not scheme then + return false + end -- we could use the first entry (lower security), or check the contents of -- each of them (slow). So for now defensive, and error -- out on multiple entries for the x-forwarded-proto header. - if type(scheme) == "table" then + if scheme:find(",", 1, true) then return nil, "Only one X-Forwarded-Proto header allowed" end - return tostring(scheme):lower() == "https" + return scheme:lower() == "https" end return false diff --git a/spec/01-unit/05-utils_spec.lua b/spec/01-unit/05-utils_spec.lua index 14e23b3d85b7..32bea716132e 100644 --- a/spec/01-unit/05-utils_spec.lua +++ b/spec/01-unit/05-utils_spec.lua @@ -80,17 +80,14 @@ describe("Utils", function() describe("https_check", function() local old_ngx - local headers = {} lazy_setup(function() old_ngx = ngx _G.ngx = { var = { - scheme = nil + scheme = nil, + http_x_forwarded_proto = nil, }, - req = { - get_headers = function() return headers end - } } end) @@ -100,7 +97,7 @@ describe("Utils", function() describe("without X-Forwarded-Proto header", function() lazy_setup(function() - headers["x-forwarded-proto"] = nil + ngx.var.http_x_forwarded_proto = nil end) it("should validate an HTTPS scheme", function() @@ -124,11 +121,11 @@ describe("Utils", function() describe("with X-Forwarded-Proto header", function() lazy_teardown(function() - headers["x-forwarded-proto"] = nil + ngx.var.http_x_forwarded_proto = nil end) it("should validate any scheme with X-Forwarded_Proto as HTTPS", function() - headers["x-forwarded-proto"] = "hTTPs" -- check mixed casing for case insensitiveness + ngx.var.http_x_forwarded_proto = "hTTPs" -- check mixed casing for case insensitiveness ngx.var.scheme = "hTTps" assert.is.truthy(tools_http.check_https(true, true)) ngx.var.scheme = "hTTp" @@ -138,7 +135,7 @@ describe("Utils", function() end) it("should validate only https scheme with X-Forwarded_Proto as non-HTTPS", function() - headers["x-forwarded-proto"] = "hTTP" + ngx.var.http_x_forwarded_proto = "hTTP" ngx.var.scheme = "hTTps" assert.is.truthy(tools_http.check_https(true, true)) ngx.var.scheme = "hTTp" @@ -148,7 +145,7 @@ describe("Utils", function() end) it("should return an error with multiple X-Forwarded_Proto headers", function() - headers["x-forwarded-proto"] = { "hTTP", "https" } + ngx.var.http_x_forwarded_proto = "hTTP, https" ngx.var.scheme = "hTTps" assert.is.truthy(tools_http.check_https(true, true)) ngx.var.scheme = "hTTp" @@ -157,19 +154,19 @@ describe("Utils", function() end) it("should not use X-Forwarded-Proto when the client is untrusted", function() - headers["x-forwarded-proto"] = "https" + ngx.var.http_x_forwarded_proto = "https" ngx.var.scheme = "http" assert.is_false(tools_http.check_https(false, false)) assert.is_false(tools_http.check_https(false, true)) - headers["x-forwarded-proto"] = "https" + ngx.var.http_x_forwarded_proto = "https" ngx.var.scheme = "https" assert.is_true(tools_http.check_https(false, false)) assert.is_true(tools_http.check_https(false, true)) end) it("should use X-Forwarded-Proto when the client is trusted", function() - headers["x-forwarded-proto"] = "https" + ngx.var.http_x_forwarded_proto = "https" ngx.var.scheme = "http" -- trusted client but do not allow terminated @@ -177,7 +174,7 @@ describe("Utils", function() assert.is_true(tools_http.check_https(true, true)) - headers["x-forwarded-proto"] = "https" + ngx.var.http_x_forwarded_proto = "https" ngx.var.scheme = "https" assert.is_true(tools_http.check_https(true, false)) assert.is_true(tools_http.check_https(true, true)) diff --git a/t/01-pdk/05-client/08-get_protocol.t b/t/01-pdk/05-client/08-get_protocol.t index b9c04899aa9b..be68dd2e0279 100644 --- a/t/01-pdk/05-client/08-get_protocol.t +++ b/t/01-pdk/05-client/08-get_protocol.t @@ -301,3 +301,34 @@ qq{ protocol=tls --- no_error_log [error] + + + +=== TEST 11: client.get_protocol() fails when kong receives an http request with multiple x-forwarded-proto headers + +--- http_config eval: $t::Util::HttpConfig +--- config + location = /t { + access_by_lua_block { + ngx.ctx.route = { + protocols = { "http", "https" } + } + + local PDK = require "kong.pdk" + local pdk = PDK.new() + pdk.ip.is_trusted = function() return true end -- mock + + local ok, err = pdk.client.get_protocol(true) + assert(ok == nil) + ngx.say("err=", err) + } + } +--- request +GET /t +--- more_headers +X-Forwarded-Proto: https +X-Forwarded-Proto: http +--- response_body +err=Only one X-Forwarded-Proto header allowed +--- no_error_log +[error]