fix(admin): duplicate consumer authentication keys are silently accepted

apisix/admin/consumers.lua

@@ -18,6 +18,7 @@ local core    = require("apisix.core")
 local plugins = require("apisix.admin.plugins")
 local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf
 local resource = require("apisix.admin.resource")
+local apisix_consumer = require("apisix.consumer")
 
 
 local function check_conf(username, conf, need_username, schema, opts)
@@ -36,6 +37,13 @@ local function check_conf(username, conf, need_username, schema, opts)
         if not ok then
             return nil, {error_msg = "invalid plugins configuration: " .. err}
         end
+
+        if not opts.skip_references_check then
+            ok, err = apisix_consumer.check_duplicate_key(conf.plugins, conf.username)
+            if not ok then
+                return nil, {error_msg = err}
+            end
+        end
     end
 
     if conf.group_id and not opts.skip_references_check then

apisix/admin/credentials.lua

@@ -18,10 +18,12 @@ local core     = require("apisix.core")
 local plugins  = require("apisix.admin.plugins")
 local plugin   = require("apisix.plugin")
 local resource = require("apisix.admin.resource")
+local apisix_consumer = require("apisix.consumer")
 local plugins_encrypt_conf = require("apisix.admin.plugins").encrypt_conf
 local pairs    = pairs
 
-local function check_conf(_id, conf, _need_id, schema)
+local function check_conf(id, conf, _need_id, schema, opts)
+    opts = opts or {}
     local ok, err = core.schema.check(schema, conf)
     if not ok then
         return nil, {error_msg = "invalid configuration: " .. err}
@@ -42,6 +44,16 @@ local function check_conf(_id, conf, _need_id, schema)
                 return nil, {error_msg = "only supports auth type plugins in consumer credential"}
             end
         end
+
+        -- opts.sub_path is in the form of {consumer_name}/credentials or
+        -- {consumer_name}/credentials/{credential_id}
+        if not opts.skip_references_check and opts.sub_path then
+            local consumer_name = core.utils.split_uri(opts.sub_path)[1]
+            ok, err = apisix_consumer.check_duplicate_key(conf.plugins, consumer_name, id)
+            if not ok then
+                return nil, {error_msg = err}
+            end
+        end
     end
 
     return true, nil

apisix/admin/resource.lua

@@ -81,7 +81,7 @@ local function check_forbidden_properties(conf, forbidden_properties)
 end
 
 
-function _M:check_conf(id, conf, need_id, typ, allow_time)
+function _M:check_conf(id, conf, need_id, typ, allow_time, sub_path)
     if self.name == "secrets" then
         id = typ .. "/" .. id
     end
@@ -123,7 +123,8 @@ function _M:check_conf(id, conf, need_id, typ, allow_time)
     end
 
     local conf_for_check = tbl_deepcopy(conf)
-    local ok, err = self.checker(id, conf_for_check, need_id, self.schema, {secret_type = typ})
+    local ok, err = self.checker(id, conf_for_check, need_id, self.schema,
+                                 {secret_type = typ, sub_path = sub_path})
 
     if self.encrypt_conf then
         self.encrypt_conf(id, conf)
@@ -253,7 +254,7 @@ function _M:put(id, conf, sub_path, args)
     end
 
     local need_id = not no_id_res[self.name]
-    local ok, err = self:check_conf(id, conf, need_id, typ)
+    local ok, err = self:check_conf(id, conf, need_id, typ, nil, sub_path)
     if not ok then
         return 400, err
     end

apisix/consumer.lua

@@ -24,6 +24,7 @@ local error          = error
 local ipairs         = ipairs
 local pairs          = pairs
 local type           = type
+local tostring       = tostring
 local string_sub     = string.sub
 local consumers
 
@@ -279,6 +280,154 @@ function _M.find_consumer(plugin_name, key, key_value)
 end
 
 
+-- The auth plugins below match the consumer by a unique key attribute, which is
+-- the attribute that find_consumer() is called with at runtime. Duplicating such
+-- keys across consumers/credentials makes the runtime matching ambiguous: the
+-- last loaded consumer silently wins.
+local plugin_unique_key_attrs = {
+    ["key-auth"]   = "key",
+    ["basic-auth"] = "username",
+    ["jwt-auth"]   = "key",
+    ["hmac-auth"]  = "key_id",
+}
+
+
+local function get_auth_unique_keys(plugins_conf)
+    local keys
+    for plugin_name, plugin_conf in pairs(plugins_conf) do
+        local key_attr = plugin_unique_key_attrs[plugin_name]
+        if key_attr and type(plugin_conf) == "table" then
+            local key_value = plugin_conf[key_attr]
+            if type(key_value) == "string" then
+                if secret.is_secret_ref(key_value) then
+                    core.log.info("skip duplicate check for the ", key_attr,
+                                  " of plugin ", plugin_name,
+                                  ": secret reference can not be resolved at write time")
+                else
+                    keys = keys or {}
+                    keys[plugin_name] = key_value
+                end
+            end
+        end
+    end
+
+    return keys
+end
+
+
+local function get_stored_unique_key(stored_plugins, plugin_name, key_attr)
+    local stored_conf = stored_plugins[plugin_name]
+    if type(stored_conf) ~= "table" then
+        return nil
+    end
+
+    -- the stored conf may contain encrypted fields (data_encryption),
+    -- decrypt a copy of it like the Admin API GET handler does
+    local conf = core.table.deepcopy(stored_conf)
+    plugin.decrypt_conf(plugin_name, conf, core.schema.TYPE_CONSUMER)
+
+    local key_value = conf[key_attr]
+    if type(key_value) ~= "string" or secret.is_secret_ref(key_value) then
+        return nil
+    end
+
+    return key_value
+end
+
+
+-- Reject the write when an auth plugin key in plugins_conf is already used by
+-- another consumer or credential, since the runtime consumer matching can not
+-- distinguish them.
+-- When writing a consumer, consumer_name is its username and credential_id is
+-- nil: entries owned by the consumer itself are not treated as duplicates.
+-- When writing a credential, both consumer_name and credential_id are set:
+-- only the credential itself is skipped.
+function _M.check_duplicate_key(plugins_conf, consumer_name, credential_id)
+    if not plugins_conf then
+        return true
+    end
+
+    -- the credential id may come from the request payload as a number
+    if credential_id then
+        credential_id = tostring(credential_id)
+    end
+
+    local in_keys = get_auth_unique_keys(plugins_conf)
+    if not in_keys then
+        return true
+    end
+
+    local res, err = core.etcd.get("/consumers", true)
+    if not res then
+        return nil, "failed to list consumers: " .. err
+    end
+
+    if res.status == 404 then
+        return true
+    end
+
+    if res.status ~= 200 then
+        return nil, "failed to list consumers, response code: " .. res.status
+    end
+
+    -- admin_api_version v3 puts the entries in body.list,
+    -- while v2 keeps them in body.node.nodes
+    local items = res.body.list
+    if not items and res.body.node then
+        items = res.body.node.nodes
+    end
+
+    for _, item in ipairs(items or {}) do
+        if type(item) ~= "table" or type(item.value) ~= "table"
+                or type(item.value.plugins) ~= "table" then
+            goto CONTINUE
+        end
+
+        local item_consumer_name, item_credential_id
+        if is_credential_etcd_key(item.key) then
+            item_consumer_name = get_consumer_name_from_credential_etcd_key(item.key)
+            item_credential_id = get_credential_id_from_etcd_key(item.key)
+        else
+            item_consumer_name = item.value.username
+        end
+
+        if credential_id then
+            -- a credential does not conflict with itself
+            if item_consumer_name == consumer_name
+                    and item_credential_id == credential_id then
+                goto CONTINUE
+            end
+        else
+            -- a consumer does not conflict with itself or its own credentials
+            if item_consumer_name == consumer_name then
+                goto CONTINUE
+            end
+        end
+
+        for plugin_name, key_value in pairs(in_keys) do
+            local key_attr = plugin_unique_key_attrs[plugin_name]
+            local stored_value = get_stored_unique_key(item.value.plugins,
+                                                       plugin_name, key_attr)
+            if stored_value == key_value then
+                local owner
+                if item_credential_id then
+                    owner = "credential: " .. item_credential_id ..
+                            " of consumer: " .. item_consumer_name
+                else
+                    owner = "consumer: " .. item_consumer_name
+                end
+                return nil, "duplicate " .. key_attr .. " of plugin " .. plugin_name
+                            .. " found with " .. owner
+            end
+        end
+
+        ::CONTINUE::
+    end
+
+    return true
+end
+
+
 local function check_consumer(consumer, key)
     local data_valid
     local err