Skip to content
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

fix(hmac-auth): hmac-auth plugin sort array param #6314

Merged
merged 4 commits into from
Feb 16, 2022

Conversation

kevinw66
Copy link
Contributor

What this PR does / why we need it:

fixes #6313

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

@@ -244,7 +244,13 @@ local function generate_signature(ctx, secret_key, params)

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

Choose a reason for hiding this comment

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

i wonder that why not use table.new

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just keep the same code style like table keys

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

@starsz starsz left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@bisakhmondal bisakhmondal left a comment

Choose a reason for hiding this comment

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

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:

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
-- 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
Copy link
Contributor Author

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
Copy link
Contributor Author

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:

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

-- 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
Copy link
Contributor Author

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:

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

-- 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
Copy link
Member

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:

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

-- 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.

for _, val in pairs(param) do
core.table.insert(vals, val)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@spacewander spacewander merged commit f37fc0e into apache:master Feb 16, 2022
@bisakhmondal
Copy link
Member

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
Copy link
Member

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

@kevinw66 kevinw66 deleted the issue-6313 branch February 18, 2022 00:12
hongbinhsu pushed a commit to fitphp/apix that referenced this pull request Feb 23, 2022
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: hmac-auth plugin will not sort array value in param
6 participants