From 6a36c4de1b5d3ae87088d42d76129eb72a5ef3a0 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. --- .../kong-ee/fix-certificate-reference.yml | 3 + kong/resty/mlcache/init.lua | 30 +- spec/02-integration/13-vaults/05-ttl_spec.lua | 439 ++++++++++++------ t/05-mlcache/07-l1_serializer.t | 90 ++++ 4 files changed, 422 insertions(+), 140 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 000000000000..6769e4f577e2 --- /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 3dd905d2eada..88bd8689b7cf 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 8066c942ed01..8d29c9664148 100644 --- a/spec/02-integration/13-vaults/05-ttl_spec.lua +++ b/spec/02-integration/13-vaults/05-ttl_spec.lua @@ -375,158 +375,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") - - 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 }) - - - assert(bp.vaults:insert({ - name = vault.name, - prefix = vault.prefix, - config = vault.config, - })) + describe("rotation", function() + lazy_setup(function() + helpers.setenv("KONG_LUA_PATH_OVERRIDE", LUA_PATH) + helpers.setenv("KONG_VAULT_ROTATION_INTERVAL", "1") - -- 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, - })) + vault:setup() + vault:create_secret(secret, ssl_fixtures.key_alt) - local service_tls = assert(bp.services:insert({ - name = "tls-service", - url = "https://example.com:16799", - client_certificate = certificate, - })) + local bp = helpers.get_db_utils(strategy, + { "vaults", "routes", "services", "certificates", "ca_certificates" }, + {}, + { vault.name }) - 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(bp.vaults:insert({ + name = vault.name, + prefix = vault.prefix, + config = vault.config, + })) - 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() - - helpers.unsetenv("KONG_LUA_PATH_OVERRIDE") - end) + local bp = helpers.get_db_utils(strategy, + { "vaults", "routes", "services", "certificates", "ca_certificates" }, + {}, + { vault.name }) - 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, - }) + assert(bp.vaults:insert({ + name = vault.name, + prefix = vault.prefix, + config = vault.config, + })) - -- Assume wrong cert-key pair still being used - local res = client:get("/tls", { - headers = { - host = "example.com", - }, - timeout = 2, - }) + -- 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) - 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") + 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 74ec9c467f8e..12739e6161b7 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]