zmartzone / lua-resty-openidc

OpenID Connect Relying Party and OAuth 2.0 Resource Server implementation in Lua for NGINX / OpenResty
Apache License 2.0
961 stars 246 forks source link

Logout not working in Firefox latest version (128.0) #521

Open omasseau opened 1 month ago

omasseau commented 1 month ago
Environment
Expected behaviour

Logout works in Firefox

Actual behaviour

Logout is not working anymore in Firefox (version 128.0) It just returns a 200 response instead of a 302. So no redirect happens.

Strangly an image(?!) is returned : image

Minimized example

Just doing :

    location /logout {
        access_by_lua_block {
            opts= {
                ...
                logout_path = "/logout",
                ...
            }
            require("resty.openidc").authenticate(opts)
        }
    }
Configuration and NGINX server log files

access.log : 127.0.0.1 - - [16/Jul/2024:15:18:24 +0200] [34bff32f803d305d47feb962e13222c3] "GET /logout HTTP/1.1" 200 79 "http://fr.localtest.com:92/" "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:128.0) Gecko/20100101 Firefox/128.0" 0.000 s (-/-)

error.log :

2024/07/16 15:18:24 [debug] 12696#13360: *253 [lua] openidc.lua:1484: authenticate(): Logout path (/logout) is currently navigated -> Processing local session removal before redirecting to next step of logout process
2024/07/16 15:18:24 [debug] 12696#13360: *253 [lua] openidc.lua:560: openidc_discover(): openidc_discover: URL is: http://127.0.0.1:8180/auth/realms/Business-Document/.well-known/openid-configuration
2024/07/16 15:18:24 [debug] 12696#13360: *253 [lua] openidc.lua:678: openidc_get_token_auth_method(): 1 => private_key_jwt
2024/07/16 15:18:24 [debug] 12696#13360: *253 [lua] openidc.lua:72: supported(): Can't use private_key_jwt without opts.client_rsa_private_key
2024/07/16 15:18:24 [debug] 12696#13360: *253 [lua] openidc.lua:678: openidc_get_token_auth_method(): 2 => client_secret_basic
2024/07/16 15:18:24 [debug] 12696#13360: *253 [lua] openidc.lua:681: openidc_get_token_auth_method(): no configuration setting for option so select the first supported method specified by the OP: client_secret_basic
2024/07/16 15:18:24 [debug] 12696#13360: *253 [lua] openidc.lua:695: openidc_get_token_auth_method(): token_endpoint_auth_method result set to client_secret_basic
ross211 commented 1 month ago

I've just been looking at this too. It appears Firefox has added image/png and image/svg+xml to the default Accept header:

Firefox v115esr: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,                        */*;q=0.8
Firefox v128:    text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/png,image/svg+xml,*/*;q=0.8

This combined with the header check in openidc.lua is preventing it from doing the usual redirect and returning a blank image instead.

A workaround would be to override the header for requests to your logout path, eg:

    location /logout {
        rewrite_by_lua_block {
            ngx.req.set_header("Accept", "text/html")
        }
        access_by_lua_block {
            opts= {
                ...
                logout_path = "/logout",
                ...
            }
            require("resty.openidc").authenticate(opts)
        }
    }

more_clear_input_headers Accept; will also work if you're running openresty or an nginx that has the ngx_headers_more module loaded.

omasseau commented 1 month ago

@ross211 Thanks, but won't there be any any drawbacks ? I guess this header check in openidc.lua is there for a good reason. Maybe some logout scenarios won't work if forcing to bypass this header check by overriding the header ?

ross211 commented 1 month ago

I think that header check is for a different method of triggering logout, probably so you can trigger a logout by calling the logout path from <img> tags of another page.

IMO if that behaviour is desired it should be triggered by passing a setting in the options instead of trying to guess based on the header content.

omasseau commented 1 month ago

I think that header check is for a different method of triggering logout, probably so you can trigger a logout by calling the logout path from <img> tags of another page.

IMO if that behaviour is desired it should be triggered by passing a setting in the options instead of trying to guess based on the header content.

I agree, it would be nice to have an options to trigger or not this behaviour ;)

vavra5 commented 1 month ago

I have run into this issue as well. Seems like this was included since FF v128.0 and was to solve a 3+ yr old bug afaik.

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/128#http https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation/List_of_default_Accept_values

I have tested the fix proposed by @ross211 and this does indeed resolve the problem. I guess this could be a workaround because I think at least in my case the returned content-type for logout requests will always be "text/html". I don't know about other use cases though.

I would be interested to have one of the maintainers weigh in on the original purpose for this handling of "image/png" in the Accept headers. It looks like this has been in place since v1.1.

makrste commented 1 month ago

Hi,

We are using a kong-oidc plugin that is based on lua-resty-openidc and ran into this issue with the latest firefox update too. As this was not expected, we get a black page with the logout url that just doesn't show anything (renders an "openidc_transparent_pixel" - how weird is this...?)

I understand the reasoning behind it, but i think this should rather be a configuration that an implicit assumption that if the client (browser or whatever) set the config, the navigation should be affected directly..

francescodedomenico commented 3 weeks ago

hi, I came accross this issue while using apache apisix, I have solved it by commenting the whole block checking the header and returning the png if it contains "image/png" in Accepts.

Even tho I do not understand this "GET Style logout" https://github.com/zmartzone/lua-resty-openidc/commit/998c7a6722b255e115131979f2b3964b112f85a5 cookies are wiped out only at client-side, an external authorization server will preserve it's sso cookies and you won't ever get logged out, that's why you typically need to perform a complete redirect cycle through every interested actor during a logout.

As far as I know OIDC is strongly based on redirects for front-channel and back-channel logout is handled with a POST http method.

OIDC standard wise, what is this for?

Thank you for your work

Update

After some reserach I did come up to where this png comes from, by following the comment on this on mod_auth_openidc

/* see if this is PF-PA style logout in which case we return a transparent pixel */
        const char *accept = oidc_http_hdr_in_accept_get(r);
        if ((_oidc_strcmp(url, OIDC_IMG_STYLE_LOGOUT_PARAM_VALUE) == 0) ||
            ((accept) && _oidc_strstr(accept, OIDC_HTTP_CONTENT_TYPE_IMAGE_PNG))) {
            return oidc_util_http_send(r, (const char *)&oidc_logout_transparent_pixel,
                           sizeof(oidc_logout_transparent_pixel),
                           OIDC_HTTP_CONTENT_TYPE_IMAGE_PNG, OK);
        }

PF stands for PingFederate and it's using a custom, non-standard way to implement single logout for PingFederate SSO products, confirmation came from this comment: https://support.pingidentity.com/s/question/0D5Do00000qtbsNKAQ/pf-support-for-oidc-session-and-logout-standards

Just my two cents on this, if a vendor proposes his own customization of the standard it shouldn't impact any OIDC RFC implementation.

bodewig commented 1 week ago

The code predates my involvement with the project. I believe what Ping Federate did back then was something that could have become a standard - but did not. This must have all happened before the front-channel logout spec has been finalized (it may have been an alternative design).

I discussed the issue with some coworkers and short of splitting the paths or removing the functionality completely the best way forward seemed to be proper Accept header parsing. If the User Agent would also accept text/html and would even prefer this, then don't return the pixel. This should also work for the newer Firefox versions.

bodewig commented 1 week ago

it would be great if anybody of you could give #525 a try