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

bug: Failed auth with body transformer not working #11608

Open
grencik opened this issue Sep 26, 2024 · 5 comments · May be fixed by #11768
Open

bug: Failed auth with body transformer not working #11608

grencik opened this issue Sep 26, 2024 · 5 comments · May be fixed by #11768
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@grencik
Copy link

grencik commented Sep 26, 2024

Current Behavior

I have in route key-auth and body-transformer plugins. When key-auth plugin fail ([lua] plugin.lua:1171: run_plugin(): key-auth exits with http status code 401) then body-transformer horibly crash with error

[error] 49#49: *325 failed to run body_filter_by_lua*: /usr/local/apisix/apisix/plugins/body-transformer.lua:220: attempt to index local 'conf' (a nil value)
stack traceback:
        /usr/local/apisix/apisix/plugins/body-transformer.lua:220: in function 'phase_func'
        /usr/local/apisix/apisix/plugin.lua:1203: in function 'common_phase'
        /usr/local/apisix/apisix/init.lua:806: in function 'http_body_filter_phase'
        body_filter_by_lua(nginx.conf:368):2: in main chunk

When I remove key-auth plugin or key-auth doesn't fail, it works as expected.

Expected Behavior

It should probably stop executing next plugins or at least body-transformer should have all data to not crash.

Error Logs

2024/09/26 09:09:26 [warn] 50#50: *14043 [lua] plugin.lua:1171: run_plugin(): key-auth exits with http status code 401, client: 172.68.213.37, server: _, request: "POST /example HTTP/2.0", host: "example.com"
2024/09/26 09:09:26 [error] 50#50: *14043 failed to run body_filter_by_lua*: /usr/local/apisix/apisix/plugins/body-transformer.lua:220: attempt to index local 'conf' (a nil value)
stack traceback:
        /usr/local/apisix/apisix/plugins/body-transformer.lua:220: in function 'phase_func'
        /usr/local/apisix/apisix/plugin.lua:1203: in function 'common_phase'
        /usr/local/apisix/apisix/init.lua:806: in function 'http_body_filter_phase'
        body_filter_by_lua(nginx.conf:368):2: in main chunk, client: 1.1.1.1, server: _, request: "POST /example HTTP/2.0", host: "example.com"
2024/09/26 09:09:26 [error] 50#50: *14043 failed to run body_filter_by_lua*: /usr/local/apisix/apisix/plugins/body-transformer.lua:220: attempt to index local 'conf' (a nil value)
stack traceback:
        /usr/local/apisix/apisix/plugins/body-transformer.lua:220: in function 'phase_func'
        /usr/local/apisix/apisix/plugin.lua:1203: in function 'common_phase'
        /usr/local/apisix/apisix/init.lua:806: in function 'http_body_filter_phase'
        body_filter_by_lua(nginx.conf:368):2: in main chunk, client: 1.1.1.1, server: _, request: "POST /example HTTP/2.0", host: "example.com"

Steps to Reproduce

  1. Create route with plugins key-auth and body-transformer
  2. Make request with failing auth

Environment

  • APISIX version (run apisix version): 3.10.0
@shreemaan-abhishek
Copy link
Contributor

this indeed looks like a bug, please share your apisix resource configurations

@grencik
Copy link
Author

grencik commented Sep 27, 2024

Consumer:

{
  "plugins": {
    "key-auth": {
      "key": "abcdef"
    }
  },
  "username": "consumer_user"
}

Route:

{
  "name": "Some route",
  "status": 1,
  "plugins": {
	"key-auth": {
		"_meta": {
			"disable": true
		},
		"hide_credentials": true
	},
    "body-transformer": {
      "request": {
        "input_format": "json",
        "template_is_base64": true,
        "template": "<base64 template>"
      }
    },
    "proxy-rewrite": {
      "regex_uri": [
        "^/some-route",
        "/other-route"
      ],
      "use_real_request_uri_unsafe": false
    }
  },
  "host": "host.example.com",
  "methods": [
    "GET",
    "POST"
  ],
  "uri": "/some-route",
  "upstream": {
    "scheme": "https",
    "type": "roundrobin",
    "nodes": [
      {
        "host": "other-host.example.com",
        "weight": 1,
        "priority": 0,
        "port": 443
      }
    ],
    "hash_on": "vars",
    "timeout": {
      "connect": 6,
      "read": 6,
      "send": 6
    },
    "keepalive_pool": {
      "idle_timeout": 60,
      "requests": 1000,
      "size": 320
    },
    "pass_host": "node"
  }
}

Request:

curl --request POST \
  --url 'https://host.example.com/some-route?apikey=abcdef' \
  --header 'Content-Type: application/json' \
  --data '{
	"attr1": "value 1",
	"attr2": "value 2"
}'

@ShaunMaher
Copy link

Hi.

I have done some digging and maybe I have something to add. I'm very new to Lua and APISIX so, I might be way off track.

The fact that run_plugin(): key-auth exits with http status code 401 ends up in the logs, implies we are getting to this line of APISIX code:

core.log.warn(plugins[i].name, " exits with http status code ", code)

A few lines later, it core.response.exit(code, body), which I think means that the client request should be closed (and maybe it is). It doesn't seem to stop lower priority plugins (body-transformer, plugin code I have written) from running though. I'm not sure if this is intentional (to let other plugins do stuff in the event of an auth failure) or a bug.

Until someone that knows what they are doing can have a look at this, I have a dodgy workaround.

  • Create a new plugin
  • Give it a priority of 1081 (one higher than the default priority of the body-transformer plugin)
  • Give it a body_filter function with the following code:
function _M.body_filter(conf, ctx)
  if ngx.status == 401 then
    for i, plugin in ipairs(ctx.plugins) do
      if plugin.name and plugin.name == "body-transformer" then
        core.log.error(plugin_name .. ":body_filter(): ctx.plugin[" .. i .. "]: " .. core.json.encode(ctx.plugins[i], true) .. ".")
        core.log.error(plugin_name .. ":body_filter(): this is the body-transformer!  Disabling functions.")
        plugin.rewrite = nil
        plugin.body_filter = nil
      end
    end
  end
end

This uses the ctx variable to enumerate all plugins enabled for this request. It finds the body-transformer plugin and replaces it's rewrite and body_filter functions with nil, effectively preventing them from running.

Complete plugin:

-- When using the key-auth and body-transformer plugins together, a failed
--  authentication can result in the body-transformer plugin failing (unhandled
--  error) and the client recieving an empty response.
-- This plugin is a work around that, if authentication has failed, disables the
--  rewrite and body_filter functions in the body-transformer plugin (just for 
--  the current request).  If the functions are disabled, they can't fail.

local plugin_name = "exit-on-auth-failure"
local core = require("apisix.core")
local ngx = ngx

local schema = {
  type = "object",
  properties = { }
}

local _M = {
  version = 0.1,
  -- body-transformer priority: 1080.  This plugin must have a higher priority
  priority = 1081,
  name = plugin_name,
  schema = schema,
  scope = "global",
}

function _M.body_filter(conf, ctx)
  if ngx.status == 401 then
    for i, plugin in ipairs(ctx.plugins) do
      if plugin.name and plugin.name == "body-transformer" then
        core.log.error(plugin_name .. ":body_filter(): ctx.plugin[" .. i .. "]: " .. core.json.encode(ctx.plugins[i], true) .. ".")
        core.log.error(plugin_name .. ":body_filter(): this is the body-transformer!  Disabling functions.")
        plugin.rewrite = nil
        plugin.body_filter = nil
      end
    end
  end
end

return _M

Put the above in a file called exit-on-auth-failure.lua in the same location as the other APISIX plugins, add it to the list of plugins in apisix.yaml, add it to the list of plugins on any route that has key-auth and body-transformer enabled. Workaround done.

@shreemaan-abhishek
Copy link
Contributor

shreemaan-abhishek commented Nov 15, 2024

@ShaunMaher, the body-transformer plugin's execution priority is lower than key-auth and when key-auth fails, the rewrite phase is aborted, this means the body-transformer plugin's rewrite function is not executed. Due to this, ctx.body_transformer_conf ends up being uninitialized:

ctx.body_transformer_conf = conf

But during body_filter phase, body-transformer's body_filter function is called and APISIX panics because ctx.body_transformer_conf is nil:

local conf = ctx.body_transformer_conf
if conf.response then
.

This can be fixed just by adding a nil check.

@shreemaan-abhishek shreemaan-abhishek added the good first issue Good for newcomers label Nov 15, 2024
@ShaunMaher
Copy link

Hi @shreemaan-abhishek

Thanks for the feedback.

One thing I still don't quite understand though is whether or not the body_filter phase should run at all if the authentication failed. I can see why you might want it to (e.g. rewrite the error message before it goes back to the client). It seems like it should be something that should be made extremely clear in the doco though. If, for example, you're using serverless-*-function to take some action, assuming that key-auth won't let that action happen if the authentication failed, you're in for trouble.

I'm happy to come up with a PR that updates the code in body-transformer and the doco if you like.

Cheers.
Shaun.

@shreemaan-abhishek shreemaan-abhishek linked a pull request Nov 21, 2024 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
Status: 📋 Backlog
Development

Successfully merging a pull request may close this issue.

3 participants