diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 859c523..1e94c97 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), @@ -651,7 +655,7 @@ local function spend_assertions(dict, dict_name, opts, assertions, expected, now w.ttl, " seconds") end end - return true + return true, spent end @@ -782,13 +786,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, self.replay_dict_name, + local spent_ok, spent_or_err = spend_assertions(self.replay_dict, self.replay_dict_name, 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) @@ -802,7 +808,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 ea7f2bf..ca8a892 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 @@ -114,6 +117,8 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== replay_dict = "saml_replay", idp_issuers = { "https://elsewhere.example.com" }, }, + save_fails_start = {}, + save_fails_replay = { replay_dict = "saml_replay" }, } SPS = {} @@ -133,6 +138,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 @@ -206,7 +214,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) @@ -302,6 +310,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() @@ -1411,8 +1429,44 @@ earlier: true 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 + -=== TEST 48: with a record, an OneTimeUse assertion is treated like any other +=== TEST 50: with a record, an OneTimeUse assertion is treated like any other --- config location /t { content_by_lua_block { @@ -1442,7 +1496,7 @@ OneTimeUse -=== TEST 49: a full dict says when the untracked login asked for single use +=== TEST 51: a full dict says when the untracked login asked for single use --- config location /t { content_by_lua_block { @@ -1476,7 +1530,7 @@ stays acceptable past its record -=== TEST 50: an OneTimeUse assertion that outlives its record says so +=== TEST 52: an OneTimeUse assertion that outlives its record says so --- config location /t { content_by_lua_block { @@ -1508,7 +1562,7 @@ qr/\[warn\] .* assertion stamped-forever from https:\/\/idp\.example\.com carrie -=== TEST 51: OneTimeUse is read wherever it sits among the conditions +=== TEST 53: OneTimeUse is read wherever it sits among the conditions --- config location /t { content_by_lua_block { @@ -1533,7 +1587,7 @@ one_time_use=false unknown_condition=Condition -=== TEST 52: without a record, an OneTimeUse assertion is accepted again +=== TEST 54: without a record, an OneTimeUse assertion is accepted again --- config location /t { content_by_lua_block { @@ -1558,7 +1612,7 @@ qr/\[warn\] .* assertion stamped-untracked from https:\/\/idp\.example\.com carr -=== TEST 54: the operator's replay_ttl is taken as given, past the day too +=== TEST 55: the operator's replay_ttl is taken as given, past the day too --- config location /t { content_by_lua_block {