diff --git a/Makefile b/Makefile index 7a7cdb6..2d8673b 100644 --- a/Makefile +++ b/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 diff --git a/README.md b/README.md index 86d8484..5da0e5c 100644 --- a/README.md +++ b/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 ``, 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 `` -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 `` 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 diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 8db5b6f..a7695fe 100644 --- a/lua/resty/saml.lua +++ b/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 diff --git a/src/lua_saml.c b/src/lua_saml.c index f0a7d2f..8c8e653 100644 --- a/src/lua_saml.c +++ b/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); diff --git a/src/saml.h b/src/saml.h index 5d7592e..863372c 100644 --- a/src/saml.h +++ b/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; diff --git a/src/xml.c b/src/xml.c index 61ba856..2822672 100644 --- a/src/xml.c +++ b/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 diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 2bb6dde..96730ec 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -108,6 +108,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== acs = { sp_acs_url = "http://127.0.0.1:1984/acs" }, replay = { replay_dict = "saml_replay" }, replay_short = { replay_dict = "saml_replay", replay_ttl = 90 }, + replay_long = { replay_dict = "saml_replay", replay_ttl = 172800 }, replay_full = { replay_dict = "saml_replay_full" }, replay_pinned = { replay_dict = "saml_replay", @@ -237,6 +238,27 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== -- the module owns this layout; naming it once here keeps a change to -- the scheme from surfacing as a comparison against nil + -- fill a zone to refusal, so the next safe_add answers no memory. + -- Hands back whether it truly got there, for the block to assert + function fill_dict(name) + local dict = ngx.shared[name] + dict:flush_all() + dict:flush_expired() + local filler = string.rep("x", 256) + local i, ok, err = 0, true, nil + while ok do + ok, err = dict:safe_set("filler-" .. i, filler, 600) + if ok then i = i + 1 end + if i > 5000 then break end + end + local j = 0 + while dict:safe_add("small-" .. j, true, 600) do + j = j + 1 + if j > 5000 then break end + end + return i > 0 and j > 0 and err == "no memory" + end + function replay_key(id, issuer) return "sp|" .. (issuer or IDP) .. "|" .. id end @@ -574,9 +596,10 @@ offers no subject confirmation this SP can satisfy ngx.say(login_with("plain", saml_response({ conditions = conditions({ body = "" }), }))) - -- OneTimeUse asks this SP to remember which assertions it has spent + -- OneTimeUse is always valid (Core 2.5.1.5); with no replay_dict it + -- asks for a record this SP does not keep, which is said, not refused ngx.say(login_with("plain", saml_response({ - conditions = conditions({ body = "" }), + id = "single", conditions = conditions({ body = "" }), }))) -- and a condition it has never heard of asks who knows what ngx.say(login_with("plain", saml_response({ @@ -589,10 +612,10 @@ offers no subject confirmation this SP can satisfy } --- response_body 302 / -401 nil +302 / 401 nil --- error_log eval -[qr/carries a condition this SP cannot satisfy: OneTimeUse/, +[qr/\[warn\] .* assertion single from https:\/\/idp\.example\.com carries OneTimeUse, which this SP cannot enforce without replay_dict/, qr/carries a condition this SP cannot satisfy: Condition/] @@ -622,6 +645,9 @@ response from IdP is addressed to http://evil.example.com/acs } --- response_body 302 / +--- no_error_log +[error] +cannot enforce without replay_dict @@ -631,7 +657,7 @@ response from IdP is addressed to http://evil.example.com/acs content_by_lua_block { local xml = sign_doc(response( assertion({ id = "a1", conditions = conditions({ not_on_or_after = "2026-07-21T00:00:00Z", - body = audience("sp") }) }) .. + body = audience("sp") .. "" }) }) .. assertion({ id = "a2", name_id = "second@example.com", confirmations = confirmation({ recipient = ACS }) }))) local doc, err = parse(xml) @@ -641,14 +667,15 @@ response from IdP is addressed to http://evil.example.com/acs ngx.say(a.id, " conditions=", tostring(a.has_conditions), " expires=", tostring(a.not_on_or_after), " audiences=", #a.audience_restrictions, - " confirmations=", #a.subject_confirmations) + " confirmations=", #a.subject_confirmations, + " one_time_use=", tostring(a.one_time_use)) end ngx.say("destination: ", tostring(saml.doc_destination(doc))) } } --- response_body -a1 conditions=true expires=2026-07-21T00:00:00Z audiences=1 confirmations=0 -a2 conditions=false expires=nil audiences=0 confirmations=1 +a1 conditions=true expires=2026-07-21T00:00:00Z audiences=1 confirmations=0 one_time_use=true +a2 conditions=false expires=nil audiences=0 confirmations=1 one_time_use=false destination: nil @@ -1154,28 +1181,16 @@ configured: true --- response_body 302 / capped: true +--- no_error_log +[error] +stays acceptable past its record === TEST 40: a full dict leaves the login working and says so --- config location /t { content_by_lua_block { - local dict = ngx.shared.saml_replay_full - dict:flush_all() - dict:flush_expired() - local filler = string.rep("x", 256) - local i, ok, err = 0, true, nil - while ok do - ok, err = dict:safe_set("filler-" .. i, filler, 600) - if ok then i = i + 1 end - if i > 5000 then break end - end - local j = 0 - while dict:safe_add("small-" .. j, true, 600) do - j = j + 1 - if j > 5000 then break end - end - ngx.say("full: ", i > 0 and j > 0 and err == "no memory") + ngx.say("full: ", fill_dict("saml_replay_full")) -- evicting would take the record away from whoever holds it and -- report it against this request, so this login goes untracked @@ -1185,8 +1200,13 @@ capped: true --- response_body full: true 302 / ---- error_log -in saml_replay_full: no memory, this login is not covered by replay tracking +--- error_log eval +qr/\[error\] .* assertion untracked from https:\/\/idp\.example\.com in saml_replay_full: no memory, this assertion is not tracked, and the login is not refused for it/ +--- no_error_log +[crit] +[alert] +[emerg] +though it carries OneTimeUse === TEST 41: a login refused after the checks leaves the assertion unspent @@ -1333,6 +1353,9 @@ tracked: true --- response_body 302 / fallback: true +--- no_error_log +[error] +stays acceptable past its record === TEST 46: acceptance ends at whichever close comes first @@ -1386,3 +1409,167 @@ earlier: true --- response_body 302 / dated one decides: true + + + +=== TEST 48: with a record, an OneTimeUse assertion is treated like any other +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + local xml = saml_response({ + id = "stamped", + conditions = conditions({ not_on_or_after = at(600), body = "" }), + }) + ngx.say(login_with("replay", xml)) + -- remembered until acceptance ends plus clock_skew, as any other + local ttl = ngx.shared.saml_replay:ttl(replay_key("stamped")) + ngx.say("recorded: ", ttl > 600 and ttl <= 660) + ngx.say(login_with("replay", xml)) + } + } +--- response_body +302 / +recorded: true +401 nil +--- error_log +assertion stamped has been presented already +--- no_error_log +[crit] +[alert] +[emerg] +OneTimeUse + + + +=== TEST 49: a full dict says when the untracked login asked for single use +--- config + location /t { + content_by_lua_block { + ngx.say("full: ", fill_dict("saml_replay_full")) + + -- twice: with no room the login fails open, both times + local xml = saml_response({ + id = "untracked-stamped", + conditions = conditions({ body = "" }), + }) + ngx.say(login_with("replay_full", xml)) + ngx.say(login_with("replay_full", xml)) + } + } +--- response_body +full: true +302 / +302 / +--- error_log eval +qr/\[error\] .* assertion untracked-stamped from https:\/\/idp\.example\.com in saml_replay_full: no memory, this assertion is not tracked though it carries OneTimeUse, and the login is not refused for it/ +--- grep_error_log eval +qr/could not remember assertion untracked-stamped [^,]*, this assertion is not tracked/ +--- grep_error_log_out +could not remember assertion untracked-stamped from https://idp.example.com in saml_replay_full: no memory, this assertion is not tracked +could not remember assertion untracked-stamped from https://idp.example.com in saml_replay_full: no memory, this assertion is not tracked +--- no_error_log +[crit] +[alert] +[emerg] +stays acceptable past its record + + + +=== TEST 50: an OneTimeUse assertion that outlives its record says so +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- nothing bounds it, so the record falls back to replay_ttl + ngx.say(login_with("replay", saml_response({ + id = "stamped-unbounded", + conditions = conditions({ body = "" }), + }))) + -- valid for years, so the record is capped at a day + ngx.say(login_with("replay", saml_response({ + id = "stamped-forever", + conditions = conditions({ not_on_or_after = "9999-12-31T23:59:59Z", + body = "" }), + }))) + } + } +--- response_body +302 / +302 / +--- error_log eval +[qr/\[warn\] .* assertion stamped-unbounded from https:\/\/idp\.example\.com carries OneTimeUse but stays acceptable past its record, which lapses in 600 seconds/, +qr/\[warn\] .* assertion stamped-forever from https:\/\/idp\.example\.com carries OneTimeUse but stays acceptable past its record, which lapses in 86400 seconds/] +--- no_error_log +[error] +[crit] +[alert] +[emerg] + + + +=== TEST 51: OneTimeUse is read wherever it sits among the conditions +--- config + location /t { + content_by_lua_block { + local unknown = 'sp' + for _, body in ipairs({ "" .. unknown, unknown .. "", + unknown }) do + local doc, err = parse(sign_doc(response(assertion({ + id = "ordered", conditions = conditions({ body = body }), + })))) + if err then ngx.say("err: ", err) return end + local a = saml.doc_assertions(doc)[1] + ngx.say("one_time_use=", tostring(a.one_time_use), + " unknown_condition=", tostring(a.unknown_condition)) + end + } + } +--- response_body +one_time_use=true unknown_condition=Condition +one_time_use=true unknown_condition=Condition +one_time_use=false unknown_condition=Condition + + + +=== TEST 52: without a record, an OneTimeUse assertion is accepted again +--- config + location /t { + content_by_lua_block { + local xml = saml_response({ + id = "stamped-untracked", + conditions = conditions({ not_on_or_after = at(600), body = "" }), + }) + ngx.say(login_with("plain", xml)) + ngx.say(login_with("plain", xml)) + } + } +--- response_body +302 / +302 / +--- error_log eval +qr/\[warn\] .* assertion stamped-untracked from https:\/\/idp\.example\.com carries OneTimeUse, which this SP cannot enforce without replay_dict/ +--- no_error_log +[error] +[crit] +[alert] +[emerg] + + + +=== TEST 54: the operator's replay_ttl is taken as given, past the day too +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- the day cap bounds what the assertion claims; this value is + -- nobody's claim but the operator's + ngx.say(login_with("replay_long", saml_response({ id = "kept-long" }))) + local ttl = ngx.shared.saml_replay:ttl(replay_key("kept-long")) + ngx.say("kept: ", ttl > 172700 and ttl <= 172800) + } + } +--- response_body +302 / +kept: true