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 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,43 @@ 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, its issuer and the zone, saying too when it carried `OneTimeUse`
+and that the login is not refused for it, 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
-the delivery window is minutes and the assertion window at most an hour, and the
+inside that window; one still acceptable more than a day from now is remembered
+for the day 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 — a zone with no room among them. Without it, the login is accepted
+and a line at `warn` level names the assertion, its issuer and `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.
+
+Consuming the assertion into a session is the immediate use Core 2.5.1.5 asks for;
+what the login retains afterwards lives in that session, whose lifetime follows
+`SessionNotOnOrAfter` where the IdP sends it and the session library's own timeouts
+where it does not. `OneTimeUse` does not shorten a session: the profile gives
+session lifetime its own instrument, and this SP honours that one where it is sent.
+
+**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 or ""),
+                " 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,28 @@ 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.
+        -- Stated as the property itself, the stored record falling short of
+        -- the lifetime, so no revision of the clamp can leave this line behind
+        local outlives = usable_until == nil or ttl < usable_until + skew - now
+
         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 +635,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 or ""), " in ", dict_name, ": ", add_err,
+                ", this assertion is not tracked",
+                assertion.one_time_use and " though it carries OneTimeUse" or "",
+                ", and the login is not refused for it")
         end
     end
 
+    -- said only once every record stands: a warn spoken sooner would describe
+    -- a record the rollback above may yet take back
+    if warned then
+        for _, w in ipairs(warned) do
+            ngx.log(ngx.WARN, "assertion ", loggable(w.id), " from ", loggable(w.issuer or ""),
+                " carries OneTimeUse but stays acceptable past its record, which lapses in ",
+                w.ttl, " seconds")
+        end
+    end
     return true
 end
 
@@ -705,7 +740,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 +784,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 +980,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