fix: fail closed when session persistence fails

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))
 

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==
                 '<saml:Subject><saml:NameID>%s</saml:NameID>%s</saml:Subject>%s%s</saml:Assertion>',
                 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 = '<saml:AttributeStatement><saml:Attribute Name="large">' ..
+                    '<saml:AttributeValue>' .. table.concat(bits) ..
+                    '</saml:AttributeValue></saml:Attribute></saml:AttributeStatement>',
+            })
+            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 {