From 992ed1fda25d26cecd0703694af8fdf8fd90d9ac Mon Sep 17 00:00:00 2001 From: Yufu Zhao Date: Tue, 24 Dec 2024 17:36:13 +0800 Subject: [PATCH] fix(certificate): unable to refresh certificate entity with vault reference when initial with an invalid string (#10984) When set the cert to an invalid string in the vault, and then correct the string, kong will not start to use the correct certificate even though `kong vault` command shows the correct certiticate being returned. This is because the initial invalid string has already been cached in the L2 cache by mlcache, and it will not be updated via the L3 cache callback afterward. When mlcache fails during the execution of the `l1_serializer`, it caches the result retrieved from the L3 cache callback the first time in the L2 cache permanently. This means that subsequent calls to `cache.get()` will never fetch data from the L3 cache but will directly retrieve it from the L2 cache. To avoid this situation, when the `l1_serializer` fails, we no longer allow mlcache to store the data retrieved from L3 into L2. (cherry picked from commit 6a36c4de1b5d3ae87088d42d76129eb72a5ef3a0) --- .../kong-ee/fix-certificate-reference.yml | 3 + kong/resty/mlcache/init.lua | 30 +- spec/02-integration/13-vaults/05-ttl_spec.lua | 441 ++++++++++++------ t/05-mlcache/07-l1_serializer.t | 90 ++++ 4 files changed, 423 insertions(+), 141 deletions(-) create mode 100644 changelog/unreleased/kong-ee/fix-certificate-reference.yml diff --git a/changelog/unreleased/kong-ee/fix-certificate-reference.yml b/changelog/unreleased/kong-ee/fix-certificate-reference.yml new file mode 100644 index 00000000000..6769e4f577e --- /dev/null +++ b/changelog/unreleased/kong-ee/fix-certificate-reference.yml @@ -0,0 +1,3 @@ +message: Fixed an issue that certificate entity configured with vault reference may not get refreshed on time when initial with an invalid string. +type: bugfix +scope: Core diff --git a/kong/resty/mlcache/init.lua b/kong/resty/mlcache/init.lua index f27eaeef585..803a01137a0 100644 --- a/kong/resty/mlcache/init.lua +++ b/kong/resty/mlcache/init.lua @@ -412,6 +412,28 @@ local function set_shm(self, shm_key, value, ttl, neg_ttl, flags, shm_set_tries, end +local function del_shm(self, shm_key, value) + local shm = self.shm + local dict = self.dict + + if value == nil then + if self.dict_miss then + shm = self.shm_miss + dict = self.dict_miss + end + end + + local ok, err = dict:delete(shm_key) + + if not ok then + ngx_log(WARN, "could not delete from lua_shared_dict '" .. shm + .. "': " .. err) + return + end + + return true +end + local function set_shm_set_lru(self, key, shm_key, value, ttl, neg_ttl, flags, shm_set_tries, l1_serializer, throw_no_mem) @@ -421,7 +443,13 @@ local function set_shm_set_lru(self, key, shm_key, value, ttl, neg_ttl, flags, return nil, err end - return set_lru(self, key, value, ttl, neg_ttl, l1_serializer) + ok, err = set_lru(self, key, value, ttl, neg_ttl, l1_serializer) + if not ok and err then + -- l1_serializer returned nil + err, do not store the cached vaule in L2 + del_shm(self, shm_key, value) + end + + return ok, err end diff --git a/spec/02-integration/13-vaults/05-ttl_spec.lua b/spec/02-integration/13-vaults/05-ttl_spec.lua index 5ad2d7ec074..98ab25591e9 100644 --- a/spec/02-integration/13-vaults/05-ttl_spec.lua +++ b/spec/02-integration/13-vaults/05-ttl_spec.lua @@ -258,158 +258,319 @@ describe("#hybrid mode dp vault ttl and rotation (#" .. strategy .. ") #" .. vau local vault_fixtures = vault:fixtures() vault_fixtures.dns_mock = tls_fixtures.dns_mock - lazy_setup(function() - helpers.setenv("KONG_LUA_PATH_OVERRIDE", LUA_PATH) - helpers.setenv("KONG_VAULT_ROTATION_INTERVAL", "1") + describe("rotation", function() + lazy_setup(function() + helpers.setenv("KONG_LUA_PATH_OVERRIDE", LUA_PATH) + helpers.setenv("KONG_VAULT_ROTATION_INTERVAL", "1") - vault:setup() - vault:create_secret(secret, ssl_fixtures.key_alt) + vault:setup() + vault:create_secret(secret, ssl_fixtures.key_alt) - local bp = helpers.get_db_utils(strategy, - { "vaults", "routes", "services", "certificates", "ca_certificates" }, - {}, - { vault.name }) + local bp = helpers.get_db_utils(strategy, + { "vaults", "routes", "services", "certificates", "ca_certificates" }, + {}, + { vault.name }) - assert(bp.vaults:insert({ - name = vault.name, - prefix = vault.prefix, - config = vault.config, - })) - - -- Prepare TLS upstream service - -- cert_alt & key_alt pair is not a correct client certificate - -- and it will fail the client TLS verification on server side - -- - -- On the other hand, cert_client & key_client pair is a correct - -- client certificate - certificate = assert(bp.certificates:insert({ - key = ssl_fixtures.key_alt, - cert = ssl_fixtures.cert_alt, - })) + assert(bp.vaults:insert({ + name = vault.name, + prefix = vault.prefix, + config = vault.config, + })) - local service_tls = assert(bp.services:insert({ - name = "tls-service", - url = "https://example.com:16799", - client_certificate = certificate, - })) - - assert(bp.routes:insert({ - name = "tls-route", - hosts = { "example.com" }, - paths = { "/tls", }, - service = { id = service_tls.id }, - })) - - assert(helpers.start_kong({ - role = "control_plane", - cluster_cert = "spec/fixtures/kong_clustering.crt", - cluster_cert_key = "spec/fixtures/kong_clustering.key", - database = strategy, - prefix = "vault_ttl_test_cp", - cluster_listen = "127.0.0.1:9005", - admin_listen = "127.0.0.1:9001", - nginx_conf = "spec/fixtures/custom_nginx.template", - vaults = vault.name, - plugins = "dummy", - log_level = "debug", - }, nil, nil, tls_fixtures )) - - assert(helpers.start_kong({ - role = "data_plane", - database = "off", - prefix = "vault_ttl_test_dp", - vaults = vault.name, - plugins = "dummy", - log_level = "debug", - nginx_conf = "spec/fixtures/custom_nginx.template", - cluster_cert = "spec/fixtures/kong_clustering.crt", - cluster_cert_key = "spec/fixtures/kong_clustering.key", - cluster_control_plane = "127.0.0.1:9005", - proxy_listen = "127.0.0.1:9002", - nginx_worker_processes = 1, - }, nil, nil, vault_fixtures )) - - admin_client = helpers.admin_client(nil, 9001) - client = helpers.proxy_client(nil, 9002) + -- Prepare TLS upstream service + -- cert_alt & key_alt pair is not a correct client certificate + -- and it will fail the client TLS verification on server side + -- + -- On the other hand, cert_client & key_client pair is a correct + -- client certificate + certificate = assert(bp.certificates:insert({ + key = ssl_fixtures.key_alt, + cert = ssl_fixtures.cert_alt, + })) + + local service_tls = assert(bp.services:insert({ + name = "tls-service", + url = "https://example.com:16799", + client_certificate = certificate, + })) + + assert(bp.routes:insert({ + name = "tls-route", + hosts = { "example.com" }, + paths = { "/tls", }, + service = { id = service_tls.id }, + })) + + assert(helpers.start_kong({ + role = "control_plane", + cluster_cert = "spec/fixtures/kong_clustering.crt", + cluster_cert_key = "spec/fixtures/kong_clustering.key", + database = strategy, + prefix = "vault_ttl_test_cp", + cluster_listen = "127.0.0.1:9005", + admin_listen = "127.0.0.1:9001", + nginx_conf = "spec/fixtures/custom_nginx.template", + vaults = vault.name, + plugins = "dummy", + log_level = "debug", + }, nil, nil, tls_fixtures )) + + assert(helpers.start_kong({ + role = "data_plane", + database = "off", + prefix = "vault_ttl_test_dp", + vaults = vault.name, + plugins = "dummy", + log_level = "debug", + nginx_conf = "spec/fixtures/custom_nginx.template", + cluster_cert = "spec/fixtures/kong_clustering.crt", + cluster_cert_key = "spec/fixtures/kong_clustering.key", + cluster_control_plane = "127.0.0.1:9005", + proxy_listen = "127.0.0.1:9002", + nginx_worker_processes = 1, + }, nil, nil, vault_fixtures )) + + admin_client = helpers.admin_client(nil, 9001) + client = helpers.proxy_client(nil, 9002) + end) + + lazy_teardown(function() + if client then + client:close() + end + if admin_client then + admin_client:close() + end + + helpers.stop_kong("vault_ttl_test_cp") + helpers.stop_kong("vault_ttl_test_dp") + vault:teardown() + + helpers.unsetenv("KONG_LUA_PATH_OVERRIDE") + end) + + it("updates plugin config references (backend: #" .. vault.name .. ")", function() + helpers.wait_for_all_config_update({ + forced_admin_port = 9001, + forced_proxy_port = 9002, + }) + -- Wrong cert-key pair is being used in the pre-configured cert object + local res = client:get("/tls", { + headers = { + host = "example.com", + }, + timeout = 2, + }) + local body = assert.res_status(400, res) + assert.matches("The SSL certificate error", body) + + -- Switch to vault referenced key field + local res = assert(admin_client:patch("/certificates/"..certificate.id, { + body = { + key = fmt("{vault://%s/%s?ttl=%s}", vault.prefix, secret, 2), + cert = ssl_fixtures.cert_client, + }, + headers = { + ["Content-Type"] = "application/json", + }, + })) + assert.res_status(200, res) + helpers.wait_for_all_config_update({ + forced_admin_port = 9001, + forced_proxy_port = 9002, + }) + + -- Assume wrong cert-key pair still being used + local res = client:get("/tls", { + headers = { + host = "example.com", + }, + timeout = 2, + }) + + local body = assert.res_status(400, res) + assert.matches("No required SSL certificate was sent", body) + + -- Update secret value and let cert be correct + vault:update_secret(secret, ssl_fixtures.key_client, { ttl = 2 }) + assert.with_timeout(7) + .with_step(0.5) + .ignore_exceptions(true) + .eventually(function() + local res = client:get("/tls", { + headers = { + host = "example.com", + }, + timeout = 2, + }) + + local body = assert.res_status(200, res) + assert.matches("it works", body) + return true + end).is_truthy("Expected certificate being refreshed") + end) end) + describe("rotation", function() + lazy_setup(function() + helpers.setenv("KONG_LUA_PATH_OVERRIDE", LUA_PATH) + helpers.setenv("KONG_VAULT_ROTATION_INTERVAL", "1") - lazy_teardown(function() - if client then - client:close() - end - if admin_client then - admin_client:close() - end + vault:setup() + vault:create_secret(secret, ssl_fixtures.key_alt) - helpers.stop_kong("vault_ttl_test_cp") - helpers.stop_kong("vault_ttl_test_dp") - vault:teardown() + local bp = helpers.get_db_utils(strategy, + { "vaults", "routes", "services", "certificates", "ca_certificates" }, + {}, + { vault.name }) - helpers.unsetenv("KONG_LUA_PATH_OVERRIDE") - end) + assert(bp.vaults:insert({ + name = vault.name, + prefix = vault.prefix, + config = vault.config, + })) - it("updates plugin config references (backend: #" .. vault.name .. ")", function() - helpers.wait_for_all_config_update({ - forced_admin_port = 9001, - forced_proxy_port = 9002, - }) - -- Wrong cert-key pair is being used in the pre-configured cert object - local res = client:get("/tls", { - headers = { - host = "example.com", - }, - timeout = 2, - }) - local body = assert.res_status(400, res) - assert.matches("The SSL certificate error", body) - - -- Switch to vault referenced key field - local res = assert(admin_client:patch("/certificates/"..certificate.id, { - body = { - key = fmt("{vault://%s/%s?ttl=%s}", vault.prefix, secret, 2), - cert = ssl_fixtures.cert_client, - }, - headers = { - ["Content-Type"] = "application/json", - }, - })) - assert.res_status(200, res) - helpers.wait_for_all_config_update({ - forced_admin_port = 9001, - forced_proxy_port = 9002, - }) - - -- Assume wrong cert-key pair still being used - local res = client:get("/tls", { - headers = { - host = "example.com", - }, - timeout = 2, - }) - - local body = assert.res_status(400, res) - assert.matches("No required SSL certificate was sent", body) - - -- Update secret value and let cert be correct - vault:update_secret(secret, ssl_fixtures.key_client, { ttl = 2 }) - assert.with_timeout(7) - .with_step(0.5) - .ignore_exceptions(true) - .eventually(function() - local res = client:get("/tls", { - headers = { - host = "example.com", - }, - timeout = 2, - }) - - local body = assert.res_status(200, res) - assert.matches("it works", body) - return true - end).is_truthy("Expected certificate being refreshed") + -- Prepare TLS upstream service + -- cert_alt & key_alt pair is not a correct client certificate + -- and it will fail the client TLS verification on server side + -- + -- On the other hand, cert_client & key_client pair is a correct + -- client certificate + certificate = assert(bp.certificates:insert({ + key = ssl_fixtures.key_alt, + cert = ssl_fixtures.cert_alt, + })) + + local service_tls = assert(bp.services:insert({ + name = "tls-service", + url = "https://example.com:16799", + client_certificate = certificate, + })) + + assert(bp.routes:insert({ + name = "tls-route", + hosts = { "example.com" }, + paths = { "/tls", }, + service = { id = service_tls.id }, + })) + + assert(helpers.start_kong({ + role = "control_plane", + cluster_cert = "spec/fixtures/kong_clustering.crt", + cluster_cert_key = "spec/fixtures/kong_clustering.key", + database = strategy, + prefix = "vault_ttl_test_cp", + cluster_listen = "127.0.0.1:9005", + admin_listen = "127.0.0.1:9001", + nginx_conf = "spec/fixtures/custom_nginx.template", + vaults = vault.name, + plugins = "dummy", + log_level = "debug", + }, nil, nil, tls_fixtures )) + + assert(helpers.start_kong({ + role = "data_plane", + database = "off", + prefix = "vault_ttl_test_dp", + vaults = vault.name, + plugins = "dummy", + log_level = "debug", + nginx_conf = "spec/fixtures/custom_nginx.template", + cluster_cert = "spec/fixtures/kong_clustering.crt", + cluster_cert_key = "spec/fixtures/kong_clustering.key", + cluster_control_plane = "127.0.0.1:9005", + proxy_listen = "127.0.0.1:9002", + nginx_worker_processes = 1, + }, nil, nil, vault_fixtures )) + + admin_client = helpers.admin_client(nil, 9001) + client = helpers.proxy_client(nil, 9002) + end) + + lazy_teardown(function() + if client then + client:close() + end + if admin_client then + admin_client:close() + end + + helpers.stop_kong("vault_ttl_test_cp") + helpers.stop_kong("vault_ttl_test_dp") + vault:teardown() + + helpers.unsetenv("KONG_LUA_PATH_OVERRIDE") + end) + + it("updates plugin config references while initial with an invalid string (backend: #" .. vault.name .. ")", function() + helpers.wait_for_all_config_update({ + forced_admin_port = 9001, + forced_proxy_port = 9002, + }) + + -- Switch to vault referenced key field + local res = assert(admin_client:patch("/certificates/"..certificate.id, { + body = { + key = fmt("{vault://%s/%s?ttl=%s}", vault.prefix, secret, 2), + cert = ssl_fixtures.cert_client, + }, + headers = { + ["Content-Type"] = "application/json", + }, + })) + assert.res_status(200, res) + helpers.wait_for_all_config_update({ + forced_admin_port = 9001, + forced_proxy_port = 9002, + }) + + -- Update secret value to an invalid key format + vault:update_secret(secret, "an invalid string", { ttl = 2 }) + + -- Wait until the invalid key is being cached + assert.with_timeout(7) + .with_step(0.5) + .ignore_exceptions(true) + .eventually(function() + helpers.clean_logfile("vault_ttl_test_dp/logs/error.log") + + local res = client:get("/tls", { + headers = { + host = "example.com", + }, + timeout = 2, + }) + + local body = assert.res_status(400, res) + assert.matches("No required SSL certificate was sent", body) + + assert.logfile("vault_ttl_test_dp/logs/error.log").has.line( + 'failed to get from node cache: could not parse PEM private key:', true) + + return true + end).is_truthy("Invalid certificate being cached") + + -- Update secret value and let cert be correct + vault:update_secret(secret, ssl_fixtures.key_client, { ttl = 2 }) + + assert.with_timeout(7) + .with_step(0.5) + .ignore_exceptions(true) + .eventually(function() + local res = client:get("/tls", { + headers = { + host = "example.com", + }, + timeout = 2, + }) + + local body = assert.res_status(200, res) + assert.matches("it works", body) + return true + end).is_truthy("Expected certificate being refreshed") + end) end) end) diff --git a/t/05-mlcache/07-l1_serializer.t b/t/05-mlcache/07-l1_serializer.t index 74ec9c467f8..12739e6161b 100644 --- a/t/05-mlcache/07-l1_serializer.t +++ b/t/05-mlcache/07-l1_serializer.t @@ -739,3 +739,93 @@ GET /t opts.l1_serializer must be a function --- no_error_log [error] + + + +=== TEST 19: l1_serializer fails will not store the cached vaule in L2 +--- http_config eval: $::HttpConfig +--- config + location = /t { + content_by_lua_block { + local dict = ngx.shared.cache_shm + local mlcache = require "kong.resty.mlcache" + + local cache, err = mlcache.new("my_mlcache", "cache_shm", { + l1_serializer = function(s) + error("cannot transform") + end, + }) + if not cache then + ngx.log(ngx.ERR, err) + return + end + + local data, err = cache:get("key", nil, function() return "foo" end) + if not data then + ngx.say(err) + end + ngx.say(data) + + local v, err = dict:get("my_mlcachekey") + if err then + ngx.log(ngx.ERR, err) + return + end + ngx.say("no value in shm: ", v == nil ) + } + } +--- request +GET /t +--- response_body_like +l1_serializer threw an error: .*?: cannot transform +nil +no value in shm: true +--- no_error_log +[error] + + + +=== TEST 20: l1_serializer fails will not store the cached vaule in L2 (L2 miss) +--- http_config eval: $::HttpConfig +--- config + location = /t { + content_by_lua_block { + local mlcache = require "kong.resty.mlcache" + + local called = false + local cache, err = mlcache.new("my_mlcache", "cache_shm", { + l1_serializer = function(s) + if not called then + called = true + error("cannot transform") + end + + return string.format("transform(%q)", s) + end, + }) + if not cache then + ngx.log(ngx.ERR, err) + return + end + + local data, err = cache:get("key", nil, function() return "foo" end) + if not data then + ngx.say(err) + end + ngx.say(data) + + local data, err = cache:get("key", nil, function() return "bar" end) + if not data then + ngx.say(err) + end + ngx.say(data) + } + } +--- request +GET /t +--- response_body_like +l1_serializer threw an error: .*?: cannot transform +nil +transform\("bar"\) +--- no_error_log +[error]