bug: openid-connect: Logout not working using Firefox Version >= 128.0

markusmueller commented 3 months ago

Current Behavior

Using Firefox version >= 128.0 the /logout path provided by the openid-connect plugin is not redirecting to the configured IDP for logout but instead returning a 1x1 pixel image.

Root Cause: Firefox Version 128.0 changed the network.http.accept default value also adding image/png.

This is causing lua-resty-openidc used by the openid-connect plugin to return the image.

There is already a discussion on lua-resty-openidc.

Once the issue is resolved upstream the dependency should be updated.

A possible workaround in the meantime is patching the openid-connect plugin and overwriting the Accept header for the /logout path in openid-connect.lua:

ngx.req.set_header("Accept", "text/html")

Expected Behavior

Using Firefox v 128.0 the path /logout will redirect to the configured IDP for logout.

Error Logs

No response

Steps to Reproduce

Configure Apisix openid-connect plugin with logout and hit /logout using Firefox version >= 128.0.

Environment

APISIX version (run apisix version):
Operating system (run uname -a):
OpenResty / Nginx version (run openresty -V or nginx -V):
etcd version, if relevant (run curl http://127.0.0.1:9090/v1/server_info):
APISIX Dashboard version, if relevant:
Plugin runner version, for issues related to plugin runners:
LuaRocks version, for installation issues (run luarocks --version):

dosubot[bot] commented 3 months ago

Hey @markusmueller! 👋 I'm here to help you with any bugs, questions, or contributions you have regarding our repository.

To address the issue with the /logout path not redirecting correctly in Firefox version 128.0 or higher, you can patch the APISIX openid-connect plugin to overwrite the Accept header. Here is how you can do it:

Modify the Plugin Code: Add a function to handle the /logout path and set the Accept header to text/html.

local function handle_logout_request()
   local uri = ngx.var.uri
   if uri == "/logout" then
       ngx.req.set_header("Accept", "text/html")
   end
end

Integrate the Function: Call this function in the main handler function of the plugin. The main handler function in the openid-connect plugin is the rewrite function.

function _M.rewrite(plugin_conf, ctx)
   local conf = core.table.clone(plugin_conf)

   -- Previously, we multiply conf.timeout before storing it in etcd.
   -- If the timeout is too large, we should not multiply it again.
   if not (conf.timeout >= 1000 and conf.timeout % 1000 == 0) then
       conf.timeout = conf.timeout * 1000
   end

   local path = ctx.var.request_uri

   -- Call the function to handle the logout request
   handle_logout_request()

   -- Continue with the rest of the plugin logic
end

Test the Patch: Ensure that the logout request is properly handled in the plugin's test cases. You can add a test case to verify that the Accept header is set correctly during the logout process.

local http = require "resty.http"
local httpc = http.new()
local uri = "http://127.0.0.1:" .. ngx.var.server_port .. "/logout"
local res, err = httpc:request_uri(uri, {method = "GET"})
ngx.status = res.status
local location = res.headers['Location']
if location and string.find(location, 'https://iresty.com') ~= -1 and
   string.find(location, 'post_logout_redirect_uri=https://iresty.com') ~= -1 then
   ngx.say(true)
end

This patch ensures that when the /logout path is accessed, the Accept header is set to text/html, which should resolve the redirection issue in Firefox version 128.0 or higher ^[1]^[2]^[3].

To continue talking to Dosu, mention @dosu.

francescodedomenico commented 3 months ago

@dosu I tried modifying the original openid-connect plugin, however I get the following error when trying: 2024-08-12 20:20:40 2024/08/12 18:20:40 [error] 40#40: *48955 [lua] config_yaml.lua:269: failed to fetch data from local file /usr/local/apisix/conf/apisix.yaml: /usr/local/apisix/apisix/plugins/openid-connect.lua:310: attempt to call field 'check_https' (a nil value), apisix.yaml key: plugin_configs, context: ngx.timer

the original plugin works correctly with the same configuration.

Update

I have personally solved following a different approach, I have written a patched openidc.lua that has the lines:

-- if header and header:find("image/png") then
  --   ngx.header["Cache-Control"] = "no-cache, no-store"
  --   ngx.header["Pragma"] = "no-cache"
  --   ngx.header["P3P"] = "CAO PSA OUR"
  --   ngx.header["Expires"] = "0"
  --   ngx.header["X-Frame-Options"] = "DENY"
  --   ngx.header.content_type = "image/png"
  --   ngx.print(openidc_transparent_pixel)
  --   ngx.exit(ngx.OK)
  --   return

commented (lines from 1314 to 1323) and I have replaced the original one shipped with apisix: /usr/local/apisix/deps/share/lua/5.1/resty/openidc.lua

Works perfectly for me

francescodedomenico commented 3 months ago

@markusmueller the issue is due to the decision of lua-resty-openidc to support a peculiar Single Logout implemented by PingFederate, so it's nothing apisix can do unless forking the library and removing this custom PF logout support. I did perform the investigation here if you're interested: https://github.com/zmartzone/lua-resty-openidc/issues/521#issuecomment-2284890175

markusmueller commented 3 months ago

Interesting to see where the PNG based logout originated, thanks for the update @francescodedomenico!

remydewa commented 2 months ago

What I did as quick win for the moment: Using the serverless-pre-function plugin and apply a part of the code mentioned here: https://github.com/apache/apisix/issues/11492#issuecomment-2275389245

"serverless-pre-function": {
  "functions": [
    "return function(conf, ctx)\n\n local ngx = ngx \n local uri = ngx.var.uri \n if uri == '/users/logout' then \n ngx.req.set_header('Accept', 'text/html') \n end \n end"
  ],
  "phase": "rewrite"
}

amaino commented 3 weeks ago

The bug is fixed in the lua-resty-openidc now. Was this code included in the last release? https://github.com/zmartzone/lua-resty-openidc/pull/525

apache / apisix