Kong: OAuth2 Protected Proxy yeilds HTTP 500 error

[jeremyjpj0916]

Summary

We have a proxy protected with ACL and OAUTH2.0, recently a consumer came to us with a specific proxy path on their wildcarded proxy returning HTTP 500's. Our logs indicate it stopped @ the gateway and never proxied. Checking Kong logs I see this error.

2018/11/21 13:54:58 [error] 84#0: *10614388 lua coroutine: runtime error: /usr/local/share/lua/5.1/kong/tools/utils.lua:430: bad argument #1 to 'pairs' (table expected, got number)
stack traceback:
coroutine 0:
               [C]: in function 'pairs'
               /usr/local/share/lua/5.1/kong/tools/utils.lua:430: in function 'retrieve_parameters'
               /usr/local/share/lua/5.1/kong/plugins/oauth2/access.lua:453: in function 'parse_access_token'
               /usr/local/share/lua/5.1/kong/plugins/oauth2/access.lua:531: in function 'do_authentication'
               /usr/local/share/lua/5.1/kong/plugins/oauth2/access.lua:603: in function 'execute'
               /usr/local/share/lua/5.1/kong/plugins/oauth2/handler.lua:12: in function </usr/local/share/lua/5.1/kong/plugins/oauth2/handler.lua:10>
coroutine 1:
               [C]: in function 'resume'
               coroutine.wrap:21: in function <coroutine.wrap:21>
               /usr/local/share/lua/5.1/kong/init.lua:468: in function 'access'
access_by_lua(nginx.conf:119):2: in function <access_by_lua(nginx.conf:119):1>, client: 10.***.*.***, server: kong, request: "POST /api/test2/provider/v1/bill/Service HTTP/1.1", host: "gateway.company.com"

OAuth2.0 Plugin on route config(pretty standard):

{
  "created_at": 1537827592725,
  "config": {
    "refresh_token_ttl": 1209600,
    "token_expiration": 7200,
    "mandatory_scope": false,
    "accept_http_if_already_terminated": false,
    "hide_credentials": false,
    "enable_implicit_grant": false,
    "global_credentials": true,
    "enable_client_credentials": true,
    "enable_password_grant": false,
    "anonymous": "",
    "enable_authorization_code": false,
    "provision_key": "function",
    "auth_header_name": "authorization"
  },
  "id": "91b369b6-fb78-4562-9138-a20fc0814b5a",
  "name": "oauth2",
  "route_id": "cc488204-fcf6-4826-b30e-3bbe8bdaee3f",
  "enabled": true
}

My initial analysis reveals this:

1. They could not be passing the token as Authorization: Bearer header otherwise the first catch in the parse_access_token would be hit and handled properly. So it goes to retrieve_parameters call which checks the http body and the uri for query parameters.

My guess is that at this spot: https://github.com/Kong/kong/blob/master/kong/plugins/oauth2/access.lua#L113 the https://github.com/Kong/kong/blob/master/kong/tools/utils.lua#L420 , table merge call fails to handle certain edge cases based on the uri_args that were collected 🔥 .

Steps To Reproduce

1. One way to reproduce is in the HTTP Body just post a raw numeric string with nothing else. Leave off the Authorization: Bearer {token} if you are running latest Kong master branch with my PR that looks for header first before body. If you run the official 14.1 Kong release it looks for body before caring about token in header so error will occur even if you send proper token in header.

Additional Details & Logs

• Kong version 14.1

[Tieske]

It's the 2nd argument that is a number, so this line https://github.com/Kong/kong/blob/master/kong/plugins/oauth2/access.lua#L111 is where the number is set.

So an edgecase in get_body_args()

[jeremyjpj0916]

Interesting thanks for the input that was quick 😄 , I plan to come back to this github issue when I have more ammo and can reproduce the error. Just wanted to get the problem out there to get the brains going. Already feels like we are on right path 👍 .

[jeremyjpj0916]

@Tieske I figured out one whacky way to reproduce,

Leave off the Authorization: Bearer {token}, and in the HTTP body of the request do not even do valid json, just type a random number and watch Kong get #rekt .

Edit - Also only one of my last PR's post 14.1 moved headers to getting checked in oauth2.0 before the body, so I can't even tell them to add the proper auth header and "things will work" grrr... Something in their http body for sure is causing issue. Darn

[Tieske]

had a quick look through the code get_body_args() just calls get_body_info(). That one in turn returns an empty table on every code path, except when invoking the MIME-decoders.

The multipart decoder returns an empty table, or the results from get_all() which is always a table. So that can't be the problem.

The json decoder returns empty tables on an error, or the decoded result. Note that the decoder used is the safe one, so it should return nil+err on failures. A flaw in the decoder could result in a bad answer being returned here.

Finally, a flaw in ngx.req.get_post_args() might result in a bad response for the form-url-encoded format.

[Tieske]

It seems to be json related. The code expects the json parser to return a table, but if it is just a single value, it will only return that value, which is unexpected.

This code:

local decode = require("cjson.safe").decode

local test = {
  [[234]],
  [["hello world"]],
  [[single_word]],
  [[{ "hello": 5 }]],
  [[null]],
}

print("=========================================")
for i,txt in ipairs(test) do
  print(i, "input:", txt)
  print(i, "output:", decode(txt))
  print("=========================================")
end

Returns this output:

Thijss-MBP:code thijs$ luajit cjson-test.lua
=========================================
1   input:  234
1   output: 234
=========================================
2   input:  "hello world"
2   output: hello world
=========================================
3   input:  single_word
3   output: nil Expected value but found invalid token at character 1
=========================================
4   input:  { "hello": 5 }
4   output: table: 0x05e3c1c0
=========================================
5   input:  null
5   output: userdata: NULL
=========================================
Thijss-MBP:code thijs$

We're typically expecting nr 4 to be returned only. Nasty problem, since it probably has a lot more occurrences than only this one.

[jeremyjpj0916]

Better to find it now than never, this one is a stinker though 😄 . why does the HTTP Body for OAuth2 tokens needs to be evaluated at all, I thought the only place people generally put the tokens are:

1. HTTP Header: Authorization: Bearer {token}
2. Query parameter in the url like : ?access_token={token}

And I suppose if you have application/x-www-form-urlencoded Content-Type then maybe it makes sense to look at the body of key=value&key2=value2& where one of those is access_token={token}? ... but I have never seen that in practice, I guess the RFC does cover it though and Kong tried to follow the RFC to the best it could.

Hopefully this bug does not pop up and bite too many other plugins/parts of the code base.

[Tieske]

Hopefully this bug does not pop up and bite too many other plugins/parts of the code base.

fierce internal debate going on 😄

[thibaultcha]

Just woke up :)

Seems like we should validate that kong.json’s input be a table. If not, returning a soft error and/or an empty table should prevent all existing code from throwing a Lua error, and we can fix all failing tests, if any. Users who wish to benefit from the default (and expected) lua-cjson behavior are free to directly use it themselves.

[Tieske]

This is 14.1 for now, so it doesn't use kong.json iirc. That's probably in next

[jeremyjpj0916]

@Tieske should this get the red bug label to track it or is Kong's stance bad payloads not failing gracefully not a bug?

[bungle]

@jeremyjpj0916 believe me, there are weird use cases, people even seem to send tokens in cookies etc. Standards don't cover everything, and people don't always follow standards. I guess the idea on our side is to stay unopinionated about it and allow people to do things. I think we should provide some plugin configuration parameter on where to read these (in order). Some of our plugins have that. Performance critical scenarios I would say reading bodies on proxies should be very very rare. I think this is bug in that it is not gracefully failing, so I will mark it as such.

[thibaultcha]

4063 looks good and is similar to my above "reject all non-table input" comment - except at a higher level than kong.json, which is fine :ok_hand: We'll include it in 1.0, thanks @bungle!