From ddfad659f15062f3efd48e7df550dd825845c88d Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 1 Sep 2026 13:09:10 +0545 Subject: [PATCH] fix: fail closed when session persistence fails --- lua/resty/saml.lua | 27 ++++++++++++++----- t/assertion-conditions.t | 57 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 77 insertions(+), 7 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 8db5b6f..7e253a9 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -224,7 +224,11 @@ local function login(self, opts, request_uri) sess:set("saml_state", state) sess:set("saml_request_id", request_id) sess:set("request_uri", request_uri) - sess:save() + local saved, save_err = sess:save() + if not saved then + ngx.log(ngx.ERR, "could not save login session: ", save_err) + return ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) + end local query_str, err = create_redirect(self.sign_key, { SAMLRequest = authn_request(opts, request_id), @@ -617,7 +621,7 @@ local function spend_assertions(dict, opts, assertions, expected, now) end end - return true + return true, spent end @@ -747,13 +751,15 @@ local function login_callback(self, opts) -- the last gate: everything that can still refuse this login has run, so -- the assertion is spent only where it actually authenticates somebody + local spent if self.replay_dict then - local unused, used_reason = spend_assertions(self.replay_dict, opts, assertions, + local spent_ok, spent_or_err = spend_assertions(self.replay_dict, opts, assertions, expected, now) - if not unused then - ngx.log(ngx.ERR, "response from IdP rejected: ", loggable(used_reason)) + if not spent_ok then + ngx.log(ngx.ERR, "response from IdP rejected: ", loggable(spent_or_err)) ngx.exit(ngx.HTTP_UNAUTHORIZED) end + spent = spent_or_err end sess:set("authenticated", true) @@ -767,7 +773,16 @@ local function login_callback(self, opts) sess:set("saml_state", nil) sess:set("saml_request_id", nil) sess:set("request_uri", nil) - sess:save() + local saved, save_err = sess:save() + if not saved then + if spent then + for _, key in ipairs(spent) do + self.replay_dict:delete(key) + end + end + ngx.log(ngx.ERR, "could not save authenticated session: ", save_err) + return ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) + end ngx.log(ngx.INFO, "login finish: name_id=", loggable(name_id)) diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 2bb6dde..6ff31dd 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -34,6 +34,9 @@ _EOC_ my $http_config = $block->http_config // <<_EOC_; lua_package_path '$pwd/lua/?.lua;$pwd/deps/share/lua/5.1/?.lua;$pwd/t/?.lua;;'; lua_package_cpath '$pwd/?.so;$pwd/deps/lib/lua/5.1/?.so;;'; + large_client_header_buffers 4 64k; + client_max_body_size 1m; + client_body_buffer_size 128k; # a zone of the same name and size is reused across a reload, so entries # outlive the block that made them under TEST_NGINX_USE_HUP=1. Blocks name @@ -113,6 +116,8 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== replay_dict = "saml_replay", idp_issuers = { "https://elsewhere.example.com" }, }, + save_fails_start = {}, + save_fails_replay = { replay_dict = "saml_replay" }, } SPS = {} @@ -132,6 +137,9 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== } for k, v in pairs(OPTS[name]) do opts[k] = v end SPS[name] = require("resty.saml").new(opts) + if name == "save_fails_start" or name == "save_fails_replay" then + SPS[name].session_config.compression_threshold = 0 + end end return SPS[name] end @@ -205,7 +213,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== '%s%s%s%s', spec.id or "a1", spec.issuer or IDP, spec.name_id or "signed\@example.com", spec.confirmations or "", spec.conditions or "", - authn_statement(spec.session_expires)) + authn_statement(spec.session_expires) .. (spec.attributes or "")) end function response(body, destination, in_response_to) @@ -280,6 +288,16 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== return res.status .. " " .. tostring(res.headers["Location"]) end + function login_start_with_large_uri() + local bits = {} + for i = 1, 1200 do bits[i] = ngx.md5(i) end + local res = assert(require("resty.http").new():request_uri( + "http://127.0.0.1:1984/?q=" .. table.concat(bits), { + headers = { ["X-Test-SP"] = "save_fails_start" }, + })) + return res.status + end + -- hand a response to a session the old code would have left behind function login_with_legacy(name, xml) local httpc = require("resty.http").new() @@ -1386,3 +1404,40 @@ earlier: true --- response_body 302 / dated one decides: true + + +=== TEST 48: a failed login-session save does not redirect to the IdP +--- config + location /t { + content_by_lua_block { + ngx.say(login_start_with_large_uri()) + } + } +--- response_body +500 +--- error_log +could not save login session: cookie size limit exceeded + + +=== TEST 49: a failed authenticated-session save returns replay entries +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + local bits = {} + for i = 1, 1200 do bits[i] = ngx.md5(i) end + local xml = saml_response({ + id = "save-failed", + attributes = '' .. + '' .. table.concat(bits) .. + '', + }) + ngx.say(login_with("save_fails_replay", xml)) + ngx.say("remembered: ", ngx.shared.saml_replay:get(replay_key("save-failed")) ~= nil) + } + } +--- response_body +500 nil +remembered: false +--- error_log +could not save authenticated session: cookie size limit exceeded