From 0d815173822c64802a61f8a2dc08d1f803acd3a8 Mon Sep 17 00:00:00 2001
From: Keery Nie <windmgc@gmail.com>
Date: Tue, 23 Apr 2024 17:21:18 +0800
Subject: [PATCH] fix(vault): vault secret without ttl configuration should not
 be refreshed (#12877)

---
 .../fix-vault-secret-update-without-ttl.yml   |   3 +
 kong/pdk/vault.lua                            |  18 ++-
 spec/02-integration/13-vaults/05-ttl_spec.lua | 117 ++++++++++++++++++
 3 files changed, 137 insertions(+), 1 deletion(-)
 create mode 100644 changelog/unreleased/kong/fix-vault-secret-update-without-ttl.yml

diff --git a/changelog/unreleased/kong/fix-vault-secret-update-without-ttl.yml b/changelog/unreleased/kong/fix-vault-secret-update-without-ttl.yml
new file mode 100644
index 000000000000..fe0622057e8b
--- /dev/null
+++ b/changelog/unreleased/kong/fix-vault-secret-update-without-ttl.yml
@@ -0,0 +1,3 @@
+message: Fixed a bug that allowed vault secrets to refresh even when they had no TTL set.
+type: bugfix
+scope: Core
diff --git a/kong/pdk/vault.lua b/kong/pdk/vault.lua
index 347c3d050f83..4a29f405aff8 100644
--- a/kong/pdk/vault.lua
+++ b/kong/pdk/vault.lua
@@ -769,7 +769,23 @@ local function new(self)
 
       else
         lru_ttl = ttl
-        shdict_ttl = DAO_MAX_TTL
+        -- shared dict ttl controls when the secret
+        -- value will be refreshed by `rotate_secrets`
+        -- timer. If a secret whose remaining time is less
+        -- than `config.resurrect_ttl`(or DAO_MAX_TTL
+        -- if not configured), it could possibly
+        -- be updated in every cycle of `rotate_secrets`.
+        --
+        -- The shdict_ttl should be
+        -- `config.ttl` + `config.resurrect_ttl`
+        -- to make sure the secret value persists for
+        -- at least `config.ttl` seconds.
+        -- When `config.resurrect_ttl` is not set and
+        -- `config.ttl` is not set, shdict_ttl will be
+        -- DAO_MAX_TTL * 2; when `config.resurrect_ttl`
+        -- is not set but `config.ttl` is set, shdict_ttl
+        -- will be ttl + DAO_MAX_TTL
+        shdict_ttl = ttl + DAO_MAX_TTL
       end
 
     else
diff --git a/spec/02-integration/13-vaults/05-ttl_spec.lua b/spec/02-integration/13-vaults/05-ttl_spec.lua
index 5ad2d7ec0747..8066c942ed01 100644
--- a/spec/02-integration/13-vaults/05-ttl_spec.lua
+++ b/spec/02-integration/13-vaults/05-ttl_spec.lua
@@ -218,6 +218,123 @@ describe("vault ttl and rotation (#" .. strategy .. ") #" .. vault.name, functio
   end)
 end)
 
+describe("vault rotation #without ttl (#" .. 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")
+
+    vault:setup()
+
+    local bp = helpers.get_db_utils(strategy,
+                                    { "vaults", "routes", "services", "plugins" },
+                                    { "dummy" },
+                                    { vault.name })
+
+
+    -- override a default config without default ttl
+    assert(bp.vaults:insert({
+      name     = vault.name,
+      prefix   = vault.prefix,
+      config   = {
+        default_value = "init",
+      },
+    }))
+
+    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}",
+                                vault.prefix, secret),
+      },
+      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()
+    vault:teardown()
+
+    helpers.unsetenv("KONG_LUA_PATH_OVERRIDE")
+  end)
+
+
+  it("update secret value should not refresh cached vault reference(backend: #" .. vault.name .. ")", function()
+    local function check_plugin_secret(expect, ttl, leeway)
+      leeway = leeway or 0.25 -- 25%
+
+      local timeout = ttl + (ttl * leeway)
+
+      -- The secret value is supposed to be not refreshed
+      -- after several rotations
+      assert.has_error(function()
+        assert
+         .with_timeout(timeout)
+         .with_step(0.5)
+         .eventually(function()
+            local res = http_get("/")
+            local value = assert.response(res).has.header(DUMMY_HEADER)
+
+            if value == expect then
+              return true
+            end
+
+            return false
+         end)
+         .is_falsy("expected plugin secret not to be updated to '" .. expect .. "' "
+                 .. "' within " .. tostring(timeout) .. "seconds")
+        end)
+    end
+
+    vault:update_secret(secret, "old")
+    check_plugin_secret("init", 5)
+
+    vault:update_secret(secret, "new")
+    check_plugin_secret("init", 5)
+  end)
+end)
 
 describe("#hybrid mode dp vault ttl and rotation (#" .. strategy .. ") #" .. vault.name, function()
   local client