fix(openid-connect): scope session cookie per route to avoid state collision

apisix/plugins/openid-connect.lua

@@ -33,11 +33,26 @@ local ngx_encode_base64 = ngx.encode_base64
 local plugin_name       = "openid-connect"
 
 
--- Session config is passed as-is to resty.session.start(); the only
--- translation is the legacy session.cookie.lifetime alias from the
--- lua-resty-session 3.x schema, which is mapped to absolute_timeout
--- when the latter is unset.
-local function build_session_opts(session_conf)
+-- Derive a stable, per-route session cookie name. resty.session names every
+-- cookie "session" by default, so sibling openid-connect routes on the same
+-- host that share a session.secret overwrite each other's pre-login state.
+-- Keying the cookie name on the route id isolates them.
+local function route_session_cookie_name(conf, ctx)
+    local seed = ctx and ctx.route_id or conf.client_id
+    if not seed then
+        return nil
+    end
+    return "session_" .. string.sub(ngx.md5(seed), 1, 16)
+end
+
+
+-- Session config is passed as-is to resty.session.start(). Two translations
+-- happen here: the legacy session.cookie.lifetime alias from the
+-- lua-resty-session 3.x schema is mapped to absolute_timeout when the latter
+-- is unset, and an unset cookie_name is defaulted to a per-route name so
+-- sibling routes don't collide (see route_session_cookie_name above). An
+-- operator-set session.cookie_name always wins.
+local function build_session_opts(session_conf, default_cookie_name)
     if not session_conf then
         return nil
     end
@@ -48,6 +63,9 @@ local function build_session_opts(session_conf)
                           "use session.absolute_timeout instead")
         end
     end
+    if default_cookie_name and not session_conf.cookie_name then
+        session_conf.cookie_name = default_cookie_name
+    end
     return session_conf
 end
 
@@ -476,6 +494,7 @@ local _M = {
     name = plugin_name,
     schema = schema,
     _build_session_opts = build_session_opts,
+    _route_session_cookie_name = route_session_cookie_name,
 }
 
 function _M.check_schema(conf)
@@ -861,7 +880,7 @@ function _M.rewrite(plugin_conf, ctx)
         -- This code path also handles when the ID provider then redirects to
         -- the configured redirect URI after successful authentication.
         response, err, _, session  = openidc.authenticate(conf, nil, unauth_action,
-                                                          build_session_opts(conf.session))
+                        build_session_opts(conf.session, route_session_cookie_name(conf, ctx)))
 
         if err then
             if session then

docs/en/latest/plugins/openid-connect.md

@@ -68,7 +68,7 @@ The `openid-connect` Plugin supports the integration with [OpenID Connect (OIDC)
 | set_refresh_token_header | boolean | False | false | | If true and if the refresh token is available, set the value in the `X-Refresh-Token` request header. |
 | session | object | False | | | Session configuration used when `bearer_only` is `false` and the Plugin uses Authorization Code flow. |
 | session.secret | string | True | | 16 or more characters | Key used for session encryption and HMAC operation when `bearer_only` is `false`. |
-| session.cookie_name | string | False | | | Session cookie name. Forwarded to [lua-resty-session](https://github.com/bungle/lua-resty-session#configuration) 4.x as `cookie_name`. |
+| session.cookie_name | string | False | | | Session cookie name. Forwarded to [lua-resty-session](https://github.com/bungle/lua-resty-session#configuration) 4.x as `cookie_name`. When unset, the plugin derives a per-route cookie name so that multiple `openid-connect` routes on the same host sharing one `session.secret` do not overwrite each other's pre-login state. Set it explicitly to share one session cookie across routes. |
 | session.cookie_path | string | False | | | Cookie path scope. Forwarded to lua-resty-session as `cookie_path`. |
 | session.cookie_domain | string | False | | | Cookie domain scope. Forwarded to lua-resty-session as `cookie_domain`. |
 | session.cookie_secure | boolean | False | | | If true, set the `Secure` cookie attribute. Forwarded to lua-resty-session as `cookie_secure`. |

t/plugin/openid-connect-redis.t

@@ -91,6 +91,8 @@ property "port" validation failed: wrong type: expected integer, got string
 
 
 === TEST 3: verify session sharing across routes with Redis (Simulate Refresh Scenario)
+# Routes opt into cross-route session sharing with an explicit shared
+# session.cookie_name; without it each route gets its own cookie name.
 --- http_config
     server {
         listen 11980;
@@ -226,6 +228,7 @@ property "port" validation failed: wrong type: expected integer, got string
                             "discovery": "http://127.0.0.1:16969/.well-known/openid-configuration",
                             "redirect_uri": "http://127.0.0.1/api/route1/callback",
                             "session": {
+                                "cookie_name": "shared_session",
                                 "secret": "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
                                 "storage": "redis",
                                 "redis": {
@@ -262,6 +265,7 @@ property "port" validation failed: wrong type: expected integer, got string
                             "discovery": "http://127.0.0.1:16969/.well-known/openid-configuration",
                             "redirect_uri": "http://127.0.0.1/api/route2/callback",
                             "session": {
+                                "cookie_name": "shared_session",
                                 "secret": "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
                                 "storage": "redis",
                                 "redis": {
@@ -310,7 +314,7 @@ property "port" validation failed: wrong type: expected integer, got string
                 return
             end
 
-            local initial_cookie = get_cookie(res.headers, "session")
+            local initial_cookie = get_cookie(res.headers, "shared_session")
             if not initial_cookie then
                 ngx.say("failed to get initial session cookie")
                 return
@@ -340,7 +344,7 @@ property "port" validation failed: wrong type: expected integer, got string
             end
 
             -- After callback, we get the FINAL authenticated session cookie.
-            local auth_cookie = get_cookie(res.headers, "session")
+            local auth_cookie = get_cookie(res.headers, "shared_session")
             if not auth_cookie then
                 ngx.say("failed to get authenticated session cookie after callback. status: ", res.status)
                 return

t/plugin/openid-connect10.t

@@ -362,3 +362,76 @@ session.cookie.lifetime is deprecated
     }
 --- response_body_like
 .*cookie_same_site.*
+
+
+
+=== TEST 13: sibling routes get distinct default session cookie names
+--- config
+    location /t {
+        content_by_lua_block {
+            local plugin = require("apisix.plugins.openid-connect")
+            local derive = plugin._route_session_cookie_name
+            local conf = { client_id = "shared" }
+            local a = derive(conf, { route_id = "route-A" })
+            local b = derive(conf, { route_id = "route-B" })
+            ngx.say("distinct=", tostring(a ~= b))
+            ngx.say("stable=", tostring(a == derive(conf, { route_id = "route-A" })))
+            ngx.say("prefixed=", tostring(a:find("^session_") ~= nil))
+        }
+    }
+--- response_body
+distinct=true
+stable=true
+prefixed=true
+
+
+
+=== TEST 14: cookie name falls back to client_id when no route id
+--- config
+    location /t {
+        content_by_lua_block {
+            local plugin = require("apisix.plugins.openid-connect")
+            local derive = plugin._route_session_cookie_name
+            local by_client = derive({ client_id = "the-client" }, nil)
+            ngx.say("from_client=", tostring(by_client ~= nil))
+            ngx.say("none=", tostring(derive({}, nil)))
+        }
+    }
+--- response_body
+from_client=true
+none=nil
+
+
+
+=== TEST 15: default cookie name is applied when session.cookie_name is unset
+--- config
+    location /t {
+        content_by_lua_block {
+            local plugin = require("apisix.plugins.openid-connect")
+            local build = plugin._build_session_opts
+            local opts = build({
+                secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+            }, "session_deadbeef")
+            ngx.say("cookie_name=", opts.cookie_name)
+        }
+    }
+--- response_body
+cookie_name=session_deadbeef
+
+
+
+=== TEST 16: operator-set cookie_name wins over the derived default
+--- config
+    location /t {
+        content_by_lua_block {
+            local plugin = require("apisix.plugins.openid-connect")
+            local build = plugin._build_session_opts
+            local opts = build({
+                secret = "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK",
+                cookie_name = "my_session",
+            }, "session_deadbeef")
+            ngx.say("cookie_name=", opts.cookie_name)
+        }
+    }
+--- response_body
+cookie_name=my_session

t/plugin/openid-connect11.t

@@ -0,0 +1,131 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+no_long_string();
+no_root_location();
+no_shuffle();
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: sibling routes sharing session.secret get isolated session cookies
+--- http_config
+    server {
+        listen 16970;
+        server_name localhost;
+
+        location /.well-known/openid-configuration {
+            content_by_lua_block {
+                ngx.header.content_type = "application/json"
+                ngx.say([[
+                {
+                    "issuer": "http://127.0.0.1:16970",
+                    "authorization_endpoint": "http://127.0.0.1:16970/authorize",
+                    "token_endpoint": "http://127.0.0.1:16970/token",
+                    "userinfo_endpoint": "http://127.0.0.1:16970/userinfo",
+                    "jwks_uri": "http://127.0.0.1:16970/jwks"
+                }
+                ]])
+            }
+        }
+
+        location /jwks {
+            content_by_lua_block {
+                ngx.header.content_type = "application/json"
+                ngx.say([[{"keys": []}]])
+            }
+        }
+    }
+--- config
+    location /t {
+        content_by_lua_block {
+            local t = require("lib.test_admin").test
+            local http = require("resty.http")
+            local host = "oidc-isolation.test"
+
+            local function route(uri, redirect)
+                return [=[{
+                    "host": "]=] .. host .. [=[",
+                    "plugins": {
+                        "openid-connect": {
+                            "client_id": "test_client",
+                            "client_secret": "test_secret",
+                            "discovery": "http://127.0.0.1:16970/.well-known/openid-configuration",
+                            "redirect_uri": "]=] .. redirect .. [=[",
+                            "ssl_verify": false,
+                            "session": {
+                                "secret": "jwcE5v3pM9VhqLxmxFOH9uZaLo8u7KQK"
+                            }
+                        }
+                    },
+                    "upstream": {
+                        "nodes": { "127.0.0.1:1980": 1 },
+                        "type": "roundrobin"
+                    },
+                    "uri": "]=] .. uri .. [=["
+                }]=]
+            end
+
+            local code = t('/apisix/admin/routes/1', ngx.HTTP_PUT,
+                           route("/api/low/*", "http://127.0.0.1/api/low/callback"))
+            if code >= 300 then ngx.say("setup low failed: " .. code); return end
+            local code = t('/apisix/admin/routes/2', ngx.HTTP_PUT,
+                           route("/api/high/*", "http://127.0.0.1/api/high/callback"))
+            if code >= 300 then ngx.say("setup high failed: " .. code); return end
+            ngx.sleep(0.5)
+
+            local function cookie_name(headers)
+                local cookies = headers["Set-Cookie"]
+                if type(cookies) == "string" then cookies = { cookies } end
+                for _, c in ipairs(cookies or {}) do
+                    local name = c:match("^([^=]+)=")
+                    if name and name:find("^session") then
+                        return name
+                    end
+                end
+                return nil
+            end
+
+            local httpc = http.new()
+            local base = "http://127.0.0.1:" .. ngx.var.server_port
+
+            local res_a = httpc:request_uri(base .. "/api/low/data",
+                                             {method = "GET", headers = {Host = host}})
+            local res_b = httpc:request_uri(base .. "/api/high/data",
+                                             {method = "GET", headers = {Host = host}})
+
+            local name_a = cookie_name(res_a.headers)
+            local name_b = cookie_name(res_b.headers)
+            ngx.say("status_a=", res_a.status)
+            ngx.say("status_b=", res_b.status)
+            ngx.say("has_names=", tostring(name_a ~= nil and name_b ~= nil))
+            ngx.say("names_differ=", tostring(name_a ~= name_b))
+        }
+    }
+--- request
+GET /t
+--- response_body
+status_a=302
+status_b=302
+has_names=true
+names_differ=true
+--- no_error_log
+[error]