From b640cdb92941c1ca7dadb202249275e9a5e4bd23 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:52:25 +0545 Subject: [PATCH 01/19] fix: weigh the conditions an assertion attaches to itself An assertion says when it is good, for whom it was issued and where it may be presented. None of that was read: a verified signature was the whole of the check, so an assertion never expired and one minted for another SP in the same federation was accepted here as-is. Conditions/@NotBefore and @NotOnOrAfter now bound the assertion, every AudienceRestriction has to name this SP, SubjectConfirmationData has to be addressed here and still open, and Response/@Destination has to be this endpoint. A constraint the IdP did not send is not invented, so an IdP that omits AudienceRestriction keeps working. Timestamps are converted with plain civil-date arithmetic. os.time reads its table as local time, which shifted every SAML timestamp by the machine's UTC offset. --- README.md | 2 + lua/resty/saml.lua | 141 +++++++++- src/lua_saml.c | 126 +++++++++ src/saml.h | 27 ++ src/xml.c | 228 +++++++++++++++++ t/assertion-conditions.t | 536 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 1059 insertions(+), 1 deletion(-) create mode 100644 t/assertion-conditions.t diff --git a/README.md b/README.md index 04a7ef6..820e531 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,8 @@ local saml = resty_saml.new(opts) | `logout_redirect_uri` | string | None | redirect uri after sucessful logout. | | `sp_cert` | string | None | SP Certificate, used to sign the saml request. | | `sp_private_key` | string | None | SP private key. | +| `sp_audiences` | array of strings | `{ sp_issuer }` | Audiences this SP answers to. An assertion carrying an `AudienceRestriction` has to name one of them; an assertion carrying none is unrestricted. | +| `clock_skew` | number | `60` | Seconds of clock difference tolerated against the IdP when weighing `NotBefore` and `NotOnOrAfter`. | #### saml:authenticate() diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 8ab7985..79655eb 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -225,6 +225,20 @@ local function login(self, opts) return ngx.redirect(opts.idp_uri .. "?" .. query_str) end +-- Days since 1970-01-01 for a civil date. os.time reads its table as local +-- time, which would shift every SAML timestamp by the machine's offset. +local function days_from_civil(year, month, day) + if month <= 2 then + year = year - 1 + end + local era = math.floor(year / 400) + local year_of_era = year - era * 400 + local day_of_year = math.floor((153 * ((month + 9) % 12) + 2) / 5) + day - 1 + local day_of_era = year_of_era * 365 + math.floor(year_of_era / 4) + - math.floor(year_of_era / 100) + day_of_year + return era * 146097 + day_of_era - 719468 +end + local function parse_iso8601_utc_time(str) -- NOTE: We accept only 'Z' for timezone. local year_s, month_s, day_s, hour_s, min_s, sec_s = str:match('(%d%d%d%d)-(%d%d)-(%d%d)T(%d%d):(%d%d):(%d%d).*Z') @@ -255,7 +269,112 @@ local function parse_iso8601_utc_time(str) if sec < 0 or 59 < sec then return nil, 'invalid sec in UTC time' end - return os.time{year=year, month=month, day=day, hour=hour, min=min, sec=sec} + return days_from_civil(year, month, day) * 86400 + hour * 3600 + min * 60 + sec +end + + +-- A signature says the message came from the IdP. It does not say the assertion +-- is still good, that it was issued for this SP, or that it may be presented +-- here. Those live in the assertion's own Conditions and SubjectConfirmation, +-- and are checked below. +-- +-- A constraint the IdP left out is not invented: an IdP that sends no +-- AudienceRestriction keeps working. One the IdP did send is enforced, which is +-- what stops an assertion minted for another SP in the same federation. +local DEFAULT_CLOCK_SKEW = 60 + +local function time_bounds_ok(not_before, not_on_or_after, now, skew) + if not_before then + local at, err = parse_iso8601_utc_time(not_before) + if not at then + return false, "carries an unreadable NotBefore " .. not_before .. ": " .. err + end + if now + skew < at then + return false, "is not valid before " .. not_before + end + end + + if not_on_or_after then + local at, err = parse_iso8601_utc_time(not_on_or_after) + if not at then + return false, "carries an unreadable NotOnOrAfter " .. not_on_or_after .. ": " .. err + end + if now - skew >= at then + return false, "is not valid on or after " .. not_on_or_after + end + end + + return true +end + + +local function audience_accepted(accepted, audiences) + for _, audience in ipairs(audiences) do + for _, expected in ipairs(accepted) do + if expected == audience then + return true + end + end + end + return false +end + + +-- The assertion may be presented to whoever the Recipient names, for as long as +-- the confirmation data allows. Several confirmations can be offered and any one +-- of them being satisfiable is enough. +local function confirmation_ok(confirmation, acs_url, now, skew) + if confirmation.recipient and confirmation.recipient ~= acs_url then + return false + end + return (time_bounds_ok(confirmation.not_before, confirmation.not_on_or_after, now, skew)) +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, acs_url, now) + local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW + local accepted = opts.sp_audiences or { opts.sp_issuer } + + for _, assertion in ipairs(assertions) do + local where = "assertion " .. tostring(assertion.id) .. " " + + -- SAML Core 2.5.1: a condition the SP does not understand leaves the + -- assertion Indeterminate, which is not a licence to use it + if assertion.unknown_condition then + return false, where .. "carries an unrecognised condition " .. assertion.unknown_condition + 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 + end + + -- each AudienceRestriction narrows the audience separately, so this SP + -- has to be named in all of them + for _, restriction in ipairs(assertion.audience_restrictions) do + if not audience_accepted(accepted, restriction) then + return false, where .. "is restricted to " .. table.concat(restriction, ", ") + end + end + + local confirmations = assertion.subject_confirmations + if #confirmations > 0 then + local satisfiable = false + for _, confirmation in ipairs(confirmations) do + if confirmation_ok(confirmation, acs_url, now, skew) then + satisfiable = true + break + end + end + if not satisfiable then + return false, where .. "offers no subject confirmation this SP can satisfy" + end + end + end + + return true end local function login_callback(self, opts) @@ -296,6 +415,26 @@ local function login_callback(self, opts) ngx.exit(ngx.HTTP_UNAUTHORIZED) end + local acs_url = saml_get_redirect_uri(opts.login_callback_uri) + + local destination = saml.doc_destination(doc) + if destination and destination ~= acs_url then + ngx.log(ngx.ERR, "response from IdP is addressed to ", destination) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end + + local assertions = saml.doc_assertions(doc) + if not assertions then + ngx.log(ngx.ERR, "could not read the assertions in response from IdP") + ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) + end + + local acceptable, reason = assertions_acceptable(opts, assertions, acs_url, ngx.time()) + if not acceptable then + ngx.log(ngx.ERR, "response from IdP rejected: ", reason) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end + local issuer = saml.doc_issuer(doc) local attrs = saml.doc_attrs(doc) local name_id = saml.doc_name_id(doc) diff --git a/src/lua_saml.c b/src/lua_saml.c index 80baa9a..c252e16 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -519,6 +519,130 @@ static int doc_attrs(lua_State* L) { } +/*** +Get the Destination attribute of the root message +@function doc_destination +@tparam xmlDoc* doc +@treturn ?string destination +*/ +static int doc_destination(lua_State* L) { + lua_settop(L, 1); + xmlDoc* doc = doc_check(L, 1); + lua_pop(L, 1); + + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { + lua_pushnil(L); + return 1; + } + + xmlChar* destination = xmlGetNoNsProp(root, (const xmlChar*)"Destination"); + if (destination == NULL) { + lua_pushnil(L); + } else { + lua_pushstring(L, (char*)destination); + xmlFree(destination); + } + return 1; +} + + +// An absent attribute is left absent rather than pushed as an empty string, so +// that the caller can tell "the IdP said nothing" from "the IdP said nothing +// useful". +static void set_str_field(lua_State* L, const char* name, const xmlChar* value) { + if (value == NULL) { + return; + } + lua_pushstring(L, name); + lua_pushstring(L, (const char*)value); + lua_settable(L, -3); +} + + +static void set_bool_field(lua_State* L, const char* name, int value) { + lua_pushstring(L, name); + lua_pushboolean(L, value); + lua_settable(L, -3); +} + + +static void push_audience_restrictions(lua_State* L, saml_assertion_t* a) { + lua_pushstring(L, "audience_restrictions"); + lua_newtable(L); + for (size_t i = 0; i < a->audience_restrictions_len; i++) { + saml_audience_restriction_t* restriction = a->audience_restrictions + i; + lua_pushinteger(L, i + 1); + lua_newtable(L); + for (size_t j = 0; j < restriction->audiences_len; j++) { + if (restriction->audiences[j] == NULL) { + continue; + } + lua_pushinteger(L, j + 1); + lua_pushstring(L, (char*)restriction->audiences[j]); + lua_settable(L, -3); + } + lua_settable(L, -3); + } + lua_settable(L, -3); +} + + +static void push_subject_confirmations(lua_State* L, saml_assertion_t* a) { + lua_pushstring(L, "subject_confirmations"); + lua_newtable(L); + for (size_t i = 0; i < a->confirmations_len; i++) { + saml_subject_confirmation_t* confirmation = a->confirmations + i; + lua_pushinteger(L, i + 1); + lua_newtable(L); + set_str_field(L, "method", confirmation->method); + set_str_field(L, "recipient", confirmation->recipient); + set_str_field(L, "not_before", confirmation->not_before); + set_str_field(L, "not_on_or_after", confirmation->not_on_or_after); + set_str_field(L, "in_response_to", confirmation->in_response_to); + lua_settable(L, -3); + } + lua_settable(L, -3); +} + + +/*** +Get the constraints each top-level assertion of the document attaches to itself +@function doc_assertions +@tparam xmlDoc* doc +@treturn table assertions +*/ +static int doc_assertions(lua_State* L) { + lua_settop(L, 1); + xmlDoc* doc = doc_check(L, 1); + lua_pop(L, 1); + + saml_assertion_t* assertions; + size_t assertions_len; + if (saml_doc_assertions(doc, &assertions, &assertions_len) < 0) { + lua_pushnil(L); + return 1; + } + + lua_newtable(L); + for (size_t i = 0; i < assertions_len; i++) { + saml_assertion_t* a = assertions + i; + lua_pushinteger(L, i + 1); + lua_newtable(L); + set_str_field(L, "id", a->id); + set_bool_field(L, "has_conditions", a->has_conditions); + 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); + push_audience_restrictions(L, a); + push_subject_confirmations(L, a); + lua_settable(L, -3); + } + saml_assertions_free(assertions, assertions_len); + return 1; +} + + static int get_key_format(lua_State* L, int narg) { #if (LUA_VERSION_NUM > 502) int format = (int)luaL_checkinteger(L, narg); @@ -1165,6 +1289,8 @@ static const struct luaL_Reg saml_funcs[] = { {"doc_session_index", doc_session_index}, {"doc_session_expires", doc_session_expires}, {"doc_attrs", doc_attrs}, + {"doc_assertions", doc_assertions}, + {"doc_destination", doc_destination}, {"key_read_memory", key_read_memory}, {"key_read_file", key_read_file}, diff --git a/src/saml.h b/src/saml.h index 7df4bfd..ac77c57 100644 --- a/src/saml.h +++ b/src/saml.h @@ -42,6 +42,31 @@ typedef struct { int num_values; } saml_attr_t; +typedef struct { + xmlChar** audiences; + size_t audiences_len; +} saml_audience_restriction_t; + +typedef struct { + xmlChar* method; + xmlChar* recipient; + xmlChar* not_before; + xmlChar* not_on_or_after; + xmlChar* in_response_to; +} saml_subject_confirmation_t; + +typedef struct { + xmlChar* id; + int has_conditions; + xmlChar* not_before; + xmlChar* not_on_or_after; + xmlChar* unknown_condition; + saml_audience_restriction_t* audience_restrictions; + size_t audience_restrictions_len; + saml_subject_confirmation_t* confirmations; + size_t confirmations_len; +} saml_assertion_t; + typedef enum { SAML_ZLIB_ERROR = -2, SAML_XMLSEC_ERROR, @@ -84,6 +109,8 @@ xmlChar* saml_doc_session_index(xmlDoc* doc); xmlChar* saml_doc_session_expires(xmlDoc* doc); int saml_doc_attrs(xmlDoc* doc, saml_attr_t** attrs, size_t* attrs_len); void saml_attrs_free(saml_attr_t* attrs, size_t attrs_len); +int saml_doc_assertions(xmlDoc* doc, saml_assertion_t** assertions, size_t* assertions_len); +void saml_assertions_free(saml_assertion_t* assertions, size_t assertions_len); xmlSecTransformCtx* saml_sign_binary(xmlSecKey* key, xmlSecTransformId transform_id, unsigned char* data, size_t data_len); int saml_verify_binary(xmlSecKey* cert, xmlSecTransformId transform_id, unsigned char* data, size_t data_len, unsigned char* sig, size_t sig_len); diff --git a/src/xml.c b/src/xml.c index bbc1bfb..aaba60d 100644 --- a/src/xml.c +++ b/src/xml.c @@ -241,3 +241,231 @@ void saml_attrs_free(saml_attr_t* attrs, size_t attrs_len) { } free(attrs); } + + +// Defined in sig.c, which saml.c includes after this file. +static int is_saml_assertion(xmlNode* node); + + +// A direct child element of node named name in the assertion namespace. +static int is_assertion_el(xmlNode* node, const char* name) { + return node->type == XML_ELEMENT_NODE && + xmlStrEqual(node->name, (const xmlChar*)name) == 1 && + node->ns != NULL && + xmlStrEqual(node->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1; +} + + +static xmlNode* assertion_child(xmlNode* node, const char* name) { + for (xmlNode* child = node->children; child != NULL; child = child->next) { + if (is_assertion_el(child, name)) { + return child; + } + } + return NULL; +} + + +static size_t count_assertion_el(xmlNode* parent, const char* name) { + size_t n = 0; + for (xmlNode* child = parent->children; child != NULL; child = child->next) { + if (is_assertion_el(child, name)) { + n++; + } + } + return n; +} + + +// Conditions this reader can hand the caller enough to weigh. SAML Core 2.5.1 +// makes an assertion carrying any other condition Indeterminate rather than +// valid, so anything else is reported as unrecognised for the caller to refuse. +static int is_known_condition(xmlNode* node) { + return is_assertion_el(node, "AudienceRestriction") || + is_assertion_el(node, "OneTimeUse") || + is_assertion_el(node, "ProxyRestriction"); +} + + +// Each AudienceRestriction is a separate restriction and the assertion applies +// only where all of them do, so they are kept apart rather than flattened. +static int read_audience_restrictions(xmlDoc* doc, xmlNode* conditions, saml_assertion_t* a) { + size_t count = count_assertion_el(conditions, "AudienceRestriction"); + if (count == 0) { + return 0; + } + + a->audience_restrictions = calloc(count, sizeof(saml_audience_restriction_t)); + if (a->audience_restrictions == NULL) { + return -1; + } + a->audience_restrictions_len = count; + + size_t i = 0; + for (xmlNode* node = conditions->children; node != NULL; node = node->next) { + if (!is_assertion_el(node, "AudienceRestriction")) { + continue; + } + + saml_audience_restriction_t* restriction = a->audience_restrictions + i++; + size_t audiences = count_assertion_el(node, "Audience"); + if (audiences == 0) { + continue; + } + + restriction->audiences = calloc(audiences, sizeof(xmlChar*)); + if (restriction->audiences == NULL) { + return -1; + } + restriction->audiences_len = audiences; + + size_t j = 0; + for (xmlNode* child = node->children; child != NULL; child = child->next) { + if (is_assertion_el(child, "Audience")) { + restriction->audiences[j++] = xmlNodeListGetString(doc, child->children, 1); + } + } + } + return 0; +} + + +static int read_subject_confirmations(xmlNode* subject, saml_assertion_t* a) { + size_t count = count_assertion_el(subject, "SubjectConfirmation"); + if (count == 0) { + return 0; + } + + a->confirmations = calloc(count, sizeof(saml_subject_confirmation_t)); + if (a->confirmations == NULL) { + return -1; + } + a->confirmations_len = count; + + size_t i = 0; + for (xmlNode* node = subject->children; node != NULL; node = node->next) { + if (!is_assertion_el(node, "SubjectConfirmation")) { + continue; + } + + saml_subject_confirmation_t* confirmation = a->confirmations + i++; + confirmation->method = xmlGetNoNsProp(node, (const xmlChar*)"Method"); + + xmlNode* data = assertion_child(node, "SubjectConfirmationData"); + if (data == NULL) { + continue; + } + confirmation->recipient = xmlGetNoNsProp(data, (const xmlChar*)"Recipient"); + confirmation->not_before = xmlGetNoNsProp(data, (const xmlChar*)"NotBefore"); + confirmation->not_on_or_after = xmlGetNoNsProp(data, (const xmlChar*)"NotOnOrAfter"); + confirmation->in_response_to = xmlGetNoNsProp(data, (const xmlChar*)"InResponseTo"); + } + return 0; +} + + +static int read_assertion(xmlDoc* doc, xmlNode* node, saml_assertion_t* a) { + a->id = xmlGetNoNsProp(node, (const xmlChar*)"ID"); + + xmlNode* conditions = assertion_child(node, "Conditions"); + if (conditions != NULL) { + a->has_conditions = 1; + a->not_before = xmlGetNoNsProp(conditions, (const xmlChar*)"NotBefore"); + a->not_on_or_after = xmlGetNoNsProp(conditions, (const xmlChar*)"NotOnOrAfter"); + + for (xmlNode* child = conditions->children; child != NULL; child = child->next) { + if (child->type == XML_ELEMENT_NODE && !is_known_condition(child)) { + a->unknown_condition = xmlStrdup(child->name); + break; + } + } + + if (read_audience_restrictions(doc, conditions, a) < 0) { + return -1; + } + } + + xmlNode* subject = assertion_child(node, "Subject"); + if (subject != NULL && read_subject_confirmations(subject, a) < 0) { + return -1; + } + return 0; +} + + +// The constraints every top-level assertion of a Response attaches to itself: +// the validity window, the audiences it is restricted to, and the subject +// confirmations that say where and until when it may be presented. They are +// reported per assertion because they belong to one assertion rather than to +// the document, and a reader consumes several. Messages carrying no assertion +// report none. +int saml_doc_assertions(xmlDoc* doc, saml_assertion_t** assertions, size_t* assertions_len) { + *assertions = NULL; + *assertions_len = 0; + + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL || xmlStrEqual(root->name, (const xmlChar*)"Response") != 1) { + return 0; + } + + size_t count = 0; + for (xmlNode* child = root->children; child != NULL; child = child->next) { + if (is_saml_assertion(child)) { + count++; + } + } + if (count == 0) { + return 0; + } + + saml_assertion_t* list = calloc(count, sizeof(saml_assertion_t)); + if (list == NULL) { + return -1; + } + + size_t i = 0; + for (xmlNode* child = root->children; child != NULL; child = child->next) { + if (!is_saml_assertion(child)) { + continue; + } + if (read_assertion(doc, child, list + i++) < 0) { + saml_assertions_free(list, count); + return -1; + } + } + + *assertions = list; + *assertions_len = count; + return 0; +} + + +void saml_assertions_free(saml_assertion_t* assertions, size_t assertions_len) { + for (size_t i = 0; i < assertions_len; i++) { + saml_assertion_t* a = assertions + i; + xmlFree(a->id); + xmlFree(a->not_before); + xmlFree(a->not_on_or_after); + xmlFree(a->unknown_condition); + + for (size_t j = 0; j < a->audience_restrictions_len; j++) { + saml_audience_restriction_t* restriction = a->audience_restrictions + j; + for (size_t k = 0; k < restriction->audiences_len; k++) { + xmlFree(restriction->audiences[k]); + } + free(restriction->audiences); + } + free(a->audience_restrictions); + + for (size_t j = 0; j < a->confirmations_len; j++) { + saml_subject_confirmation_t* confirmation = a->confirmations + j; + xmlFree(confirmation->method); + xmlFree(confirmation->recipient); + xmlFree(confirmation->not_before); + xmlFree(confirmation->not_on_or_after); + xmlFree(confirmation->in_response_to); + } + free(a->confirmations); + } + free(assertions); +} diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t new file mode 100644 index 0000000..403ddb1 --- /dev/null +++ b/t/assertion-conditions.t @@ -0,0 +1,536 @@ +use Test::Nginx::Socket::Lua; + +log_level('info'); +no_long_string(); +repeat_each(1); +no_shuffle(); +plan 'no_plan'; + +my $pwd = `pwd`; +chomp $pwd; + +add_block_preprocessor(sub { + my ($block) = @_; + + if ((!defined $block->error_log) && (!defined $block->no_error_log)) { + $block->set_value("no_error_log", "[error]"); + } + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } + + my $main_config = $block->main_config // <<_EOC_; + env SAML_DATA_DIR=./; +_EOC_ + + $block->set_value("main_config", $main_config); + + 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;;'; + + init_by_lua_block { + saml = require "saml" + local err = saml.init({ debug = true, data_dir = os.getenv("SAML_DATA_DIR") }) + if err then assert(nil, err) end + + SUCCESS = "urn:oasis:names:tc:SAML:2.0:status:Success" + IDP = "https://idp.example.com" + ACS = "http://127.0.0.1:1984/acs" + BEARER = "urn:oasis:names:tc:SAML:2.0:cm:bearer" + + KEY_PEM = [[-----BEGIN PRIVATE KEY----- +MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQDYYOJFazEru+eF +1bGFzH8xuC2clcWjnpIvXf5Jrseg7gfMh0nMM83OddLWB2Er+RWmVj361qaQR35p +JHGm3hFw20b2S+zBPxA6LCrHJ7vD/kOKEiDKxU3Ls5QK9+fTHFXIbpDtGAuISmmc +eWNaTZPIMdxPlpKYIyNJIUc2RxSREjsGlsrWWEtsroMjxpaHNNupadRUmkHXvZsC +EAsi3penjfZxG6v9R22tBwJxgj/ceXZwtTQJ7tuNtthv+kWP6/Q9owHW3uGL8Bin +46GRqAfHSGC64No+NwETF5iuephkIggtbvrlazTdPwu8Ddl8l4I1QfYmNxKPxnzJ +7pDwvBeRAgMBAAECggEAFkMTjKZcav48cg/cIaK6VGx5XuKm8LBcJHz0cHLHzbYn +vcKOlHChBFSpgkVEmWBZeqFlY5Upkm8Uoa8y9ULkQvsAiE8j9vbszbtlFFPxdNcI +bmBymMIngKWDfgRnCNiht8suZIJkj1tulb+EehJAuehtXQ/mGbqFwxymJb627jzk +MJ5bDsaVeBNu4gBQAp0USzreMO3AN9YxXmcJapZ5Bdc8avQzhzWRxNNJxtp6Uw56 +cviuDxg7OJCaEHhUBFiDVu4O2HmrS/XdYUAwFcRO1hY/JfcaJ3DOHOl6y5eoRHwC +kMb8DhT/qECJ9rWc+APdUqiY1ag0Kq9BcRxkEGlcMQKBgQD32hzAPpuwW9Z0M9qd +x70PPkrJD8jgIprC92DHpHfztiZ2ctH3WxupH7UtZfI8tSVzh7WhWPPtrQ01ZcFh +ZPsFN74c7pWtW+JSm0pvDCQQG5qX9eJLna8GeI6f3hpM+u8pXr6p2ZQJGnjlGZfc +VNfJhvqCVH7hiG9fdAavsH1dKQKBgQDffeUD7x8I3ARbiZqDgANA9HqJi1ffhqFZ +xTWKLtr8NCPS8X+DvFrUDlGhBoDY7IGZhDhmBcb8/v7Kke3GT0/mff8GFsj9TUqh +fgzDxj5I/9HEjBKgpAG1J4B87QYZueLriMfX5Ff2wmCeqCwF4ftfjZVU9izyIa7B +hKYubQBMKQKBgQDslAk1h41cfYzqRkS6rllMH42K9cIsD1viFfcPGXJV8twr29WH +YjO470clGlZqlA43hKZeaGYNzEz7VzGLIbRpepfBTgsY+sfBSfF2pgQWTAL4Yf+r +ZcwXRSP+fSZlrHB08LbVsZWYSuhy5kcKTQHcnzanCLhD1tNYLYvkT3aaYQKBgQDK +c3nMuYUMenn8DceJTaIk6hJCnJZqZsOs1UdtuIooona9NITFag+BPsNVMdXwKzYv +QaXxTVR3g+p8x/pzhQ8lBYfKFUPWqXhsmAmqIt/zMsHr4NNS756YYoMzJ2c6ULgt +ksctW60PW/84WbEfVxll8pSO1T3bzQVISghbz+PQGQKBgQCEptD2bKHhF8RzRyfC +QXydnF7O6GEK3au3OKPb6BsLwJpTP2Wc1feTcg/lzCS5eUhNMxPv+4Ua7SLiF4li +vnI8SyPV2nGlsjna9maSkBq01YrLEMsPPSqw01Nf4W5jtUgk+jbZt9K3SrvTGzpJ +/2lpqvTIUUQTrTJNL6GZUBY1/Q== +-----END PRIVATE KEY-----]] + + CERT_PEM = [[-----BEGIN CERTIFICATE----- +MIIDFTCCAf2gAwIBAgIUC9GZCQFhxDfguRhTjIcG/LxOZMQwDQYJKoZIhvcNAQEL +BQAwGjEYMBYGA1UEAwwPaWRwLmV4YW1wbGUuY29tMB4XDTI2MDgxMDExMDkwNFoX +DTM2MDgwNzExMDkwNFowGjEYMBYGA1UEAwwPaWRwLmV4YW1wbGUuY29tMIIBIjAN +BgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA2GDiRWsxK7vnhdWxhcx/MbgtnJXF +o56SL13+Sa7HoO4HzIdJzDPNznXS1gdhK/kVplY9+tamkEd+aSRxpt4RcNtG9kvs +wT8QOiwqxye7w/5DihIgysVNy7OUCvfn0xxVyG6Q7RgLiEppnHljWk2TyDHcT5aS +mCMjSSFHNkcUkRI7BpbK1lhLbK6DI8aWhzTbqWnUVJpB172bAhALIt6Xp432cRur +/UdtrQcCcYI/3Hl2cLU0Ce7bjbbYb/pFj+v0PaMB1t7hi/AYp+OhkagHx0hguuDa +PjcBExeYrnqYZCIILW765Ws03T8LvA3ZfJeCNUH2JjcSj8Z8ye6Q8LwXkQIDAQAB +o1MwUTAdBgNVHQ4EFgQUlbLjSTfPYYltgF5anYLJxHTRS/owHwYDVR0jBBgwFoAU +lbLjSTfPYYltgF5anYLJxHTRS/owDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0B +AQsFAAOCAQEAjCv57yzpZMReoVJaZor6NGd5kcf8DfI2LLWJ4MGXzq/6kZLYy+Op +M1CxHA2wnxFmqcVmEra0zi2H2PkbM9p3oPK3upPdrL/ke2dIChP1yokaQoW9f2bY +K2INu9LIVuSD8hOUHDXPiH4Smt91V0GfrFHcxysfm97Y+TC+84grwcFE3JiRgfF+ +WYG9w8xaCTTorUKUGum8/5beRd8qNCxVnh4Ke5vaRaUj28MbqLSQp1dvm0cqe+4d +kna+UpbWKQOQ8uAAtFIH+bX2uh8NbCBfATfwEMYzAffGKkmRkkoQHNv0Uf5uIduu +GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== +-----END CERTIFICATE-----]] + + -- one SP per configuration under test, picked by request header + OPTS = { + plain = {}, + skew = { clock_skew = 300 }, + audiences = { sp_audiences = { "https://sp.example.com/metadata" } }, + } + SPS = {} + + function sp(name) + if SPS[name] == nil then + local opts = { + sp_issuer = "sp", + idp_uri = "http://127.0.0.1:1984/idp", + login_callback_uri = "/acs", + logout_uri = "/logout", + logout_callback_uri = "/sls", + logout_redirect_uri = "/logout_ok", + sp_cert = CERT_PEM, + sp_private_key = KEY_PEM, + idp_cert = CERT_PEM, + secret = "very-secret-key-that-is-32-byte!", + } + for k, v in pairs(OPTS[name]) do opts[k] = v end + SPS[name] = require("resty.saml").new(opts) + end + return SPS[name] + end + + function sign_doc(xml) + local key = assert(saml.key_read_memory(KEY_PEM, saml.KeyDataFormatPem)) + saml.key_add_cert_memory(key, CERT_PEM, saml.KeyDataFormatCertPem) + local transform = saml.find_transform_by_href( + "http://www.w3.org/2001/04/xmldsig-more#rsa-sha256") + local out = assert(saml.sign_xml(key, transform, xml, + { id_attr = "ID", insert_after = { saml.XMLNS_ASSERTION, "Issuer" } })) + return (out:gsub("<%?xml.-%?>%s*", "")) + end + + -- an IdP timestamp this many seconds away from now + function at(offset) + return os.date("!%Y-%m-%dT%TZ", ngx.time() + offset) + end + + function attr(name, value) + if value == nil then return "" end + return string.format(' %s="%s"', name, value) + end + + function audience(...) + local out = {} + for _, name in ipairs({...}) do + out[#out + 1] = "" .. name .. "" + end + return "" .. table.concat(out) .. "" + end + + function conditions(spec) + spec = spec or {} + return string.format('%s', + attr("NotBefore", spec.not_before), attr("NotOnOrAfter", spec.not_on_or_after), + spec.body or "") + end + + function confirmation(spec) + spec = spec or {} + local data = "" + if spec.data ~= false then + data = string.format('', + attr("Recipient", spec.recipient), attr("NotBefore", spec.not_before), + attr("NotOnOrAfter", spec.not_on_or_after)) + end + return string.format('%s', + spec.method or BEARER, data) + end + + -- Conditions follows Subject, the order the schema prescribes + function assertion(spec) + spec = spec or {} + return string.format('' .. + '%s' .. + '%s%s%s', + spec.id or "a1", IDP, spec.name_id or "signed\@example.com", + spec.confirmations or "", spec.conditions or "") + end + + function response(body, destination) + return string.format('%s' .. + '%s', + attr("Destination", destination), IDP, SUCCESS, body) + end + + -- only the assertion is signed, the shape an IdP sends by default + function saml_response(spec, destination) + return response(sign_doc(assertion(spec)), destination) + end + + -- start a login, then hand the crafted response back to the callback + -- with the session and RelayState that login handed out + function login_with(name, xml) + local httpc = require("resty.http").new() + local base = "http://127.0.0.1:1984" + local headers = { ["X-Test-SP"] = name } + + local res, err = httpc:request_uri(base .. "/", { headers = headers }) + if not res then return "login request: " .. err end + local cookie = res.headers["Set-Cookie"] + if type(cookie) == "table" then cookie = cookie[1] end + local state = res.headers["Location"]:match("RelayState=([^&]+)") + + res, err = httpc:request_uri(base .. "/acs", { + method = "POST", + body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. + "&RelayState=" .. state, + headers = { + ["X-Test-SP"] = name, + ["Cookie"] = cookie:match("^[^;]+"), + ["Content-Type"] = "application/x-www-form-urlencoded", + }, + }) + if not res then return "callback request: " .. err end + return res.status .. " " .. tostring(res.headers["Location"]) + end + + function parse(xml) + local key = assert(saml.key_read_memory(CERT_PEM, saml.KeyDataFormatCertPem)) + local mngr = assert(saml.create_keys_manager({ key })) + saml.key_add_ca_memory(mngr, CERT_PEM) + return saml.binding_post_parse(saml.base64_encode(xml), function(_) return mngr end) + end + } + + server { + listen 1984; + + location / { + access_by_lua_block { + sp(ngx.var.http_x_test_sp or "plain"):authenticate() + } + + content_by_lua_block { + ngx.exit(200) + } + } + } +_EOC_ + + $block->set_value("http_config", $http_config); +}); + +run_tests(); + +__DATA__ + +=== TEST 1: an assertion inside its validity window is accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_before = at(-60), not_on_or_after = at(600) }), + }))) + } + } +--- response_body +302 / + + + +=== TEST 2: an expired assertion is refused however it is replayed +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_before = at(-7200), not_on_or_after = at(-3600) }), + }))) + } + } +--- response_body +401 nil +--- error_log +is not valid on or after + + + +=== TEST 3: an assertion whose window has not opened is refused +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_before = at(3600), not_on_or_after = at(7200) }), + }))) + } + } +--- response_body +401 nil +--- error_log +is not valid before + + + +=== TEST 4: the clock skew allowance covers a small difference with the IdP +--- config + location /t { + content_by_lua_block { + local spec = { conditions = conditions({ not_on_or_after = at(-120) }) } + ngx.say(login_with("plain", saml_response(spec))) + ngx.say(login_with("skew", saml_response(spec))) + } + } +--- response_body +401 nil +302 / +--- error_log +is not valid on or after + + + +=== TEST 5: an assertion restricted to another SP is refused +--- config + location /t { + content_by_lua_block { + -- what an IdP serving a federation mints for a different SP + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ body = audience("https://other.example.com") }), + }))) + } + } +--- response_body +401 nil +--- error_log +is restricted to https://other.example.com + + + +=== TEST 6: an assertion restricted to this SP is accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ body = audience("https://other.example.com", "sp") }), + }))) + } + } +--- response_body +302 / + + + +=== TEST 7: sp_audiences names the audience the IdP was configured with +--- config + location /t { + content_by_lua_block { + local spec = { + conditions = conditions({ body = audience("https://sp.example.com/metadata") }), + } + ngx.say(login_with("plain", saml_response(spec))) + ngx.say(login_with("audiences", saml_response(spec))) + } + } +--- response_body +401 nil +302 / +--- error_log +is restricted to https://sp.example.com/metadata + + + +=== TEST 8: each AudienceRestriction narrows the audience on its own +--- config + location /t { + content_by_lua_block { + -- named in the first restriction, left out of the second + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ + body = audience("sp") .. audience("https://other.example.com"), + }), + }))) + } + } +--- response_body +401 nil +--- error_log +is restricted to https://other.example.com + + + +=== TEST 9: a confirmation addressed to another endpoint is refused +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = "http://evil.example.com/acs" }), + }))) + } + } +--- response_body +401 nil +--- error_log +offers no subject confirmation this SP can satisfy + + + +=== TEST 10: a confirmation addressed here and still open is accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = ACS, not_on_or_after = at(300) }), + }))) + } + } +--- response_body +302 / + + + +=== TEST 11: a confirmation that has run out is refused +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = ACS, not_on_or_after = at(-3600) }), + }))) + } + } +--- response_body +401 nil +--- error_log +offers no subject confirmation this SP can satisfy + + + +=== TEST 12: one satisfiable confirmation among several is enough +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = "http://evil.example.com/acs" }) .. + confirmation({ recipient = ACS, not_on_or_after = at(300) }), + }))) + } + } +--- response_body +302 / + + + +=== TEST 13: an unrecognised condition leaves the assertion indeterminate +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ body = "" }), + }))) + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ + body = 'sp', + }), + }))) + } + } +--- response_body +302 / +401 nil +--- error_log +carries an unrecognised condition Condition + + + +=== TEST 14: a response addressed to another endpoint is refused +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({}, "http://evil.example.com/acs"))) + ngx.say(login_with("plain", saml_response({}, ACS))) + } + } +--- response_body +401 nil +302 / +--- error_log +response from IdP is addressed to http://evil.example.com/acs + + + +=== TEST 15: an assertion carrying no constraints is still accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({}))) + } + } +--- response_body +302 / + + + +=== TEST 16: the constraints are reported per assertion, not pooled +--- config + location /t { + 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") }) }) .. + assertion({ id = "a2", name_id = "second@example.com", + confirmations = confirmation({ recipient = ACS }) }))) + local doc, err = parse(xml) + if err then ngx.say("err: ", err) return end + + for _, a in ipairs(saml.doc_assertions(doc)) do + ngx.say(a.id, " conditions=", tostring(a.has_conditions), + " expires=", tostring(a.not_on_or_after), + " audiences=", #a.audience_restrictions, + " confirmations=", #a.subject_confirmations) + 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 +destination: nil + + + +=== TEST 17: a UTC timestamp is read as UTC whatever the machine's timezone is +--- config + location /t { + content_by_lua_block { + -- an assertion good for another hour, with the worker fourteen + -- hours ahead of UTC: read as local time it would already have run + -- out + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_before = at(-60), not_on_or_after = at(3600) }), + }))) + } + } +--- main_config +env SAML_DATA_DIR=./; +env TZ=XXX-14; +--- response_body +302 / From a9fa958fc74e1cc37c817f75b8063cf0f57c7f02 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:57:19 +0545 Subject: [PATCH 02/19] fix: bind the assertion to the request this SP issued login generated an AuthnRequest ID and threw it away, so nothing tied the response back to a login this SP started. An assertion captured from one login stayed usable in any later one. The ID is kept on the session now. A SubjectConfirmationData naming a different request makes that confirmation unsatisfiable, and a Response answering a different request is refused outright. The confirmation is the binding that holds: it sits inside the signature, while the Response around it is usually unsigned. --- lua/resty/saml.lua | 39 ++++++++++++++----- src/lua_saml.c | 29 ++++++++++++++ t/assertion-conditions.t | 83 ++++++++++++++++++++++++++++++++++++---- 3 files changed, 134 insertions(+), 17 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 79655eb..8b433a1 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -149,13 +149,13 @@ local AUTHN_REQUEST = [[ ]] -local function authn_request(opts) +local function authn_request(opts, request_id) return interp(AUTHN_REQUEST, { acs_url = saml_get_redirect_uri(opts.login_callback_uri), destination = opts.idp_uri, issue_instant = os.date("!%Y-%m-%dT%TZ"), issuer = opts.sp_issuer, - uuid = generate_saml_id(), + uuid = request_id, auth_protocol_binding_method = opts.auth_protocol_binding_method, }) end @@ -205,13 +205,17 @@ local function login(self, opts) local state = uuid.generate_v4() local request_uri = ngx.var.request_uri + -- kept so the callback can tell the answer to this request from the answer + -- to some other one + local request_id = generate_saml_id() sess:set("saml_state", state) + sess:set("saml_request_id", request_id) sess:set("request_uri", request_uri) sess:save() local query_str, err = create_redirect(self.sign_key, { - SAMLRequest = authn_request(opts), + SAMLRequest = authn_request(opts, request_id), SigAlg = RSA_SHA_512_HREF, RelayState = state, }) @@ -323,8 +327,11 @@ end -- The assertion may be presented to whoever the Recipient names, for as long as -- the confirmation data allows. Several confirmations can be offered and any one -- of them being satisfiable is enough. -local function confirmation_ok(confirmation, acs_url, now, skew) - if confirmation.recipient and confirmation.recipient ~= acs_url then +local function confirmation_ok(confirmation, expected, now, skew) + if confirmation.recipient and confirmation.recipient ~= expected.acs_url then + return false + end + if confirmation.in_response_to and confirmation.in_response_to ~= expected.request_id then return false end return (time_bounds_ok(confirmation.not_before, confirmation.not_on_or_after, now, skew)) @@ -333,7 +340,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, acs_url, now) +local function assertions_acceptable(opts, assertions, expected, now) local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW local accepted = opts.sp_audiences or { opts.sp_issuer } @@ -363,7 +370,7 @@ local function assertions_acceptable(opts, assertions, acs_url, now) if #confirmations > 0 then local satisfiable = false for _, confirmation in ipairs(confirmations) do - if confirmation_ok(confirmation, acs_url, now, skew) then + if confirmation_ok(confirmation, expected, now, skew) then satisfiable = true break end @@ -415,10 +422,21 @@ local function login_callback(self, opts) ngx.exit(ngx.HTTP_UNAUTHORIZED) end - local acs_url = saml_get_redirect_uri(opts.login_callback_uri) + local expected = { + acs_url = saml_get_redirect_uri(opts.login_callback_uri), + request_id = sess:get("saml_request_id"), + } + + -- the Response is often left unsigned, so this only catches a stray answer; + -- the binding that holds is the one inside the signed assertion below + local in_response_to = saml.doc_in_response_to(doc) + if in_response_to and in_response_to ~= expected.request_id then + ngx.log(ngx.ERR, "response from IdP answers request ", in_response_to) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end local destination = saml.doc_destination(doc) - if destination and destination ~= acs_url then + if destination and destination ~= expected.acs_url then ngx.log(ngx.ERR, "response from IdP is addressed to ", destination) ngx.exit(ngx.HTTP_UNAUTHORIZED) end @@ -429,7 +447,7 @@ local function login_callback(self, opts) ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) end - local acceptable, reason = assertions_acceptable(opts, assertions, acs_url, ngx.time()) + local acceptable, reason = assertions_acceptable(opts, assertions, expected, ngx.time()) if not acceptable then ngx.log(ngx.ERR, "response from IdP rejected: ", reason) ngx.exit(ngx.HTTP_UNAUTHORIZED) @@ -468,6 +486,7 @@ local function login_callback(self, opts) -- clear temporary authentication state no longer needed after successful login sess:set("saml_state", nil) + sess:set("saml_request_id", nil) sess:set("request_uri", nil) sess:save() diff --git a/src/lua_saml.c b/src/lua_saml.c index c252e16..1b3458f 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -519,6 +519,34 @@ static int doc_attrs(lua_State* L) { } +/*** +Get the InResponseTo attribute of the root message +@function doc_in_response_to +@tparam xmlDoc* doc +@treturn ?string in_response_to +*/ +static int doc_in_response_to(lua_State* L) { + lua_settop(L, 1); + xmlDoc* doc = doc_check(L, 1); + lua_pop(L, 1); + + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { + lua_pushnil(L); + return 1; + } + + xmlChar* in_response_to = xmlGetNoNsProp(root, (const xmlChar*)"InResponseTo"); + if (in_response_to == NULL) { + lua_pushnil(L); + } else { + lua_pushstring(L, (char*)in_response_to); + xmlFree(in_response_to); + } + return 1; +} + + /*** Get the Destination attribute of the root message @function doc_destination @@ -1291,6 +1319,7 @@ static const struct luaL_Reg saml_funcs[] = { {"doc_attrs", doc_attrs}, {"doc_assertions", doc_assertions}, {"doc_destination", doc_destination}, + {"doc_in_response_to", doc_in_response_to}, {"key_read_memory", key_read_memory}, {"key_read_file", key_read_file}, diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 403ddb1..a8df40a 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -156,9 +156,10 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== spec = spec or {} local data = "" if spec.data ~= false then - data = string.format('', + data = string.format('', attr("Recipient", spec.recipient), attr("NotBefore", spec.not_before), - attr("NotOnOrAfter", spec.not_on_or_after)) + attr("NotOnOrAfter", spec.not_on_or_after), + attr("InResponseTo", spec.in_response_to)) end return string.format('%s', spec.method or BEARER, data) @@ -175,17 +176,31 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== spec.confirmations or "", spec.conditions or "") end - function response(body, destination) + function response(body, destination, in_response_to) return string.format('%s' .. '%s', - attr("Destination", destination), IDP, SUCCESS, body) + attr("Destination", destination), attr("InResponseTo", in_response_to), + IDP, SUCCESS, body) end -- only the assertion is signed, the shape an IdP sends by default - function saml_response(spec, destination) - return response(sign_doc(assertion(spec)), destination) + function saml_response(spec, destination, in_response_to) + return response(sign_doc(assertion(spec)), destination, in_response_to) + end + + -- the ID of the AuthnRequest the SP just issued, read back out of the + -- redirect it sent the browser + function authn_request_id(location) + local args = {} + for k, v in location:gmatch("([^?&=]+)=([^&]*)") do + args[k] = ngx.unescape_uri(v) + end + local cert = assert(saml.key_read_memory(CERT_PEM, saml.KeyDataFormatCertPem)) + local doc = assert(saml.binding_redirect_parse("SAMLRequest", args, + function(_) return cert end)) + return saml.doc_id(doc) end -- start a login, then hand the crafted response back to the callback @@ -201,6 +216,12 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== if type(cookie) == "table" then cookie = cookie[1] end local state = res.headers["Location"]:match("RelayState=([^&]+)") + -- a response that has to name the request gets built once the SP + -- has issued one + if type(xml) == "function" then + xml = xml(authn_request_id(res.headers["Location"])) + end + res, err = httpc:request_uri(base .. "/acs", { method = "POST", body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. @@ -534,3 +555,51 @@ env SAML_DATA_DIR=./; env TZ=XXX-14; --- response_body 302 / + + + +=== TEST 18: a response answering another request is refused +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({}, nil, "ID_some-other-request"))) + } + } +--- response_body +401 nil +--- error_log +response from IdP answers request ID_some-other-request + + + +=== TEST 19: a confirmation answering another request is refused +--- config + location /t { + content_by_lua_block { + -- inside the signature, so this is the binding an attacker replaying + -- a captured assertion cannot rewrite + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = ACS, in_response_to = "ID_some-other-request" }), + }))) + } + } +--- response_body +401 nil +--- error_log +offers no subject confirmation this SP can satisfy + + + +=== TEST 20: a response answering this SP's own request is accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", function(request_id) + return saml_response({ + confirmations = confirmation({ recipient = ACS, in_response_to = request_id }), + }, ACS, request_id) + end)) + } + } +--- response_body +302 / From 19e96e06d1c0a68b0e447e3199c38fad93ecf179 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 17:14:15 +0545 Subject: [PATCH 03/19] fix: let an assertion be presented only once Nothing stopped the same assertion being posted back a second time inside its validity window. Its ID is remembered now, in an lua_shared_dict the deployment names, and a second presentation is refused. The entry lives as long as the assertion's own Conditions leave it usable, so the cache holds exactly what could still be replayed. An assertion that names no expiry is remembered for replay_ttl, since nothing in the assertion says when to stop. Unset replay_dict leaves assertions untracked, which is what deployments with no shared dict to spare get today. --- README.md | 2 ++ lua/resty/saml.lua | 64 +++++++++++++++++++++++++++++++++++++++- t/assertion-conditions.t | 59 ++++++++++++++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 820e531..5205453 100644 --- a/README.md +++ b/README.md @@ -81,6 +81,8 @@ local saml = resty_saml.new(opts) | `sp_private_key` | string | None | SP private key. | | `sp_audiences` | array of strings | `{ sp_issuer }` | Audiences this SP answers to. An assertion carrying an `AudienceRestriction` has to name one of them; an assertion carrying none is unrestricted. | | `clock_skew` | number | `60` | Seconds of clock difference tolerated against the IdP when weighing `NotBefore` and `NotOnOrAfter`. | +| `replay_dict` | string | None | Name of an `lua_shared_dict` in which to remember the assertions already presented, so none is accepted twice. Unset leaves them untracked. | +| `replay_ttl` | number | `600` | Seconds to remember an assertion that names no `NotOnOrAfter` of its own. One that names it is remembered until it expires. | #### saml:authenticate() diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 8b433a1..2712f45 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -287,6 +287,9 @@ end -- what stops an assertion minted for another SP in the same federation. local DEFAULT_CLOCK_SKEW = 60 +-- how long an assertion that sets no expiry of its own is remembered +local DEFAULT_REPLAY_TTL = 600 + local function time_bounds_ok(not_before, not_on_or_after, now, skew) if not_before then local at, err = parse_iso8601_utc_time(not_before) @@ -384,6 +387,52 @@ local function assertions_acceptable(opts, assertions, expected, now) return true end +-- A bearer assertion is good for one login. Nothing above stops the same one +-- being presented again inside its validity window, so its ID is kept until it +-- expires and a second presentation is refused. +-- +-- The window from the assertion's own Conditions decides how long the entry +-- lives, so the cache holds exactly what is still usable. An assertion that +-- names no expiry is replayable for as long as it is remembered, which is what +-- replay_ttl bounds. +local function assertions_unused(dict, opts, assertions, now) + local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW + + for _, assertion in ipairs(assertions) do + if not assertion.id then + return false, "an assertion without an ID cannot be tracked" + end + + local ttl = opts.replay_ttl or DEFAULT_REPLAY_TTL + if assertion.not_on_or_after then + local expires = parse_iso8601_utc_time(assertion.not_on_or_after) + if expires then + ttl = expires + skew - now + end + end + if ttl < 1 then + ttl = 1 + end + + -- an SP name in the key so instances sharing one dict stay apart + local key = tostring(opts.sp_issuer) .. "|" .. assertion.id + local added, err, forcible = dict:add(key, true, ttl) + if not added then + if err == "exists" then + return false, "assertion " .. assertion.id .. " has been presented already" + end + return false, "could not track assertion " .. assertion.id .. ": " .. tostring(err) + end + if forcible then + ngx.log(ngx.WARN, "the assertion replay dict is full, older assertions are ", + "no longer tracked") + end + end + + return true +end + + local function login_callback(self, opts) local sess = session.start(self.session_config) @@ -447,12 +496,21 @@ local function login_callback(self, opts) ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) end - local acceptable, reason = assertions_acceptable(opts, assertions, expected, ngx.time()) + local now = ngx.time() + local acceptable, reason = assertions_acceptable(opts, assertions, expected, now) if not acceptable then ngx.log(ngx.ERR, "response from IdP rejected: ", reason) ngx.exit(ngx.HTTP_UNAUTHORIZED) end + if self.replay_dict then + local unused, used_reason = assertions_unused(self.replay_dict, opts, assertions, now) + if not unused then + ngx.log(ngx.ERR, "response from IdP rejected: ", used_reason) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end + end + local issuer = saml.doc_issuer(doc) local attrs = saml.doc_attrs(doc) local name_id = saml.doc_name_id(doc) @@ -652,6 +710,10 @@ function _M.new(opts) obj.key_mngr_from_doc = function(doc) return obj.idp_cert_manager end obj.idp_cert_func = function(doc) return idp_cert end obj.auth_protocol_binding_method = opts.auth_protocol_binding_method + if opts.replay_dict then + obj.replay_dict = assert(ngx.shared[opts.replay_dict], + "no lua_shared_dict named " .. opts.replay_dict) + end local cookie_secure, cookie_same_site if opts.auth_protocol_binding_method == "HTTP-POST" then cookie_secure = true diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index a8df40a..7df3131 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -30,6 +30,8 @@ _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;;'; + lua_shared_dict saml_replay 1m; + init_by_lua_block { saml = require "saml" local err = saml.init({ debug = true, data_dir = os.getenv("SAML_DATA_DIR") }) @@ -94,6 +96,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== plain = {}, skew = { clock_skew = 300 }, audiences = { sp_audiences = { "https://sp.example.com/metadata" } }, + replay = { replay_dict = "saml_replay" }, } SPS = {} @@ -603,3 +606,59 @@ offers no subject confirmation this SP can satisfy } --- response_body 302 / + + + +=== TEST 21: an assertion is good for one login +--- config + location /t { + content_by_lua_block { + local xml = saml_response({ conditions = conditions({ not_on_or_after = at(600) }) }) + ngx.say(login_with("replay", xml)) + ngx.say(login_with("replay", xml)) + } + } +--- response_body +302 / +401 nil +--- error_log +assertion a1 has been presented already + + + +=== TEST 22: a second assertion of its own is accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("replay", saml_response({ id = "a1" }))) + ngx.say(login_with("replay", saml_response({ id = "a2" }))) + } + } +--- response_body +302 / +302 / + + + +=== TEST 23: an assertion is remembered for as long as it is usable +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("replay", saml_response({ + conditions = conditions({ not_on_or_after = at(600) }), + }))) + -- the window plus the skew allowance, which is when it stops being + -- accepted and so stops being worth remembering + local ttl = ngx.shared.saml_replay:ttl("sp|a1") + ngx.say("tracked: ", ttl > 600 and ttl <= 660) + + ngx.say(login_with("replay", saml_response({ id = "a2" }))) + local default = ngx.shared.saml_replay:ttl("sp|a2") + ngx.say("default: ", default > 590 and default <= 600) + } + } +--- response_body +302 / +tracked: true +302 / +default: true From 8144136a9c1b65ee94f68a58cdb10fd3ae8ab341 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 14:01:02 +0545 Subject: [PATCH 04/19] fix: let the ACS URL be configured, and keep audience lists dense The endpoint checks compared against a URL assembled from the request's scheme and host. That value has only ever fed the AssertionConsumerService URL announced to the IdP, which many IdPs ignore in favour of the one registered against the SP, so a wrong value carried no symptom. Making it an acceptance criterion turns the same divergence into every login being refused, and a proxy terminating TLS outside the trusted addresses is enough to cause it. sp_acs_url states the endpoint outright. It is announced to the IdP and enforced on the way back, so the two cannot drift, and it settles what Destination and Recipient are measured against rather than leaving that to headers. Unset keeps the assembled value. An Audience with no text also left a hole in the list handed to Lua, where ipairs stops early and the error path then walked onto the nil. The index is dense now. --- README.md | 1 + lua/resty/saml.lua | 13 ++++++-- src/lua_saml.c | 5 +++- t/assertion-conditions.t | 65 ++++++++++++++++++++++++++++++++++++---- 4 files changed, 75 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 820e531..c887cfd 100644 --- a/README.md +++ b/README.md @@ -79,6 +79,7 @@ local saml = resty_saml.new(opts) | `logout_redirect_uri` | string | None | redirect uri after sucessful logout. | | `sp_cert` | string | None | SP Certificate, used to sign the saml request. | | `sp_private_key` | string | None | SP private key. | +| `sp_acs_url` | string | built from the request | Absolute URL of this SP's assertion consumer service. It is announced to the IdP and is what `Destination` and `SubjectConfirmationData/@Recipient` have to name. Unset assembles it from the request's scheme and host, which needs a proxy that sets `X-Forwarded-Proto` and `X-Forwarded-Host` correctly. | | `sp_audiences` | array of strings | `{ sp_issuer }` | Audiences this SP answers to. An assertion carrying an `AudienceRestriction` has to name one of them; an assertion carrying none is unrestricted. | | `clock_skew` | number | `60` | Seconds of clock difference tolerated against the IdP when weighing `NotBefore` and `NotOnOrAfter`. | diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 79655eb..340b980 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -126,6 +126,15 @@ local function saml_get_redirect_uri(path) return scheme .. "://" .. host .. path end +-- The endpoint the IdP delivers the response to. A configured value wins over +-- the one assembled from request headers, which the requester can steer, and it +-- is what an SP behind a proxy that rewrites neither scheme nor host needs. +-- The same value is announced to the IdP and enforced on the way back, so the +-- two cannot drift. +local function sp_acs_url(opts) + return opts.sp_acs_url or saml_get_redirect_uri(opts.login_callback_uri) +end + local function interp(s, tab) return s:gsub('($%b{})', function(w) local key = w:sub(3, -2) @@ -151,7 +160,7 @@ local AUTHN_REQUEST = [[ local function authn_request(opts) return interp(AUTHN_REQUEST, { - acs_url = saml_get_redirect_uri(opts.login_callback_uri), + acs_url = sp_acs_url(opts), destination = opts.idp_uri, issue_instant = os.date("!%Y-%m-%dT%TZ"), issuer = opts.sp_issuer, @@ -415,7 +424,7 @@ local function login_callback(self, opts) ngx.exit(ngx.HTTP_UNAUTHORIZED) end - local acs_url = saml_get_redirect_uri(opts.login_callback_uri) + local acs_url = sp_acs_url(opts) local destination = saml.doc_destination(doc) if destination and destination ~= acs_url then diff --git a/src/lua_saml.c b/src/lua_saml.c index c252e16..7412122 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -574,11 +574,14 @@ static void push_audience_restrictions(lua_State* L, saml_assertion_t* a) { saml_audience_restriction_t* restriction = a->audience_restrictions + i; lua_pushinteger(L, i + 1); lua_newtable(L); + // a dense index, so an audience with no text leaves no hole for ipairs to + // stop at and shorten the list the assertion declared + int n = 0; for (size_t j = 0; j < restriction->audiences_len; j++) { if (restriction->audiences[j] == NULL) { continue; } - lua_pushinteger(L, j + 1); + lua_pushinteger(L, ++n); lua_pushstring(L, (char*)restriction->audiences[j]); lua_settable(L, -3); } diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 403ddb1..25fa0c7 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -94,6 +94,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== plain = {}, skew = { clock_skew = 300 }, audiences = { sp_audiences = { "https://sp.example.com/metadata" } }, + acs = { sp_acs_url = "http://127.0.0.1:1984/acs" }, } SPS = {} @@ -188,9 +189,19 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== return response(sign_doc(assertion(spec)), destination) end + function callback_headers(name, cookie, extra) + local headers = { + ["X-Test-SP"] = name, + ["Cookie"] = cookie:match("^[^;]+"), + ["Content-Type"] = "application/x-www-form-urlencoded", + } + for k, v in pairs(extra or {}) do headers[k] = v end + return headers + end + -- start a login, then hand the crafted response back to the callback -- with the session and RelayState that login handed out - function login_with(name, xml) + function login_with(name, xml, extra) local httpc = require("resty.http").new() local base = "http://127.0.0.1:1984" local headers = { ["X-Test-SP"] = name } @@ -205,11 +216,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== method = "POST", body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. "&RelayState=" .. state, - headers = { - ["X-Test-SP"] = name, - ["Cookie"] = cookie:match("^[^;]+"), - ["Content-Type"] = "application/x-www-form-urlencoded", - }, + headers = callback_headers(name, cookie, extra), }) if not res then return "callback request: " .. err end return res.status .. " " .. tostring(res.headers["Location"]) @@ -534,3 +541,49 @@ env SAML_DATA_DIR=./; env TZ=XXX-14; --- response_body 302 / + + + +=== TEST 18: a configured ACS URL settles what the endpoint checks compare against +--- config + location /t { + content_by_lua_block { + local elsewhere = saml_response({ + confirmations = confirmation({ recipient = "https://sp.example.com/acs" }), + }) + local here = saml_response({ confirmations = confirmation({ recipient = ACS }) }) + local forged = { + ["X-Forwarded-Proto"] = "https", + ["X-Forwarded-Host"] = "sp.example.com", + } + + -- assembled from the request, the endpoint moves with the headers + ngx.say(login_with("plain", elsewhere, forged)) + -- configured, it stays where the deployment put it + ngx.say(login_with("acs", elsewhere, forged)) + -- and headers that disagree cannot refuse an assertion that names it + ngx.say(login_with("acs", here, forged)) + } + } +--- response_body +302 / +401 nil +302 / +--- error_log +offers no subject confirmation this SP can satisfy + + + +=== TEST 19: an audience with no text leaves the rest of its restriction readable +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ body = "" .. + "sp" .. + "" }), + }))) + } + } +--- response_body +302 / From 90671a145d4315c89c58fb77f5dcd2298a460e12 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 14:42:10 +0545 Subject: [PATCH 05/19] fix: refuse OneTimeUse, which nothing here can honour OneTimeUse sat on the list of conditions this SP claims to satisfy while nothing acted on it. 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. Off the list, so it lands on the same path as a condition nobody here has heard of. The message says the SP cannot satisfy the condition rather than that it does not recognise it, which is the truth for both. ProxyRestriction stays, since it binds an IdP issuing on behalf of another IdP and asks nothing of the SP consuming the assertion. --- lua/resty/saml.lua | 5 +++-- src/xml.c | 13 +++++++++---- t/assertion-conditions.t | 16 ++++++++++++---- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 340b980..fbe5814 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -349,10 +349,11 @@ local function assertions_acceptable(opts, assertions, acs_url, now) for _, assertion in ipairs(assertions) do local where = "assertion " .. tostring(assertion.id) .. " " - -- SAML Core 2.5.1: a condition the SP does not understand leaves the + -- SAML Core 2.5.1: a condition the SP cannot satisfy leaves the -- assertion Indeterminate, which is not a licence to use it if assertion.unknown_condition then - return false, where .. "carries an unrecognised condition " .. assertion.unknown_condition + return false, where .. "carries a condition this SP cannot satisfy: " .. + assertion.unknown_condition end local ok, err = time_bounds_ok(assertion.not_before, assertion.not_on_or_after, now, skew) diff --git a/src/xml.c b/src/xml.c index aaba60d..d854529 100644 --- a/src/xml.c +++ b/src/xml.c @@ -277,12 +277,17 @@ static size_t count_assertion_el(xmlNode* parent, const char* name) { } -// Conditions this reader can hand the caller enough to weigh. SAML Core 2.5.1 -// makes an assertion carrying any other condition Indeterminate rather than -// valid, so anything else is reported as unrecognised for the caller to refuse. +// Conditions this SP can actually satisfy. 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. static int is_known_condition(xmlNode* node) { return is_assertion_el(node, "AudienceRestriction") || - is_assertion_el(node, "OneTimeUse") || is_assertion_el(node, "ProxyRestriction"); } diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 25fa0c7..b9445ad 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -445,13 +445,19 @@ offers no subject confirmation this SP can satisfy -=== TEST 13: an unrecognised condition leaves the assertion indeterminate +=== TEST 13: a condition this SP cannot satisfy leaves the assertion indeterminate --- config location /t { content_by_lua_block { + -- ProxyRestriction binds the IdP, not this SP, so it is satisfied ngx.say(login_with("plain", saml_response({ - conditions = conditions({ body = "" }), + conditions = conditions({ body = "" }), }))) + -- OneTimeUse asks this SP to remember which assertions it has spent + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ body = "" }), + }))) + -- and a condition it has never heard of asks who knows what ngx.say(login_with("plain", saml_response({ conditions = conditions({ body = ' Date: Fri, 21 Aug 2026 16:32:17 +0545 Subject: [PATCH 06/19] test: start each replay block from an empty dict An shm zone of the same name and size is reused across a reload, so under TEST_NGINX_USE_HUP=1 the entries one block wrote outlived it and the next refused its own first login. The suite passed only because Test::Nginx restarts nginx per block by default. Reported on #43. Without the flush, TEST_NGINX_USE_HUP=1 fails 5 subtests across TESTs 33 and 34; with it both modes pass. --- t/assertion-conditions.t | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 788a7fc..633ecb2 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -35,6 +35,9 @@ _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;;'; + # blocks driving it flush it first: a zone of the same name and size is + # reused across a reload, so entries otherwise outlive the block that made + # them under TEST_NGINX_USE_HUP=1 lua_shared_dict saml_replay 1m; init_by_lua_block { @@ -995,6 +998,7 @@ session carries no request id, starting the login again --- config location /t { content_by_lua_block { + ngx.shared.saml_replay:flush_all() local xml = saml_response({ conditions = conditions({ not_on_or_after = at(600) }) }) ngx.say(login_with("replay", xml)) ngx.say(login_with("replay", xml)) @@ -1011,6 +1015,7 @@ assertion a1 has been presented already --- config location /t { content_by_lua_block { + ngx.shared.saml_replay:flush_all() ngx.say(login_with("replay", saml_response({ id = "a1" }))) ngx.say(login_with("replay", saml_response({ id = "a2" }))) } @@ -1024,6 +1029,7 @@ assertion a1 has been presented already --- config location /t { content_by_lua_block { + ngx.shared.saml_replay:flush_all() ngx.say(login_with("replay", saml_response({ conditions = conditions({ not_on_or_after = at(600) }), }))) From 8d4cba94295f38a76738d5c3aa1b64d3590a3001 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 15:05:25 +0545 Subject: [PATCH 07/19] feat: carry each assertion's issuer through the reader An assertion ID is unique only within the IdP that minted it, and idp_issuers takes a list, so anything keyed on the ID alone conflates two IdPs that pick the same one. The reader already had issuer_of for the Response, so the assertion table carries the same value now. Absent and empty read alike, as they already do for doc_issuers. --- src/lua_saml.c | 1 + src/saml.h | 1 + src/xml.c | 5 +++++ 3 files changed, 7 insertions(+) diff --git a/src/lua_saml.c b/src/lua_saml.c index 05f56fb..f0a7d2f 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -696,6 +696,7 @@ static int doc_assertions(lua_State* L) { lua_pushinteger(L, i + 1); lua_newtable(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_str_field(L, "not_before", a->not_before); set_str_field(L, "not_on_or_after", a->not_on_or_after); diff --git a/src/saml.h b/src/saml.h index ac54afc..5d7592e 100644 --- a/src/saml.h +++ b/src/saml.h @@ -57,6 +57,7 @@ typedef struct { typedef struct { xmlChar* id; + xmlChar* issuer; int has_conditions; xmlChar* not_before; xmlChar* not_on_or_after; diff --git a/src/xml.c b/src/xml.c index 6858757..61ba856 100644 --- a/src/xml.c +++ b/src/xml.c @@ -497,6 +497,10 @@ static int read_assertion(xmlDoc* doc, xmlNode* node, saml_assertion_t* a) { return -1; } + // An ID is unique only within the IdP that minted it, so the caller keeps the + // two together. Absent and empty read alike here, as they do for doc_issuers. + a->issuer = issuer_of(doc, node); + xmlNode* conditions = assertion_child(node, "Conditions"); if (conditions != NULL) { a->has_conditions = 1; @@ -608,6 +612,7 @@ void saml_assertions_free(saml_assertion_t* assertions, size_t assertions_len) { for (size_t i = 0; i < assertions_len; i++) { saml_assertion_t* a = assertions + i; xmlFree(a->id); + xmlFree(a->issuer); xmlFree(a->not_before); xmlFree(a->not_on_or_after); xmlFree(a->unknown_condition); From 9d59d5a596f34114b9c80b77c99cc5c232d60c5c Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 15:06:11 +0545 Subject: [PATCH 08/19] fix: key the replay record on the IdP as well as the SP An assertion ID is only unique within the IdP that issued it, so two IdPs in idp_issuers picking the same one made the second login look like a replay of the first and answered a 401 blaming the user. TEST 35 covers it; without the issuer in the key it fails. The tests name their own assertions rather than sharing the default a1, so a block's entries cannot be mistaken for another's, and one helper owns the key layout: reading the dict by a hand-built key made a change to the scheme surface as a comparison against nil. --- lua/resty/saml.lua | 11 +++++++-- t/assertion-conditions.t | 50 ++++++++++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 14 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 71c642d..ab3c93b 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -504,6 +504,14 @@ local function issuers_allowed(allowed, issuers) return true end +-- An ID is unique only within the IdP that minted it, and idp_issuers takes a +-- list, so the two travel together. The SP name keeps instances sharing one +-- dict apart. +local function replay_key(opts, assertion) + return tostring(opts.sp_issuer) .. "|" .. (assertion.issuer or "") .. "|" .. assertion.id +end + + -- A bearer assertion is good for one login. Nothing above stops the same one -- being presented again inside its validity window, so its ID is kept until it -- expires and a second presentation is refused. @@ -531,8 +539,7 @@ local function assertions_unused(dict, opts, assertions, now) ttl = 1 end - -- an SP name in the key so instances sharing one dict stay apart - local key = tostring(opts.sp_issuer) .. "|" .. assertion.id + local key = replay_key(opts, assertion) local added, err, forcible = dict:add(key, true, ttl) if not added then if err == "exists" then diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 633ecb2..65eefd4 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -35,9 +35,9 @@ _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;;'; - # blocks driving it flush it first: a zone of the same name and size is - # reused across a reload, so entries otherwise outlive the block that made - # them under TEST_NGINX_USE_HUP=1 + # 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 + # their own assertions to stay apart, and flush as well lua_shared_dict saml_replay 1m; init_by_lua_block { @@ -196,7 +196,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== 'ID="%s" Version="2.0" IssueInstant="2026-07-21T00:00:00Z">' .. '%s' .. '%s%s%s%s', - spec.id or "a1", IDP, spec.name_id or "signed\@example.com", + 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)) end @@ -228,6 +228,12 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== return saml.doc_id(doc) end + -- the module owns this layout; naming it once here keeps a change to + -- the scheme from surfacing as a comparison against nil + function replay_key(id, issuer) + return "sp|" .. (issuer or IDP) .. "|" .. id + end + function callback_headers(name, cookie, extra) local headers = { ["X-Test-SP"] = name, @@ -999,7 +1005,9 @@ session carries no request id, starting the login again location /t { content_by_lua_block { ngx.shared.saml_replay:flush_all() - local xml = saml_response({ conditions = conditions({ not_on_or_after = at(600) }) }) + local xml = saml_response({ + id = "once", conditions = conditions({ not_on_or_after = at(600) }), + }) ngx.say(login_with("replay", xml)) ngx.say(login_with("replay", xml)) } @@ -1008,7 +1016,7 @@ session carries no request id, starting the login again 302 / 401 nil --- error_log -assertion a1 has been presented already +assertion once has been presented already === TEST 33: a second assertion of its own is accepted @@ -1016,8 +1024,8 @@ assertion a1 has been presented already location /t { content_by_lua_block { ngx.shared.saml_replay:flush_all() - ngx.say(login_with("replay", saml_response({ id = "a1" }))) - ngx.say(login_with("replay", saml_response({ id = "a2" }))) + ngx.say(login_with("replay", saml_response({ id = "first" }))) + ngx.say(login_with("replay", saml_response({ id = "second" }))) } } --- response_body @@ -1031,15 +1039,15 @@ assertion a1 has been presented already content_by_lua_block { ngx.shared.saml_replay:flush_all() ngx.say(login_with("replay", saml_response({ - conditions = conditions({ not_on_or_after = at(600) }), + id = "bounded", conditions = conditions({ not_on_or_after = at(600) }), }))) -- the window plus the skew allowance, which is when it stops being -- accepted and so stops being worth remembering - local ttl = ngx.shared.saml_replay:ttl("sp|a1") + local ttl = ngx.shared.saml_replay:ttl(replay_key("bounded")) ngx.say("tracked: ", ttl > 600 and ttl <= 660) - ngx.say(login_with("replay", saml_response({ id = "a2" }))) - local default = ngx.shared.saml_replay:ttl("sp|a2") + ngx.say(login_with("replay", saml_response({ id = "unbounded" }))) + local default = ngx.shared.saml_replay:ttl(replay_key("unbounded")) ngx.say("default: ", default > 590 and default <= 600) } } @@ -1048,3 +1056,21 @@ assertion a1 has been presented already tracked: true 302 / default: true + + +=== TEST 35: two IdPs may mint the same assertion ID +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- an ID is unique only within the IdP that issued it, so sharing + -- one is not a replay + ngx.say(login_with("replay", saml_response({ id = "shared" }))) + ngx.say(login_with("replay", saml_response({ + id = "shared", issuer = "https://second-idp.example.com", + }))) + } + } +--- response_body +302 / +302 / From 02f214f55c7b7742341c9e181b8d10437ba25da3 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 15:07:15 +0545 Subject: [PATCH 09/19] fix: remember an assertion against every expiry this SP weighed The record's lifetime came from Conditions/@NotOnOrAfter alone, which is not the only bound the login is accepted against: confirmation_ok weighs SubjectConfirmationData/@NotOnOrAfter, and profile 4.1.4.2 puts a bearer assertion's expiry there. A Conditions carrying nothing but an audience is therefore the ordinary shape, and it fell to the replay_ttl fallback: the entry lapsed at ten minutes while the same assertion stayed acceptable for the hour its confirmation allowed, replaying cleanly against a conformant IdP with the dict configured and nothing logged. The latest of every bound decides now. Remembering too long costs a slot; remembering too little reopens the window the record is there to close. The parse guard beside it could not fire, since assertions_acceptable has already refused an unreadable NotOnOrAfter, and its silent fallback to replay_ttl was the shape that would have hidden the case above. It fails the login instead. TEST 36 covers the confirmation expiry, TEST 37 the fallback that TEST 34 used to assert while claiming the opposite, and TEST 38 replay_ttl, which nothing exercised. --- lua/resty/saml.lua | 46 +++++++++++++++++++++++++--- t/assertion-conditions.t | 66 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 101 insertions(+), 11 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index ab3c93b..7825c7f 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -504,6 +504,41 @@ local function issuers_allowed(allowed, issuers) return true end +-- The last moment any bound this SP weighed would still admit the assertion. +-- Conditions/@NotOnOrAfter is one of them; profile 4.1.4.2 puts a bearer +-- assertion's expiry on its confirmation instead, so a Conditions carrying +-- nothing but an audience is the profile-minimal shape rather than an odd one. +-- The latest of them decides: remembering too long costs a slot, remembering +-- too little reopens the window the record is there to close. Nil when nothing +-- names one at all, which is what replay_ttl stands in for. +local function last_moment_usable(assertion) + local bounds = {} + if assertion.not_on_or_after then + bounds[#bounds + 1] = assertion.not_on_or_after + end + for _, confirmation in ipairs(assertion.subject_confirmations) do + if confirmation.not_on_or_after then + bounds[#bounds + 1] = confirmation.not_on_or_after + end + end + + local latest + for _, bound in ipairs(bounds) do + -- assertions_acceptable has already refused an unreadable one, so this + -- refuses the login rather than quietly shortening what is remembered + local at, err = parse_iso8601_utc_time(bound) + if not at then + return nil, "carries an unreadable NotOnOrAfter " .. bound .. ": " .. err + end + if latest == nil or at > latest then + latest = at + end + end + + return latest +end + + -- An ID is unique only within the IdP that minted it, and idp_issuers takes a -- list, so the two travel together. The SP name keeps instances sharing one -- dict apart. @@ -529,11 +564,12 @@ local function assertions_unused(dict, opts, assertions, now) end local ttl = opts.replay_ttl or DEFAULT_REPLAY_TTL - if assertion.not_on_or_after then - local expires = parse_iso8601_utc_time(assertion.not_on_or_after) - if expires then - ttl = expires + skew - now - end + local usable_until, err = last_moment_usable(assertion) + if err then + return false, "assertion " .. assertion.id .. " " .. err + end + if usable_until then + ttl = usable_until + skew - now end if ttl < 1 then ttl = 1 diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 65eefd4..a65879c 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -106,6 +106,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== audiences = { sp_audiences = { "https://sp.example.com/metadata" } }, 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 }, } SPS = {} @@ -1045,17 +1046,11 @@ assertion once has been presented already -- accepted and so stops being worth remembering local ttl = ngx.shared.saml_replay:ttl(replay_key("bounded")) ngx.say("tracked: ", ttl > 600 and ttl <= 660) - - ngx.say(login_with("replay", saml_response({ id = "unbounded" }))) - local default = ngx.shared.saml_replay:ttl(replay_key("unbounded")) - ngx.say("default: ", default > 590 and default <= 600) } } --- response_body 302 / tracked: true -302 / -default: true === TEST 35: two IdPs may mint the same assertion ID @@ -1074,3 +1069,62 @@ default: true --- response_body 302 / 302 / + + +=== TEST 36: an expiry the IdP puts on the confirmation decides it too +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- profile 4.1.4.2 puts a bearer assertion's expiry here, so a + -- Conditions naming only an audience is the ordinary shape. Reading + -- only Conditions forgot the assertion while it was still accepted. + ngx.say(login_with("replay", function(request_id) + return saml_response({ + id = "on-confirmation", + conditions = conditions({ body = audience("sp") }), + confirmations = confirmation({ + recipient = ACS, not_on_or_after = at(3600), + in_response_to = request_id, + }), + }, ACS, request_id) + end)) + local ttl = ngx.shared.saml_replay:ttl(replay_key("on-confirmation")) + ngx.say("tracked: ", ttl > 3600 and ttl <= 3660) + } + } +--- response_body +302 / +tracked: true + + +=== TEST 37: an assertion naming no expiry falls back to replay_ttl +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- nothing bounds it, so it is replayable once the record lapses. + -- That is what replay_ttl is for and the README says so. + ngx.say(login_with("replay", saml_response({ id = "unbounded" }))) + local ttl = ngx.shared.saml_replay:ttl(replay_key("unbounded")) + ngx.say("default: ", ttl > 590 and ttl <= 600) + } + } +--- response_body +302 / +default: true + + +=== TEST 38: replay_ttl settles that fallback +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + ngx.say(login_with("replay_short", saml_response({ id = "configured" }))) + local ttl = ngx.shared.saml_replay:ttl(replay_key("configured")) + ngx.say("configured: ", ttl > 80 and ttl <= 90) + } + } +--- response_body +302 / +configured: true From ffae70a5396317f28eb244606acb9386b4145fa4 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 15:07:38 +0545 Subject: [PATCH 10/19] fix: cap how long an assertion is remembered The lifetime was clamped from below and left open above. The schema takes any year up to 9999 and time_bounds_ok only refuses a NotOnOrAfter in the past, so an IdP with a generous window pinned entries that the dict never reclaims, evicting live ones to make room. A day is longer than anyone is still trying to finish that login. --- lua/resty/saml.lua | 7 +++++++ t/assertion-conditions.t | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 7825c7f..3843f37 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -323,6 +323,11 @@ local DEFAULT_CLOCK_SKEW = 60 -- how long an assertion that sets no expiry of its own is remembered 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. +local MAX_REPLAY_TTL = 86400 + local function time_bounds_ok(not_before, not_on_or_after, now, skew) local opens, closes, err @@ -573,6 +578,8 @@ local function assertions_unused(dict, opts, assertions, now) end if ttl < 1 then ttl = 1 + elseif ttl > MAX_REPLAY_TTL then + ttl = MAX_REPLAY_TTL end local key = replay_key(opts, assertion) diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index a65879c..caef74a 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -1128,3 +1128,23 @@ default: true --- response_body 302 / configured: true + + +=== TEST 39: an assertion good for years is remembered for a day +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- the schema takes any year up to 9999, and an entry that never + -- lapses is a slot the dict never reclaims + ngx.say(login_with("replay", saml_response({ + id = "forever", + conditions = conditions({ not_on_or_after = "9999-12-31T23:59:59Z" }), + }))) + local ttl = ngx.shared.saml_replay:ttl(replay_key("forever")) + ngx.say("capped: ", ttl > 86300 and ttl <= 86400) + } + } +--- response_body +302 / +capped: true From 41bd5661a4048ad8cbd5db45886759139cf6bdcd Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 15:08:58 +0545 Subject: [PATCH 11/19] fix: leave a login untracked rather than evicting somebody else's record add makes room by evicting, so a full dict took the record away from an earlier login that was still relying on it, and returned forcible to whichever request needed the space. The replay it enabled arrived later, found no key, and was accepted cleanly with nothing logged: the login that should have been refused was the one that said nothing, and the warning named a request that had done nothing wrong. safe_add refuses instead of evicting. This login goes untracked, which is the same exposure as before for one login rather than for someone else's, and the error names the request it actually applies to. Deliberately not a refusal. A zone holds one entry per accepted login for the assertion's remaining life, so an SP taking ten logins a second against ten-minute assertions holds thousands at once and a full zone is an ordinary Tuesday. Failing shut there takes the whole application down over a sizing mistake. --- lua/resty/saml.lua | 24 +++++++++++------------- t/assertion-conditions.t | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 3843f37..69958bc 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -553,13 +553,14 @@ end -- A bearer assertion is good for one login. Nothing above stops the same one --- being presented again inside its validity window, so its ID is kept until it --- expires and a second presentation is refused. +-- being presented again inside its validity window, so its ID is remembered for +-- as long as it could still be used and a second presentation is refused. -- --- The window from the assertion's own Conditions decides how long the entry --- lives, so the cache holds exactly what is still usable. An assertion that --- names no expiry is replayable for as long as it is remembered, which is what --- replay_ttl bounds. +-- A dict with no room leaves this assertion untracked rather than evicting one +-- that is still 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 happened to +-- need the space. local function assertions_unused(dict, opts, assertions, now) local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW @@ -583,16 +584,13 @@ local function assertions_unused(dict, opts, assertions, now) end local key = replay_key(opts, assertion) - local added, err, forcible = dict:add(key, true, ttl) + local added, add_err = dict:safe_add(key, true, ttl) if not added then - if err == "exists" then + if add_err == "exists" then return false, "assertion " .. assertion.id .. " has been presented already" end - return false, "could not track assertion " .. assertion.id .. ": " .. tostring(err) - end - if forcible then - ngx.log(ngx.WARN, "the assertion replay dict is full, older assertions are ", - "no longer tracked") + ngx.log(ngx.ERR, "could not remember assertion ", loggable(assertion.id), ": ", + add_err, ", this login is not covered by replay tracking") end end diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index caef74a..3c009c6 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -39,6 +39,7 @@ _EOC_ # outlive the block that made them under TEST_NGINX_USE_HUP=1. Blocks name # their own assertions to stay apart, and flush as well lua_shared_dict saml_replay 1m; + lua_shared_dict saml_replay_full 32k; init_by_lua_block { saml = require "saml" @@ -107,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_full = { replay_dict = "saml_replay_full" }, } SPS = {} @@ -1148,3 +1150,36 @@ configured: true --- response_body 302 / capped: true + + +=== 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") + + -- evicting would take the record away from whoever holds it and + -- report it against this request, so this login goes untracked + ngx.say(login_with("replay_full", saml_response({ id = "untracked" }))) + } + } +--- response_body +full: true +302 / +--- error_log +this login is not covered by replay tracking From 0494f328b9376ca9ecddf10cbc8cbdecb844e580 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 15:09:59 +0545 Subject: [PATCH 12/19] fix: spend an assertion only where it authenticates somebody The record was written before the rest of the callback could still refuse the login. issuers_allowed, the missing name id and the unreadable SessionNotOnOrAfter all sit below it, so a refused login left the assertion spent: the operator fixing the configuration and retrying was told the assertion had been presented already rather than what was actually wrong, and after the fix the same response was refused as a replay although it would now be accepted. Writing it at the last gate closes all three without collecting keys or tracking what to undo. TEST 41 covers it. Inside the loop there is still something to undo. A response carrying a fresh assertion beside a spent one authenticates nobody, so the fresh one is handed back rather than left dead for the rest of its window. TEST 42 covers that. --- lua/resty/saml.lua | 37 ++++++++++++++++++---------- t/assertion-conditions.t | 52 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 69958bc..7c76def 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -556,13 +556,16 @@ end -- being presented again inside its validity window, so its ID is remembered for -- as long as it could still be used and a second presentation is refused. -- --- A dict with no room leaves this assertion untracked rather than evicting one +-- Called at the last gate rather than beside the checks, so a login the rest of +-- the callback still refuses leaves the assertion unspent. A dict with no room +-- leaves this assertion untracked rather than evicting one -- that is still 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 happened to -- need the space. -local function assertions_unused(dict, opts, assertions, now) +local function spend_assertions(dict, opts, assertions, now) local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW + local spent = {} for _, assertion in ipairs(assertions) do if not assertion.id then @@ -585,10 +588,16 @@ local function assertions_unused(dict, opts, assertions, now) local key = replay_key(opts, assertion) local added, add_err = dict:safe_add(key, true, ttl) - if not added then - if add_err == "exists" then - return false, "assertion " .. assertion.id .. " has been presented already" + if added then + spent[#spent + 1] = key + elseif add_err == "exists" then + -- this response authenticates nobody, so the assertions already + -- taken from it are handed back rather than left spent + for _, taken in ipairs(spent) do + dict:delete(taken) end + return false, "assertion " .. assertion.id .. " has been presented already" + else ngx.log(ngx.ERR, "could not remember assertion ", loggable(assertion.id), ": ", add_err, ", this login is not covered by replay tracking") end @@ -688,14 +697,6 @@ local function login_callback(self, opts) ngx.exit(ngx.HTTP_UNAUTHORIZED) end - if self.replay_dict then - local unused, used_reason = assertions_unused(self.replay_dict, opts, assertions, now) - if not unused then - ngx.log(ngx.ERR, "response from IdP rejected: ", loggable(used_reason)) - ngx.exit(ngx.HTTP_UNAUTHORIZED) - end - end - local issuer = saml.doc_issuer(doc) local attrs = saml.doc_attrs(doc) local name_id = saml.doc_name_id(doc) @@ -730,6 +731,16 @@ local function login_callback(self, opts) end + -- 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, now) + if not unused then + ngx.log(ngx.ERR, "response from IdP rejected: ", loggable(used_reason)) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end + end + sess:set("authenticated", true) sess:set("name_id", name_id) sess:set("session_index", session_index) diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 3c009c6..80cdcb3 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -109,6 +109,10 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== replay = { replay_dict = "saml_replay" }, replay_short = { replay_dict = "saml_replay", replay_ttl = 90 }, replay_full = { replay_dict = "saml_replay_full" }, + replay_pinned = { + replay_dict = "saml_replay", + idp_issuers = { "https://elsewhere.example.com" }, + }, } SPS = {} @@ -1183,3 +1187,51 @@ full: true 302 / --- error_log this login is not covered by replay tracking + + +=== TEST 41: a login refused after the checks leaves the assertion unspent +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- idp_issuers refuses this one below where the record used to be + -- written, so writing it early told the retry it was a replay + local xml = saml_response({ id = "unspent" }) + ngx.say(login_with("replay_pinned", xml)) + ngx.say("remembered: ", ngx.shared.saml_replay:get(replay_key("unspent")) ~= nil) + ngx.say(login_with("replay", xml)) + } + } +--- response_body +401 nil +remembered: false +302 / +--- error_log +unexpected issuer in response from IdP + + +=== TEST 42: a response refused part way spends none of its assertions +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- one signature over the whole Response, so it carries two + -- assertions and the reader draws identity from both + local spent = sign_doc(response(assertion({ id = "pair-b" }))) + ngx.say(login_with("replay", spent)) + + local pair = sign_doc(response( + assertion({ id = "pair-a" }) .. assertion({ id = "pair-b" }))) + ngx.say(login_with("replay", pair)) + ngx.say("remembered: ", ngx.shared.saml_replay:get(replay_key("pair-a")) ~= nil) + + ngx.say(login_with("replay", sign_doc(response(assertion({ id = "pair-a" }))))) + } + } +--- response_body +302 / +401 nil +remembered: false +302 / +--- error_log +assertion pair-b has been presented already From c5d206bb59bcab77b3307fd7f183b90201a5df42 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 15:10:31 +0545 Subject: [PATCH 13/19] fix: weigh the replay options where they are given assert raises rather than returning nil, and the gateway plugin builds this object per request through lrucache with no pcall, so a mistyped dict name was an uncaught error on every request and the plugin's own fallback never ran. Both answer 500, so what is actually lost is the message: a traceback about concatenating a boolean instead of the name of the option that is wrong. The message was also concatenated on every successful call, being an argument rather than a branch. Three values are weighed now, at construction, the way issuer_set already does above. sp_issuer is half the replay key, and tostring turned a missing one into the literal nil that two deployments would then share. replay_ttl of 0 means never expire to lua_shared_dict, which is the opposite of what it did here: it reached the floor and became one second, switching the feature off in the name of turning it up. And a number arriving as text, which is what a YAML or environment config path hands over, compared against nothing and raised, but only for assertions naming no expiry, so it read as logins failing with the wind. --- lua/resty/saml.lua | 28 ++++++++++++++++++++++++---- t/assertion-conditions.t | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 7c76def..448b0e6 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -548,7 +548,7 @@ end -- list, so the two travel together. The SP name keeps instances sharing one -- dict apart. local function replay_key(opts, assertion) - return tostring(opts.sp_issuer) .. "|" .. (assertion.issuer or "") .. "|" .. assertion.id + return opts.sp_issuer .. "|" .. (assertion.issuer or "") .. "|" .. assertion.id end @@ -917,9 +917,29 @@ function _M.new(opts) obj.idp_cert_func = function(doc) return idp_cert end obj.auth_protocol_binding_method = opts.auth_protocol_binding_method obj.idp_issuers = issuer_set(opts.idp_issuers) - if opts.replay_dict then - obj.replay_dict = assert(ngx.shared[opts.replay_dict], - "no lua_shared_dict named " .. opts.replay_dict) + -- read once, and raised rather than returned so a mistyped name names + -- itself. A message built as an argument to assert is built on every + -- successful call too, and a non-string one fails on the concatenation + -- rather than on the option. + if opts.replay_dict ~= nil then + if type(opts.replay_dict) ~= "string" then + error("replay_dict must be the name of a lua_shared_dict", 2) + end + obj.replay_dict = ngx.shared[opts.replay_dict] + if obj.replay_dict == nil then + error("no lua_shared_dict named " .. opts.replay_dict, 2) + end + -- 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 + error("sp_issuer must be a string to track assertions", 2) + end + -- zero means never expire to lua_shared_dict, and a number arriving + -- from YAML or the environment as text compares against nothing + if opts.replay_ttl ~= nil and + (type(opts.replay_ttl) ~= "number" or opts.replay_ttl < 1) then + error("replay_ttl must be a positive number of seconds", 2) + end end local cookie_secure, cookie_same_site if opts.auth_protocol_binding_method == "HTTP-POST" then diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 80cdcb3..b05c657 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -1235,3 +1235,43 @@ remembered: false 302 / --- error_log assertion pair-b has been presented already + + +=== TEST 43: replay configuration is weighed when the SP is built +--- config + location /t { + content_by_lua_block { + local resty_saml = require("resty.saml") + local function build(extra, drop) + local opts = { + sp_issuer = "sp", + idp_uri = "http://127.0.0.1:1984/idp", + login_callback_uri = "/acs", + sp_cert = CERT_PEM, + sp_private_key = KEY_PEM, + idp_cert = CERT_PEM, + secret = "very-secret-key-that-is-32-byte!", + } + for k, v in pairs(extra) do opts[k] = v end + if drop then opts[drop] = nil end + local ok, err = pcall(resty_saml.new, opts) + return ok and "built" or err:gsub("^.-:%d+: ", "") + end + + ngx.say(build({ replay_dict = true })) + ngx.say(build({ replay_dict = "no-such-dict" })) + ngx.say(build({ replay_dict = "saml_replay" }, "sp_issuer")) + -- zero means never expire to lua_shared_dict, and text is what a + -- YAML or environment config path hands over + ngx.say(build({ replay_dict = "saml_replay", replay_ttl = 0 })) + ngx.say(build({ replay_dict = "saml_replay", replay_ttl = "600" })) + ngx.say(build({ replay_dict = "saml_replay", replay_ttl = 90 })) + } + } +--- response_body +replay_dict must be the name of a lua_shared_dict +no lua_shared_dict named no-such-dict +sp_issuer must be a string to track assertions +replay_ttl must be a positive number of seconds +replay_ttl must be a positive number of seconds +built From 975d2f1047b349c138a928fccf865517f68b2e01 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 15:10:52 +0545 Subject: [PATCH 14/19] docs: say how far the replay guarantee reaches "so none is accepted twice" is more than a lua_shared_dict delivers. The zone is shared between the workers of one gateway and nowhere else, so a captured assertion replayed through a load balancer lands on a replica that has never seen it and is accepted. Across replicas the request binding is what carries the weight, since it travels in the user's own session, and this option is the defence for the deployments that binding leaves uncovered: the ones whose IdP sends no InResponseTo. The two sections point at each other now. Sizing was undocumented, and it is what decides whether an operator meets the untracked-login path at all. One entry per accepted login held for the assertion's remaining life, which is thousands at once for a busy SP, so the 1m in the test file is an example rather than a recommendation. Two behaviours stated rather than left to be discovered: OneTimeUse is still refused outright, so an IdP asking for this protection cannot log in even with the option on, 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. The replay_ttl row said an assertion is remembered until it expires, where the record runs to that moment plus clock_skew, and now applies when nothing names an expiry anywhere rather than only on Conditions. --- README.md | 32 ++++++++++++++++++++++++++++++-- lua/resty/saml.lua | 9 ++++----- 2 files changed, 34 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 0704c5d..a4ec7e4 100644 --- a/README.md +++ b/README.md @@ -83,8 +83,8 @@ local saml = resty_saml.new(opts) | `sp_acs_url` | string | built from the request | Absolute URL of this SP's assertion consumer service. It is announced to the IdP, every `SubjectConfirmationData/@Recipient` has to name it, and a `Destination` has to name it on a response carrying one. Unset, it is assembled from the request's scheme and host, which is only as trustworthy as whatever sits in front: set it wherever the ingress does not normalise `Forwarded` and `X-Forwarded-*`, or terminates TLS without setting `X-Forwarded-Proto`. | | `sp_audiences` | array of strings | `{ sp_issuer }` | Audiences this SP answers to. An assertion carrying an `AudienceRestriction` has to name one of them; an assertion carrying none is unrestricted. | | `clock_skew` | number | `60` | Seconds of clock difference tolerated against the IdP when weighing `NotBefore` and `NotOnOrAfter`. | -| `replay_dict` | string | None | Name of an `lua_shared_dict` in which to remember the assertions already presented, so none is accepted twice. Unset leaves them untracked. | -| `replay_ttl` | number | `600` | Seconds to remember an assertion that names no `NotOnOrAfter` of its own. One that names it is remembered until it expires. | +| `replay_dict` | string | None | Name of an `lua_shared_dict` in which to remember the assertions this instance has already accepted, so it accepts none of them twice. Unset leaves them untracked. See [Remembering assertions](#remembering-assertions) for what the zone has to hold and how far the guarantee reaches. | +| `replay_ttl` | number | `600` | Seconds to remember an assertion that names no `NotOnOrAfter` anywhere, on its `Conditions` or on any subject confirmation. One that names it is remembered until that moment plus `clock_skew`, capped at a day. | #### Binding a response to the request @@ -108,6 +108,34 @@ One note for upgrading. A session minted before this SP kept the ID has nothing the assertion to name, so the login is started again rather than refused. The window lasts as long as an `AuthnRequest` is outstanding across the upgrade. +#### Remembering assertions + +Set `replay_dict` and every assertion this instance accepts is remembered until it +could no longer be used, so presenting the same one again is refused. Leave it unset +and assertions go untracked, which is what happened before the option existed. + +**The guarantee is per instance.** An `lua_shared_dict` is shared between the workers +of one gateway and nowhere else, so a captured assertion replayed through a load +balancer lands on a replica that has never seen it and is accepted. Across replicas +the binding in [Binding a response to the request](#binding-a-response-to-the-request) +is what carries the weight, since it travels in the user's own session, and this +option is the defence for the deployments that binding leaves uncovered: the ones +whose IdP sends no `InResponseTo`. + +**Size the zone for what it holds.** One entry per accepted login, held for the +assertion's remaining lifetime. An SP taking ten logins a second against an IdP +issuing ten-minute assertions holds around six thousand of them at once, so `1m` is +too small for that and a busy deployment wants more. A zone with no room leaves the +login untracked and logs an error naming it, rather than evicting an entry that is +still protecting somebody else. + +**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. + #### Seeding the worker Request IDs and `RelayState` both come from `resty.jit-uuid`, which is seeded when diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 448b0e6..1ae0faf 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -558,11 +558,10 @@ end -- -- Called at the last gate rather than beside the checks, so a login the rest of -- the callback still refuses leaves the assertion unspent. A dict with no room --- leaves this assertion untracked rather than evicting one --- that is still 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 happened to --- need the space. +-- leaves this assertion untracked rather than evicting one that is still +-- 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, now) local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW local spent = {} From c3086094cdc33051746f17275b1a24e6a7460037 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 16:18:43 +0545 Subject: [PATCH 15/19] fix: skip a bound this parser will not take rather than refusing the login The comment claimed assertions_acceptable had already refused an unreadable NotOnOrAfter. It has not: confirmation_ok answers false for the confirmation carrying one and the loop moves on, since one satisfiable confirmation among several is enough. Only Conditions/@NotOnOrAfter is guaranteed readable by this point, because time_bounds_ok weighs that copy unconditionally. So an assertion with one conforming bearer confirmation beside one naming 2030-01-01T00:00:00+00:00, legal xs:dateTime that this parser refuses because SAML times carry no offset, logged in with replay_dict unset and was refused with it set. An option about remembering assertions decided which ones authenticate, which is how a security option gets switched back off. Skipping is right on its own terms rather than merely convenient: a bound that cannot be read belongs to a confirmation that cannot be satisfied, so it can never extend how long the assertion is usable and has nothing to contribute to the latest one. The error naming a full zone names the zone now, so an operator sharing several can tell which to resize. --- lua/resty/saml.lua | 24 +++++++++++------------- t/assertion-conditions.t | 32 +++++++++++++++++++++++++++++++- 2 files changed, 42 insertions(+), 14 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 1ae0faf..1cb7a75 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -529,13 +529,13 @@ local function last_moment_usable(assertion) local latest for _, bound in ipairs(bounds) do - -- assertions_acceptable has already refused an unreadable one, so this - -- refuses the login rather than quietly shortening what is remembered - local at, err = parse_iso8601_utc_time(bound) - if not at then - return nil, "carries an unreadable NotOnOrAfter " .. bound .. ": " .. err - end - if latest == nil or at > latest then + -- A bound this parser will not take, a legal xs:dateTime carrying an + -- offset rather than Z, makes its own confirmation unsatisfiable and so + -- can never extend how long the assertion is usable. Refusing on it + -- would let replay_dict decide which logins are accepted, and one + -- satisfiable confirmation among several is enough for the checks above. + local at = parse_iso8601_utc_time(bound) + if at and (latest == nil or at > latest) then latest = at end end @@ -572,10 +572,7 @@ local function spend_assertions(dict, opts, assertions, now) end local ttl = opts.replay_ttl or DEFAULT_REPLAY_TTL - local usable_until, err = last_moment_usable(assertion) - if err then - return false, "assertion " .. assertion.id .. " " .. err - end + local usable_until = last_moment_usable(assertion) if usable_until then ttl = usable_until + skew - now end @@ -597,8 +594,9 @@ local function spend_assertions(dict, opts, assertions, now) end return false, "assertion " .. assertion.id .. " has been presented already" else - ngx.log(ngx.ERR, "could not remember assertion ", loggable(assertion.id), ": ", - add_err, ", this login is not covered by replay tracking") + 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") end end diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index b05c657..c1ac3a5 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -1186,7 +1186,7 @@ capped: true full: true 302 / --- error_log -this login is not covered by replay tracking +in saml_replay_full: no memory, this login is not covered by replay tracking === TEST 41: a login refused after the checks leaves the assertion unspent @@ -1275,3 +1275,33 @@ sp_issuer must be a string to track assertions replay_ttl must be a positive number of seconds replay_ttl must be a positive number of seconds built + + +=== TEST 44: a confirmation bound this parser will not take is skipped +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- an offset rather than Z is legal xs:dateTime and refused here, so + -- that confirmation is unsatisfiable and the login rides on the + -- other one. Refusing on it would let replay_dict decide who is let + -- in, which is what the option must never do. + ngx.say(login_with("replay", function(request_id) + return saml_response({ + id = "unreadable-bound", + confirmations = confirmation({ + recipient = ACS, not_on_or_after = at(600), + in_response_to = request_id, + }) .. confirmation({ + recipient = ACS, not_on_or_after = "2030-01-01T00:00:00+00:00", + in_response_to = request_id, + }), + }, ACS, request_id) + end)) + local ttl = ngx.shared.saml_replay:ttl(replay_key("unreadable-bound")) + ngx.say("tracked: ", ttl > 600 and ttl <= 660) + } + } +--- response_body +302 / +tracked: true From d48a9a54aeeadc029247f0676268a4b265aa6e25 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 16:19:12 +0545 Subject: [PATCH 16/19] docs: describe the zone by what it stores An entry is written per assertion rather than per login, and a response may carry several, so the sizing rule was worded a size too coarse. The worked figure is unchanged, since a response normally carries one. A zone with no room was described as leaving the login untracked, where a response carrying several assertions can end up partly tracked. That is the safe direction and worth saying rather than making the write atomic: a later replay still collides on whichever assertion was recorded, and rolling the recorded ones back would give that up. The error was said to name the zone, which it does as of the previous commit. --- README.md | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index a4ec7e4..438bd76 100644 --- a/README.md +++ b/README.md @@ -122,12 +122,15 @@ is what carries the weight, since it travels in the user's own session, and this option is the defence for the deployments that binding leaves uncovered: the ones whose IdP sends no `InResponseTo`. -**Size the zone for what it holds.** One entry per accepted login, held for the -assertion's remaining lifetime. An SP taking ten logins a second against an IdP -issuing ten-minute assertions holds around six thousand of them at once, so `1m` is -too small for that and a busy deployment wants more. A zone with no room leaves the -login untracked and logs an error naming it, rather than evicting an entry that is -still protecting somebody else. +**Size the zone for what it holds.** One entry per assertion accepted, held for as +long as that assertion could still be used. A response normally carries one, so an SP +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, +which is the safe direction: a later replay still collides on whichever of them was +recorded. **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 From 30788cddd52f0a4544d5afbd24d0429c71ba55dc Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 26 Aug 2026 17:44:34 +0545 Subject: [PATCH 17/19] fix: bound the record by what actually ends acceptance Taking the latest close in sight read a confirmation naming no close as contributing nothing, when it means the opposite: that confirmation never gives out, so the confirmations impose no limit at all. A dated sibling beside it then shrank the record below what an absent sibling would have left it, sixty seconds of memory against acceptance that never ends, where the same assertion without the dated sibling got replay_ttl. The checks combine as an AND, the Conditions window and one satisfiable confirmation, so acceptance ends at whichever gives out first and the record follows that: the latest close among the confirmations that could ever confirm here, none if a satisfiable one names no close, then the earlier of that and the Conditions close, then replay_ttl when nothing bounds acceptance. Only confirmations naming this SP's endpoint and request have a say, the same ones confirmation_ok weighs, since one addressed elsewhere can never keep the assertion alive here and must not unbound the record. Taking the earlier of the two ends reverses the previous commit's later, deliberately: with unbounded confirmations now meaning no limit, later would hand replay_ttl back where Conditions itself names an hour. Surveyed the field before settling this (Shibboleth/OpenSAML, pac4j, Sustainsys, ITfoxtec, SimpleSAMLphp, python3-saml, ruby-saml, node-saml, Spring, Keycloak): the three that derive a record lifetime from the assertion read one attribute and require it to exist, Shibboleth uses a fixed freshness window off IssueInstant instead, and the rest keep no record at all. Accepting a dateless confirmation while keeping a record is territory none of them enter, which is why the rule is spelled out rather than borrowed. TESTs 45 to 47 pin the three edges: the dateless sibling, Conditions closing first, and a confirmation addressed elsewhere having no say. --- lua/resty/saml.lua | 76 +++++++++++++++++++++++-------------- t/assertion-conditions.t | 81 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 29 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 1cb7a75..8db5b6f 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -509,38 +509,55 @@ local function issuers_allowed(allowed, issuers) return true end --- The last moment any bound this SP weighed would still admit the assertion. --- Conditions/@NotOnOrAfter is one of them; profile 4.1.4.2 puts a bearer --- assertion's expiry on its confirmation instead, so a Conditions carrying --- nothing but an audience is the profile-minimal shape rather than an odd one. --- The latest of them decides: remembering too long costs a slot, remembering --- too little reopens the window the record is there to close. Nil when nothing --- names one at all, which is what replay_ttl stands in for. -local function last_moment_usable(assertion) - local bounds = {} - if assertion.not_on_or_after then - bounds[#bounds + 1] = assertion.not_on_or_after - end +-- The last moment the checks above would still admit the assertion. They +-- combine as an AND: the Conditions window has to hold, and one confirmation +-- has to be satisfiable, so acceptance ends at whichever gives out first, the +-- Conditions close or the last confirmation still standing. Profile 4.1.4.2 +-- puts a bearer assertion's expiry on its confirmation, so a Conditions +-- carrying nothing but an audience is the profile-minimal shape rather than an +-- odd one. Nil when nothing bounds acceptance, which replay_ttl stands in for. +-- +-- Only confirmations that could ever confirm at this SP have a say, the same +-- ones confirmation_ok weighs, minus the clock: one naming another Recipient +-- or another request can never keep the assertion alive here, and one whose +-- close this parser will not take, a legal xs:dateTime carrying an offset +-- rather than Z, is unsatisfiable in the same way. Reading those as +-- contributing nothing rather than as unbounded matters in both directions, +-- since a confirmation naming no close never gives out: one satisfiable such +-- confirmation means the confirmations impose no limit at all, where the +-- earlier reading let a shorter sibling shrink the record below what an +-- absent sibling would have left it. +local function last_moment_usable(assertion, expected) + local notes_close + local unbounded = #assertion.subject_confirmations == 0 for _, confirmation in ipairs(assertion.subject_confirmations) do - if confirmation.not_on_or_after then - bounds[#bounds + 1] = confirmation.not_on_or_after + local confirms_here = confirmation.recipient == expected.acs_url and + (confirmation.in_response_to == nil or + confirmation.in_response_to == expected.request_id) + if confirms_here then + if confirmation.not_on_or_after == nil then + unbounded = true + else + local at = parse_iso8601_utc_time(confirmation.not_on_or_after) + if at and (notes_close == nil or at > notes_close) then + notes_close = at + end + end end end + if unbounded then + notes_close = nil + end - local latest - for _, bound in ipairs(bounds) do - -- A bound this parser will not take, a legal xs:dateTime carrying an - -- offset rather than Z, makes its own confirmation unsatisfiable and so - -- can never extend how long the assertion is usable. Refusing on it - -- would let replay_dict decide which logins are accepted, and one - -- satisfiable confirmation among several is enough for the checks above. - local at = parse_iso8601_utc_time(bound) - if at and (latest == nil or at > latest) then - latest = at - end + local conditions_close + if assertion.not_on_or_after then + conditions_close = parse_iso8601_utc_time(assertion.not_on_or_after) end - return latest + if conditions_close and notes_close then + return math.min(conditions_close, notes_close) + end + return conditions_close or notes_close end @@ -562,7 +579,7 @@ 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, now) +local function spend_assertions(dict, opts, assertions, expected, now) local skew = opts.clock_skew or DEFAULT_CLOCK_SKEW local spent = {} @@ -572,7 +589,7 @@ local function spend_assertions(dict, opts, assertions, now) end local ttl = opts.replay_ttl or DEFAULT_REPLAY_TTL - local usable_until = last_moment_usable(assertion) + local usable_until = last_moment_usable(assertion, expected) if usable_until then ttl = usable_until + skew - now end @@ -731,7 +748,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, now) + local unused, used_reason = spend_assertions(self.replay_dict, opts, assertions, + expected, now) if not unused then ngx.log(ngx.ERR, "response from IdP rejected: ", loggable(used_reason)) ngx.exit(ngx.HTTP_UNAUTHORIZED) diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index c1ac3a5..2bb6dde 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -1305,3 +1305,84 @@ built --- response_body 302 / tracked: true + + +=== TEST 45: a confirmation naming no close keeps the fallback in charge +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- the dated sibling gives out in a minute; the dateless one never + -- does, so it decides, and the record falls to replay_ttl rather + -- than to the shortest date in sight + ngx.say(login_with("replay", function(request_id) + return saml_response({ + id = "never-gives-out", + confirmations = confirmation({ + recipient = ACS, not_on_or_after = at(60), + in_response_to = request_id, + }) .. confirmation({ + recipient = ACS, in_response_to = request_id, + }), + }, ACS, request_id) + end)) + local ttl = ngx.shared.saml_replay:ttl(replay_key("never-gives-out")) + ngx.say("fallback: ", ttl > 590 and ttl <= 600) + } + } +--- response_body +302 / +fallback: true + + +=== TEST 46: acceptance ends at whichever close comes first +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- the Conditions window and the confirmations combine as an AND, + -- so the Conditions closing first is when acceptance ends + ngx.say(login_with("replay", function(request_id) + return saml_response({ + id = "conditions-first", + conditions = conditions({ not_on_or_after = at(300) }), + confirmations = confirmation({ + recipient = ACS, not_on_or_after = at(3600), + in_response_to = request_id, + }), + }, ACS, request_id) + end)) + local ttl = ngx.shared.saml_replay:ttl(replay_key("conditions-first")) + ngx.say("earlier: ", ttl > 300 and ttl <= 360) + } + } +--- response_body +302 / +earlier: true + + +=== TEST 47: a confirmation that cannot confirm here has no say in the record +--- config + location /t { + content_by_lua_block { + ngx.shared.saml_replay:flush_all() + -- the dateless one is addressed elsewhere, so it can never keep + -- this assertion alive here and does not unbound the record + ngx.say(login_with("replay", function(request_id) + return saml_response({ + id = "elsewhere-dateless", + confirmations = confirmation({ + recipient = ACS, not_on_or_after = at(3600), + in_response_to = request_id, + }) .. confirmation({ + recipient = "https://other-sp.example.com/acs", + }), + }, ACS, request_id) + end)) + local ttl = ngx.shared.saml_replay:ttl(replay_key("elsewhere-dateless")) + ngx.say("dated one decides: ", ttl > 3600 and ttl <= 3660) + } + } +--- response_body +302 / +dated one decides: true From 04c236a3ded071ab698c66d7bf318d58f23f8ed7 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 27 Aug 2026 09:47:17 +0545 Subject: [PATCH 18/19] docs: state where the record is bounded and acceptance is not Two residues of the same shape, both behind IdPs far outside shipped defaults: an assertion naming no expiry is refusable only inside replay_ttl, and one made valid beyond a day is accepted again past the cap. The cap is the trade against a record nothing reclaims, so the README carries it rather than a knob re-enabling the pinned slot. The replay_ttl row also said the fallback applies when the assertion names no NotOnOrAfter anywhere, which drifted when the rule became satisfiability-aware: a dated close on a confirmation that cannot confirm here leaves acceptance unbounded, and the fallback applies then too. It reads "when nothing bounds its acceptance" now. --- README.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 438bd76..938c025 100644 --- a/README.md +++ b/README.md @@ -84,7 +84,7 @@ local saml = resty_saml.new(opts) | `sp_audiences` | array of strings | `{ sp_issuer }` | Audiences this SP answers to. An assertion carrying an `AudienceRestriction` has to name one of them; an assertion carrying none is unrestricted. | | `clock_skew` | number | `60` | Seconds of clock difference tolerated against the IdP when weighing `NotBefore` and `NotOnOrAfter`. | | `replay_dict` | string | None | Name of an `lua_shared_dict` in which to remember the assertions this instance has already accepted, so it accepts none of them twice. Unset leaves them untracked. See [Remembering assertions](#remembering-assertions) for what the zone has to hold and how far the guarantee reaches. | -| `replay_ttl` | number | `600` | Seconds to remember an assertion that names no `NotOnOrAfter` anywhere, on its `Conditions` or on any subject confirmation. One that names it is remembered until that moment plus `clock_skew`, capped at a day. | +| `replay_ttl` | number | `600` | Seconds to remember an assertion when nothing bounds its acceptance: no `NotOnOrAfter` on its `Conditions` and none on a satisfiable subject confirmation. A bounded one is remembered until acceptance ends, plus `clock_skew`, capped at a day. | #### Binding a response to the request @@ -132,6 +132,14 @@ 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 that names no +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 +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 From 1b92b9d2e237faaffe0bd83ed6576e370fbd321c Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 27 Aug 2026 10:25:41 +0545 Subject: [PATCH 19/19] docs: keep the opening sentence inside the bounds the section states It promised every accepted assertion is remembered until it could no longer be used, which the residue paragraph below retracts for the two unbounded shapes. It defers to those bounds now. And an assertion can name an expiry, on a confirmation that cannot confirm here, and still be unbounded, so the residue paragraph says "no usable expiry", matching the replay_ttl row. --- README.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 938c025..86d8484 100644 --- a/README.md +++ b/README.md @@ -110,9 +110,10 @@ lasts as long as an `AuthnRequest` is outstanding across the upgrade. #### Remembering assertions -Set `replay_dict` and every assertion this instance accepts is remembered until it -could no longer be used, so presenting the same one again is refused. Leave it unset -and assertions go untracked, which is what happened before the option existed. +Set `replay_dict` and every assertion this instance accepts is remembered for as +long as it could still be used, within the bounds below, and presenting one that is +remembered is refused. Leave it unset and assertions go untracked, which is what +happened before the option existed. **The guarantee is per instance.** An `lua_shared_dict` is shared between the workers of one gateway and nowhere else, so a captured assertion replayed through a load @@ -132,7 +133,7 @@ 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 that names no +**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