You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/11/15 09:05:30 UTC
[GitHub] [apisix] Firstsawyou opened a new pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Firstsawyou opened a new pull request #2749:
URL: https://github.com/apache/apisix/pull/2749
fix #2710
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [ ] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2749:
URL: https://github.com/apache/apisix/pull/2749#discussion_r523929264
##########
File path: doc/zh-cn/plugins/hmac-auth.md
##########
@@ -44,6 +44,7 @@
| clock_skew | integer | 可选 | 0 | | 签名允许的时间偏移，以秒为单位的计时。比如允许时间偏移 10 秒钟，那么就应设置为 `10`。特别地，`0` 表示不对 `Date` 进行检查。 |
| signed_headers | array[string] | 可选 | | | 限制加入加密计算的 headers ，指定后客户端请求只能在此范围内指定 headers ，此项为空时将把所有客户端请求指定的 headers 加入加密计算。如： ["User-Agent", "Accept-Language", "x-custom-a"] |
| keep_headers | boolean | 可选 | false | [ true, false ] | 认证成功后的 http 请求中是否需要保留 `X-HMAC-SIGNATURE`、`X-HMAC-ALGORITHM` 和 `X-HMAC-SIGNED-HEADERS` 的请求头。true: 表示保留 http 请求头，false: 表示移除 http 请求头。 |
+| enable_encode | boolean | 可选 | true | [ true, false ] | 是否对签名中的 uri 参数进行转义,例如: `params1=hello%2Cworld` 进行了转义，`params2=hello,world` 没有进行转义。true: 表示对签名中的 uri 参数进行转义，false: 不对签名中的 uri 参数转义。 |
Review comment:
added.
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -74,6 +74,11 @@ local consumer_schema = {
type = "boolean",
title = "whether to keep the http request header",
default = false,
+ },
+ enable_encode = {
Review comment:
done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2749:
URL: https://github.com/apache/apisix/pull/2749#discussion_r523887304
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -194,14 +209,28 @@ local function generate_signature(ctx, secret_key, params)
end
core.table.sort(keys)
+ local field_val = get_conf_field(params.access_key, "enable_encode")
+ core.log.info("enable_encode: ", field_val)
+
for _, key in pairs(keys) do
local param = args[key]
- if type(param) == "table" then
- for _, val in pairs(param) do
- core.table.insert(query_tab, escape_uri(key) .. "=" .. escape_uri(val))
+ -- whether to escape the uri parameters
+ if field_val then
Review comment:
@spacewander
You mean we need to use a function like this: `local function encode_or_not(key) return key end` . To handle the situation where this option is disabled?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander merged pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Posted by GitBox <gi...@apache.org>.
spacewander merged pull request #2749:
URL: https://github.com/apache/apisix/pull/2749
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2749:
URL: https://github.com/apache/apisix/pull/2749#discussion_r524851784
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -194,14 +209,28 @@ local function generate_signature(ctx, secret_key, params)
end
core.table.sort(keys)
+ local field_val = get_conf_field(params.access_key, "enable_encode")
+ core.log.info("enable_encode: ", field_val)
+
for _, key in pairs(keys) do
local param = args[key]
- if type(param) == "table" then
- for _, val in pairs(param) do
- core.table.insert(query_tab, escape_uri(key) .. "=" .. escape_uri(val))
+ -- whether to escape the uri parameters
+ if field_val then
Review comment:
Actually I mean this change:
```diff
diff --git apisix/plugins/hmac-auth.lua apisix/plugins/hmac-auth.lua
index bc09d21..45cc0cc 100644
--- apisix/plugins/hmac-auth.lua
+++ apisix/plugins/hmac-auth.lua
@@ -190,6 +190,11 @@ local function get_conf_field(access_key, field_name)
end
+local function do_nothing(v)
+ return v
+end
+
+
local function generate_signature(ctx, secret_key, params)
local canonical_uri = ctx.var.uri
local canonical_query_string = ""
@@ -212,25 +217,22 @@ local function generate_signature(ctx, secret_key, params)
local encode_or_not = get_conf_field(params.access_key, "encode_uri_params")
core.log.info("encode_uri_params: ", encode_or_not)
+ local encode_uri_param = do_nothing
+ if encode_or_not then
+ encode_uri_param = escape_uri
+ end
+
for _, key in pairs(keys) do
local param = args[key]
-- whether to encode the uri parameters
- if encode_or_not then
- if type(param) == "table" then
- for _, val in pairs(param) do
- core.table.insert(query_tab, escape_uri(key) .. "=" .. escape_uri(val))
- end
- else
- core.table.insert(query_tab, escape_uri(key) .. "=" .. escape_uri(param))
+ if type(param) == "table" then
+ for _, val in pairs(param) do
+ core.table.insert(query_tab, encode_uri_param(key) .. "="
+ .. encode_uri_param(val))
end
else
- if type(param) == "table" then
- for _, val in pairs(param) do
- core.table.insert(query_tab, key .. "=" .. val)
- end
- else
- core.table.insert(query_tab, key .. "=" .. param)
- end
+ core.table.insert(query_tab, encode_uri_param(key) .. "="
+ .. encode_uri_param(param))
end
end
canonical_query_string = core.table.concat(query_tab, "&")
```
You can integrate it into your code.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2749:
URL: https://github.com/apache/apisix/pull/2749#discussion_r524843528
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -194,14 +209,28 @@ local function generate_signature(ctx, secret_key, params)
end
core.table.sort(keys)
+ local field_val = get_conf_field(params.access_key, "enable_encode")
+ core.log.info("enable_encode: ", field_val)
+
for _, key in pairs(keys) do
local param = args[key]
- if type(param) == "table" then
- for _, val in pairs(param) do
- core.table.insert(query_tab, escape_uri(key) .. "=" .. escape_uri(val))
+ -- whether to escape the uri parameters
+ if field_val then
Review comment:
@spacewander PTAL. I am not sure if the modification here is correct.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Firstsawyou commented on a change in pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Posted by GitBox <gi...@apache.org>.
Firstsawyou commented on a change in pull request #2749:
URL: https://github.com/apache/apisix/pull/2749#discussion_r524888967
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -194,14 +209,28 @@ local function generate_signature(ctx, secret_key, params)
end
core.table.sort(keys)
+ local field_val = get_conf_field(params.access_key, "enable_encode")
+ core.log.info("enable_encode: ", field_val)
+
for _, key in pairs(keys) do
local param = args[key]
- if type(param) == "table" then
- for _, val in pairs(param) do
- core.table.insert(query_tab, escape_uri(key) .. "=" .. escape_uri(val))
+ -- whether to escape the uri parameters
+ if field_val then
Review comment:
ok, thank you very much.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2749:
URL: https://github.com/apache/apisix/pull/2749#discussion_r523847260
##########
File path: doc/zh-cn/plugins/hmac-auth.md
##########
@@ -44,6 +44,7 @@
| clock_skew | integer | 可选 | 0 | | 签名允许的时间偏移，以秒为单位的计时。比如允许时间偏移 10 秒钟，那么就应设置为 `10`。特别地，`0` 表示不对 `Date` 进行检查。 |
| signed_headers | array[string] | 可选 | | | 限制加入加密计算的 headers ，指定后客户端请求只能在此范围内指定 headers ，此项为空时将把所有客户端请求指定的 headers 加入加密计算。如： ["User-Agent", "Accept-Language", "x-custom-a"] |
| keep_headers | boolean | 可选 | false | [ true, false ] | 认证成功后的 http 请求中是否需要保留 `X-HMAC-SIGNATURE`、`X-HMAC-ALGORITHM` 和 `X-HMAC-SIGNED-HEADERS` 的请求头。true: 表示保留 http 请求头，false: 表示移除 http 请求头。 |
+| enable_encode | boolean | 可选 | true | [ true, false ] | 是否对签名中的 uri 参数进行转义,例如: `params1=hello%2Cworld` 进行了转义，`params2=hello,world` 没有进行转义。true: 表示对签名中的 uri 参数进行转义，false: 不对签名中的 uri 参数转义。 |
Review comment:
Better to update the example in `测试插件`
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -74,6 +74,11 @@ local consumer_schema = {
type = "boolean",
title = "whether to keep the http request header",
default = false,
+ },
+ enable_encode = {
Review comment:
Better to use `encode_uri_param` so people can know what is encoded.
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -194,14 +209,28 @@ local function generate_signature(ctx, secret_key, params)
end
core.table.sort(keys)
+ local field_val = get_conf_field(params.access_key, "enable_encode")
+ core.log.info("enable_encode: ", field_val)
+
for _, key in pairs(keys) do
local param = args[key]
- if type(param) == "table" then
- for _, val in pairs(param) do
- core.table.insert(query_tab, escape_uri(key) .. "=" .. escape_uri(val))
+ -- whether to escape the uri parameters
+ if field_val then
Review comment:
Would be better if we use an empty function to escape the uri params when the option is disable. Like, `core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(val))`. When you do it, note that, please avoid creating a new function per request.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #2749: fix(hmac-auth): when the request contains escape characters, the signature verification fails
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #2749:
URL: https://github.com/apache/apisix/pull/2749#discussion_r523888632
##########
File path: apisix/plugins/hmac-auth.lua
##########
@@ -194,14 +209,28 @@ local function generate_signature(ctx, secret_key, params)
end
core.table.sort(keys)
+ local field_val = get_conf_field(params.access_key, "enable_encode")
+ core.log.info("enable_encode: ", field_val)
+
for _, key in pairs(keys) do
local param = args[key]
- if type(param) == "table" then
- for _, val in pairs(param) do
- core.table.insert(query_tab, escape_uri(key) .. "=" .. escape_uri(val))
+ -- whether to escape the uri parameters
+ if field_val then
Review comment:
`encode_or_not` is a variable, like that: `if xxx then encode_or_not = encode else encode_or_not = do_nothing`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org