fix(key-auth) properly re-encode request body

[p0pr0ck5]

Summary

When both key_in_body and hide_credentials are set, we previously re-encoded the request body as a form/url-encoded entity, regardless of the request content type (and without changing the content type
header!). This commit refactors the key_in_body behavior to be a bit more robust and handle request types that the existing Kong API can decode/encode via the 'get_body_info' public function.

This PR is to be accompanied by a getkong.org PR that explicitly documents this behavior, and notifies Kong administrators that the simultaneous use of hide_credentials and key_in_body is limited to a given set of request body types.

Full changelog

• Hide key-auth credentials sent in the request body, and re-encode the body, when the hide_credentialsandkey_in_bodyplugin values are set, forapplication/json, multipart/form-data, and application/www-form-urlencoded` request bodies.
• Bail with a BAD_REQUEST status when the client sends a request with an unsupported body type, when both hide_credentials and key_in_body are set.
• Expand the key-auth plugin access tests to cover this new behavior.

Issues resolved

Fix #3108.

[p0pr0ck5]

Travis failure looks unrelated

[thibaultcha]

Neat, thanks! That will help. I will try to get a look tomorrow! cc @Kong/team-core

[thibaultcha]

Should we catch this condition earlier? Like:

body_data, err, raw_body, req_mime = public_tools.get_body_info()

if err or not hide_body_credentials[req_mime] then
  return false, { status = 400, message = "Cannot process request body" }
end

Ideally, the public API exposes a can_parse() utility to prevent reading the request body if we already know we cannot decode it, but it would already be a slight improvement?

[p0pr0ck5]

I'm not sure this approach is valid. Just because we cannot parse the body to search for the key does not mean mean we should immediately fail. If the request contains an unparseable body but the auth key is in a header or query param, we don't need to mangle the body, and thus I don't believe we fail to process the request.

[thibaultcha]

True, let's not do that!

[thibaultcha]

Unfortunately this will produce invalid tests because our client:send() utility will mutate the given request (opts.body = cjson.encode(...) and co.)

Thus, the first run of this loop (we could use ipairs here but on my machine, it will be the first element inserted) will mutate the above test harness, and our body properties will not be Lua table, but strings (because of the application/x-www-form-urlencoded MIME type). Subsequent runs silently keep sending the form-urlencoded data since we give opts.body as already valid, urlencoded payload (this nullifying this loop's effect).

An easy fix would be to make this loop supersede the one iterating over the hardness (another good reason to do so it related to another comment)

[thibaultcha]

I am of the opinion that we should add a test proving that the body's MIME type has not been tampered with. Considering the above comment wrt. mutating requests, I propose such a patch to introduce this test:

diff --git a/spec/03-plugins/10-key-auth/02-access_spec.lua b/spec/03-plugins/10-key-auth/02-access_spec.lua
index 3095e698..f076d5da 100644
--- a/spec/03-plugins/10-key-auth/02-access_spec.lua
+++ b/spec/03-plugins/10-key-auth/02-access_spec.lua
@@ -319,54 +319,55 @@ describe("Plugin: key-auth (access)", function()
 
   describe("config.hide_credentials", function()
 
-    local harness = {
-      uri_args = { -- query string
-        {
-          headers = { Host = "key-auth1.com" },
-          path    = "/request?apikey=kong",
-          method  = "GET",
+    for _, content_type in pairs({
+      "application/x-www-form-urlencoded",
+      "application/json",
+      "multipart/form-data",
+      }) do
+
+      local harness = {
+        uri_args = { -- query string
+          {
+            headers = { Host = "key-auth1.com" },
+            path    = "/request?apikey=kong",
+            method  = "GET",
+          },
+          {
+            headers = { Host = "key-auth2.com" },
+            path    = "/request?apikey=kong",
+            method  = "GET",
+          }
         },
-        {
-          headers = { Host = "key-auth2.com" },
-          path    = "/request?apikey=kong",
-          method  = "GET",
-        }
-      },
-      headers = {
-        {
-          headers = { Host = "key-auth1.com", ["apikey"] = "kong" },
-          path    = "/request",
-          method  = "GET",
-        },
-        {
-          headers = { Host = "key-auth2.com", ["apikey"] = "kong" },
-          path    = "/request",
-          method  = "GET",
-        },
-      },
-      ["post_data.params"] = {
-        {
-          headers = { ["Host"] = "key-auth5.com", },
-          body    = { apikey = "kong" },
-          method  = "POST",
-          path    = "/request",
-        },
-        {
-          headers = { ["Host"] = "key-auth6.com", },
-          body    = { apikey = "kong" },
-          method  = "POST",
-          path    = "/request",
+        headers = {
+          {
+            headers = { Host = "key-auth1.com", ["apikey"] = "kong" },
+            path    = "/request",
+            method  = "GET",
+          },
+          {
+            headers = { Host = "key-auth2.com", ["apikey"] = "kong" },
+            path    = "/request",
+            method  = "GET",
+          },
         },
+        ["post_data.params"] = {
+          {
+            headers = { ["Host"] = "key-auth5.com", },
+            body    = { apikey = "kong" },
+            method  = "POST",
+            path    = "/request",
+          },
+          {
+            headers = { ["Host"] = "key-auth6.com", },
+            body    = { apikey = "kong", foo = "bar" },
+            method  = "POST",
+            path    = "/request",
+          },
+        }
       }
-    }
 
-    for type, _ in pairs(harness) do
-      describe(type, function()
-        for _, content_type in pairs({
-          "application/x-www-form-urlencoded",
-          "application/json",
-          "multipart/form-data",
-          }) do
+      for type, _ in pairs(harness) do
+        describe(type, function()
 
           if type == "post_data.params" then
             harness[type][1].headers["Content-Type"] = content_type
@@ -387,7 +388,17 @@ describe("Plugin: key-auth (access)", function()
             local field = type == "post_data.params" and json.post_data.params or json[type]
             assert.is_nil(field.apikey)
           end)
-        end
+        end)
+      end
+
+      it("(" .. content_type .. ") true preserves body MIME type", function()
+        local req = harness["post_data.params"][2]
+        req.headers["Content-Type"] = content_type
+
+        local res   = assert(client:send(req))
+        local body  = assert.res_status(200, res)
+        local json  = cjson.decode(body)
+        assert.equal("bar", json.post_data.params.foo)
       end)
     end

I am currently hitting an exception for the multipart encoding with lua-multipart 0.5.4 which I have not investigated, but pinged @bungle (here is the test-case to reproduce, with the above body { apikey = "kong", foo = "bar" })

[thibaultcha]

@p0pr0ck5 If you won't have time to make those changes, tell me and I'll just take care of it while merging :) Let me know!

[p0pr0ck5]

@thibaultcha I can snag this over the weekend. If you want to work on it sooner feel free! :)

[p0pr0ck5]

@thibaultcha sorry for the delay. I won't have time to work on the test changes you've noted. If you want to make the adjustment and pull this in, please feel free. Otherwise I can likely circle back to this in a few weeks.

[thibaultcha]

Gave this another look now that we have lua-multipart 0.5.4, but unfortunately the test is still failing (because of lua-multipart) :/

[thibaultcha]

Fixed and merged, yay! Thank you!