diff --git a/apisix/admin/consumers.lua b/apisix/admin/consumers.lua index 32b1a9440420..dee37227e809 100644 --- a/apisix/admin/consumers.lua +++ b/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 diff --git a/apisix/admin/credentials.lua b/apisix/admin/credentials.lua index d15b78ecac24..d8240b468d36 100644 --- a/apisix/admin/credentials.lua +++ b/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 diff --git a/apisix/admin/resource.lua b/apisix/admin/resource.lua index 87c5692f6076..d002f765c0f6 100644 --- a/apisix/admin/resource.lua +++ b/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 diff --git a/apisix/consumer.lua b/apisix/consumer.lua index cd60a0209ff0..86a27ca2fe9b 100644 --- a/apisix/consumer.lua +++ b/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,113 @@ 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 cannot be resolved at write time") + else + keys = keys or {} + keys[plugin_name] = key_value + end + end + end + end + + return keys +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. +-- +-- The lookup goes through find_consumer(), i.e. the consumer data that this +-- process has already watched and synced from etcd -- the same view the +-- runtime uses for auth matching -- instead of pulling the full /consumers +-- range from etcd on every admin write. The tradeoff is that the local view +-- may lag the latest writes by one sync cycle, so a duplicate written moments +-- earlier can slip through under rapid successive or concurrent writes. This +-- is accepted: the check is best-effort protection against misconfiguration, +-- and the authoritative behavior at runtime remains last-loaded-wins. +-- +-- When writing a consumer, consumer_name is its username and credential_id is +-- nil: a key owned by the consumer itself (inline or by its credentials) is +-- not treated as a duplicate. 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 /consumers watcher may not be initialized yet, e.g. the write + -- arrives right after the worker starts: degrade to allow, consistent + -- with the best-effort nature of this check + if not consumers then + return true + end + + local in_keys = get_auth_unique_keys(plugins_conf) + if not in_keys then + return true + end + + -- the credential id may come from the request payload as a number, while + -- the cached credential_id is taken from the etcd key as a string + if credential_id then + credential_id = tostring(credential_id) + end + + for plugin_name, key_value in pairs(in_keys) do + local key_attr = plugin_unique_key_attrs[plugin_name] + local owner = _M.find_consumer(plugin_name, key_attr, key_value) + if owner then + local is_self + if credential_id then + -- a credential does not conflict with itself + is_self = owner.consumer_name == consumer_name + and owner.credential_id == credential_id + else + -- a consumer does not conflict with itself or its own credentials + is_self = owner.consumer_name == consumer_name + end + + if not is_self then + local owner_desc + if owner.credential_id then + owner_desc = "credential: " .. owner.credential_id .. + " of consumer: " .. owner.consumer_name + else + owner_desc = "consumer: " .. owner.consumer_name + end + return nil, "duplicate " .. key_attr .. " of plugin " .. plugin_name + .. " found with " .. owner_desc + end + end + end + + return true +end + + local function check_consumer(consumer, key) local data_valid local err diff --git a/t/admin/consumers.t b/t/admin/consumers.t index 2e9c5e206bfc..6a5fba171c91 100644 --- a/t/admin/consumers.t +++ b/t/admin/consumers.t @@ -360,3 +360,268 @@ GET /t --- error_code: 400 --- response_body eval qr/\{"error_msg":"the property is forbidden:.*"\}/ + + + +=== TEST 12: add consumer case_a with key-auth key +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "case_a", + "plugins": { + "key-auth": { + "key": "duplicate-check-key-1" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 13: add consumer case_b with the same key-auth key, should fail +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "case_b", + "plugins": { + "key-auth": { + "key": "duplicate-check-key-1" + } + } + }]] + ) + + ngx.status = code + ngx.print(body) + } + } +--- request +GET /t +--- error_code: 400 +--- response_body +{"error_msg":"duplicate key of plugin key-auth found with consumer: case_a"} + + + +=== TEST 14: add consumer case_b with a different key-auth key, should success +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "case_b", + "plugins": { + "key-auth": { + "key": "duplicate-check-key-2" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 15: update consumer case_a with its own key unchanged, should success +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "case_a", + "desc": "updated description", + "plugins": { + "key-auth": { + "key": "duplicate-check-key-1" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 16: duplicate basic-auth username between consumers, should fail +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "case_c", + "plugins": { + "basic-auth": { + "username": "case-user", + "password": "the-password" + } + } + }]] + ) + if code >= 300 then + ngx.status = code + ngx.say(body) + return + end + + -- wait for the watcher to sync the previous write, as the + -- duplicate check runs against the locally synced consumer data + ngx.sleep(0.5) + + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "case_d", + "plugins": { + "basic-auth": { + "username": "case-user", + "password": "another-password" + } + } + }]] + ) + + ngx.status = code + ngx.print(body) + } + } +--- request +GET /t +--- error_code: 400 +--- response_body +{"error_msg":"duplicate username of plugin basic-auth found with consumer: case_c"} + + + +=== TEST 17: store consumer with data_encryption explicitly enabled +--- extra_yaml_config +apisix: + data_encryption: + enable_encrypt_fields: true + keyring: + - qeddd145sfvddff3 +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "enc_case_a", + "plugins": { + "key-auth": { + "key": "duplicate-check-enc-key" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 18: duplicate key against the stored encrypted consumer, should fail +--- extra_yaml_config +apisix: + data_encryption: + enable_encrypt_fields: true + keyring: + - qeddd145sfvddff3 +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "enc_case_b", + "plugins": { + "key-auth": { + "key": "duplicate-check-enc-key" + } + } + }]] + ) + + ngx.status = code + ngx.print(body) + } + } +--- request +GET /t +--- error_code: 400 +--- response_body +{"error_msg":"duplicate key of plugin key-auth found with consumer: enc_case_a"} + + + +=== TEST 19: clean up consumers +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + for _, name in ipairs({"case_a", "case_b", "case_c", "enc_case_a"}) do + local code, body = t('/apisix/admin/consumers/' .. name, ngx.HTTP_DELETE) + if code >= 300 then + ngx.say("failed to delete consumer ", name, ": ", body) + return + end + end + ngx.say("passed") + } + } +--- request +GET /t +--- response_body +passed diff --git a/t/admin/credentials.t b/t/admin/credentials.t index 15119829c2e3..50953e9ab3ef 100644 --- a/t/admin/credentials.t +++ b/t/admin/credentials.t @@ -110,7 +110,7 @@ passed "desc": "basic-auth for jack", "plugins": { "basic-auth": { - "username": "the-user", + "username": "the-new-user", "password": "the-password" } } @@ -119,7 +119,7 @@ passed "value":{ "desc":"basic-auth for jack", "id":"credential_a", - "plugins":{"basic-auth":{"username":"the-user","password":"WvF5kpaLvIzjuk4GNIMTJg=="}} + "plugins":{"basic-auth":{"username":"the-new-user","password":"WvF5kpaLvIzjuk4GNIMTJg=="}} }, "key":"/apisix/consumers/jack/credentials/credential_a" }]] @@ -492,3 +492,265 @@ GET /t --- error_code: 400 --- response_body {"error_msg":"missing credential id"} + + + +=== TEST 17: prepare consumers for duplicate key checks +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ "username": "alex" }]] + ) + if code >= 300 then + ngx.status = code + ngx.say(body) + return + end + + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "bob", + "plugins": { + "key-auth": { + "key": "the-key-of-bob" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 18: create a credential with key-auth for consumer alex +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers/alex/credentials/cred-alex-a', + ngx.HTTP_PUT, + [[{ + "plugins": { + "key-auth": { + "key": "the-key-of-alex" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 19: credential of another consumer duplicates the credential key, should fail +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers/bob/credentials/cred-bob-a', + ngx.HTTP_PUT, + [[{ + "plugins": { + "key-auth": { + "key": "the-key-of-alex" + } + } + }]] + ) + + ngx.status = code + ngx.print(body) + } + } +--- request +GET /t +--- error_code: 400 +--- response_body +{"error_msg":"duplicate key of plugin key-auth found with credential: cred-alex-a of consumer: alex"} + + + +=== TEST 20: credential duplicates the key of another consumer, should fail +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers/alex/credentials/cred-alex-b', + ngx.HTTP_PUT, + [[{ + "plugins": { + "key-auth": { + "key": "the-key-of-bob" + } + } + }]] + ) + + ngx.status = code + ngx.print(body) + } + } +--- request +GET /t +--- error_code: 400 +--- response_body +{"error_msg":"duplicate key of plugin key-auth found with consumer: bob"} + + + +=== TEST 21: consumer duplicates the key of an existing credential, should fail +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "carol", + "plugins": { + "key-auth": { + "key": "the-key-of-alex" + } + } + }]] + ) + + ngx.status = code + ngx.print(body) + } + } +--- request +GET /t +--- error_code: 400 +--- response_body +{"error_msg":"duplicate key of plugin key-auth found with credential: cred-alex-a of consumer: alex"} + + + +=== TEST 22: update the credential with its own key unchanged, should success +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers/alex/credentials/cred-alex-a', + ngx.HTTP_PUT, + [[{ + "desc": "new description", + "plugins": { + "key-auth": { + "key": "the-key-of-alex" + } + } + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed + + + +=== TEST 23: sibling credential of the same consumer duplicates the key, should fail +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers/alex/credentials/cred-alex-c', + ngx.HTTP_PUT, + [[{ + "plugins": { + "key-auth": { + "key": "the-key-of-alex" + } + } + }]] + ) + + ngx.status = code + ngx.print(body) + } + } +--- request +GET /t +--- error_code: 400 +--- response_body +{"error_msg":"duplicate key of plugin key-auth found with credential: cred-alex-a of consumer: alex"} + + + +=== TEST 24: credential duplicates the key of its own consumer, should fail +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers/bob/credentials/cred-bob-b', + ngx.HTTP_PUT, + [[{ + "plugins": { + "key-auth": { + "key": "the-key-of-bob" + } + } + }]] + ) + + ngx.status = code + ngx.print(body) + } + } +--- request +GET /t +--- error_code: 400 +--- response_body +{"error_msg":"duplicate key of plugin key-auth found with consumer: bob"} + + + +=== TEST 25: clean up consumers and credentials +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + for _, name in ipairs({"alex", "bob", "jack"}) do + local code, body = t('/apisix/admin/consumers/' .. name, ngx.HTTP_DELETE) + if code >= 300 then + ngx.say("failed to delete consumer ", name, ": ", body) + return + end + end + ngx.say("passed") + } + } +--- request +GET /t +--- response_body +passed