diff --git a/CHANGELOG/unreleased/kong/11502.yaml b/CHANGELOG/unreleased/kong/11502.yaml new file mode 100644 index 000000000000..66d1d45e2658 --- /dev/null +++ b/CHANGELOG/unreleased/kong/11502.yaml @@ -0,0 +1,7 @@ +message: Fix upstream ssl failure when plugins use response handler +type: bugfix +scope: Core +prs: + - 11502 +jiras: + - "FTI-5347" diff --git a/kong-3.4.1-0.rockspec b/kong-3.4.1-0.rockspec index 094de28164b3..7cfc52ce7026 100644 --- a/kong-3.4.1-0.rockspec +++ b/kong-3.4.1-0.rockspec @@ -166,6 +166,7 @@ build = { ["kong.runloop.log_level"] = "kong/runloop/log_level.lua", ["kong.runloop.certificate"] = "kong/runloop/certificate.lua", ["kong.runloop.plugins_iterator"] = "kong/runloop/plugins_iterator.lua", + ["kong.runloop.upstream_ssl"] = "kong/runloop/upstream_ssl.lua", ["kong.runloop.balancer"] = "kong/runloop/balancer/init.lua", ["kong.runloop.balancer.balancers"] = "kong/runloop/balancer/balancers.lua", ["kong.runloop.balancer.consistent_hashing"] = "kong/runloop/balancer/consistent_hashing.lua", diff --git a/kong/runloop/balancer/init.lua b/kong/runloop/balancer/init.lua index a532412c4ff8..7515cfc1c875 100644 --- a/kong/runloop/balancer/init.lua +++ b/kong/runloop/balancer/init.lua @@ -1,11 +1,11 @@ local pl_tablex = require "pl.tablex" local utils = require "kong.tools.utils" local hooks = require "kong.hooks" -local get_certificate = require("kong.runloop.certificate").get_certificate local recreate_request = require("ngx.balancer").recreate_request local healthcheckers = require "kong.runloop.balancer.healthcheckers" local balancers = require "kong.runloop.balancer.balancers" +local upstream_ssl = require "kong.runloop.upstream_ssl" local upstreams = require "kong.runloop.balancer.upstreams" local targets = require "kong.runloop.balancer.targets" @@ -38,7 +38,7 @@ local EMPTY_T = pl_tablex.readonly {} local set_authority -local set_upstream_cert_and_key = require("resty.kong.tls").set_upstream_cert_and_key +local fallback_upstream_client_cert = upstream_ssl.fallback_upstream_client_cert if ngx.config.subsystem ~= "stream" then set_authority = require("resty.kong.grpc").set_authority @@ -321,6 +321,8 @@ local function execute(balancer_data, ctx) -- store for retries balancer_data.balancer = balancer + -- store for use in subrequest `ngx.location.capture("kong_buffered_http")` + balancer_data.upstream = upstream -- calculate hash-value -- only add it if it doesn't exist, in case a plugin inserted one @@ -330,27 +332,7 @@ local function execute(balancer_data, ctx) balancer_data.hash_value = hash_value end - if ctx and ctx.service and not ctx.service.client_certificate then - -- service level client_certificate is not set - local cert, res, err - local client_certificate = upstream.client_certificate - - -- does the upstream object contains a client certificate? - if client_certificate then - cert, err = get_certificate(client_certificate) - if not cert then - log(ERR, "unable to fetch upstream client TLS certificate ", - client_certificate.id, ": ", err) - return - end - - res, err = set_upstream_cert_and_key(cert.cert, cert.key) - if not res then - log(ERR, "unable to apply upstream client TLS certificate ", - client_certificate.id, ": ", err) - end - end - end + fallback_upstream_client_cert(ctx, upstream) end end diff --git a/kong/runloop/handler.lua b/kong/runloop/handler.lua index 1f25c3f406be..8de5c69352f2 100644 --- a/kong/runloop/handler.lua +++ b/kong/runloop/handler.lua @@ -6,9 +6,9 @@ local Router = require "kong.router" local balancer = require "kong.runloop.balancer" local events = require "kong.runloop.events" local wasm = require "kong.runloop.wasm" +local upstream_ssl = require "kong.runloop.upstream_ssl" local reports = require "kong.reports" local constants = require "kong.constants" -local certificate = require "kong.runloop.certificate" local concurrency = require "kong.concurrency" local lrucache = require "resty.lrucache" local ktls = require "resty.kong.tls" @@ -72,7 +72,6 @@ local NOOP = function() end local ERR = ngx.ERR -local CRIT = ngx.CRIT local NOTICE = ngx.NOTICE local WARN = ngx.WARN local INFO = ngx.INFO @@ -110,10 +109,7 @@ local STREAM_TLS_PASSTHROUGH_SOCK local set_authority -local set_upstream_cert_and_key = ktls.set_upstream_cert_and_key -local set_upstream_ssl_verify = ktls.set_upstream_ssl_verify -local set_upstream_ssl_verify_depth = ktls.set_upstream_ssl_verify_depth -local set_upstream_ssl_trusted_store = ktls.set_upstream_ssl_trusted_store +local set_service_ssl = upstream_ssl.set_service_ssl if is_http_module then set_authority = require("resty.kong.grpc").set_authority @@ -763,9 +759,6 @@ end local balancer_prepare do - local get_certificate = certificate.get_certificate - local get_ca_certificate_store = certificate.get_ca_certificate_store - local function sleep_once_for_balancer_init() ngx.sleep(0) sleep_once_for_balancer_init = NOOP @@ -814,60 +807,7 @@ do ctx.route = route ctx.balancer_data = balancer_data - if service then - local res, err - local client_certificate = service.client_certificate - - if client_certificate then - local cert, err = get_certificate(client_certificate) - if not cert then - log(ERR, "unable to fetch upstream client TLS certificate ", - client_certificate.id, ": ", err) - return - end - - res, err = set_upstream_cert_and_key(cert.cert, cert.key) - if not res then - log(ERR, "unable to apply upstream client TLS certificate ", - client_certificate.id, ": ", err) - end - end - - local tls_verify = service.tls_verify - if tls_verify then - res, err = set_upstream_ssl_verify(tls_verify) - if not res then - log(CRIT, "unable to set upstream TLS verification to: ", - tls_verify, ", err: ", err) - end - end - - local tls_verify_depth = service.tls_verify_depth - if tls_verify_depth then - res, err = set_upstream_ssl_verify_depth(tls_verify_depth) - if not res then - log(CRIT, "unable to set upstream TLS verification to: ", - tls_verify, ", err: ", err) - -- in case verify can not be enabled, request can no longer be - -- processed without potentially compromising security - return kong.response.exit(500) - end - end - - local ca_certificates = service.ca_certificates - if ca_certificates then - res, err = get_ca_certificate_store(ca_certificates) - if not res then - log(CRIT, "unable to get upstream TLS CA store, err: ", err) - - else - res, err = set_upstream_ssl_trusted_store(res) - if not res then - log(CRIT, "unable to set upstream TLS CA store, err: ", err) - end - end - end - end + set_service_ssl(ctx) if is_stream_module and scheme == "tcp" then local res, err = disable_proxy_ssl() diff --git a/kong/runloop/upstream_ssl.lua b/kong/runloop/upstream_ssl.lua new file mode 100644 index 000000000000..dc32f64c2c50 --- /dev/null +++ b/kong/runloop/upstream_ssl.lua @@ -0,0 +1,115 @@ +local certificate = require "kong.runloop.certificate" +local ktls = require "resty.kong.tls" + + +local kong = kong +local ngx = ngx +local log = ngx.log +local ERR = ngx.ERR +local CRIT = ngx.CRIT + +local get_certificate = certificate.get_certificate +local get_ca_certificate_store = certificate.get_ca_certificate_store +local set_upstream_cert_and_key = ktls.set_upstream_cert_and_key +local set_upstream_ssl_verify = ktls.set_upstream_ssl_verify +local set_upstream_ssl_verify_depth = ktls.set_upstream_ssl_verify_depth +local set_upstream_ssl_trusted_store = ktls.set_upstream_ssl_trusted_store + + +local function set_service_ssl(ctx) + local service = ctx and ctx.service + + if service then + local res, err + local client_certificate = service.client_certificate + + if client_certificate then + local cert, err = get_certificate(client_certificate) + if not cert then + log(ERR, "unable to fetch upstream client TLS certificate ", + client_certificate.id, ": ", err) + return + end + + res, err = set_upstream_cert_and_key(cert.cert, cert.key) + if not res then + log(ERR, "unable to apply upstream client TLS certificate ", + client_certificate.id, ": ", err) + end + end + + local tls_verify = service.tls_verify + if tls_verify then + res, err = set_upstream_ssl_verify(tls_verify) + if not res then + log(CRIT, "unable to set upstream TLS verification to: ", + tls_verify, ", err: ", err) + end + end + + local tls_verify_depth = service.tls_verify_depth + if tls_verify_depth then + res, err = set_upstream_ssl_verify_depth(tls_verify_depth) + if not res then + log(CRIT, "unable to set upstream TLS verification to: ", + tls_verify, ", err: ", err) + -- in case verify can not be enabled, request can no longer be + -- processed without potentially compromising security + return kong.response.exit(500) + end + end + + local ca_certificates = service.ca_certificates + if ca_certificates then + res, err = get_ca_certificate_store(ca_certificates) + if not res then + log(CRIT, "unable to get upstream TLS CA store, err: ", err) + + else + res, err = set_upstream_ssl_trusted_store(res) + if not res then + log(CRIT, "unable to set upstream TLS CA store, err: ", err) + end + end + end + end +end + +local function fallback_upstream_client_cert(ctx, upstream) + if not ctx then + return + end + + upstream = upstream or (ctx.balancer_data and ctx.balancer_data.upstream) + + if not upstream then + return + end + + if ctx.service and not ctx.service.client_certificate then + -- service level client_certificate is not set + local cert, res, err + local client_certificate = upstream.client_certificate + + -- does the upstream object contains a client certificate? + if client_certificate then + cert, err = get_certificate(client_certificate) + if not cert then + log(ERR, "unable to fetch upstream client TLS certificate ", + client_certificate.id, ": ", err) + return + end + + res, err = set_upstream_cert_and_key(cert.cert, cert.key) + if not res then + log(ERR, "unable to apply upstream client TLS certificate ", + client_certificate.id, ": ", err) + end + end + end +end + +return { + set_service_ssl = set_service_ssl, + fallback_upstream_client_cert = fallback_upstream_client_cert, +} diff --git a/kong/templates/nginx_kong.lua b/kong/templates/nginx_kong.lua index 41b6c3fd1994..9c6a351fc06e 100644 --- a/kong/templates/nginx_kong.lua +++ b/kong/templates/nginx_kong.lua @@ -280,7 +280,16 @@ server { default_type ''; set $kong_proxy_mode 'http'; - rewrite_by_lua_block {;} + rewrite_by_lua_block { + -- ngx.localtion.capture will create a new nginx request, + -- so the upstream ssl-related info attached to the `r` gets lost. + -- we need to re-set them here to the new nginx request. + local ctx = ngx.ctx + local upstream_ssl = require("kong.runloop.upstream_ssl") + + upstream_ssl.set_service_ssl(ctx) + upstream_ssl.fallback_upstream_client_cert(ctx) + } access_by_lua_block {;} header_filter_by_lua_block {;} body_filter_by_lua_block {;} diff --git a/spec/02-integration/05-proxy/18-upstream_tls_spec.lua b/spec/02-integration/05-proxy/18-upstream_tls_spec.lua index aaaaae5df5e9..2b251e8fb51f 100644 --- a/spec/02-integration/05-proxy/18-upstream_tls_spec.lua +++ b/spec/02-integration/05-proxy/18-upstream_tls_spec.lua @@ -89,6 +89,20 @@ local function gen_route(flavor, r) return r end +local function gen_plugin(route) + return { + name = "pre-function", + route = { id = route.id }, + config = { + access = { + [[ + kong.service.request.enable_buffering() + ]] + } + } + } +end + for _, flavor in ipairs({ "traditional", "traditional_compatible" }) do for _, strategy in helpers.each_strategy() do @@ -104,6 +118,8 @@ for _, strategy in helpers.each_strategy() do local tls_upstream local tls_service_mtls_upstream + local route_mtls_buffered_proxying, route_tls_buffered_proxying, route_mtls_upstream_buffered_proxying + reload_router(flavor) lazy_setup(function() @@ -172,6 +188,30 @@ for _, strategy in helpers.each_strategy() do paths = { "/mtls-upstream", }, }))) + route_mtls_buffered_proxying = assert(bp.routes:insert(gen_route(flavor,{ + service = { id = service_mtls.id, }, + hosts = { "example.com", }, + paths = { "/mtls-buffered-proxying", }, + }))) + + route_tls_buffered_proxying = assert(bp.routes:insert(gen_route(flavor,{ + service = { id = service_tls.id, }, + hosts = { "example.com", }, + paths = { "/tls-buffered-proxying", }, + }))) + + route_mtls_upstream_buffered_proxying = assert(bp.routes:insert(gen_route(flavor,{ + service = { id = service_mtls_upstream.id, }, + hosts = { "example.com", }, + paths = { "/mtls-upstream-buffered-proxying", }, + }))) + + -- use pre-function to enable buffered_proxying in order to trigger the + -- `ngx.location.capture("/kong_buffered_http")` in `Kong.response()` + assert(bp.plugins:insert(gen_plugin(route_mtls_buffered_proxying))) + assert(bp.plugins:insert(gen_plugin(route_tls_buffered_proxying))) + assert(bp.plugins:insert(gen_plugin(route_mtls_upstream_buffered_proxying))) + -- tls tls_service_mtls = assert(bp.services:insert({ name = "tls-protected-service-mtls", @@ -294,6 +334,23 @@ for _, strategy in helpers.each_strategy() do assert.matches("400 No required SSL certificate was sent", body, nil, true) assert(proxy_client:close()) end) + + -- buffered_proxying + if subsystems == "http" then + it("accessing protected upstream, buffered_proxying = true", function() + local proxy_client = get_proxy_client(subsystems, 19000) + local res = assert(proxy_client:send { + path = "/mtls-buffered-proxying", + headers = { + ["Host"] = "example.com", + } + }) + + local body = assert.res_status(400, res) + assert.matches("400 No required SSL certificate was sent", body, nil, true) + assert(proxy_client:close()) + end) + end end) describe(subsystems .. " #db client certificate supplied via service.client_certificate", function() @@ -332,6 +389,28 @@ for _, strategy in helpers.each_strategy() do end, 10) end) + -- buffered_proxying + if subsystems == "http" then + it("accessing protected upstream, buffered_proxying = true", function() + helpers.wait_until(function() + local proxy_client = get_proxy_client(subsystems, 19000) + local path = "/mtls-buffered-proxying" + local res = assert(proxy_client:send { + path = path, + headers = { + ["Host"] = "example.com", + } + }) + + return pcall(function() + local body = assert.res_status(200, res) + assert.equals("it works", body) + assert(proxy_client:close()) + end) + end, 10) + end) + end + it("send updated client certificate", function () local proxy_client = get_proxy_client(subsystems, 19000) local path @@ -350,6 +429,21 @@ for _, strategy in helpers.each_strategy() do local res_cert = res.headers["X-Cert"] assert(proxy_client:close()) + -- buffered_proxying + local res_cert_buffered + if subsystems == "http" then + local proxy_client = get_proxy_client(subsystems, 19000) + local res = assert(proxy_client:send { + path = "/mtls-buffered-proxying", + headers = { + ["Host"] = "example.com", + } + }) + assert.res_status(200, res) + res_cert_buffered = res.headers["X-Cert"] + assert(proxy_client:close()) + end + res = admin_client:patch("/certificates/" .. certificate.id, { body = { cert = ssl_fixtures.cert_client2, @@ -376,6 +470,21 @@ for _, strategy in helpers.each_strategy() do assert.res_status(200, res) local res_cert2 = res.headers["X-Cert"] assert.not_equals(res_cert, res_cert2) + + -- buffered_proxying + local res_cert2_buffered + if subsystems == "http" then + res = assert(proxy_client2:send { + path = "/mtls-buffered-proxying", + headers = { + ["Host"] = "example.com", + } + }) + assert.res_status(200, res) + res_cert2_buffered = res.headers["X-Cert"] + assert.not_equals(res_cert_buffered, res_cert2_buffered) + end + -- restore old res = admin_client:patch("/certificates/" .. certificate.id, { body = { @@ -416,6 +525,26 @@ for _, strategy in helpers.each_strategy() do end, 10) assert.matches("400 No required SSL certificate was sent", body, nil, true) + + -- buffered_proxying + if subsystems == "http" then + helpers.wait_until(function() + local proxy_client= get_proxy_client(subsystems, 19000) + res = assert(proxy_client:send { + path = "/mtls-buffered-proxying", + headers = { + ["Host"] = "example.com", + } + }) + + return pcall(function() + body = assert.res_status(400, res) + assert(proxy_client:close()) + end) + end, 10) + + assert.matches("400 No required SSL certificate was sent", body, nil, true) + end end) end) end) @@ -435,6 +564,23 @@ for _, strategy in helpers.each_strategy() do assert.matches("400 No required SSL certificate was sent", body, nil, true) assert(proxy_client:close()) end) + + -- buffered_proxying + if subsystems == "http" then + it("accessing protected upstream, buffered_proxying = true", function() + local proxy_client= get_proxy_client(subsystems, 19002) + local res = assert(proxy_client:send { + path = "/mtls-upstream-buffered-proxying", + headers = { + ["Host"] = "example.com", + } + }) + + local body = assert.res_status(400, res) + assert.matches("400 No required SSL certificate was sent", body, nil, true) + assert(proxy_client:close()) + end) + end end) describe("#db client certificate supplied via upstream.client_certificate", function() @@ -479,6 +625,28 @@ for _, strategy in helpers.each_strategy() do end, 10) end) + -- buffered_proxying + if subsystems == "http" then + it("accessing protected upstream, buffered_proxying = true", function() + helpers.wait_until(function() + local proxy_client = get_proxy_client(subsystems, 19002) + local path = "/mtls-upstream-buffered-proxying" + local res = assert(proxy_client:send { + path = path, + headers = { + ["Host"] = "example.com", + } + }) + + return pcall(function() + local body = assert.res_status(200, res) + assert.equals("it works", body) + assert(proxy_client:close()) + end) + end, 10) + end) + end + it("remove client_certificate removes access", function() local upstream_id if subsystems == "http" then @@ -514,6 +682,26 @@ for _, strategy in helpers.each_strategy() do end, 10) assert.matches("400 No required SSL certificate was sent", body, nil, true) + + -- buffered_proxying + if subsystems == "http" then + helpers.wait_until(function() + local proxy_client = get_proxy_client(subsystems, 19002) + res = assert(proxy_client:send { + path = "/mtls-upstream-buffered-proxying", + headers = { + ["Host"] = "example.com", + } + }) + + return pcall(function() + body = assert.res_status(400, res) + assert(proxy_client:close()) + end) + end, 10) + + assert.matches("400 No required SSL certificate was sent", body, nil, true) + end end) end) @@ -572,6 +760,28 @@ for _, strategy in helpers.each_strategy() do end) end, 10) end) + + -- buffered_proxying + if subsystems == "http" then + it("access is allowed because Service.client_certificate overrides Upstream.client_certificate, buffered_proxy = true", function() + helpers.wait_until(function() + local proxy_client = get_proxy_client(subsystems, 19002) + local path = "/mtls-upstream-buffered-proxying" + local res = assert(proxy_client:send { + path = path, + headers = { + ["Host"] = "example.com", + } + }) + + return pcall(function() + local body = assert.res_status(200, res) + assert.equals("it works", body) + assert(proxy_client:close()) + end) + end, 10) + end) + end end) end) @@ -596,6 +806,23 @@ for _, strategy in helpers.each_strategy() do assert(proxy_client:close()) end) + -- buffered_proxying + if subsystems == "http" then + it("default is off, buffered_proxying = true", function() + local proxy_client = get_proxy_client(subsystems, 19001) + local path = "/tls-buffered-proxying" + local res = proxy_client:send { + path = path, + headers = { + ["Host"] = "example.com", + } + } + local body = assert.res_status(200, res) + assert.equals("it works", body) + assert(proxy_client:close()) + end) + end + it("#db turn it on, request is blocked", function() local service_tls_id if subsystems == "http" then @@ -640,6 +867,25 @@ for _, strategy in helpers.each_strategy() do if subsystems == "http" then assert.equals("An invalid response was received from the upstream server", body) end + + -- buffered_proxying + if subsystems == "http" then + helpers.wait_until(function() + local proxy_client = get_proxy_client(subsystems, 19001) + res = proxy_client:send { + path = "/tls-buffered-proxying", + headers = { + ["Host"] = "example.com", + } + } + return pcall(function() + body = assert.res_status(502, res) + assert(proxy_client:close()) + end) + end, 10) + + assert.equals("An invalid response was received from the upstream server", body) + end end) end) @@ -685,6 +931,26 @@ for _, strategy in helpers.each_strategy() do end, 10) assert.equals("it works", body) + + -- buffered_proxying + if subsystems == "http" then + helpers.wait_until(function() + local proxy_client = get_proxy_client(subsystems, 19001) + local path = "/tls-buffered-proxying" + local res = proxy_client:send { + path = path, + headers = { + ["Host"] = "example.com", + } + } + return pcall(function() + body = assert.res_status(200, res) + assert(proxy_client:close()) + end) + end, 10) + + assert.equals("it works", body) + end end) end) @@ -755,6 +1021,25 @@ for _, strategy in helpers.each_strategy() do if subsystems == "http" then assert.equals("An invalid response was received from the upstream server", body) end + + -- buffered_proxying + if subsystems == "http" then + helpers.wait_until(function() + local proxy_client = get_proxy_client(subsystems, 19001) + local res = proxy_client:send { + path = "/tls-buffered-proxying", + headers = { + ["Host"] = "example.com", + } + } + + return pcall(function() + body = assert.res_status(502, res) + assert(proxy_client:close()) + end) + end, 10) + assert.equals("An invalid response was received from the upstream server", body) + end end) it("request is allowed through if depth limit is sufficient", function() @@ -798,6 +1083,27 @@ for _, strategy in helpers.each_strategy() do end, 10) assert.equals("it works", body) + + -- buffered_proxying + if subsystems == "http" then + helpers.wait_until(function() + local proxy_client = get_proxy_client(subsystems, 19001) + local path = "/tls-buffered-proxying" + res = assert(proxy_client:send { + path = path, + headers = { + ["Host"] = "example.com", + } + }) + + return pcall(function() + body = assert.res_status(200, res) + assert(proxy_client:close()) + end) + end, 10) + + assert.equals("it works", body) + end end) end) end)