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

dingjiayi commented 1 month ago

Is there an existing issue for this?

[x] 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()`.
https://github.com/Kong/kong/blob/e89feb5b85286394330968054c02ac851fd2b781/kong/plugins/key-auth/handler.lua#L116
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.
https://github.com/Kong/kong/blob/e89feb5b85286394330968054c02ac851fd2b781/kong/plugins/key-auth/handler.lua#L147

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 commented 1 month ago

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

bungle commented 1 month ago

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): https://github.com/Kong/kong/pull/13619

though I am not sure should they.

dingjiayi commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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!

Kong / kong