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 3dd905d2ead..88bd8689b7c 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 8066c942ed0..8d29c966414 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 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]