Skip to content

fix: consumer key duplication check #12040

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion apisix/admin/consumers.lua
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
local core = require("apisix.core")
local plugins = require("apisix.admin.plugins")
local resource = require("apisix.admin.resource")

local utils = require("apisix.admin.utils")

local function check_conf(username, conf, need_username, schema)
local ok, err = core.schema.check(schema, conf)
Expand All @@ -30,10 +30,17 @@ local function check_conf(username, conf, need_username, schema)
end

if conf.plugins then
-- check_schema encrypts the key in the plugin.
-- check duplicate key require the original key.
local conf_plugins_copy = core.table.deepcopy(conf.plugins)
ok, err = plugins.check_schema(conf.plugins, core.schema.TYPE_CONSUMER)
if not ok then
return nil, {error_msg = "invalid plugins configuration: " .. err}
end
local ok, err = utils.check_duplicate_key(conf_plugins_copy, conf.username)
if not ok then
return nil, {error_msg = err}
end
end

if conf.group_id then
Expand Down
11 changes: 10 additions & 1 deletion apisix/admin/credentials.lua
Original file line number Diff line number Diff line change
Expand Up @@ -18,20 +18,29 @@ local core = require("apisix.core")
local plugins = require("apisix.admin.plugins")
local plugin = require("apisix.plugin")
local resource = require("apisix.admin.resource")
local utils = require("apisix.admin.utils")
local pairs = pairs

local function check_conf(_id, conf, _need_id, schema)
local function check_conf(id, conf, _need_id, schema)
local ok, err = core.schema.check(schema, conf)
if not ok then
return nil, {error_msg = "invalid configuration: " .. err}
end

if conf.plugins then
-- check_schema encrypts the key in the plugin.
-- check duplicate key require the original key.
local conf_plugins_copy = core.table.deepcopy(conf.plugins)
ok, err = plugins.check_schema(conf.plugins, core.schema.TYPE_CONSUMER)
if not ok then
return nil, {error_msg = "invalid plugins configuration: " .. err}
end

local ok, err = utils.check_duplicate_key(conf_plugins_copy, nil, id)
if not ok then
return nil, {error_msg = err}
end

for name, _ in pairs(conf.plugins) do
local plugin_obj = plugin.get(name)
if not plugin_obj then
Expand Down
49 changes: 49 additions & 0 deletions apisix/admin/utils.lua
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ local ngx_time = ngx.time
local tonumber = tonumber
local ipairs = ipairs
local pairs = pairs
local consumer = require("apisix.consumer")
local plugin = require("apisix.plugin")


local _M = {}
Expand Down Expand Up @@ -110,4 +112,51 @@ function _M.decrypt_params(decrypt_func, body, schema_type)
end
end


local plugin_key_map = {
["key-auth"] = "key",
["basic-auth"] = "username",
["jwt-auth"] = "key",
["hmac-auth"] = "key_id"
}


function _M.check_duplicate_key(plugins_conf, username, credential_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest putting the check_deuplicate_key function in apisix/consumer.lua

if not plugins_conf then
return true
end

for plugin_name, plugin_conf in pairs(plugins_conf) do
local plugin_obj = plugin.get(plugin_name)
if not plugin_obj then
return nil, "unknown plugin " .. plugin_name
end

if plugin_obj.type ~= "auth" then
goto continue
end

local key_field = plugin_key_map[plugin_name]
if not key_field then
goto continue
end

local key_value = plugin_conf[key_field]
if not key_value then
goto continue
end

local consumer = consumer.find_consumer(plugin_name, key_field, key_value)
if consumer and
((username and consumer.username ~= username) or
(credential_id and consumer.credential_id ~= credential_id)) then
return nil, "duplicate key found with consumer: " .. consumer.username
end

::continue::
end

return true
end

return _M
83 changes: 83 additions & 0 deletions t/admin/consumers2.t
Original file line number Diff line number Diff line change
Expand Up @@ -174,3 +174,86 @@ __DATA__
}
--- response_body
{"error_msg":"wrong username"}



=== TEST 6: create consumer
--- 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": "jack",
"desc": "key-auth for jack",
"plugins": {
"key-auth": {
"key": "the-key"
}
}
}]]
)
}
}
--- request
GET /t



=== TEST 7: duplicate consumer key, PUT
--- 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": "jack2",
"desc": "key-auth for jack2",
"plugins": {
"key-auth": {
"key": "the-key"
}
}
}]]
)

ngx.status = code
ngx.print(body)
}
}
--- request
GET /t
--- error_code: 400
--- response_body
{"error_msg":"duplicate key found with consumer: jack"}



=== TEST 8: update consumer jack
--- 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": "jack",
"desc": "key-auth for jack",
"plugins": {
"key-auth": {
"key": "the-key"
}
}
}]]
)

ngx.status = code
ngx.say(body)
}
}
--- request
GET /t
--- response_body
passed
100 changes: 98 additions & 2 deletions t/admin/credentials.t
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ passed
"desc": "basic-auth for jack",
"plugins": {
"basic-auth": {
"username": "the-user",
"username": "the-new-user",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding the duplicate judgment, it cannot be added here again, so I made a modification.

"password": "the-password"
}
}
Expand All @@ -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"
}]]
Expand Down Expand Up @@ -492,3 +492,99 @@ GET /t
--- error_code: 400
--- response_body
{"error_msg":"missing credential id"}



=== TEST 17: create a consumer bar
--- 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": "bar" }]])
}
}
--- request
GET /t



=== TEST 18: create a credential with key-auth for the consumer bar
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/consumers/bar/credentials/credential_c',
ngx.HTTP_PUT,
[[{
"desc": "key-auth for bar",
"plugins": {
"key-auth": {
"key": "the-key-bar"
}
}
}]]
)
}
}
--- request
GET /t



=== TEST 19: can not create a credential with duplicate key
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test
local code, body = t('/apisix/admin/consumers/bar/credentials/credential_d',
ngx.HTTP_PUT,
[[{
"desc": "key-auth for bar",
"plugins": {
"key-auth": {
"key": "the-key-bar"
}
}
}]]
)

ngx.status = code
ngx.print(body)
}
}
--- request
GET /t
--- error_code: 400
--- response_body
{"error_msg":"duplicate key found with consumer: bar"}



=== TEST 20: can update credential credential_c with same key
--- config
location /t {
content_by_lua_block {
local t = require("lib.test_admin").test

-- update desc, keep same key
local code, body = t('/apisix/admin/consumers/bar/credentials/credential_c',
ngx.HTTP_PUT,
[[{
"desc": "new description",
"plugins": {
"key-auth": {
"key": "the-key-bar"
}
}
}]]
)

ngx.status = code
ngx.say(body)
}
}
--- request
GET /t
--- response_body
passed
--- error_code: 200
2 changes: 2 additions & 0 deletions t/secret/aws.t
Original file line number Diff line number Diff line change
Expand Up @@ -314,3 +314,5 @@ GET /t
}
--- response_body
all done
--- error_log
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The testing of these changes is an expected change. These tests have an admin api call to the consumer and use a non-existent secret reference, which in the new logic triggers a secret lookup failure and an error log. It didn't break the original test path.

failed to fetch secret value: no secret conf, secret_uri: $secret://aws/mysecret/jack/key
2 changes: 2 additions & 0 deletions t/secret/gcp.t
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,8 @@ kEJQcmfVew5mFXyxuEn3zA==
}
--- response_body
all done
--- error_log
failed to fetch secret value: no secret conf, secret_uri: $secret://gcp/mysecret/jack/key



Expand Down
2 changes: 2 additions & 0 deletions t/secret/secret_lru.t
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,5 @@ GET /t
}
--- response_body
nil
--- error_log
failed to fetch secret value: no secret conf, secret_uri: $secret://vault/mysecret/jack/auth-key
Loading