openid-connect plugin leaving server-side sessions locked by not calling explicit session:close()

[brentmjohnson]

Current Behavior

Server-side session stores that support locking remain locked after authentication until the default or configured lock ttl (https://github.com/bungle/lua-resty-session#redis-storage-adapter) expires. This results in subsequent requests (using the same APISIX session) hanging until the lock expires.

Apisix plugin code:

apisix/apisix/plugins/openid-connect.lua

Line 312 in cf8de8e

response, err, _, session = openidc.authenticate(conf)

Does not implement explicit session:close() as recommended by zmartzone/lua-resty-openidc https://github.com/zmartzone/lua-resty-openidc#sessions-and-locking

Expected Behavior

APISIX openid-connect plugin should call explicit session:close() and subsequent requests (using the same APISIX session) should complete immediately.

Error Logs

No response

Steps to Reproduce

1. Configure APISIX openid-connect plugin for a server-side session store that supports locking
2. Configure route using the openid-connect plugin
3. Authenticate on protected route
4. Immediately refresh route using the same session cookie - request will hang until the default or configured lock ttl

Environment

• APISIX version (run apisix version): 2.15.0
• Operating system (run uname -a): Linux apisix-6fcbf58999-t8x9r 5.15.0-48-generic # 54-Ubuntu SMP Fri Aug 26 13:26:29 UTC 2022 x86_64 Linux
• OpenResty / Nginx version (run openresty -V or nginx -V): nginx version: openresty/1.21.4.1
• etcd version, if relevant (run curl http://127.0.0.1:9090/v1/server_info): N/A
• APISIX Dashboard version, if relevant: N/A
• Plugin runner version, for issues related to plugin runners: N/A
• LuaRocks version, for installation issues (run luarocks --version): /usr/local/bin/luarocks 3.8.

[tzssangglass]

• Configure APISIX openid-connect plugin for a server-side session store that supports locking

Can you give detailed configuration information? This would help us to reproduce it.

[brentmjohnson]

Sure, this is my configuration with keycloak as the idp in kubernetes:

Keycloak realm configuration (KeycloakRealmImport):

apiVersion: k8s.keycloak.org/v2alpha1
kind: KeycloakRealmImport
metadata:
  name: apisix
  namespace: keycloak-operator
  labels:
    realm: apisix
spec:
  keycloakCRName: keycloak
  realm:
    realm: apisix
    enabled: true
    displayName: APISIX
    attributes:
      frontendUrl: https://10.0.0.2/keycloak/
    clients:
      - id: httpbin
        name: httpbin
        protocol: openid-connect
        rootUrl: https://10.0.0.2/httpbin
        redirectUris:
          - /login
        directAccessGrantsEnabled: true
        attributes:
          pkce.code.challenge.method: S256
          post.logout.redirect.uris: /
    users:
      - username: user
        firstName: first
        lastName: last
        email: email@email.com
        enabled: true
        credentials:
          - type: password
            value: pass

notes:

• basically following this: https://www.keycloak.org/2021/12/apisix
• 10.0.0.2 is used as an external load balancer

Keycloak APISIX route configuration (ApisixRoute):

apiVersion: apisix.apache.org/v2beta3
kind: ApisixRoute
metadata:
  name: keycloak-route
  namespace: keycloak-operator
spec:
  http:
  - name: rule1
    match:
      paths:
      - /keycloak/*
    backends:
       - serviceName: keycloak-service
         servicePort: 8080
    plugins:
      - name: proxy-rewrite
        enable: true
        config:
          host: keycloak-service.keycloak-operator.svc.cluster.local
      - name: serverless-pre-function
        enable: true
        config:
          phase: rewrite
          functions:
            - return function(conf, ctx) ngx.var.var_x_forwarded_proto = "https"; ngx.var.var_x_forwarded_port = 443 end
  - name: rule2
    match:
      paths:
      - /keycloak-internal/*
    backends:
       - serviceName: keycloak-service
         servicePort: 8080
    plugins:
      - name: proxy-rewrite
        enable: true
        config:
          host: keycloak-service.keycloak-operator.svc.cluster.local
          regex_uri:
            - "^/keycloak-internal/(.*)"
            - "/$1"
      - name: serverless-pre-function
        enable: true`
        config:
          phase: rewrite
          functions:
            - return function(conf, ctx) ngx.var.var_x_forwarded_port = 8080; ngx.var.var_x_forwarded_host = "keycloak-service.keycloak-operator.svc.cluster.local" end

notes:

• serverless pre-functions were required to satisfy keycloak's request handling for differentiating internal and external services

Httpbin deployment (httpbin)

apiVersion: v1
kind: ServiceAccount
metadata:
  name: httpbin
---
apiVersion: v1
kind: Service
metadata:
  name: httpbin
  labels:
    app: httpbin
    service: httpbin
spec:
  ports:
  - name: http
    port: 80
    targetPort: 80
  selector:
    app: httpbin
---
apiVersion: apps/v1
kind: Deployment
metadata:
  name: httpbin
spec:
  replicas: 1
  selector:
    matchLabels:
      app: httpbin
      version: v1
  template:
    metadata:
      labels:
        app: httpbin
        version: v1
    spec:
      serviceAccountName: httpbin
      containers:
      - image: docker.io/kennethreitz/httpbin
        imagePullPolicy: IfNotPresent
        name: httpbin
        ports:
        - containerPort: 80

Httpbin APISIX route configuration (ApisixRoute):

apiVersion: apisix.apache.org/v2beta3
kind: ApisixRoute
metadata:
  name: httpbin-route
  namespace: default
spec:
  http:
  - name: rule1
    match:
      paths:
      - /httpbin/*
    backends:
      - serviceName: httpbin
        servicePort: 80
    plugins:
      - name: proxy-rewrite
        enable: true
        config:
          host: httpbin.default.svc.cluster.local
          regex_uri:
            - "^/httpbin/(.*)"
            - "/$1"
      - name: openid-connect
        enable: true
        config:
          client_id: httpbin
          client_secret: ReplaceWithKeycloakGeneratedClientSecret
          discovery: http://localhost:9080/keycloak-internal/keycloak/realms/apisix/.well-known/openid-configuration
          scope: openid profile
          bearer_only: false
          introspection_endpoint: http://keycloak-service.keycloak-operator.svc.cluster.local:8080/keycloak/realms/apisix/protocol/openid-connect/token/introspect
          introspection_endpoint_auth_method: client_secret_post
          logout_path: /httpbin/logout
          post_logout_redirect_uri: https://10.0.0.2/httpbin/
          redirect_uri: https://10.0.0.2/httpbin/login
          use_pkce: true

notes:

• actual configuration of openid-connect plugin

Server-side session store patch for configmap/apisix:

nginx_config:
genarate nginx.conf
  http_configuration_snippet: |
    lua_shared_dict redis_cluster_slot_locks 100k;
  http_server_configuration_snippet: |
    set $session_name "apisix_session";
            large_client_header_buffers 4 16k;
            set $session_strategy regenerate;
            set $session_storage redis;
            set $session_redis_uselocking on;
            set $session_redis_cluster_name redis-cluster;
            set $session_redis_cluster_nodes 'redis-cluster-leader-headless.redis.svc.cluster.local redis-cluster-follower-headless.redis.svc.cluster.local';

notes:

• redis cluster deployed separately
• only really tested with redis cluster but assume this impacts other server-side session stores that support locking

Resulting nginx.conf sections:

http {
    # http configuration snippet starts
    lua_shared_dict redis_cluster_slot_locks 100k;
    # http configuration snippet ends

    server {
        # http server configuration snippet starts
        set $session_name "apisix_session";
        large_client_header_buffers 4 16k;
        set $session_strategy regenerate;
        set $session_storage redis;
        set $session_redis_uselocking on;
        set $session_redis_cluster_name redis-cluster;
        set $session_redis_cluster_nodes 'redis-cluster-leader-headless.redis.svc.cluster.local redis-cluster-follower-headless.redis.svc.cluster.local';
        # http server configuration snippet ends
    }
}

Let me know if you need anything else. Thanks for a great product!

[kingluo]

@brentmjohnson Yes, for server-side session storages, it's necessary to close session explicicitly. However, this plugin assumes the user to use default storage adapter, i.e. save-all-in-cookie and does not pass any session options to lua-resty-openidc. But for compatibility with all stroages, we need to close it at right place. Do you expect a bugfix or make a PR youself?

[tzssangglass]

We can provide a session_storage option, which defaults to cookie, and if user set session_storage to redis, call session:close() in right place?

[kingluo]

@tzssangglass Not necessary at all. The authenticate() encapsulates the auth logic all-in-one and starts the session (for some server-side storages, e.g. shm, not only redis, does locking if option enabled), the session is left open to the caller (so that locking is hold during open) only if the caller wants to make the session manipulation a transaction, but it's useless for our plugin. Moreover, it is safe to close it immediately, because the close is provided for all storages and idempotent.

[brentmjohnson]

@kingluo that is what I understand as well - that session:close() should be safe to call explicitly regardless of the configured storage implementation (supported by the underlying bungle/lua-resty-session).

I am happy to make a PR for the change, but it will be a few weeks until I can get to it if that is okay.

One note on the current cookie storage implementation. It seems like with nginx.conf worker_processes != 1, there are no guarantees for worker affinity for subsequent connections to the same protected route. When the request is handled by a different worker, the session cookie is not valid. I believe the result is actually a plugin error rather than a redirect to the auth endpoint.

Since APISIX ships with worker_processes: auto as the default, it might be worth considering moving to the shared-dictionary-storage-adapter as a new default session store.

[tzssangglass]

Since APISIX ships with worker_processes: auto as the default, it might be worth considering moving to the shared-dictionary-storage-adapter as a new default session store.

+1 cc @spacewander @sober-wang @bzp2010 @kingluo

[kingluo]

One note on the current cookie storage implementation. It seems like with nginx.conf worker_processes != 1, there are no guarantees for worker affinity for subsequent connections to the same protected route. When the request is handled by a different worker, the session cookie is not valid. I believe the result is actually a plugin error rather than a redirect to the auth endpoint.

No, there are two misunderstandings here.

• The storage only determines where to store the session. It does not change the fact that the session key comes from the cookie. That is, even if you set the storage adapter as shm, when you request to different route with different hosts (keep in mind that the cookie is only bound to the host, without scheme and port), you do not get the same session, because the cookie is different.
• The routes are shared by all worker processes. Even if you keep sending requests to the same worker process but with different hosts, you still get different results. On the other hand, if you send reqquests to different worker processes but with the same host, you would get the same session, even if you use the default all-in-cookie session.

[brentmjohnson]

Okay got it - thanks for clarifying. And the issue I was encountering related to worker process actually turned out to be this: #2426 (comment)

Using a worker_processes: 1 seems to avoid potential errors related to an uninitialized session_secret on first login / logout redirect. Confirmed this for both default cookie storage as well as server-side redis.

[kingluo]

Okay got it - thanks for clarifying. And the issue I was encountering related to worker process actually turned out to be this: #2426 (comment)

Using a worker_processes: 1 seems to avoid potential errors related to an uninitialized session_secret on first login / logout redirect. Confirmed this for both default cookie storage as well as server-side redis.

@brentmjohnson Yes, it's a bug of the plugin. My colleague is making a PR to add options to invoke authenticate() to specifiy secret.
Before that, see my work-aournd: #2426 (comment)

[brentmjohnson]

Thanks, works great @kingluo!

[shreemaan-abhishek]

@kingluo is there any progress on the PR?

[sheharyaar]

@shreemaan-abhishek , please assign me this issue.

[monkeyDluffy6017]

@sheharyaar please help to check this: #10788