From 9c8409733fcaa3c276f4a7d6a10f61a805234862 Mon Sep 17 00:00:00 2001 From: wklken Date: Wed, 1 Nov 2023 15:27:09 +0800 Subject: [PATCH 1/6] fix(bk-rate-limit): update the key to use new limits if the config changed (#62) --- src/apisix/plugins/bk-rate-limit/init.lua | 14 ++++++++++++++ src/apisix/plugins/bk-resource-rate-limit.lua | 2 ++ src/apisix/plugins/bk-stage-rate-limit.lua | 1 - src/apisix/tests/bk-rate-limit/test-init.lua | 19 +++++++++++++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/src/apisix/plugins/bk-rate-limit/init.lua b/src/apisix/plugins/bk-rate-limit/init.lua index 7b0f8be..128e74b 100644 --- a/src/apisix/plugins/bk-rate-limit/init.lua +++ b/src/apisix/plugins/bk-rate-limit/init.lua @@ -35,6 +35,7 @@ -- redis_database: 0 -- redis_timeout: 1001 -- +local apisix_plugin = require("apisix.plugin") local core = require("apisix.core") local rate_limit_redis = require("apisix.plugins.bk-rate-limit.rate-limit-redis") local lrucache = core.lrucache.new( @@ -77,6 +78,14 @@ local _M = { }, } +local function gen_limit_key(conf, key) + -- Here we use plugin-level conf version to prevent the counter from being resetting + -- because of the change elsewhere. + -- e.g. conf_version = 1969078430 + local new_key = key .. ':' .. apisix_plugin.conf_version(conf) + return new_key +end + ---Create rate-limit-redis object ---@param plugin_name string @apisix plugin name ---@return table @rate-limit-redis object @@ -108,6 +117,7 @@ function _M.rate_limit(conf, ctx, plugin_name, key, count, time_window) return 500 end + key = gen_limit_key(conf, key) core.log.info("limit key: ", key) local delay, remaining, reset = lim:incoming(key, count, time_window) @@ -145,4 +155,8 @@ function _M.rate_limit(conf, ctx, plugin_name, key, count, time_window) end end +if _TEST then + _M.gen_limit_key = gen_limit_key +end + return _M diff --git a/src/apisix/plugins/bk-resource-rate-limit.lua b/src/apisix/plugins/bk-resource-rate-limit.lua index 142a8d7..4775ee4 100644 --- a/src/apisix/plugins/bk-resource-rate-limit.lua +++ b/src/apisix/plugins/bk-resource-rate-limit.lua @@ -62,6 +62,8 @@ function _M.access(conf, ctx) end -- TODO: make it lazy, share the key with other plugins + -- FIXME: should change the bk_reosurce_name to bk_resource_id if you need more accurate rate limit + -- while the developer may change the bk_resource_name from the frontend page. local key = table_concat( { bk_app_code, diff --git a/src/apisix/plugins/bk-stage-rate-limit.lua b/src/apisix/plugins/bk-stage-rate-limit.lua index 4851fc7..a233322 100644 --- a/src/apisix/plugins/bk-stage-rate-limit.lua +++ b/src/apisix/plugins/bk-stage-rate-limit.lua @@ -85,7 +85,6 @@ function _M.access(conf, ctx) else for i, rate in ipairs(rates) do -- here we should add the rate index into key, otherwise the rate limit will be shared(will be wrong) - -- FIXME: if the rate changes, will wait for the period to effect local limit_key = key .. ":" .. tostring(i) local code = ratelimit.rate_limit(conf, ctx, plugin_name, limit_key, rate.tokens, rate.period) if code then diff --git a/src/apisix/tests/bk-rate-limit/test-init.lua b/src/apisix/tests/bk-rate-limit/test-init.lua index 75a1d26..95d4cf5 100644 --- a/src/apisix/tests/bk-rate-limit/test-init.lua +++ b/src/apisix/tests/bk-rate-limit/test-init.lua @@ -48,6 +48,25 @@ describe( end ) + context( + "gen_limit_key", function() + it("ok", function() + local conf = {} + local key = ratelimit.gen_limit_key(conf, "key") + assert.equal(key, "key:223132457") + assert.equal(conf._version, "223132457") + + -- mock the conf change + conf._version = nil + conf["hello"] = "world" + key = ratelimit.gen_limit_key(conf, "key") + assert.equal(key, "key:3624485329") + end + ) + end + ) + + context( "rate_limit", function() local conf From a6822abf94c7e03fc5aeb144aed1f35cbe6e1b71 Mon Sep 17 00:00:00 2001 From: wklken Date: Tue, 7 Nov 2023 11:42:19 +0800 Subject: [PATCH 2/6] =?UTF-8?q?fix(patch/opentelemetry.lua):=20span=20kind?= =?UTF-8?q?=20from=20server=20to=20client,=20for=20to=E2=80=A6=20(#63)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../005_opentelemetry_span_kind_to_client.patch | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 src/build/patches/005_opentelemetry_span_kind_to_client.patch diff --git a/src/build/patches/005_opentelemetry_span_kind_to_client.patch b/src/build/patches/005_opentelemetry_span_kind_to_client.patch new file mode 100644 index 0000000..63eee6b --- /dev/null +++ b/src/build/patches/005_opentelemetry_span_kind_to_client.patch @@ -0,0 +1,13 @@ +diff --git a/apisix/plugins/opentelemetry.lua b/apisix/plugins/opentelemetry.lua +index f8013e6f..f9d2f3ca 100644 +--- a/apisix/plugins/opentelemetry.lua ++++ b/apisix/plugins/opentelemetry.lua +@@ -329,7 +329,7 @@ function _M.rewrite(conf, api_ctx) + end + + local ctx = tracer:start(upstream_context, api_ctx.var.request_uri, { +- kind = span_kind.server, ++ kind = span_kind.client, + attributes = attributes, + }) + api_ctx.otel_context_token = ctx:attach() From d530ead0cb4772c1ba42da521d599f8795e7de8e Mon Sep 17 00:00:00 2001 From: wklken Date: Tue, 7 Nov 2023 15:31:11 +0800 Subject: [PATCH 3/6] fix(bk-auth/legacy): patch for invalid params of old gateway (#67) --- src/apisix/plugins/README.md | 7 +- .../plugins/bk-legacy-invalid-params.lua | 66 ++++++++++ src/apisix/t/bk-legacy-invalid-params.t | 116 ++++++++++++++++++ .../tests/test-bk-legacy-invalid-params.lua | 94 ++++++++++++++ 4 files changed, 280 insertions(+), 3 deletions(-) create mode 100644 src/apisix/plugins/bk-legacy-invalid-params.lua create mode 100644 src/apisix/t/bk-legacy-invalid-params.t create mode 100644 src/apisix/tests/test-bk-legacy-invalid-params.lua diff --git a/src/apisix/plugins/README.md b/src/apisix/plugins/README.md index 9aa8fad..ecfa077 100644 --- a/src/apisix/plugins/README.md +++ b/src/apisix/plugins/README.md @@ -18,11 +18,12 @@ 上下文注入，优先级：18000 ~ 19000 +- bk-legacy-invalid-params # priority: 18880 # 用于兼容老版本 go1.16 使用 `;` 作为 query string 分隔符 - bk-opentelemetry # priority: 18870 # 这个插件用于 opentelemetry, 需要尽量精准统计全局的耗时，同时需要注入 trace_id/span_id 作为后面所有插件自定义 opentelemetry 上报的 trace_id 即 parent span_id - bk-not-found-handler # priority: 18860 # 该插件仅适用于由 operator 创建的默认根路由，用以规范化 404 消息。该插件以较高优先级结束请求返回 404 错误信息 - bk-request-id # priority: 18850 - bk-stage-context # priority: 18840 -- bk-service-context # priority: 18830 +- bk-service-context # priority: 18830 (abandonned) - bk-resource-context # priority: 18820 - bk-status-rewrite # priority: 18815 - bk-verified-user-exempted-apps # priority: 18810 (will be deprecated) @@ -31,8 +32,8 @@ 认证： -- bk-workflow-parameters # priority: 18750 -- bk-auth-parameters # priority: 18740 +- bk-workflow-parameters # priority: 18750 (abandonned) +- bk-auth-parameters # priority: 18740 (abandonned) - bk-auth-verify # priority: 18730 执行 - 响应：优先级：17500 ~ 18000 diff --git a/src/apisix/plugins/bk-legacy-invalid-params.lua b/src/apisix/plugins/bk-legacy-invalid-params.lua new file mode 100644 index 0000000..ec51c4a --- /dev/null +++ b/src/apisix/plugins/bk-legacy-invalid-params.lua @@ -0,0 +1,66 @@ +-- +-- TencentBlueKing is pleased to support the open source community by making +-- 蓝鲸智云 - API 网关(BlueKing - APIGateway) available. +-- Copyright (C) 2017 THL A29 Limited, a Tencent company. All rights reserved. +-- Licensed under the MIT License (the "License"); you may not use this file except +-- in compliance with the License. You may obtain a copy of the License at +-- +-- http://opensource.org/licenses/MIT +-- +-- Unless required by applicable law or agreed to in writing, software distributed under +-- the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +-- either express or implied. See the License for the specific language governing permissions and +-- limitations under the License. +-- +-- We undertake not to change the open source license (MIT license) applicable +-- to the current version of the project delivered to anyone in the future. +-- + +-- # bk-legacy-invalid-params +-- +-- For old gateway calling, because go 1.16 support both `&` and `;` as query string separator, but lua only support `&` +-- and in some case, the caller html escaped the `&` to `&`(it's ok for go 1.16 gateway) +-- so we need to adapat the old gateway calling. +-- e.g. +-- ?app_code=appC&app_secret=appC +-- ?app_code=appC&amp;app_secret=appC +-- ?app_code=appC;app_secret=appC +-- ?a=1;a=2 + +local string_replace = require("pl.stringx").replace +local string_find = string.find +local core = require("apisix.core") + +local schema = {} + +local _M = { + version = 0.1, + priority = 18880, + name = "bk-legacy-invalid-params", + schema = schema, +} + +function _M.check_schema(conf) + return core.schema.check(schema, conf) +end + +function _M.rewrite(conf, ctx) + -- FIXME: 未来新的接口使用`;`也不生效, 怎么控制范围? + + -- FIX 1 + -- in golang 1.16: strings.IndexAny(key, "&;") + -- so here we just need to replace `;` to `&`, then reset the uri_args + -- args will be decoded like golang version + + -- core.log.error(ctx.var.args) + -- only query string contains `;` should be processed + if ctx.var.args ~= nil and string_find(ctx.var.args, ";") then + local new_args = string_replace(ctx.var.args, ";", "&") + -- core.log.error("replace ; to &: ", new_args) + core.request.set_uri_args(ctx, new_args) + end + -- local args = core.request.get_uri_args() + -- core.log.error(core.json.delay_encode(args)) +end + +return _M diff --git a/src/apisix/t/bk-legacy-invalid-params.t b/src/apisix/t/bk-legacy-invalid-params.t new file mode 100644 index 0000000..b086474 --- /dev/null +++ b/src/apisix/t/bk-legacy-invalid-params.t @@ -0,0 +1,116 @@ +# +# TencentBlueKing is pleased to support the open source community by making +# 蓝鲸智云 - API 网关(BlueKing - APIGateway) available. +# Copyright (C) 2017 THL A29 Limited, a Tencent company. All rights reserved. +# Licensed under the MIT License (the "License"); you may not use this file except +# in compliance with the License. You may obtain a copy of the License at +# +# http://opensource.org/licenses/MIT +# +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +# either express or implied. See the License for the specific language governing permissions and +# limitations under the License. +# +# We undertake not to change the open source license (MIT license) applicable +# to the current version of the project delivered to anyone in the future. +# + +use t::APISIX 'no_plan'; + +log_level('debug'); +repeat_each(1); +no_long_string(); +no_root_location(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!$block->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests; + +__DATA__ +=== TEST 1: sanity +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.bk-legacy-invalid-params") + local ok, err = plugin.check_schema({}) + if not ok then + ngx.say(err) + end + ngx.say("done") + } + } +--- response_body +done +=== TEST 2: add plugin +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "bk-legacy-invalid-params": { + }, + "mocking": { + "content_type": "text/plain", + "response_status": 200, + "response_example": "args:$args\n" + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1982": 1 + }, + "type": "roundrobin" + }, + "uri": "/hello" + }]] + ) + if code >= 300 then + ngx.status = code + end + -- code is 201, body is passed + ngx.say(body) + } + } +--- response_body +passed +=== TEST 3: call no args +--- request +GET /hello +--- response_body +args: + +=== TEST 4: call with normal args +--- request +GET /hello?a=1&b=2 +--- response_body +args:a=1&b=2 + +=== TEST 5: call with `;` +--- request +GET /hello?a=1;b=2 +--- response_body +args:a=1&b=2 + +=== TEST 6: call with `&` +--- request +GET /hello?a=1&b=2 +--- response_body +args:a=1&&b=2 + +=== TEST 7: call with `&amp;` +--- request +GET /hello?a=1&amp;b=2 +--- response_body +args:a=1&&&b=2 + + diff --git a/src/apisix/tests/test-bk-legacy-invalid-params.lua b/src/apisix/tests/test-bk-legacy-invalid-params.lua new file mode 100644 index 0000000..934697d --- /dev/null +++ b/src/apisix/tests/test-bk-legacy-invalid-params.lua @@ -0,0 +1,94 @@ +-- +-- TencentBlueKing is pleased to support the open source community by making +-- 蓝鲸智云 - API 网关(BlueKing - APIGateway) available. +-- Copyright (C) 2017 THL A29 Limited, a Tencent company. All rights reserved. +-- Licensed under the MIT License (the "License"); you may not use this file except +-- in compliance with the License. You may obtain a copy of the License at +-- +-- http://opensource.org/licenses/MIT +-- +-- Unless required by applicable law or agreed to in writing, software distributed under +-- the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +-- either express or implied. See the License for the specific language governing permissions and +-- limitations under the License. +-- +-- We undertake not to change the open source license (MIT license) applicable +-- to the current version of the project delivered to anyone in the future. +-- + +local core = require("apisix.core") + +local plugin = require("apisix.plugins.bk-legacy-invalid-params") + +describe( + "bk-legacy-invalid-params", + function() + context( + "rewrite", + function() + local ctx + before_each( + function() + ctx = { + var = {} + } + stub(core.request, "set_uri_args") + end + ) + + after_each( + function() + -- ngx.req.clear_header:revert() + core.request.set_uri_args:revert() + end + ) + + it( + "no args", + function() + plugin.rewrite({}, ctx) + + assert.stub(core.request.set_uri_args).was_not_called() + end + ) + it( + "normal args with &", + function() + ctx.var.args = "a=1&b=2" + plugin.rewrite({}, ctx) + + assert.stub(core.request.set_uri_args).was_not_called() + end + ) + it( + "args with ;", + function() + ctx.var.args = "a=1;b=2" + plugin.rewrite({}, ctx) + + assert.stub(core.request.set_uri_args).was_called_with(ctx, "a=1&b=2") + end + ) + it( + "args with &", + function() + ctx.var.args = "a=1&b=2" + plugin.rewrite({}, ctx) + + assert.stub(core.request.set_uri_args).was_called_with(ctx, "a=1&&b=2") + end + ) + it( + "args with &amp;", + function() + ctx.var.args = "a=1&amp;b=2" + plugin.rewrite({}, ctx) + + assert.stub(core.request.set_uri_args).was_called_with(ctx, "a=1&&&b=2") + end + ) + + end + ) + end +) From 39db8e6369f9e4bee3a64ba1179499317e69b95b Mon Sep 17 00:00:00 2001 From: wklken Date: Tue, 7 Nov 2023 15:31:28 +0800 Subject: [PATCH 4/6] fix(bin/apisix-start.sh): change the order of apisix start command (#68) --- Dockerfile | 3 ++- src/build/bin/apisix-start.sh | 13 ++++--------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/Dockerfile b/Dockerfile index d12a0b8..527a728 100644 --- a/Dockerfile +++ b/Dockerfile @@ -35,5 +35,6 @@ RUN ls /usr/local/apisix/patches | sort | xargs -L1 -I __patch_file__ sh -c 'cat RUN chmod 755 /data/bkgateway/bin/* && chmod 777 /usr/local/apisix/logs - CMD ["sh", "-c", "/usr/bin/apisix init && /usr/bin/apisix init_etcd && /usr/local/openresty/bin/openresty -p /usr/local/apisix -g 'daemon off;'"] + +STOPSIGNAL SIGQUIT diff --git a/src/build/bin/apisix-start.sh b/src/build/bin/apisix-start.sh index 1f6a37b..38f8182 100755 --- a/src/build/bin/apisix-start.sh +++ b/src/build/bin/apisix-start.sh @@ -1,7 +1,6 @@ #!/bin/sh echo "starting......" - # if standaloneConfigPath not empty, watch the path if [ x"${standaloneConfigPath}" != x"" ] then @@ -17,11 +16,6 @@ then echo "config-watcher for ${standaloneConfigPath} started" fi -# start -echo "start apisix" -apisix start -echo "apisix started" - # start nginx error to sentry if [ x"${BK_APIGW_NGINX_ERROR_LOG_SENTRY_DSN}" != x"" ] then @@ -34,8 +28,9 @@ fi echo "start config-watcher for ${apisixDebugConfigPath}(note: will wait until the container quit)" # note the shell will wait here, so, YOU SHOULD NOT PUT ANY COMMANDS AFTER HERE -sh /data/bkgateway/bin/config-watcher-start.sh "-sourcePath ${apisixDebugConfigPath} -destPath /usr/local/apisix/conf -files debug.yaml -isConfigMap" +sh /data/bkgateway/bin/config-watcher-start.sh "-sourcePath ${apisixDebugConfigPath} -destPath /usr/local/apisix/conf -files debug.yaml -isConfigMap" & +echo "start apisix" +/usr/bin/apisix init && /usr/bin/apisix init_etcd && /usr/local/openresty/bin/openresty -p /usr/local/apisix -g 'daemon off;' -echo "done" - +echo "quit" From 04ea6d4890aa415bfea2a53602cf9de667d3395c Mon Sep 17 00:00:00 2001 From: alex-smile <443677891@qq.com> Date: Mon, 27 Nov 2023 10:36:24 +0800 Subject: [PATCH 5/6] =?UTF-8?q?=E5=85=81=E8=AE=B8=E4=B8=8D=E4=BB=8E?= =?UTF-8?q?=E8=AF=B7=E6=B1=82=E5=8F=82=E6=95=B0=E8=8E=B7=E5=8F=96=E8=AE=A4?= =?UTF-8?q?=E8=AF=81=E5=8F=82=E6=95=B0=20(#65)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/apisix/plugins/bk-auth-verify.lua | 18 +- .../plugins/bk-define/context-api-bkauth.lua | 14 +- src/apisix/plugins/bk-delete-sensitive.lua | 13 +- src/apisix/plugins/bk-stage-context.lua | 17 +- src/apisix/t/README.md | 2 +- src/apisix/t/bk-auth-validate.t | 52 +++ src/apisix/t/bk-auth-verify.t | 234 ++++++++++++ src/apisix/t/bk-delete-sensitive.t | 347 ++++++++++++++++++ src/apisix/t/bk-stage-context.t | 84 +++++ .../bk-define/test-context-api-bkauth.lua | 72 +++- src/apisix/tests/test-bk-auth-verify.lua | 102 ++++- src/apisix/tests/test-bk-delete-sensitive.lua | 82 +++-- src/apisix/tests/test-bk-stage-context.lua | 4 +- src/apisix/typing.lua | 2 +- 14 files changed, 975 insertions(+), 68 deletions(-) create mode 100644 src/apisix/t/bk-auth-validate.t create mode 100644 src/apisix/t/bk-auth-verify.t create mode 100644 src/apisix/t/bk-delete-sensitive.t create mode 100644 src/apisix/t/bk-stage-context.t diff --git a/src/apisix/plugins/bk-auth-verify.lua b/src/apisix/plugins/bk-auth-verify.lua index 3d06c73..d1f7927 100644 --- a/src/apisix/plugins/bk-auth-verify.lua +++ b/src/apisix/plugins/bk-auth-verify.lua @@ -118,11 +118,25 @@ local function get_auth_params_from_request(ctx, authorization_keys) if err ~= nil then return nil, err elseif auth_params ~= nil then + -- 记录认证参数位置，便于统计哪些请求将认证参数放到请求参数，推动优化 + ctx.var.auth_params_location = "header" return auth_params, nil end + if not ctx.var.bk_api_auth:allow_get_auth_params_from_parameters() then + -- 不允许从请求参数获取认证参数，直接返回 + return {}, nil + end + -- from the querystring and body - return get_auth_params_from_parameters(ctx, authorization_keys), nil + auth_params = get_auth_params_from_parameters(ctx, authorization_keys) + + if not pl_types.is_empty(auth_params) then + -- 记录认证参数位置，便于统计哪些请求将认证参数放到请求参数，推动优化 + ctx.var.auth_params_location = "params" + end + + return auth_params end -- utils end @@ -174,6 +188,8 @@ function _M.rewrite(conf, ctx) -- luacheck: no unused ctx.var.bk_user = user ctx.var.bk_app_code = app["app_code"] ctx.var.bk_username = user["username"] + -- 记录认证参数位置，便于统计哪些请求将认证参数放到请求参数，推动优化 + ctx.var.auth_params_location = ctx.var.auth_params_location or "" end if _TEST then -- luacheck: ignore diff --git a/src/apisix/plugins/bk-define/context-api-bkauth.lua b/src/apisix/plugins/bk-define/context-api-bkauth.lua index ce8d578..9387d80 100644 --- a/src/apisix/plugins/bk-define/context-api-bkauth.lua +++ b/src/apisix/plugins/bk-define/context-api-bkauth.lua @@ -112,6 +112,8 @@ function ContextApiBkAuth.new(bk_api_auth) api_type = bk_api_auth.api_type, unfiltered_sensitive_keys = bk_api_auth.unfiltered_sensitive_keys or {}, include_system_headers_mapping = include_system_headers_mapping, + allow_auth_from_params = bk_api_auth.allow_auth_from_params, + allow_delete_sensitive_params = bk_api_auth.allow_delete_sensitive_params, uin_conf = UinConf.new(bk_api_auth.uin_conf), rtx_conf = RtxConf.new(bk_api_auth.rtx_conf), user_conf = UserConf.new(bk_api_auth.user_conf), @@ -123,6 +125,13 @@ function ContextApiBkAuth.get_api_type(self) return self.api_type end +---Allow get auth_params from request parameters, such as querystring, body +---@return boolean +function ContextApiBkAuth.allow_get_auth_params_from_parameters(self) + -- 默认允许从参数获取认证信息 + return self.allow_auth_from_params ~= false +end + ---Get the unfiltered sensitive keys. ---@return table function ContextApiBkAuth.get_unfiltered_sensitive_keys(self) @@ -147,8 +156,9 @@ end ---Filter the sensitive params or not, do the filter if api_type is not ESB. ---@return boolean -function ContextApiBkAuth.is_filter_sensitive_params(self) - return self.api_type ~= ESB +function ContextApiBkAuth.should_delete_sensitive_params(self) + -- 非 esb，且允许删除敏感参数（默认允许删除） + return self.api_type ~= ESB and self.allow_delete_sensitive_params ~= false end function ContextApiBkAuth.is_user_type_uin(self) diff --git a/src/apisix/plugins/bk-delete-sensitive.lua b/src/apisix/plugins/bk-delete-sensitive.lua index b45df2b..8084bf1 100644 --- a/src/apisix/plugins/bk-delete-sensitive.lua +++ b/src/apisix/plugins/bk-delete-sensitive.lua @@ -34,6 +34,7 @@ local core = require("apisix.core") local bk_core = require("apisix.plugins.bk-core.init") local ngx = ngx -- luacheck: ignore local ipairs = ipairs +local tostring = tostring local plugin_name = "bk-delete-sensitive" @@ -55,7 +56,6 @@ function _M.check_schema(conf) return core.schema.check(schema, conf) end - ---Delete the sensitive parameters in the request header, uri args and body, ---it will check the first, then do the modification. ---@param ctx apisix.Context @@ -103,6 +103,15 @@ local function delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive ::continue:: end + if ctx.var.auth_params_location == "header" and (query_changed or form_changed or body_changed) then + core.log.warn( + "auth params are present in both header and request parameters, request_id: " .. + tostring(ctx.var.bk_request_id) + ) + -- 记录认证参数位置，便于统计哪些请求将认证参数放到请求参数，推动优化 + ctx.var.auth_params_location = "header_and_params" + end + if check_query and query_changed then core.request.set_uri_args(ctx, uri_args) end @@ -127,7 +136,7 @@ local function delete_sensitive_headers() end function _M.rewrite(conf, ctx) -- luacheck: no unused - if ctx.var.bk_api_auth and ctx.var.bk_api_auth:is_filter_sensitive_params() then + if ctx.var.bk_api_auth and ctx.var.bk_api_auth:should_delete_sensitive_params() then delete_sensitive_params( ctx, bk_core.config.get_sensitive_keys(), ctx.var.bk_api_auth:get_unfiltered_sensitive_keys() ) diff --git a/src/apisix/plugins/bk-stage-context.lua b/src/apisix/plugins/bk-stage-context.lua index 5d4ef21..59cc957 100644 --- a/src/apisix/plugins/bk-stage-context.lua +++ b/src/apisix/plugins/bk-stage-context.lua @@ -15,7 +15,6 @@ -- We undertake not to change the open source license (MIT license) applicable -- to the current version of the project delivered to anyone in the future. -- - local core = require("apisix.core") local bk_core = require("apisix.plugins.bk-core.init") local context_api_bkauth = require("apisix.plugins.bk-define.context-api-bkauth") @@ -63,6 +62,14 @@ local schema = { type = "string", }, }, + allow_auth_from_params = { + type = "boolean", + default = true, + }, + allow_delete_sensitive_params = { + type = "boolean", + default = true, + }, uin_conf = { type = "object", properties = { @@ -132,20 +139,17 @@ local _M = { schema = schema, } - ---@param jwt_private_key string: JWT private key (encoded in Base64) ----@return string|nil, string: The decoded JWT private key, or nil and error message if the decoding fails +---@return string|nil, string|nil: The decoded JWT private key, or nil and error message if the decoding fails local function decode_jwt_private_key(jwt_private_key) local decoded_private_key = ngx_decode_base64(jwt_private_key) if not decoded_private_key then core.log.error("failed to decode jwt_private_key with base64. jwt_private_key=", jwt_private_key) return nil, "failed to decode jwt_private_key with base64" end - return decoded_private_key + return decoded_private_key, nil end - - ---@param conf table: The user-provided configuration for the plugin instance ---@return boolean, string|nil: true and nil if the configuration is valid, false and error message if not function _M.check_schema(conf) @@ -165,7 +169,6 @@ function _M.check_schema(conf) return true end - ---@param conf table: The user-provided configuration for the plugin instance ---@param ctx api.Context: The request context function _M.rewrite(conf, ctx) diff --git a/src/apisix/t/README.md b/src/apisix/t/README.md index 9a9c2ab..4908d82 100644 --- a/src/apisix/t/README.md +++ b/src/apisix/t/README.md @@ -33,4 +33,4 @@ - [apisix source code : t](https://github.com/apache/apisix/tree/master/t) - [test-nginx](https://github.com/openresty/test-nginx) - [test-nginx doc: user guide](https://openresty.gitbooks.io/programming-openresty/content/testing/index.html) - +- [Test::Nginx::Socket](https://metacpan.org/pod/Test::Nginx::Socket) diff --git a/src/apisix/t/bk-auth-validate.t b/src/apisix/t/bk-auth-validate.t new file mode 100644 index 0000000..77f7fc4 --- /dev/null +++ b/src/apisix/t/bk-auth-validate.t @@ -0,0 +1,52 @@ +# +# TencentBlueKing is pleased to support the open source community by making +# 蓝鲸智云 - API 网关(BlueKing - APIGateway) available. +# Copyright (C) 2017 THL A29 Limited, a Tencent company. All rights reserved. +# Licensed under the MIT License (the "License"); you may not use this file except +# in compliance with the License. You may obtain a copy of the License at +# +# http://opensource.org/licenses/MIT +# +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +# either express or implied. See the License for the specific language governing permissions and +# limitations under the License. +# +# We undertake not to change the open source license (MIT license) applicable +# to the current version of the project delivered to anyone in the future. +# + +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_root_location(); +no_shuffle(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: sanity +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.bk-auth-validate") + local ok, err = plugin.check_schema({}) + if not ok then + ngx.say(err) + end + + ngx.say("done") + } + } +--- response_body +done diff --git a/src/apisix/t/bk-auth-verify.t b/src/apisix/t/bk-auth-verify.t new file mode 100644 index 0000000..1243d67 --- /dev/null +++ b/src/apisix/t/bk-auth-verify.t @@ -0,0 +1,234 @@ +# +# TencentBlueKing is pleased to support the open source community by making +# 蓝鲸智云 - API 网关(BlueKing - APIGateway) available. +# Copyright (C) 2017 THL A29 Limited, a Tencent company. All rights reserved. +# Licensed under the MIT License (the "License"); you may not use this file except +# in compliance with the License. You may obtain a copy of the License at +# +# http://opensource.org/licenses/MIT +# +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +# either express or implied. See the License for the specific language governing permissions and +# limitations under the License. +# +# We undertake not to change the open source license (MIT license) applicable +# to the current version of the project delivered to anyone in the future. +# + +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_root_location(); +no_shuffle(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: sanity +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.bk-auth-verify") + local ok, err = plugin.check_schema({}) + if not ok then + ngx.say(err) + end + + ngx.say("done") + } + } +--- response_body +done + +=== TEST 2: enable plugin to test auth_params_location, allow_auth_from_params=True +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "bk-stage-context": { + "bk_gateway_name": "demo", + "bk_stage_name": "prod", + "jwt_private_key": "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + "bk_api_auth": { + "api_type": 10, + "allow_auth_from_params": true, + "unfiltered_sensitive_keys": {} + } + }, + "bk-resource-context": { + "bk_resource_name": "echo", + "bk_resource_auth": { + "verified_app_required": false, + "verified_user_required": false, + "resource_perm_required": false, + "skip_user_verification": true + } + }, + "bk-auth-verify": {}, + "file-logger": { + "path": "file.log", + "log_format": { + "auth_location": "$auth_params_location" + } + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/hello" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + +=== TEST 3: auth_params_location maybe header, params +--- config + location /t { + content_by_lua_block { + os.remove("file.log") + + local core = require("apisix.core") + local t = require("lib.test_admin").test + -- auth-params in header + t("/hello", ngx.HTTP_GET, nil, nil, { + ["x-bkapi-authorization"] = '{"bk_app_code": "demo"}' + }) + -- auth-params in params + t("/hello?bk_app_secret=secret", ngx.HTTP_GET) + + local fd, err = io.open("file.log", "r") + + if not fd then + core.log.error("failed to open file: file.log, error info: ", err) + return + end + + local line1 = fd:read() + local line2 = fd:read() + + local new_line1 = core.json.decode(line1) + local new_line2 = core.json.decode(line2) + ngx.say(new_line1.auth_location) + ngx.say(new_line2.auth_location) + } + } +--- response_body +header +params + + +=== TEST 4: enable plugin to test auth_params_location, allow_auth_from_params=False +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "bk-stage-context": { + "bk_gateway_name": "demo", + "bk_stage_name": "prod", + "jwt_private_key": "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + "bk_api_auth": { + "api_type": 10, + "allow_auth_from_params": false, + "unfiltered_sensitive_keys": {} + } + }, + "bk-resource-context": { + "bk_resource_name": "echo", + "bk_resource_auth": { + "verified_app_required": false, + "verified_user_required": false, + "resource_perm_required": false, + "skip_user_verification": true + } + }, + "bk-auth-verify": {}, + "file-logger": { + "path": "file.log", + "log_format": { + "auth_location": "$auth_params_location" + } + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/hello" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + +=== TEST 5: auth_params_location maybe header +--- config + location /t { + content_by_lua_block { + os.remove("file.log") + + local core = require("apisix.core") + local t = require("lib.test_admin").test + -- auth-params in header + t("/hello", ngx.HTTP_GET, nil, nil, { + ["x-bkapi-authorization"] = '{"bk_app_code": "demo"}' + }) + -- auth-params in params + t("/hello?bk_app_secret=secret", ngx.HTTP_GET) + + local fd, err = io.open("file.log", "r") + + if not fd then + core.log.error("failed to open file: file.log, error info: ", err) + return + end + + local line1 = fd:read() + local line2 = fd:read() + + local new_line1 = core.json.decode(line1) + local new_line2 = core.json.decode(line2) + ngx.say(new_line1.auth_location) + -- it's empty string, add prefix for test + ngx.say("foo" .. new_line2.auth_location) + } + } +--- response_body +header +foo diff --git a/src/apisix/t/bk-delete-sensitive.t b/src/apisix/t/bk-delete-sensitive.t new file mode 100644 index 0000000..873c70d --- /dev/null +++ b/src/apisix/t/bk-delete-sensitive.t @@ -0,0 +1,347 @@ +# +# TencentBlueKing is pleased to support the open source community by making +# 蓝鲸智云 - API 网关(BlueKing - APIGateway) available. +# Copyright (C) 2017 THL A29 Limited, a Tencent company. All rights reserved. +# Licensed under the MIT License (the "License"); you may not use this file except +# in compliance with the License. You may obtain a copy of the License at +# +# http://opensource.org/licenses/MIT +# +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +# either express or implied. See the License for the specific language governing permissions and +# limitations under the License. +# +# We undertake not to change the open source license (MIT license) applicable +# to the current version of the project delivered to anyone in the future. +# + +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_root_location(); +no_shuffle(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: sanity +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.bk-delete-sensitive") + local ok, err = plugin.check_schema({}) + if not ok then + ngx.say(err) + end + + ngx.say("done") + } + } +--- response_body +done + +=== TEST 2: enable bk-delete-sensitive plugin using admin api +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "bk-stage-context": { + "bk_gateway_name": "demo", + "bk_stage_name": "prod", + "jwt_private_key": "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + "bk_api_auth": { + "api_type": 10, + "unfiltered_sensitive_keys": {} + } + }, + "bk-delete-sensitive": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/echo" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + +=== TEST 3: delete header x-bkapi-authorization +--- request +GET /echo +--- more_headers +x-bkapi-authorization: {"bk_app_code": "demo", "bk_app_secret": "secret"} +foo: bar +--- response_headers +!x-bkapi-authorization +foo: bar + +=== TEST 4: delete sensitive params +--- request +POST /echo +{"bk_app_code": "demo", "bk_app_secret": "secret", "access_token": "token"} +--- response_body chomp +{"bk_app_code":"demo"} + +=== TEST 5: enable plugin, api_type=0, will not delete sensitive params +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "bk-stage-context": { + "bk_gateway_name": "demo", + "bk_stage_name": "prod", + "jwt_private_key": "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + "bk_api_auth": { + "api_type": 0, + "unfiltered_sensitive_keys": {} + } + }, + "bk-delete-sensitive": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/echo" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + +=== TEST 6: api_type=0, will not delete sensitive params +--- request +POST /echo +{"bk_app_code":"demo", "bk_app_secret":"secret"} +--- response_body chomp +{"bk_app_code":"demo", "bk_app_secret":"secret"} + +=== TEST 7: enable plugin, api_type=10 and unfiltered_sensitive_keys exist, will not delete sensitive params +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "bk-stage-context": { + "bk_gateway_name": "demo", + "bk_stage_name": "prod", + "jwt_private_key": "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + "bk_api_auth": { + "api_type": 10, + "unfiltered_sensitive_keys": ["access_token"] + } + }, + "bk-delete-sensitive": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/echo" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + +=== TEST 8: unfiltered_sensitive_keys exist, will not delete sensitive params +--- request +POST /echo +{"access_token":"token"} +--- response_body chomp +{"access_token":"token"} + +=== TEST 9: enable plugin, allow_delete_sensitive_params=false, will not delete sensitive params +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "bk-stage-context": { + "bk_gateway_name": "demo", + "bk_stage_name": "prod", + "jwt_private_key": "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + "bk_api_auth": { + "api_type": 10, + "allow_delete_sensitive_params": false, + "unfiltered_sensitive_keys": {} + } + }, + "bk-delete-sensitive": {} + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/echo" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + +=== TEST 10: allow_delete_sensitive_params=false, will not delete sensitive params +--- request +POST /echo +{"bk_app_code":"demo", "bk_app_secret":"secret", "access_token":"token"} +--- response_body chomp +{"bk_app_code":"demo", "bk_app_secret":"secret", "access_token":"token"} + + +=== TEST 11: enable plugin to test auth_params_location +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "bk-stage-context": { + "bk_gateway_name": "demo", + "bk_stage_name": "prod", + "jwt_private_key": "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + "bk_api_auth": { + "api_type": 10, + "allow_delete_sensitive_params": true, + "unfiltered_sensitive_keys": {} + } + }, + "bk-resource-context": { + "bk_resource_name": "echo", + "bk_resource_auth": { + "verified_app_required": false, + "verified_user_required": false, + "resource_perm_required": false, + "skip_user_verification": true + } + }, + "bk-auth-verify": {}, + "bk-delete-sensitive": {}, + "file-logger": { + "path": "file.log", + "log_format": { + "auth_location": "$auth_params_location" + } + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uri": "/hello" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + +=== TEST 12: auth_params_location maybe changed to header_and_params if sensitive params exist in request params +--- config + location /t { + content_by_lua_block { + os.remove("file.log") + + local core = require("apisix.core") + local t = require("lib.test_admin").test + -- auth-params in header, and sensitive params exist in request params + t("/hello?bk_app_code=demo&bk_app_secret=secret", ngx.HTTP_GET, nil, nil, { + ["x-bkapi-authorization"] = '{"bk_app_code": "demo"}' + }) + -- auth-params in header, but sensitive params do not exist in request params + t("/hello?bk_app_code=demo", ngx.HTTP_GET, nil, nil, { + ["x-bkapi-authorization"] = '{"bk_app_code": "demo"}' + }) + -- auth-params only in params + t("/hello?bk_app_secret=secret", ngx.HTTP_GET) + + local fd, err = io.open("file.log", "r") + + if not fd then + core.log.error("failed to open file: file.log, error info: ", err) + return + end + + local line1 = fd:read() + local line2 = fd:read() + local line3 = fd:read() + + local new_line1 = core.json.decode(line1) + local new_line2 = core.json.decode(line2) + local new_line3 = core.json.decode(line3) + ngx.say(new_line1.auth_location) + ngx.say(new_line2.auth_location) + ngx.say(new_line3.auth_location) + } + } +--- response_body +header_and_params +header +params diff --git a/src/apisix/t/bk-stage-context.t b/src/apisix/t/bk-stage-context.t new file mode 100644 index 0000000..5ef3f34 --- /dev/null +++ b/src/apisix/t/bk-stage-context.t @@ -0,0 +1,84 @@ +# +# TencentBlueKing is pleased to support the open source community by making +# 蓝鲸智云 - API 网关(BlueKing - APIGateway) available. +# Copyright (C) 2017 THL A29 Limited, a Tencent company. All rights reserved. +# Licensed under the MIT License (the "License"); you may not use this file except +# in compliance with the License. You may obtain a copy of the License at +# +# http://opensource.org/licenses/MIT +# +# Unless required by applicable law or agreed to in writing, software distributed under +# the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, +# either express or implied. See the License for the specific language governing permissions and +# limitations under the License. +# +# We undertake not to change the open source license (MIT license) applicable +# to the current version of the project delivered to anyone in the future. +# + +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_shuffle(); +no_root_location(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests; + +__DATA__ + +=== TEST 1: sanity +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.bk-stage-context") + local ok, err = plugin.check_schema({ + bk_gateway_name = "demo", + bk_stage_name = "prod", + jwt_private_key = "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + bk_api_auth = { + api_type = 10 + } + }) + if not ok then + ngx.say(err) + end + + ngx.say("done") + } + } +--- response_body +done + +=== TEST 2: with allow_auth_from_params/allow_delete_sensitive_params +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.bk-stage-context") + local ok, err = plugin.check_schema({ + bk_gateway_name = "demo", + bk_stage_name = "prod", + jwt_private_key = "dGhpcyBpcyBhIGZha2Ugand0IHByaXZhdGUga2V5", + bk_api_auth = { + api_type = 10, + allow_auth_from_params = false, + allow_delete_sensitive_params = false, + } + }) + if not ok then + ngx.say(err) + end + + ngx.say("done") + } + } +--- response_body +done diff --git a/src/apisix/tests/bk-define/test-context-api-bkauth.lua b/src/apisix/tests/bk-define/test-context-api-bkauth.lua index 6db7abf..0ff4b50 100644 --- a/src/apisix/tests/bk-define/test-context-api-bkauth.lua +++ b/src/apisix/tests/bk-define/test-context-api-bkauth.lua @@ -15,7 +15,6 @@ -- We undertake not to change the open source license (MIT license) applicable -- to the current version of the project delivered to anyone in the future. -- - local context_api_bkauth = require("apisix.plugins.bk-define.context-api-bkauth") describe( @@ -151,6 +150,19 @@ describe( end ) + it( + "allow_get_auth_params_from_parameters", function() + bk_api_auth.allow_auth_from_params = nil + assert.is_true(bk_api_auth:allow_get_auth_params_from_parameters()) + + bk_api_auth.allow_auth_from_params = true + assert.is_true(bk_api_auth:allow_get_auth_params_from_parameters()) + + bk_api_auth.allow_auth_from_params = false + assert.is_false(bk_api_auth:allow_get_auth_params_from_parameters()) + end + ) + it( "get_uin_conf", function() assert.is_same( @@ -202,11 +214,59 @@ describe( ) it( - "is_filter_sensitive_params", function() - assert.is_true(bk_api_auth:is_filter_sensitive_params()) - - bk_api_auth.api_type = 0 - assert.is_false(bk_api_auth:is_filter_sensitive_params()) + "should_delete_sensitive_params", function() + local data = { + -- esb + { + params = { + api_type = 0, + allow_delete_sensitive_params = nil, + }, + expected = false, + }, + { + params = { + api_type = 0, + allow_delete_sensitive_params = true, + }, + expected = false, + }, + { + params = { + api_type = 0, + allow_delete_sensitive_params = false, + }, + expected = false, + }, + -- normal + { + params = { + api_type = 10, + allow_delete_sensitive_params = nil, + }, + expected = true, + }, + { + params = { + api_type = 10, + allow_delete_sensitive_params = true, + }, + expected = true, + }, + { + params = { + api_type = 10, + allow_delete_sensitive_params = false, + }, + expected = false, + }, + } + for _, item in ipairs(data) do + bk_api_auth.api_type = item.params.api_type + bk_api_auth.allow_delete_sensitive_params = item.params.allow_delete_sensitive_params + + assert.is_equal(bk_api_auth:should_delete_sensitive_params(), item.expected) + end end ) diff --git a/src/apisix/tests/test-bk-auth-verify.lua b/src/apisix/tests/test-bk-auth-verify.lua index 2754d81..d6a756d 100644 --- a/src/apisix/tests/test-bk-auth-verify.lua +++ b/src/apisix/tests/test-bk-auth-verify.lua @@ -15,7 +15,6 @@ -- We undertake not to change the open source license (MIT license) applicable -- to the current version of the project delivered to anyone in the future. -- - local core = require("apisix.core") local bk_core = require("apisix.plugins.bk-core.init") local bk_auth_verify_init = require("apisix.plugins.bk-auth-verify.init") @@ -38,6 +37,7 @@ describe( local json_body local form_data local multipart_form_data + local ctx before_each( function() @@ -46,6 +46,9 @@ describe( json_body = nil form_data = nil multipart_form_data = nil + ctx = { + var = {}, + } stub( ngx.req, "get_method", function() @@ -261,6 +264,7 @@ describe( "get_auth_params_from_request", function() it( "from header", function() + ctx.var.bk_api_auth = context_api_bkauth.new({}) stub( core.request, "header", core.json.encode( { @@ -270,7 +274,7 @@ describe( ) ) - local auth_params = plugin._get_auth_params_from_request({}, authorization_keys) + local auth_params, err = plugin._get_auth_params_from_request(ctx, authorization_keys) assert.is_same( auth_params, { bk_app_code = "my-app", @@ -278,25 +282,83 @@ describe( a = "b", } ) + assert.is_nil(err) + assert.is_same(ctx.var.auth_params_location, "header") core.request.header:revert() end ) it( - "from parameters", function() + "from header with error", function() + ctx.var.bk_api_auth = context_api_bkauth.new({}) + stub(core.request, "header", "no-valid-json") + + local auth_params, err = plugin._get_auth_params_from_request(ctx, authorization_keys) + assert.is_nil(auth_params) + assert.is_not_nil(err) + assert.is_nil(ctx.var.auth_params_location) + + core.request.header:revert() + end + ) + + it( + "from parameters with auth params", function() + ctx.var.bk_api_auth = context_api_bkauth.new({}) stub( core.request, "get_uri_args", { bk_app_code = "my-app", a = "b", } ) - local auth_params = plugin._get_auth_params_from_request({}, authorization_keys) + local auth_params, err = plugin._get_auth_params_from_request(ctx, authorization_keys) assert.is_same( auth_params, { bk_app_code = "my-app", } ) + assert.is_nil(err) + assert.is_same(ctx.var.auth_params_location, "params") + + core.request.get_uri_args:revert() + end + ) + + it( + "from parameters without auth params", function() + ctx.var.bk_api_auth = context_api_bkauth.new({}) + stub( + core.request, "get_uri_args", { + a = "b", + } + ) + local auth_params, err = plugin._get_auth_params_from_request(ctx, authorization_keys) + assert.is_same(auth_params, {}) + assert.is_nil(err) + assert.is_nil(ctx.var.auth_params_location) + + core.request.get_uri_args:revert() + end + ) + + it( + "do not allow get auth params from parameters", function() + ctx.var.bk_api_auth = context_api_bkauth.new( + { + allow_auth_from_params = false, + } + ) + stub( + core.request, "get_uri_args", { + bk_app_code = "my-app", + a = "b", + } + ) + local auth_params, err = plugin._get_auth_params_from_request(ctx, authorization_keys) + assert.is_same(auth_params, {}) + assert.is_nil(err) + assert.is_nil(ctx.var.auth_params_location) core.request.get_uri_args:revert() end @@ -304,7 +366,8 @@ describe( it( "provide no valid inputs", function() - local auth_params, err = plugin._get_auth_params_from_request({}, authorization_keys) + ctx.var.bk_api_auth = context_api_bkauth.new({}) + local auth_params, err = plugin._get_auth_params_from_request(ctx, authorization_keys) assert.are_same(auth_params, {}) assert.is_nil(err) end @@ -319,12 +382,14 @@ describe( it( "with no auth params", function() - local app, user = plugin.verify({ - var = { - bk_api_auth = bk_api_auth, - bk_resource_auth = bk_resource_auth, - }, - }) + local app, user = plugin.verify( + { + var = { + bk_api_auth = bk_api_auth, + bk_resource_auth = bk_resource_auth, + }, + } + ) assert.is_equal(app.app_code, "") assert.is_false(app.verified) -- The legacy verifier was triggered by default. @@ -345,12 +410,14 @@ describe( "with invalid JSON auth_params in header", function() stub(core.request, "header", "not valid json") - local app, user = plugin.verify({ - var = { - bk_api_auth = bk_api_auth, - bk_resource_auth = bk_resource_auth, - }, - }) + local app, user = plugin.verify( + { + var = { + bk_api_auth = bk_api_auth, + bk_resource_auth = bk_resource_auth, + }, + } + ) assert.is_equal(app.app_code, "") assert.is_false(app.verified) assert.is_equal( @@ -504,6 +571,7 @@ describe( ) assert.is_same(ctx.var.bk_app_code, "my-app") assert.is_same(ctx.var.bk_username, "admin") + assert.is_same(ctx.var.auth_params_location, "") plugin.verify:revert() end diff --git a/src/apisix/tests/test-bk-delete-sensitive.lua b/src/apisix/tests/test-bk-delete-sensitive.lua index 4c42ea6..f4409ac 100644 --- a/src/apisix/tests/test-bk-delete-sensitive.lua +++ b/src/apisix/tests/test-bk-delete-sensitive.lua @@ -15,7 +15,6 @@ -- We undertake not to change the open source license (MIT license) applicable -- to the current version of the project delivered to anyone in the future. -- - local core = require("apisix.core") local bk_core = require("apisix.plugins.bk-core.init") local context_api_bkauth = require("apisix.plugins.bk-define.context-api-bkauth") @@ -34,10 +33,14 @@ describe( "access_token", } local unfiltered_sensitive_keys + local ctx before_each( function() unfiltered_sensitive_keys = {} + ctx = { + var = {}, + } stub(ngx.req, "clear_header") stub( @@ -83,9 +86,9 @@ describe( bk_app_secret = "fake-secret", foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_called_with( - {}, { + ctx, { foo = "bar", } ) @@ -98,7 +101,7 @@ describe( uri_args = { foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_not_called() assert.stub(ngx.req.set_body_data).was_not_called() end @@ -110,7 +113,7 @@ describe( bk_app_secret = "fake-secret", foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_not_called() assert.stub(ngx.req.set_body_data).was_called_with("foo=bar") end @@ -122,7 +125,7 @@ describe( a = "b", foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_not_called() assert.stub(ngx.req.set_body_data).was_not_called() end @@ -134,7 +137,7 @@ describe( bk_app_secret = "fake-secret", foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_not_called() assert.stub(ngx.req.set_body_data).was_called_with("{\"foo\":\"bar\"}") end @@ -145,7 +148,7 @@ describe( json_body = { foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_not_called() assert.stub(ngx.req.set_body_data).was_not_called() end @@ -163,9 +166,9 @@ describe( access_token = "fake-token", foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_called_with( - {}, { + ctx, { foo = "bar", } ) @@ -185,9 +188,9 @@ describe( access_token = "fake-token", foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_called_with( - {}, { + ctx, { foo = "bar", } ) @@ -205,15 +208,36 @@ describe( access_token = "fake-token", foo = "bar", } - plugin._delete_sensitive_params({}, sensitive_keys, unfiltered_sensitive_keys) + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) assert.stub(core.request.set_uri_args).was_called_with( - {}, { + ctx, { bk_app_secret = "fake-secret", foo = "bar", } ) end ) + + it( + "auth_params_location", function() + ctx.var.auth_params_location = "header" + + uri_args = { + bk_app_secret = "fake-secret", + foo = "bar", + } + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) + assert.is_same(ctx.var.auth_params_location, "header_and_params") + + ctx.var.auth_params_location = "" + uri_args = { + bk_app_secret = "fake-secret", + foo = "bar", + } + plugin._delete_sensitive_params(ctx, sensitive_keys, unfiltered_sensitive_keys) + assert.is_same(ctx.var.auth_params_location, "") + end + ) end ) @@ -232,16 +256,6 @@ describe( context( "rewrite", function() - local ctx - - before_each( - function() - ctx = { - var = {}, - } - end - ) - it( "delete sensitive params", function() ctx.var.bk_api_auth = context_api_bkauth.new( @@ -266,18 +280,28 @@ describe( it( "does not need to delete sensitive params", function() - ctx.var.bk_api_auth = context_api_bkauth.new( - { - api_type = 0, - } - ) uri_args = { bk_app_secret = "fake-secret", foo = "bar", } + -- esb + ctx.var.bk_api_auth = context_api_bkauth.new( + { + api_type = 0, + } + ) plugin.rewrite({}, ctx) + assert.stub(core.request.set_uri_args).was_not_called() + -- allow_delete_sensitive_params = false + ctx.var.bk_api_auth = context_api_bkauth.new( + { + api_type = 10, + allow_delete_sensitive_params = false, + } + ) + plugin.rewrite({}, ctx) assert.stub(core.request.set_uri_args).was_not_called() end ) diff --git a/src/apisix/tests/test-bk-stage-context.lua b/src/apisix/tests/test-bk-stage-context.lua index e467454..d5463dd 100644 --- a/src/apisix/tests/test-bk-stage-context.lua +++ b/src/apisix/tests/test-bk-stage-context.lua @@ -15,8 +15,6 @@ -- We undertake not to change the open source license (MIT license) applicable -- to the current version of the project delivered to anyone in the future. -- - -local core = require("apisix.core") local plugin = require("apisix.plugins.bk-stage-context") describe( @@ -102,6 +100,8 @@ describe( assert.is_equal(ctx.var.jwt_private_key, "test") assert.is_equal(ctx.var.bk_resource_name, ctx.route_name) assert.is_equal(ctx.var.bk_service_name, ctx.service_name) + assert.is_true(ctx.var.bk_api_auth.allow_auth_from_params) + assert.is_true(ctx.var.bk_api_auth.allow_delete_sensitive_params) end ) end diff --git a/src/apisix/typing.lua b/src/apisix/typing.lua index b6664b3..08c3e25 100644 --- a/src/apisix/typing.lua +++ b/src/apisix/typing.lua @@ -15,7 +15,6 @@ -- We undertake not to change the open source license (MIT license) applicable -- to the current version of the project delivered to anyone in the future. -- - ---@class nginx.ContextVar nginx 内建变量 --- Embedded Variables --- see https://nginx.org/en/docs/http/ngx_http_core_module.html#variables @@ -155,6 +154,7 @@ ---@field upstream_uri_without_args string 后端 uri ---@field bk_apigw_error table 网关返回的错误信息 ---@field bk_concurrency_limit_key string 并发限制 key; 仅在 bk-concurrency-limit 中使用 +---@field auth_params_location string 认证参数位置 --- ---@class bk_gateway.LogContextVar 日志相关的上下文变量，仅在 log 阶段可用 ---@field bk_log_request_timestamp integer|nil 日志请求秒级时间戳 From a71cfd9aa16914774d036221480ffd6014f6dce3 Mon Sep 17 00:00:00 2001 From: wklken Date: Thu, 30 Nov 2023 15:13:09 +0800 Subject: [PATCH 6/6] fix(bk-auth-verify): add app_code/app_secret length check (#70) --- .../bk-auth-verify/app-account-verifier.lua | 9 ++++++ .../test-app-account-verifier.lua | 30 +++++++++++++++++++ ..._use_encoded_uri_for_radixtree_match.patch | 14 +++++++++ 3 files changed, 53 insertions(+) create mode 100644 src/build/patches/006_use_encoded_uri_for_radixtree_match.patch diff --git a/src/apisix/plugins/bk-auth-verify/app-account-verifier.lua b/src/apisix/plugins/bk-auth-verify/app-account-verifier.lua index a456613..7780eae 100644 --- a/src/apisix/plugins/bk-auth-verify/app-account-verifier.lua +++ b/src/apisix/plugins/bk-auth-verify/app-account-verifier.lua @@ -20,6 +20,7 @@ local app_account_utils = require("apisix.plugins.bk-auth-verify.app-account-uti local bk_app_define = require("apisix.plugins.bk-define.app") local bk_cache = require("apisix.plugins.bk-cache.init") local setmetatable = setmetatable +local string = string local _M = {} @@ -46,6 +47,14 @@ function _M.verify_app(self) end if not pl_types.is_empty(self.app_secret) then + -- check the length before call bkauth apis + if string.len(self.app_code) > 32 then + return bk_app_define.new_anonymous_app("app code cannot be longer than 32 characters") + end + if string.len(self.app_secret) > 128 then + return bk_app_define.new_anonymous_app("app secret cannot be longer than 128 characters") + end + return self:verify_by_app_secret() end diff --git a/src/apisix/tests/bk-auth-verify/test-app-account-verifier.lua b/src/apisix/tests/bk-auth-verify/test-app-account-verifier.lua index 9ed25f9..069210c 100644 --- a/src/apisix/tests/bk-auth-verify/test-app-account-verifier.lua +++ b/src/apisix/tests/bk-auth-verify/test-app-account-verifier.lua @@ -83,6 +83,36 @@ describe( end ) + it( + "app_code length is greather 32", function() + local auth_params = auth_params_mod.new({ + bk_app_code = "123456789012345678901234567890123", + bk_app_secret = "world", + }) + local verifier = app_account_verifier_mod.new(auth_params) + + local app = verifier:verify_app() + assert.is_equal(app.app_code, "") + assert.is_false(app.verified) + assert.is_equal(app.valid_error_message, "app code cannot be longer than 32 characters") + end + ) + + it( + "app_secret length is greather 128", function() + local auth_params = auth_params_mod.new({ + bk_app_code = "hello", + bk_app_secret = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + }) + local verifier = app_account_verifier_mod.new(auth_params) + + local app = verifier:verify_app() + assert.is_equal(app.app_code, "") + assert.is_false(app.verified) + assert.is_equal(app.valid_error_message, "app secret cannot be longer than 128 characters") + end + ) + it( "app secret is not empty", function() auth_params = auth_params_mod.new( diff --git a/src/build/patches/006_use_encoded_uri_for_radixtree_match.patch b/src/build/patches/006_use_encoded_uri_for_radixtree_match.patch new file mode 100644 index 0000000..2400dd2 --- /dev/null +++ b/src/build/patches/006_use_encoded_uri_for_radixtree_match.patch @@ -0,0 +1,14 @@ +diff --git a/apisix/http/route.lua b/apisix/http/route.lua +index d475646b..bc97ef82 100644 +--- a/apisix/http/route.lua ++++ b/apisix/http/route.lua +@@ -111,7 +111,8 @@ function _M.match_uri(uri_router, match_opts, api_ctx) + match_opts.vars = api_ctx.var + match_opts.matched = core.tablepool.fetch("matched_route_record", 0, 4) + +- local ok = uri_router:dispatch(api_ctx.var.uri, match_opts, api_ctx, match_opts) ++ local encoded_uri = core.utils.uri_safe_encode(api_ctx.var.uri) ++ local ok = uri_router:dispatch(encoded_uri, match_opts, api_ctx, match_opts) + return ok + end +