From a1dae605a7a77d80c0d2fdbcf17533fc308e22f3 Mon Sep 17 00:00:00 2001 From: Aapo Talvensaari Date: Thu, 26 Oct 2023 10:46:03 +0200 Subject: [PATCH] fix(vault): resurrect positive results in lru cache for ttl + resurrect ttl (#11815) ### Summary The vault is rotating secrets on every minute which updates the shared dictionary cache with new values, both negative and positive results. This commit changes the Negative results handling on LRU. Previously the LRU was cleared for negative results, and we just used to cache for config.ttl amount of time. This commit changes it so that LRU values are deleted, and we cache things config.ttl + config.resurrect_ttl amount of time in lru cache too. It was reported by @Hayk-S on KAG-2833. Signed-off-by: Aapo Talvensaari (cherry picked from commit 1b6c394ad8d69a5925a8b9bcc62a38364a371ce8) --- changelog/unreleased/kong/vault-resurrect.yml | 3 + kong/pdk/vault.lua | 33 ++- spec/02-integration/13-vaults/05-ttl_spec.lua | 2 +- .../13-vaults/07-resurrect_spec.lua | 240 ++++++++++++++++++ .../custom_vaults/kong/vaults/test/schema.lua | 6 + 5 files changed, 271 insertions(+), 13 deletions(-) create mode 100644 changelog/unreleased/kong/vault-resurrect.yml create mode 100644 spec/02-integration/13-vaults/07-resurrect_spec.lua diff --git a/changelog/unreleased/kong/vault-resurrect.yml b/changelog/unreleased/kong/vault-resurrect.yml new file mode 100644 index 000000000000..7dc1b5d9ee1e --- /dev/null +++ b/changelog/unreleased/kong/vault-resurrect.yml @@ -0,0 +1,3 @@ +message: Vault resurrect time is respected in case a vault secret is deleted from a vault +type: bugfix +scope: Core diff --git a/kong/pdk/vault.lua b/kong/pdk/vault.lua index 00fa633168fd..9473ae9a2e09 100644 --- a/kong/pdk/vault.lua +++ b/kong/pdk/vault.lua @@ -27,6 +27,7 @@ local get_updated_now_ms = utils.get_updated_now_ms local ngx = ngx local get_phase = ngx.get_phase local max = math.max +local min = math.min local fmt = string.format local sub = string.sub local byte = string.byte @@ -753,15 +754,25 @@ local function new(self) local function get_cache_value_and_ttl(value, config, ttl) local cache_value, shdict_ttl, lru_ttl if value then - -- adjust ttl to the minimum and maximum values configured - lru_ttl = adjust_ttl(ttl, config) - shdict_ttl = max(lru_ttl + (config.resurrect_ttl or DAO_MAX_TTL), SECRETS_CACHE_MIN_TTL) cache_value = value + -- adjust ttl to the minimum and maximum values configured + ttl = adjust_ttl(ttl, config) + + if config.resurrect_ttl then + lru_ttl = min(ttl + config.resurrect_ttl, DAO_MAX_TTL) + shdict_ttl = max(lru_ttl, SECRETS_CACHE_MIN_TTL) + + else + lru_ttl = ttl + shdict_ttl = DAO_MAX_TTL + end + else + cache_value = NEGATIVELY_CACHED_VALUE + -- negatively cached values will be rotated on each rotation interval shdict_ttl = max(config.neg_ttl or 0, SECRETS_CACHE_MIN_TTL) - cache_value = NEGATIVELY_CACHED_VALUE end return cache_value, shdict_ttl, lru_ttl @@ -794,14 +805,13 @@ local function new(self) return nil, cache_err end - if not value then - LRU:delete(reference) + if cache_value == NEGATIVELY_CACHED_VALUE then return nil, fmt("could not get value from external vault (%s)", err) end - LRU:set(reference, value, lru_ttl) + LRU:set(reference, cache_value, lru_ttl) - return value + return cache_value end @@ -823,8 +833,7 @@ local function new(self) -- @usage -- local value, err = get(reference, cache_only) local function get(reference, cache_only) - -- the LRU stale value is ignored as the resurrection logic - -- is deferred to the shared dictionary + -- the LRU stale value is ignored local value = LRU:get(reference) if value then return value @@ -1359,8 +1368,8 @@ local function new(self) return nil, cache_err end - if value then - LRU:set(reference, value, lru_ttl) + if cache_value ~= NEGATIVELY_CACHED_VALUE then + LRU:set(reference, cache_value, lru_ttl) end return true diff --git a/spec/02-integration/13-vaults/05-ttl_spec.lua b/spec/02-integration/13-vaults/05-ttl_spec.lua index 21736bb94b18..e6f65fd56465 100644 --- a/spec/02-integration/13-vaults/05-ttl_spec.lua +++ b/spec/02-integration/13-vaults/05-ttl_spec.lua @@ -64,7 +64,7 @@ local VAULTS = { }, create_secret = function(self, _, value) - -- Currently, crate_secret is called _before_ starting Kong. + -- Currently, create_secret is called _before_ starting Kong. -- -- This means our backend won't be available yet because it is -- piggy-backing on Kong as an HTTP mock fixture. diff --git a/spec/02-integration/13-vaults/07-resurrect_spec.lua b/spec/02-integration/13-vaults/07-resurrect_spec.lua new file mode 100644 index 000000000000..d91bbcabd86b --- /dev/null +++ b/spec/02-integration/13-vaults/07-resurrect_spec.lua @@ -0,0 +1,240 @@ +local helpers = require "spec.helpers" + +-- using the full path so that we don't have to modify package.path in +-- this context +local test_vault = require "spec.fixtures.custom_vaults.kong.vaults.test" + +local CUSTOM_VAULTS = "./spec/fixtures/custom_vaults" +local CUSTOM_PLUGINS = "./spec/fixtures/custom_plugins" + +local LUA_PATH = CUSTOM_VAULTS .. "/?.lua;" .. + CUSTOM_VAULTS .. "/?/init.lua;" .. + CUSTOM_PLUGINS .. "/?.lua;" .. + CUSTOM_PLUGINS .. "/?/init.lua;;" + +local DUMMY_HEADER = "Dummy-Plugin" +local fmt = string.format + + + +--- A vault test harness is a driver for vault backends, which implements +--- all the necessary glue for initializing a vault backend and performing +--- secret read/write operations. +--- +--- All functions defined here are called as "methods" (e.g. harness:fn()), so +--- it is permitted to keep state on the harness object (self). +--- +---@class vault_test_harness +--- +---@field name string +--- +--- this table is passed directly to kong.db.vaults:insert() +---@field config table +--- +--- create_secret() is called once per test run for a given secret +---@field create_secret fun(self: vault_test_harness, secret: string, value: string, opts?: table) +--- +--- update_secret() may be called more than once per test run for a given secret +---@field update_secret fun(self: vault_test_harness, secret: string, value: string, opts?: table) +--- +--- setup() is called before kong is started and before any DB entities +--- have been created and is best used for things like validating backend +--- credentials and establishing a connection to a backend +---@field setup fun(self: vault_test_harness) +--- +--- teardown() is exactly what you'd expect +---@field teardown fun(self: vault_test_harness) +--- +--- fixtures() output is passed directly to `helpers.start_kong()` +---@field fixtures fun(self: vault_test_harness):table|nil +--- +--- +---@field prefix string # generated by the test suite +---@field host string # generated by the test suite + + +---@type vault_test_harness[] +local VAULTS = { + { + name = "test", + + config = { + default_value = "DEFAULT", + default_value_ttl = 1, + }, + + create_secret = function(self, _, value) + -- Currently, create_secret is called _before_ starting Kong. + -- + -- This means our backend won't be available yet because it is + -- piggy-backing on Kong as an HTTP mock fixture. + -- + -- We can, however, inject a default value into our configuration. + self.config.default_value = value + end, + + update_secret = function(_, secret, value, opts) + return test_vault.client.put(secret, value, opts) + end, + + delete_secret = function(_, secret) + return test_vault.client.delete(secret) + end, + + fixtures = function() + return { + http_mock = { + test_vault = test_vault.http_mock, + } + } + end, + }, +} + + +local noop = function(...) end + +for _, vault in ipairs(VAULTS) do + -- fill out some values that we'll use in route/service/plugin config + vault.prefix = vault.name .. "-ttl-test" + vault.host = vault.name .. ".vault-ttl.test" + + -- ...and fill out non-required methods + vault.setup = vault.setup or noop + vault.teardown = vault.teardown or noop + vault.fixtures = vault.fixtures or noop +end + + +for _, strategy in helpers.each_strategy() do +for _, vault in ipairs(VAULTS) do + + +describe("vault resurrect_ttl and rotation (#" .. strategy .. ") #" .. vault.name, function() + local client + local secret = "my-secret" + + + local function http_get(path) + path = path or "/" + + local res = client:get(path, { + headers = { + host = assert(vault.host), + }, + }) + + assert.response(res).has.status(200) + + return res + end + + + lazy_setup(function() + helpers.setenv("KONG_LUA_PATH_OVERRIDE", LUA_PATH) + helpers.setenv("KONG_VAULT_ROTATION_INTERVAL", "1") + + helpers.test_conf.loaded_plugins = { + dummy = true, + } + + vault:setup() + vault:create_secret(secret, "init") + + local bp = helpers.get_db_utils(strategy, + { "vaults", "routes", "services", "plugins" }, + { "dummy" }, + { vault.name }) + + + assert(bp.vaults:insert({ + name = vault.name, + prefix = vault.prefix, + config = vault.config, + })) + + local route = assert(bp.routes:insert({ + name = vault.host, + hosts = { vault.host }, + paths = { "/" }, + service = assert(bp.services:insert()), + })) + + + -- used by the plugin config test case + assert(bp.plugins:insert({ + name = "dummy", + config = { + resp_header_value = fmt("{vault://%s/%s?ttl=%d&resurrect_ttl=%d}", + vault.prefix, secret, 2, 2), + }, + route = { id = route.id }, + })) + + assert(helpers.start_kong({ + database = strategy, + nginx_conf = "spec/fixtures/custom_nginx.template", + vaults = vault.name, + plugins = "dummy", + log_level = "info", + }, nil, nil, vault:fixtures() )) + + client = helpers.proxy_client() + end) + + + lazy_teardown(function() + if client then + client:close() + end + + helpers.stop_kong(nil, true) + vault:teardown() + + helpers.unsetenv("KONG_LUA_PATH_OVERRIDE") + end) + + + it("resurrects plugin config references when secret is deleted (backend: #" .. vault.name .. ")", function() + local function check_plugin_secret(expect, ttl, leeway) + leeway = leeway or 0.25 -- 25% + + local timeout = ttl + (ttl * leeway) + + assert + .with_timeout(timeout) + .with_step(0.5) + .eventually(function() + local res = http_get("/") + local value + if expect == "" then + value = res.headers[DUMMY_HEADER] or "" + if value == "" then + return true + end + + else + value = assert.response(res).has.header(DUMMY_HEADER) + if value == expect then + return true + end + end + + return nil, { expected = expect, got = value } + end) + .is_truthy("expected plugin secret to be updated to '" .. tostring(expect) .. "' " + .. "within " .. tostring(timeout) .. " seconds") + end + + vault:update_secret(secret, "old", { ttl = 2, resurrect_ttl = 2 }) + check_plugin_secret("old", 5) + vault:delete_secret(secret) + ngx.sleep(2.5) + check_plugin_secret("old", 5) + check_plugin_secret("", 5) + end) +end) + + +end -- each vault backend +end -- each strategy diff --git a/spec/fixtures/custom_vaults/kong/vaults/test/schema.lua b/spec/fixtures/custom_vaults/kong/vaults/test/schema.lua index 4b4a335e9cb8..019179b2a5ab 100644 --- a/spec/fixtures/custom_vaults/kong/vaults/test/schema.lua +++ b/spec/fixtures/custom_vaults/kong/vaults/test/schema.lua @@ -1,3 +1,6 @@ +local typedefs = require "kong.db.schema.typedefs" + + return { name = "test", fields = { @@ -7,6 +10,9 @@ return { fields = { { default_value = { type = "string", required = false } }, { default_value_ttl = { type = "number", required = false } }, + { ttl = typedefs.ttl }, + { neg_ttl = typedefs.ttl }, + { resurrect_ttl = typedefs.ttl }, }, }, },