fix(hmac-auth): hmac-auth plugin sort array param by kevinw66 · Pull Request #6314 · apache/apisix

kevinw66 · 2022-02-14T06:38:42Z

What this PR does / why we need it:

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

tylitianrui · 2022-02-14T09:49:51Z


            -- whether to encode the uri parameters
            if type(param) == "table" then
+                local vals = {}


i wonder that why not use table.new

Just keep the same code style like table keys

Not necessary to keep old code style here. We can specify the initialization size when we create a new table by using core.table.new(narray, nhash)

starsz

Hi, @kevinw66.Can you add a test case to cover it.

bisakhmondal

Hi! Thanks for detecting the issue and the PR.
After looking into the codebase, I find the way of forming the canonical query string unnecessarily complex yet it's not accurate.
I would go with a simpler approach rather than pathing this up.

We can replace:

apisix/apisix/plugins/hmac-auth.lua

Lines 219 to 255 in 605141c

    
           if type(args) == "table" then 
        
               local keys = {} 
        
               local query_tab = {} 
        
               for k, v in pairs(args) do 
        
                   core.table.insert(keys, k) 
        
               end 
        
               core.table.sort(keys) 
        
               local field_val = get_conf_field(params.access_key, "encode_uri_params") 
        
               core.log.info("encode_uri_params: ", field_val) 
        
               local encode_or_not = do_nothing 
        
               if field_val then 
        
                   encode_or_not = escape_uri 
        
               end 
        
               for _, key in pairs(keys) do 
        
                   local param = args[key] 
        
                   -- when args without `=<value>`, value is treated as true. 
        
                   -- In order to be compatible with args lacking `=<value>`, 
        
                   -- we need to replace true with an empty string. 
        
                   if type(param) == "boolean" then 
        
                       param = "" 
        
                   end 
        
                   -- whether to encode the uri parameters 
        
                   if type(param) == "table" then 
        
                       for _, val in pairs(param) do 
        
                           core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(val)) 
        
                       end 
        
                   else 
        
                       core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(param)) 
        
                   end 
        
               end 
        
               canonical_query_string = core.table.concat(query_tab, "&") 
        
           end

with

apisix/apisix/plugins/aws-lambda.lua

Lines 123 to 130 in 605141c

    
           -- computing canonical query string 
        
           local canonical_qs = {} 
        
           for k, v in pairs(params.query) do 
        
               canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(v) 
        
           end 
        
           tab_sort(canonical_qs) 
        
           canonical_qs = tab_concat(canonical_qs, "&")

It's very simple, yet effective.
cc @spacewander to share your views.

kevinw66 · 2022-02-15T01:29:09Z

Hi, @kevinw66.Can you add a test case to cover it.

Can you teach me how to run unit tests in my local machine ? I left a comment on issue.
Thanks.

kevinw66 · 2022-02-15T01:50:26Z

Hi! Thanks for detecting the issue and the PR. After looking into the codebase, I find the way of forming the canonical query string unnecessarily complex yet it's not accurate. I would go with a simpler approach rather than pathing this up.

We can replace:

apisix/apisix/plugins/hmac-auth.lua

Lines 219 to 255 in 605141c

if type(args) == "table" then

local keys = {}

local query_tab = {}

for k, v in pairs(args) do

core.table.insert(keys, k)

end

core.table.sort(keys)

local field_val = get_conf_field(params.access_key, "encode_uri_params")

core.log.info("encode_uri_params: ", field_val)

local encode_or_not = do_nothing

if field_val then

encode_or_not = escape_uri

end

for _, key in pairs(keys) do

local param = args[key]

-- when args without `=<value>`, value is treated as true.

-- In order to be compatible with args lacking `=<value>`,

-- we need to replace true with an empty string.

if type(param) == "boolean" then

param = ""

end

-- whether to encode the uri parameters

if type(param) == "table" then

for _, val in pairs(param) do

core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(val))

end

else

core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(param))

end

end

canonical_query_string = core.table.concat(query_tab, "&")

end

with

apisix/apisix/plugins/aws-lambda.lua

Lines 123 to 130 in 605141c

-- computing canonical query string

local canonical_qs = {}

for k, v in pairs(params.query) do

canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(v)

end

tab_sort(canonical_qs)

canonical_qs = tab_concat(canonical_qs, "&")

It's very simple, yet effective.
cc @spacewander to share your views.

This may still need to check table param

for k, v in pairs(args) do
    if type(v) == "table" then
        for _, sv in pairs(v) do
            canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(sv)
        end
    else
        canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(v)
    end
end

kevinw66 · 2022-02-15T02:04:18Z

Hi! Thanks for detecting the issue and the PR. After looking into the codebase, I find the way of forming the canonical query string unnecessarily complex yet it's not accurate. I would go with a simpler approach rather than pathing this up.

We can replace:

apisix/apisix/plugins/hmac-auth.lua

Lines 219 to 255 in 605141c

if type(args) == "table" then

local keys = {}

local query_tab = {}

for k, v in pairs(args) do

core.table.insert(keys, k)

end

core.table.sort(keys)

local field_val = get_conf_field(params.access_key, "encode_uri_params")

core.log.info("encode_uri_params: ", field_val)

local encode_or_not = do_nothing

if field_val then

encode_or_not = escape_uri

end

for _, key in pairs(keys) do

local param = args[key]

-- when args without `=<value>`, value is treated as true.

-- In order to be compatible with args lacking `=<value>`,

-- we need to replace true with an empty string.

if type(param) == "boolean" then

param = ""

end

-- whether to encode the uri parameters

if type(param) == "table" then

for _, val in pairs(param) do

core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(val))

end

else

core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(param))

end

end

canonical_query_string = core.table.concat(query_tab, "&")

end

with

apisix/apisix/plugins/aws-lambda.lua

Lines 123 to 130 in 605141c

-- computing canonical query string

local canonical_qs = {}

for k, v in pairs(params.query) do

canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(v)

end

tab_sort(canonical_qs)

canonical_qs = tab_concat(canonical_qs, "&")

It's very simple, yet effective.
cc @spacewander to share your views.

This may not work correctly in some cases, in ASCII table, the '=' symbol is behind number sequence, so if I pass param like a1a=2&a1a=1&a=2, it will sort as a1a=1&a1a=2&a=2, but I think the result a=2&a1a=1&a1a=2 may correct.

spacewander · 2022-02-15T06:47:00Z

Hi! Thanks for detecting the issue and the PR. After looking into the codebase, I find the way of forming the canonical query string unnecessarily complex yet it's not accurate. I would go with a simpler approach rather than pathing this up.

We can replace:

apisix/apisix/plugins/hmac-auth.lua

Lines 219 to 255 in 605141c

if type(args) == "table" then

local keys = {}

local query_tab = {}

for k, v in pairs(args) do

core.table.insert(keys, k)

end

core.table.sort(keys)

local field_val = get_conf_field(params.access_key, "encode_uri_params")

core.log.info("encode_uri_params: ", field_val)

local encode_or_not = do_nothing

if field_val then

encode_or_not = escape_uri

end

for _, key in pairs(keys) do

local param = args[key]

-- when args without `=<value>`, value is treated as true.

-- In order to be compatible with args lacking `=<value>`,

-- we need to replace true with an empty string.

if type(param) == "boolean" then

param = ""

end

-- whether to encode the uri parameters

if type(param) == "table" then

for _, val in pairs(param) do

core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(val))

end

else

core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(param))

end

end

canonical_query_string = core.table.concat(query_tab, "&")

end

with

apisix/apisix/plugins/aws-lambda.lua

Lines 123 to 130 in 605141c

-- computing canonical query string

local canonical_qs = {}

for k, v in pairs(params.query) do

canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(v)

end

tab_sort(canonical_qs)

canonical_qs = tab_concat(canonical_qs, "&")

It's very simple, yet effective.
cc @spacewander to share your views.

Yes, the way in aws-lambda still needs to handle multiple values in the same query key.

spacewander · 2022-02-15T06:51:18Z

            if type(param) == "table" then
+                local vals = {}
                for _, val in pairs(param) do
+                    core.table.insert(vals, val)


Remember to convert boolean to '' before sorting. table.sort only supports sorting in the same type.

bisakhmondal · 2022-02-16T13:51:54Z

This may still need to check table param

for k, v in pairs(args) do
    if type(v) == "table" then
        for _, sv in pairs(v) do
            canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(sv)
        end
    else
        canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(v)
    end
end

Ohh, Thanks! I missed the scenario : )
Btw, in that case, something still looks odd! We are considering one extra level nesting in query params. Query params with nesting level > 2 would fail.

For example: (random example, taken from web)

main_key[other_key][main_array][0][name]=Bob&main_key[other_key][main_array][0][nickname]=bobby&main_key[other_key][main_array][1][name]=Tom&main_key[other_key][main_array][1][nickname]=Tommy

{
    "main_key": {
        "other_key": {
            "main_array": [{
                    "name": "Bob",
                    "nickname": "bobby"
                },
                {
                    "name": "Tom",
                    "nickname": "Tommy"
                }
            ]
        }
    }
}

I think we need to perform a recursive DFS! Else it will introduce some sort of indeterminism

🔥➜ ~ resty -e 'print(ngx.escape_uri({["a"]="b"}))' 

table%3A%200x7f5aa8abc608
🔥➜ ~ resty -e 'print(ngx.escape_uri({["a"]="b"}))'

table%3A%200x7f7eca005608

spacewander · 2022-02-16T14:05:31Z

AFAIK, the ngx.req.get_uri_args only returns two level table at most.

Multiple occurrences of an argument key will result in a table value holding all the values for that key in order.

https://github.com/openresty/lua-nginx-module#ngxreqget_uri_args

Here is the source: https://github.com/openresty/lua-resty-core/blob/97d2e5e08d9ddf328e33d2b5f1710a9ce3d8bbd0/lib/resty/core/request.lua#L237

* upstream: (52 commits) feat: add kubernetes discovery module (apache#4880) docs: fix For L7 proxy -> For L4 proxy (apache#6423) fix(deps): upgrade jsonschema to 0.9.8 (apache#6407) docs: translate Chinese to English in en clickhouse-logger (apache#6416) docs: add zh proxy-control.md&modify other doc error (apache#6346) docs: update public API relative usage (apache#6318) docs(cn): remove datadog from sidebar & fix doc lint conf (apache#6411) fix(request-validation): should not limit the urlencoded post args number (apache#6396) docs: fix configuration file typo (apache#6395) docs(extern-plugin): the implementation of runner (apache#6336) docs: polishing skywalking-logger plugin's docs (apache#6377) doc: adjust the directory structure of observability's documents (apache#6391) change(admin): empty nodes should be encoded as array (apache#6384) fix: should not limit the header number (apache#6379) ci: remove unnecessary tmate action (apache#6367) fix(opentelemetry): batch_span_processor export zero length spans (apache#6349) feat(graphql): support http get and post json request (apache#6343) feat: support for configuring the number of etcd health check retries (apache#6322) feat(wasm): support getting request body (apache#6325) fix(hmac-auth): hmac-auth plugin sort array param (apache#6314) ...

hmac-auth plugin sort array param

cb82fb8

tylitianrui reviewed Feb 14, 2022

View reviewed changes

starsz reviewed Feb 14, 2022

View reviewed changes

bisakhmondal reviewed Feb 14, 2022

View reviewed changes

spacewander reviewed Feb 15, 2022

View reviewed changes

wuzhiguo added 3 commits February 15, 2022 15:49

handle boolean value in array param

f2390f4

add testcase

6d1cdd3

remove extra ONLY comment in testcase

d55b932

spacewander approved these changes Feb 16, 2022

View reviewed changes

starsz approved these changes Feb 16, 2022

View reviewed changes

spacewander merged commit f37fc0e into apache:master Feb 16, 2022

kevinw66 deleted the issue-6313 branch February 18, 2022 00:12

spacewander added the need backport label Feb 18, 2022

	if type(args) == "table" then
	local keys = {}
	local query_tab = {}

	for k, v in pairs(args) do
	core.table.insert(keys, k)
	end
	core.table.sort(keys)

	local field_val = get_conf_field(params.access_key, "encode_uri_params")
	core.log.info("encode_uri_params: ", field_val)

	local encode_or_not = do_nothing
	if field_val then
	encode_or_not = escape_uri
	end

	for _, key in pairs(keys) do
	local param = args[key]
	-- when args without `=<value>`, value is treated as true.
	-- In order to be compatible with args lacking `=<value>`,
	-- we need to replace true with an empty string.
	if type(param) == "boolean" then
	param = ""
	end

	-- whether to encode the uri parameters
	if type(param) == "table" then
	for _, val in pairs(param) do
	core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(val))
	end
	else
	core.table.insert(query_tab, encode_or_not(key) .. "=" .. encode_or_not(param))
	end
	end
	canonical_query_string = core.table.concat(query_tab, "&")
	end

	-- computing canonical query string
	local canonical_qs = {}
	for k, v in pairs(params.query) do
	canonical_qs[#canonical_qs+1] = ngx.unescape_uri(k) .. "=" .. ngx.unescape_uri(v)
	end

	tab_sort(canonical_qs)
	canonical_qs = tab_concat(canonical_qs, "&")

Uh oh!

Conversation

kevinw66 commented Feb 14, 2022

What this PR does / why we need it:

Pre-submission checklist:

Uh oh!

tylitianrui Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

kevinw66 Feb 14, 2022

Choose a reason for hiding this comment

Uh oh!

tzssangglass Feb 16, 2022

Choose a reason for hiding this comment

Uh oh!

starsz left a comment

Choose a reason for hiding this comment

Uh oh!

bisakhmondal left a comment

Choose a reason for hiding this comment

Uh oh!

kevinw66 commented Feb 15, 2022

Uh oh!

kevinw66 commented Feb 15, 2022

Uh oh!

kevinw66 commented Feb 15, 2022

Uh oh!

spacewander commented Feb 15, 2022

Uh oh!

spacewander Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

kevinw66 Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

bisakhmondal commented Feb 16, 2022

Uh oh!

spacewander commented Feb 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants