fix(key-auth): keep query params order if hide_credentials is true

When hide_credentials=true, query parameters should not be sorted
alphabetically. This alters the original request and can have unintended
consequences for upstream services. One example is authentication with
the pseudo header (request-target) as described in
https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures#section-2.3.
Since the order is not preserved, the signature does not match and the
upstream rejects the request.

* query parameters order is not altered when hide_credentials=true
* add two new test cases in spec/03-plugins/09-key-auth

changelog/unreleased/kong/fix-key-auth-query-params-order.yml

@@ -0,0 +1,3 @@
+message: "keep query params order in key-auth plugin if hide_credentials is true"
+type: bugfix
+scope: Plugin

kong/plugins/key-auth/handler.lua

@@ -8,6 +8,7 @@ local error = error
 local ipairs = ipairs
 local tostring = tostring
 local fmt = string.format
+local ngx_re_gsub = ngx.re.gsub
 
 
 local HEADERS_CONSUMER_ID           = constants.HEADERS.CONSUMER_ID
@@ -105,6 +106,13 @@ local function unauthorized(message, www_auth_content)
   return { status = 401, message = message, headers = { ["WWW-Authenticate"] = www_auth_content } }
 end
 
+local function remove_query_key(raw_query, key)
+  local pattern = key .. "=[^&]*&?"
+  local new_query = ngx_re_gsub(raw_query, pattern, "", "oj")
+  new_query = ngx_re_gsub(new_query, "&$", "", "oj")
+  return new_query
+end
+
 local function do_authentication(conf)
   if type(conf.key_names) ~= "table" then
     kong.log.err("no conf.key_names set, aborting plugin execution")
@@ -143,9 +151,14 @@ local function do_authentication(conf)
       key = v
 
       if conf.hide_credentials then
-        query[name] = nil
-        kong.service.request.set_query(query)
-        kong.service.request.clear_header(name)
+        if conf.key_in_query then
+          local raw_query = kong.request.get_raw_query()
+          local new_query = remove_query_key(raw_query, name)
+          kong.service.request.set_raw_query(new_query)
+        end
+        if conf.key_in_header then
+          kong.service.request.clear_header(name)
+        end
 
         if conf.key_in_body then
           if not body then
@@ -163,7 +176,6 @@ local function do_authentication(conf)
       end
 
       break
-
     elseif type(v) == "table" then
       -- duplicate API key
       return nil, unauthorized(ERR_DUPLICATE_API_KEY, www_auth_content)

spec/03-plugins/09-key-auth/02-access_spec.lua

@@ -712,6 +712,34 @@ for _, strategy in helpers.each_strategy() do
         assert.matches("No API key found in request", json.message)
         assert.equal('Key', res.headers["WWW-Authenticate"])
       end)
+
+      it("false does not remove apikey and preserves order of query parameters", function()
+        local res = assert(proxy_client:send {
+          method = "GET",
+          path = "/request?c=value1&b=value2&apikey=kong&a=value3",
+          headers = {
+            ["Host"] = "key-auth1.test"
+          }
+        })
+        local body = assert.res_status(200, res)
+        local json = cjson.decode(body)
+
+        assert.equal("/request?c=value1&b=value2&apikey=kong&a=value3", json.vars.request_uri)
+      end)
+
+      it("true removes apikey and preserves order of query parameters", function()
+        local res = assert(proxy_client:send{
+          method = "GET",
+          path = "/request?c=value1&b=value2&apikey=kong&a=value3",
+          headers = {
+            ["Host"] = "key-auth2.test"
+          }
+        })
+        local body = assert.res_status(200, res)
+        local json = cjson.decode(body)
+
+        assert.equal("/request?c=value1&b=value2&a=value3", json.vars.request_uri)
+      end)
     end)
 
     describe("config.anonymous", function()