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

[key-auth]: Query parameters truncated to first 100 when hide_credentials is set to true #13635

Closed
1 task done
dingjiayi opened this issue Sep 8, 2024 · 7 comments
Closed
1 task done
Labels
pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc...

Comments

@dingjiayi
Copy link
Contributor

dingjiayi commented Sep 8, 2024

Is there an existing issue for this?

  • I have searched the existing issues

Kong version ($ kong version)

3.7.1

Current Behavior

When I configure a route with the key-auth plugin and set the hide_credentials option to true, the upstream server only receives the first 100 query parameters. Any query parameters beyond the first 100 are missing.

Expected Behavior

When proxying, Kong should send all the request query arguments

Steps To Reproduce

  • Deploy and start Kong 3.7.1

  • Configure a route.

  • Apply the key-auth plugin to the route and set the hide_credentials option to true.

  • Make a request to the configured route that includes more than 100 query parameters.

#!/bin/bash

query_params=""

for i in {1..150}; do
    if [ $i -eq 1 ]; then
        query_params="k$i=v$i"
    else
        query_params="$query_params&k$i=v$i"
    fi
done

url="http://localhost:8000/echo" -- replace with your test route
apikey="xxx" -- replace with your own apikey

curl "$url?$query_params"  -H "apikey: $apikey"

Anything else?

Root Cause:

  1. The key-auth plugin retrieves the query arguments using kong.request.get_query().
    local query = kong.request.get_query()
  2. In the if conf.hide_credentials branch, it removes the apikey from the query_args and then calls kong.service.request.set_query(query) to set the modified query arguments.
    kong.service.request.set_query(query)

Due to the behavior of kong.request.get_query():

By default, this function returns up to 100 arguments (or what has been configured using lua_max_uri_args). The optional max_args argument can be specified to customize this limit, but must be greater than 1 and not greater than 1000.

This results in the loss of query arguments beyond the first 100.

Additional:

Other plugins that use similar functionality may also be affected by this issue.

@nowNick
Copy link
Contributor

nowNick commented Sep 9, 2024

Hi @dingjiayi !

Thank you for sending this issue. I'm wondering - have you configured the mentioned parameter: lua_max_uri_args? If you set it to a different value in kong.conf then Kong should pick it up and allow you to pass more than 100 query params.

Here's some info on the Kong Conifguration File

and here's a link to a template with the option that you might want to change:
https://github.com/Kong/kong/blob/master/kong.conf.default#L1762

@nowNick nowNick added the pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc... label Sep 9, 2024
@bungle
Copy link
Member

bungle commented Sep 9, 2024

they are not truncated if we get this in (though I am not 100% sure if there is some truncation in nginx or ada for too long strings):
#13619

though I am not sure should they.

@dingjiayi
Copy link
Contributor Author

Hi @dingjiayi !

Thank you for sending this issue. I'm wondering - have you configured the mentioned parameter: lua_max_uri_args? If you set it to a different value in kong.conf then Kong should pick it up and allow you to pass more than 100 query params.

Here's some info on the Kong Conifguration File

and here's a link to a template with the option that you might want to change: https://github.com/Kong/kong/blob/master/kong.conf.default#L1762

Hi @nowNick ,

Thank you for your response.

I have been using the default value for lua_max_uri_args and have not explicitly specified it. However, based on the documentation for this parameter, it should not affect the query arguments when proxying. In practice, it seems that due to the configuration of this parameter being set to 100, some query arguments are being lost.

That's why i created this issue.

I would like to understand whether this is a bug or an issue with the documentation description.

Thank you!

@dingjiayi
Copy link
Contributor Author

they are not truncated if we get this in (though I am not 100% sure if there is some truncation in nginx or ada for too long strings): #13619

though I am not sure should they.

Hi @bungle

I believe that merging this PR (#13619) will indeed resolve the issue mentioned above. The lua-resty-ada solution is very efficient and useful. Thank you.

@dingjiayi
Copy link
Contributor Author

Additionally, would the Kong team plan to adding a function in kong.pdk.request that performs the same functionality as kong.request.get_query_arg(name) but without being limited by lua_max_uri_args? This would ensure that even if the api-key argument is at the tail end of the query arguments and its position exceeds the lua_max_uri_args limit, the key-auth authentication plugin can still correctly retrieve the api-key.

For instance, this could be achieved using the get_all function from lua-resty-ada or ngx.var.http_$name from OpenResty.

Thank you.

@bungle
Copy link
Member

bungle commented Sep 9, 2024

Additionally, would the Kong team plan to adding a function in kong.pdk.request that performs the same functionality as kong.request.get_query_arg(name) but without being limited by lua_max_uri_args?

This I have been thinking. I personally think that max_uri_args is bad measure as both keys and values can be potentially huge. So how much does it help to parse 100 arguments that are all huge?

For instance, this could be achieved using the get_all function from lua-resty-ada or ngx.var.http_$name from OpenResty.

I think you mean $arg_name. I don't recall whether that returned first parameter, but if I were to guess, it with query like ?a=b&a=c it will return b with ngx.var.arg_a. I think I would ultimately use Ada on all query argument PDK methods. We just added the library, so it may take a bit time and learning where to apply it. Also there is this "it may be a breaking change" that can make it a bit difficult to introduce in some cases in minor releases. But let's see.

@dingjiayi
Copy link
Contributor Author

Hi @bungle
Thank you for your response.

Yes, you are correct; it should be $arg_name.

I'm looking forward to seeing the improvements that the integration of Ada will bring.

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending author feedback Waiting for the issue author to get back to a maintainer with findings, more details, etc...
Projects
None yet
Development

No branches or pull requests

3 participants