search

apache / apisix

The Cloud-Native API Gateway
https://apisix.apache.org/blog/
Apache License 2.0
14.03k stars 2.46k forks source link

bug: openid-connect plugin - existing session randomly unavailable for introspection / token renewal #9306

Open brentmjohnson opened 1 year ago

brentmjohnson commented 1 year ago

Current Behavior

The openid-connect plugin will randomly redirect requests with valid session cookie and non-expired tokens back through the authentication flow. No errors were generated as the redirect happens exactly the same way a request with missing / expired session cookie is handled.

Note about current configuration where this is observed: apisix / openid-connect plugin configured for server-side sessions in redis-cluster with regenerate session-strategy (but could be an issue with other configuration).

After a lot of troubleshooting potential configuration issues across apisix, nginx, and lua-resty-session config, it now appears there is a timing issue with the reference to conf.session in this invocation of openidc.authenticate: https://github.com/apache/apisix/blob/f39cadde59d02d87919a91e0013bdbd6f6f00655/apisix/plugins/openid-connect.lua#L350

When the call is modified to: response, err, _, session = openidc.authenticate(conf, nil, unauth_action, conf)

The behavior is resolved. Token renewal occurs silently (to user) and session cookies are updated appropriately with no random redirects to the authentication flow as if there is a missing / expired session cookie.

Sending the full conf / opts object rather than just the session is supported by lua-resty-openidc:

-- main routine for OpenID Connect user authentication
function openidc.authenticate(opts, target_url, unauth_action, session_or_opts)

  if opts.redirect_uri_path then
    log(WARN, "using deprecated option `opts.redirect_uri_path`; switch to using an absolute URI and `opts.redirect_uri` instead")
  end

  local err

  local session
  if is_session(session_or_opts) then
    session = session_or_opts
  else
    local session_error
    session, session_error = r_session.start(session_or_opts)
    if session == nil then
      log(ERROR, "Error starting session: " .. session_error)
      return nil, session_error, target_url, session
    end
  end

https://github.com/zmartzone/lua-resty-openidc/blob/734a3f4dba0faf037abe993c678e43b1bab3025a/lib/resty/openidc.lua#L1440-L1459

Currently running a patched version of the openid-connect plugin (with this change) without issue (for described configuration).

Expected Behavior

With a valid session cookie and non-expired tokens, a user should not be redirected to the authentication flow.

Error Logs

No response

Steps to Reproduce

  1. Run APISIX with server-side session_storage (may also be an issue for cookies). Sample config: 
    httpSrv: |
proxy_buffer_size 32k;
proxy_buffers 8 32k;
proxy_busy_buffers_size 32k;
set $session_name "apisix_session";
set $session_cookie_samesite Strict;
large_client_header_buffers 4 16k;
set $session_strategy regenerate;
set $session_storage redis;
set $session_secret XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX;
set $session_redis_uselocking off;
set $session_redis_cluster_name redis-cluster;
set $session_redis_cluster_nodes '1 2 3 4 5 6';
  2. Configure a route protected by the openid-connect plugin: 
    - name: openid-connect
enable: true
config:
client_id: clientid
client_secret: XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
discovery: https://example.com/realms/example/.well-known/openid-configuration
scope: openid profile email
set_refresh_token_header: true
bearer_only: false
introspection_endpoint: https://example.com/realms/example/protocol/openid-connect/token/introspect
introspection_endpoint_auth_method: client_secret_post
logout_path: /logout
post_logout_redirect_uri: https://example.com/
redirect_uri: https://example.com/login
use_pkce: true
  3. Continue reloading a page in a protected route and observe occasional and random redirects to the IDP for authentication

Environment

shreemaan-abhishek commented 1 year ago

Thanks for the detailed explanation. A question: Are all the tests passing in the patched version? And which provider are you using?

brentmjohnson commented 1 year ago

I have not run any of the included tests - i'm assuming that would be these: https://github.com/apache/apisix/blob/master/t/plugin/openid-connect.t https://github.com/apache/apisix/blob/master/t/plugin/openid-connect2.t

but can - would just take some research into the test environment requirements since I haven't run those before.

I am using keycloak for the identify provider.

shreemaan-abhishek commented 1 year ago

Just raise a pull request to your fork of the project the github actions CI will run the tests.

james-mchugh commented 8 months ago

I'm seeing similar behavior

james-mchugh commented 8 months ago

I wonder if it is related to https://github.com/zmartzone/lua-resty-openidc/issues/334

james-mchugh commented 8 months ago

Actually, after reviewing the code a bit more and doing some experimenting, setting session.secret in the plugin config appeared to fix this for me.

james-mchugh commented 8 months ago

Related issue: https://github.com/zmartzone/lua-resty-openidc/issues/278

kayx23 commented 6 months ago

I'm seeing similar behavior Actually, after reviewing the code a bit more and doing some experimenting, setting session.secret in the plugin config appeared to fix this for me.

@james-mchugh Are you using APISIX in standalone mode? Because in traditional / decoupled mode, secret should be randomly generated and saved to etcd if not explicitly configured.

kayx23 commented 6 months ago

@brentmjohnson is this issue still current / have you opened a PR to this repo?

brentmjohnson commented 6 months ago

@kayx23 yes still current. I have been on a patched version of the plugin for a while with no issues.

PR: #10737

pshettyk commented 6 months ago

Try adding following under openid-connect extension session: secret:1234567891012121314