diff --git a/ChangeLog b/ChangeLog index da6e721..88bd2f6 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,4 +1,6 @@ 07/01/2026 +- propagate session destroy and closed-mode revocation store failures from + logout and session start instead of continuing as if logout succeeded - do not use the shared JWT verification cache for bearer JWT validation calls that pass additional claim validators, preventing a token accepted by one validator set from bypassing a stricter validator set on cache hit. diff --git a/lib/resty/openidc.lua b/lib/resty/openidc.lua index f50210a..622c78c 100644 --- a/lib/resty/openidc.lua +++ b/lib/resty/openidc.lua @@ -292,6 +292,37 @@ local function is_session_present(session) return session ~= nil and next(session:get_data()) ~= nil end +-- start a session; propagate closed-mode revocation store failures +local function openidc_start_session(session_opts) + local session, err = r_session.start(session_opts) + if not session then + log(ERROR, "Error starting session: " .. (err or "unknown error")) + return nil, err + end + + -- session uses either message when revocation_fail_mode is closed and the + -- revocation store is unavailable (on open or destroy) + if err and session.revocation_fail_mode == "closed" + and (err:find("unable to check session revocation", 1, true) + or err:find("unable to mark session revoked", 1, true)) then + log(ERROR, "Error starting session: " .. err) + return session, err + end + + return session, nil +end + +-- destroy an open session; propagate destroy() failures +local function openidc_destroy_session(session) + if session and session.state == "open" and is_session_present(session) then + local ok, err = session:destroy() + if not ok then + log(ERROR, "failed to destroy session: " .. err) + return err + end + end +end + -- set value in server-wide cache if available local function openidc_cache_set(type, key, value, exp) local dict = ngx.shared[type] @@ -1870,8 +1901,9 @@ local function openidc_logout(opts, session) end end - if is_session_present(session) then - session:destroy() + err = openidc_destroy_session(session) + if err then + return err end if opts.revoke_tokens_on_logout then @@ -2047,11 +2079,9 @@ function openidc.authenticate(opts, target_url, unauth_action, session_or_opts) 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 + session, err = openidc_start_session(session_or_opts) + if err then + return nil, err, target_url, session end end @@ -2084,7 +2114,10 @@ function openidc.authenticate(opts, target_url, unauth_action, session_or_opts) return nil, err, session:get("original_url"), session end - openidc_logout(opts, session) + err = openidc_logout(opts, session) + if err then + return nil, err, target_url, session + end return nil, nil, target_url, session end @@ -2185,7 +2218,13 @@ end -- get a valid access_token (eventually refreshing the token), or nil if there's no valid access_token function openidc.access_token(opts, session_opts) - local session = r_session.start(session_opts) + local session, err = openidc_start_session(session_opts) + if err then + if session then + session:close() + end + return nil, err + end local token, err = openidc_access_token(opts, session, true) session:close() return token, err diff --git a/lua-resty-openidc-1.9.0-1.rockspec b/lua-resty-openidc-1.9.0-1.rockspec index 3be0206..51ae2d4 100644 --- a/lua-resty-openidc-1.9.0-1.rockspec +++ b/lua-resty-openidc-1.9.0-1.rockspec @@ -24,7 +24,7 @@ description = { dependencies = { "lua >= 5.1", "lua-resty-http >= 0.08", - "lua-resty-session >= 4.0.3", + "lua-resty-session == 4.2.0-1", "lua-resty-jwt >= 0.2.0", "lua-resty-openssl >= 1.8.0" } diff --git a/tests/Dockerfile b/tests/Dockerfile index 44355dd..9d6de97 100644 --- a/tests/Dockerfile +++ b/tests/Dockerfile @@ -1,13 +1,13 @@ FROM openresty/openresty:focal # install dependencies -RUN ["luarocks", "install", "lua-resty-session", "4.0.3"] +RUN ["luarocks", "install", "lua-resty-session", "4.2.0-1"] RUN ["luarocks", "install", "lua-resty-http"] RUN ["luarocks", "install", "lua-resty-jwt"] # install test dependencies RUN apt-get update \ - && apt-get install -y --no-install-recommends git \ + && apt-get install -y --no-install-recommends ca-certificates git \ && rm -rf /var/lib/apt/lists/* RUN ["luarocks", "install", "busted"] RUN ["luarocks", "install", "LuaSocket"] diff --git a/tests/spec/access_token_access_spec.lua b/tests/spec/access_token_access_spec.lua index bd2347b..174f9b3 100644 --- a/tests/spec/access_token_access_spec.lua +++ b/tests/spec/access_token_access_spec.lua @@ -39,6 +39,59 @@ describe("if there is an active non-expired login", function() end) end) +describe("if revocation_fail_mode is closed and the revocation store read fails", function() + test_support.start_server({ + revocation_test = { + fail_mode = "closed", + get_fails_after = 1, + }, + }) + teardown(test_support.stop_server) + + local _, _, cookies = test_support.login() + local content_table = {} + local _, status = http.request({ + url = "http://localhost/access_token", + redirect = false, + headers = { cookie = cookies }, + sink = ltn12.sink.table(content_table) + }) + local body = table.concat(content_table) + + it("returns the session start failure", function() + assert.are.equals(401, status) + assert.truthy(string.find(body, "unable to check session revocation", 1, true)) + end) + + it("does not return the access token", function() + assert.is_nil(string.find(body, "a_token", 1, true)) + end) +end) + +describe("if revocation_fail_mode is open and the revocation store read fails", function() + test_support.start_server({ + revocation_test = { + fail_mode = "open", + get_fails_after = 1, + }, + }) + teardown(test_support.stop_server) + + local _, _, cookies = test_support.login() + local content_table = {} + local _, status = http.request({ + url = "http://localhost/access_token", + redirect = false, + headers = { cookie = cookies }, + sink = ltn12.sink.table(content_table) + }) + + it("returns the access token", function() + assert.are.equals(200, status) + assert.are.equals("a_token\n", table.concat(content_table)) + end) +end) + describe("if there is an active non-expired login but access token is not stored in session", function() test_support.start_server({ access_token_opts = { diff --git a/tests/spec/logout_revocation_spec.lua b/tests/spec/logout_revocation_spec.lua new file mode 100644 index 0000000..a1d591d --- /dev/null +++ b/tests/spec/logout_revocation_spec.lua @@ -0,0 +1,135 @@ +local http = require("socket.http") +local ltn12 = require("ltn12") +local test_support = require("test_support") +require 'busted.runner'() + +describe("when session startup returns no session", function() + test_support.start_server({ + session_start_fails = true, + }) + teardown(test_support.stop_server) + + local response_body = {} + local _, status = http.request({ + url = "http://127.0.0.1/default/t", + redirect = false, + sink = ltn12.sink.table(response_body), + }) + + it("returns the startup error without authenticating the request", function() + assert.are.equals(401, status) + assert.truthy(string.find(table.concat(response_body), "session start failed", 1, true)) + end) +end) + +describe("when revocation_fail_mode is closed and the revocation store read fails", function() + test_support.start_server({ + revocation_test = { + fail_mode = "closed", + get_fails_after = 1, + }, + }) + teardown(test_support.stop_server) + + local _, _, cookie = test_support.login() + + local response_body = {} + local _, status = http.request({ + url = "http://127.0.0.1/default/t", + headers = { cookie = cookie }, + redirect = false, + sink = ltn12.sink.table(response_body), + }) + local body = table.concat(response_body) + + it("propagates the session start failure", function() + assert.are.equals(401, status) + assert.truthy(string.find(body, "unable to check session revocation", 1, true)) + end) + + it("does not authenticate the request", function() + assert.is_nil(string.find(body, "hello, world!", 1, true)) + end) +end) + +describe("when revocation_fail_mode is open and the revocation store read fails", function() + test_support.start_server({ + revocation_test = { + fail_mode = "open", + get_fails_after = 1, + }, + }) + teardown(test_support.stop_server) + + local _, _, cookie = test_support.login() + + local response_body = {} + local _, status = http.request({ + url = "http://127.0.0.1/default/t", + headers = { cookie = cookie }, + redirect = false, + sink = ltn12.sink.table(response_body), + }) + + it("authenticates the request", function() + assert.are.equals(200, status) + assert.truthy(string.find(table.concat(response_body), "hello, world!", 1, true)) + end) +end) + +describe("when revocation_fail_mode is closed and the revocation store is unreachable during logout", function() + test_support.start_server({ + revocation_test = { + fail_mode = "closed", + set_fails = true, + }, + }) + teardown(test_support.stop_server) + + local _, _, cookie = test_support.login() + + local response_body = {} + local _, status = http.request({ + url = "http://127.0.0.1/default/logout", + headers = { cookie = cookie }, + redirect = false, + sink = ltn12.sink.table(response_body), + }) + local body = table.concat(response_body) + + it("propagates destroy failure", function() + assert.are.equals(401, status) + assert.truthy(string.find(body, "unable to mark session revoked", 1, true)) + end) + + it("leaves the session valid for replay", function() + local _, replay_status = http.request({ + url = "http://127.0.0.1/default/t", + headers = { cookie = cookie }, + redirect = false, + }) + assert.are.equals(200, replay_status) + end) +end) + +describe("when revocation_fail_mode is open and the revocation store is unreachable during logout", function() + test_support.start_server({ + revocation_test = { + fail_mode = "open", + set_fails = true, + }, + }) + teardown(test_support.stop_server) + + local _, _, cookie = test_support.login() + + local _, status = http.request({ + url = "http://127.0.0.1/default/logout", + headers = { cookie = cookie }, + redirect = false, + }) + + it("logout still completes", function() + assert.are.equals(200, status) + end) +end) diff --git a/tests/spec/test_support.lua b/tests/spec/test_support.lua index e173f91..2250a15 100644 --- a/tests/spec/test_support.lua +++ b/tests/spec/test_support.lua @@ -95,6 +95,12 @@ local DEFAULT_SHARE_OIDC_OPTS = "false" local DEFAULT_DELAY_RESPONSE = "0" +local DEFAULT_REVOCATION_TEST_ENABLED = "false" +local DEFAULT_REVOCATION_FAIL_MODE = '"closed"' +local DEFAULT_REVOCATION_SET_FAILS = "false" +local DEFAULT_REVOCATION_GET_FAILS_AFTER = "nil" +local DEFAULT_SESSION_START_FAILS = "false" + local DEFAULT_INIT_TEMPLATE = [[ local test_globals = {} local sign_secret = [=[ @@ -103,6 +109,11 @@ JWT_SIGN_SECRET]=] if os.getenv('coverage') then require("luacov.runner")("/spec/luacov/settings.luacov") end +if SESSION_START_FAILS then + require("resty.session").start = function() + return nil, "session start failed" + end +end test_globals.oidc = require "resty.openidc" test_globals.cjson = require "cjson" if FIXED_NGX_TIME ~= nil then @@ -146,6 +157,30 @@ test_globals.body_decorator = function(req) return req end test_globals.jwks = [=[JWK]=] +test_globals.session_opts = nil +if REVOCATION_TEST_ENABLED then + local revoked = ngx.shared.revocation_test + test_globals.session_opts = { + storage = "cookie", + revocation_fail_mode = REVOCATION_FAIL_MODE, + revocation = { + set = function(_, key, value, ttl) + if REVOCATION_SET_FAILS then + return nil, "connection refused" + end + revoked:set(key, value, ttl) + return true + end, + get = function(_, key) + local calls = revoked:incr("get_calls", 1, 0) + if REVOCATION_GET_FAILS_AFTER and calls > REVOCATION_GET_FAILS_AFTER then + return nil, "connection refused" + end + return revoked:get(key) + end, + }, + } +end return test_globals ]] @@ -163,6 +198,7 @@ http { lua_package_path '~/lua/?.lua;/tmp/server/conf/?.lua;;'; lua_shared_dict discovery 1m; lua_shared_dict jwt_verification 1m; + lua_shared_dict revocation_test 1m; init_by_lua_block { test_globals = require("test_globals") } @@ -213,7 +249,7 @@ http { if opts.decorate then opts.http_request_decorator = opts.decorate == "body" and test_globals.body_decorator or test_globals.query_decorator end - local res, err, target, session = test_globals.oidc.authenticate(opts, nil, UNAUTH_ACTION) + local res, err, target, session = test_globals.oidc.authenticate(opts, nil, UNAUTH_ACTION, test_globals.session_opts) if err then ngx.status = 401 ngx.log(ngx.ERR, "authenticate failed: " .. err) @@ -514,10 +550,11 @@ http { location /access_token { content_by_lua_block { - local access_token, err = test_globals.oidc.access_token(ACCESS_TOKEN_OPTS) + local access_token, err = test_globals.oidc.access_token(ACCESS_TOKEN_OPTS, test_globals.session_opts) if not access_token then ngx.status = 401 ngx.log(ngx.ERR, "access_token error: " .. (err or 'no message')) + ngx.say("access_token failed: " .. (err or 'no message')) else ngx.header.content_type = 'text/plain' ngx.say(access_token) @@ -654,6 +691,14 @@ local function write_template(out, template, custom_config) :gsub("FIXED_NGX_TIME", custom_config["fixed_ngx_time"] or "nil") :gsub("UNAUTH_ACTION", custom_config["unauth_action"] and ('"' .. custom_config["unauth_action"] .. '"') or DEFAULT_UNAUTH_ACTION) :gsub("SHARE_OIDC_OPTS", custom_config["share_oidc_opts"] and "true" or DEFAULT_SHARE_OIDC_OPTS) + :gsub("REVOCATION_TEST_ENABLED", custom_config["revocation_test"] and "true" or DEFAULT_REVOCATION_TEST_ENABLED) + :gsub("REVOCATION_FAIL_MODE", custom_config["revocation_test"] and + ('"' .. (custom_config["revocation_test"].fail_mode or "closed") .. '"') or DEFAULT_REVOCATION_FAIL_MODE) + :gsub("REVOCATION_SET_FAILS", custom_config["revocation_test"] and + (custom_config["revocation_test"].set_fails and "true" or "false") or DEFAULT_REVOCATION_SET_FAILS) + :gsub("REVOCATION_GET_FAILS_AFTER", custom_config["revocation_test"] and + tostring(custom_config["revocation_test"].get_fails_after) or DEFAULT_REVOCATION_GET_FAILS_AFTER) + :gsub("SESSION_START_FAILS", custom_config["session_start_fails"] and "true" or DEFAULT_SESSION_START_FAILS) out:write(content) end