fix: accept OneTimeUse, and let the replay record honour it

Makefile

@@ -37,14 +37,14 @@ test: build deps/
 clean:
 	rm -rf *.so *.o xmlsec1-$(XMLSEC_VER)*
 
-saml.o: src/*.c
+saml.o: src/*.c src/*.h Makefile
 	$(CC) -c $(CFLAGS_ALL) -o saml.o src/saml.c
 
-lua_saml.o: src/lua_saml.c
+lua_saml.o: src/lua_saml.c src/*.h Makefile
 	$(CC) -c $(CFLAGS_ALL) -I$(LUA_INCDIR) -Isrc/ -o $@ $<
 
-saml.so: lua_saml.o saml.o
-	$(CC) -o $@ $^ $(LDFLAGS_ALL)
+saml.so: lua_saml.o saml.o $(XMLSEC1_STATIC_LIBS) Makefile
+	$(CC) -o $@ lua_saml.o saml.o $(LDFLAGS_ALL)
 
 ### install:      Install the library to runtime
 .PHONY: install

README.md

@@ -68,6 +68,10 @@ local saml = resty_saml.new(opts)
 
 `opts` is a table of below items:
 
+`new` keeps `opts` by reference and reads it for the SP's whole life: hand the
+table over and do not mutate it afterwards. An embedder whose configuration table
+is shared or reused passes a copy (`core.table.deepcopy(conf)` in APISIX).
+
 | key      | type | default value      | Description |
 | ----------- | ----------- | ----------- | ----------- |
 | `sp_issuer`      | string       | None      | SP name to access IdP.       |
@@ -128,25 +132,33 @@ long as that assertion could still be used. A response normally carries one, so
 taking ten logins a second against an IdP issuing ten-minute assertions holds around
 six thousand entries at once: `1m` is too small for that and a busy deployment wants
 more. A zone with no room leaves that assertion untracked and logs an error naming
-the assertion and the zone, rather than evicting an entry that is still protecting
-somebody else. A response carrying several assertions can end up partly tracked,
+the assertion and the zone, saying too when that assertion carried `OneTimeUse`,
+rather than evicting an entry that is still protecting somebody else. A response carrying several assertions can end up partly tracked,
 which is the safe direction: a later replay still collides on whichever of them was
 recorded.
 
 **The record is bounded even where acceptance is not.** An assertion with no usable
 expiry is remembered for `replay_ttl` and accepted for good, so it is refusable only
 inside that window; one the IdP made valid beyond a day is remembered for the day
-and accepted again past it. Both need an IdP far outside shipped defaults, where
+and accepted again past it. Where either happens to an assertion carrying
+`<saml:OneTimeUse/>`, the login says so at `warn` level, since the single use its
+IdP asked for ends with the record. Both need an IdP far outside shipped defaults, where
 the delivery window is minutes and the assertion window at most an hour, and the
 alternative is a record nothing reclaims. The limit an operator can move is
 `replay_ttl`; the day cap is fixed.
 
-**Two things it deliberately does not do.** An assertion carrying `<saml:OneTimeUse/>`
-is still refused outright, so an IdP asking for exactly this protection cannot log in
-even with the option on; that is tracked separately and the two do not meet yet. And
-re-submitting a response that already logged in is refused, which is what a browser
-does when it loses the redirect that ends a login. Returning to the application starts
-a fresh login, and the IdP will not ask for a password again.
+**This is what `<saml:OneTimeUse/>` asks for.** An IdP stamps that condition on an
+assertion to ask the SP to keep exactly this record. SAML Core 2.5.1.5 makes the
+condition always valid, a condition on use rather than on validity, so the login goes
+through with or without the option. With it, the assertion is single-use within the
+bounds above, the zone with no room included. Without it, the login is accepted and a line at `warn` level names `replay_dict`,
+so an IdP that asks for this is the signal to set it; a deployment logging at `error`
+or above does not see it.
+
+**One thing it deliberately does not do.** Re-submitting a response that already logged
+in is refused, which is what a browser does when it loses the redirect that ends a
+login. Returning to the application starts a fresh login, and the IdP will not ask for
+a password again.
 
 #### Seeding the worker
 

lua/resty/saml.lua

@@ -325,7 +325,8 @@ local DEFAULT_REPLAY_TTL = 600
 
 -- and how long any assertion is remembered at most, whatever it claims. An
 -- assertion valid for years would pin a slot the dict never reclaims, and
--- nobody is still trying to complete that login a day later.
+-- nobody is still trying to complete that login a day later. It bounds the
+-- IdP's window, never replay_ttl: that one is the operator's own choice
 local MAX_REPLAY_TTL = 86400
 
 local function time_bounds_ok(not_before, not_on_or_after, now, skew)
@@ -405,7 +406,7 @@ end
 
 -- Every top-level assertion the verified signature left in the document is one
 -- the readers draw identity from, so every one of them has to hold up.
-local function assertions_acceptable(opts, assertions, expected, now)
+local function assertions_acceptable(opts, assertions, expected, now, replay_dict)
     local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW
     local accepted = opts.sp_audiences or { opts.sp_issuer }
 
@@ -419,6 +420,17 @@ local function assertions_acceptable(opts, assertions, expected, now)
                 assertion.unknown_condition
         end
 
+        -- Core 2.5.1.5: OneTimeUse is always valid, and asks the SP to keep a
+        -- record of the assertions it has spent. replay_dict is that record;
+        -- without it the IdP's request goes unmet, and the operator is told
+        -- what to configure rather than the user refused. The handle is the
+        -- one the last gate enforces on, so the two cannot disagree
+        if assertion.one_time_use and not replay_dict then
+            ngx.log(ngx.WARN, "assertion ", loggable(assertion.id),
+                " from ", loggable(assertion.issuer),
+                " carries OneTimeUse, which this SP cannot enforce without replay_dict")
+        end
+
         local ok, err = time_bounds_ok(assertion.not_before, assertion.not_on_or_after, now, skew)
         if not ok then
             return false, where .. err
@@ -579,9 +591,10 @@ end
 -- protecting somebody else's login, which is what add would do on its own: the
 -- entry it takes belongs to another user, the login it stops protecting is
 -- theirs, and the warning is reported against whoever needed the space.
-local function spend_assertions(dict, opts, assertions, expected, now)
+local function spend_assertions(dict, dict_name, opts, assertions, expected, now)
     local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW
     local spent = {}
+    local warned
 
     for _, assertion in ipairs(assertions) do
         if not assertion.id then
@@ -592,17 +605,27 @@ local function spend_assertions(dict, opts, assertions, expected, now)
         local usable_until = last_moment_usable(assertion, expected)
         if usable_until then
             ttl = usable_until + skew - now
-        end
-        if ttl < 1 then
-            ttl = 1
-        elseif ttl > MAX_REPLAY_TTL then
-            ttl = MAX_REPLAY_TTL
+            if ttl < 1 then
+                ttl = 1
+            elseif ttl > MAX_REPLAY_TTL then
+                ttl = MAX_REPLAY_TTL
+            end
         end
 
+        -- the record is bounded where acceptance is not, so past it the
+        -- assertion is accepted again. An IdP that asked for single use is
+        -- told, since it is the IdP's window that made the record fall short.
+        -- Weighed before the clamp: a window of exactly the cap is covered
+        local outlives = usable_until == nil or usable_until + skew - now > MAX_REPLAY_TTL
+
         local key = replay_key(opts, assertion)
         local added, add_err = dict:safe_add(key, true, ttl)
         if added then
             spent[#spent + 1] = key
+            if assertion.one_time_use and outlives then
+                warned = warned or {}
+                warned[#warned + 1] = { id = assertion.id, issuer = assertion.issuer, ttl = ttl }
+            end
         elseif add_err == "exists" then
             -- this response authenticates nobody, so the assertions already
             -- taken from it are handed back rather than left spent
@@ -611,12 +634,23 @@ local function spend_assertions(dict, opts, assertions, expected, now)
             end
             return false, "assertion " .. assertion.id .. " has been presented already"
         else
-            ngx.log(ngx.ERR, "could not remember assertion ", loggable(assertion.id), " in ",
-                opts.replay_dict, ": ", add_err,
-                ", this login is not covered by replay tracking")
+            ngx.log(ngx.ERR, "could not remember assertion ", loggable(assertion.id),
+                " from ", loggable(assertion.issuer), " in ", dict_name, ": ", add_err,
+                ", this assertion is not tracked",
+                assertion.one_time_use and " though it carries OneTimeUse" or "")
         end
     end
 
+    -- said only once every record stands: sooner would describe a record the
+    -- add may yet refuse, or one the rollback above takes back. The facts are
+    -- what was kept, so the line is built only where the level prints it
+    if warned then
+        for _, w in ipairs(warned) do
+            ngx.log(ngx.WARN, "assertion ", loggable(w.id), " from ", loggable(w.issuer),
+                " carries OneTimeUse but stays acceptable past its record, which lapses in ",
+                w.ttl, " seconds")
+        end
+    end
     return true
 end
 
@@ -705,7 +739,8 @@ local function login_callback(self, opts)
     end
 
     local now = ngx.time()
-    local acceptable, reason = assertions_acceptable(opts, assertions, expected, now)
+    local acceptable, reason = assertions_acceptable(opts, assertions, expected, now,
+        self.replay_dict)
     if not acceptable then
         ngx.log(ngx.ERR, "response from IdP rejected: ", loggable(reason))
         ngx.exit(ngx.HTTP_UNAUTHORIZED)
@@ -748,8 +783,8 @@ 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
     if self.replay_dict then
-        local unused, used_reason = spend_assertions(self.replay_dict, opts, assertions,
-            expected, now)
+        local unused, used_reason = 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))
             ngx.exit(ngx.HTTP_UNAUTHORIZED)
@@ -944,6 +979,9 @@ function _M.new(opts)
         if obj.replay_dict == nil then
             error("no lua_shared_dict named " .. opts.replay_dict, 2)
         end
+        -- the handle carries no name accessor, so the name it was resolved
+        -- from rides beside it for the diagnostics
+        obj.replay_dict_name = opts.replay_dict
         -- it is half the key, and tostring would turn a missing one into the
         -- literal nil that two deployments would then share
         if type(opts.sp_issuer) ~= "string" then

src/lua_saml.c

@@ -698,6 +698,7 @@ static int doc_assertions(lua_State* L) {
     set_str_field(L, "id", a->id);
     set_str_field(L, "issuer", a->issuer);
     set_bool_field(L, "has_conditions", a->has_conditions);
+    set_bool_field(L, "one_time_use", a->one_time_use);
     set_str_field(L, "not_before", a->not_before);
     set_str_field(L, "not_on_or_after", a->not_on_or_after);
     set_str_field(L, "unknown_condition", a->unknown_condition);

src/saml.h

@@ -59,6 +59,7 @@ typedef struct {
   xmlChar* id;
   xmlChar* issuer;
   int has_conditions;
+  int one_time_use;
   xmlChar* not_before;
   xmlChar* not_on_or_after;
   xmlChar* unknown_condition;

src/xml.c

@@ -396,18 +396,19 @@ static size_t count_assertion_el(xmlNode* parent, const char* name) {
 }
 
 
-// Conditions this SP can actually satisfy. SAML Core 2.5.1 makes an assertion
+// Conditions this SP understands. SAML Core 2.5.1 makes an assertion
 // carrying any other one Indeterminate rather than valid, so everything else is
 // reported for the caller to refuse.
 //
-// ProxyRestriction is here because it binds an IdP issuing on behalf of another
-// IdP and asks nothing of the SP consuming the assertion. OneTimeUse is not,
-// because honouring it means remembering which assertions have been spent, and
-// Core 2.5.1.5 tells a party that cannot keep that record to treat the
-// assertion as invalid.
+// ProxyRestriction binds an IdP issuing on behalf of another IdP and asks
+// nothing of the SP consuming the assertion. OneTimeUse is always valid by
+// Core 2.5.1.5, a condition on use rather than on validity: it asks the SP to
+// keep a record of the assertions it has spent, which the caller has or has
+// not, so it is reported as a flag.
 static int is_known_condition(xmlNode* node) {
   return is_assertion_el(node, "AudienceRestriction") ||
-    is_assertion_el(node, "ProxyRestriction");
+    is_assertion_el(node, "ProxyRestriction") ||
+    is_assertion_el(node, "OneTimeUse");
 }
 
 
@@ -509,6 +510,10 @@ static int read_assertion(xmlDoc* doc, xmlNode* node, saml_assertion_t* a) {
       return -1;
     }
 
+    // answered on its own, so the refusal scan below owes it nothing and
+    // reads the same whatever the order of the conditions
+    a->one_time_use = assertion_child(conditions, "OneTimeUse") != NULL;
+
     for (xmlNode* child = conditions->children; child != NULL; child = child->next) {
       if (child->type == XML_ELEMENT_NODE && !is_known_condition(child)) {
         // the caller refuses the assertion on this name, so losing it would