From b640cdb92941c1ca7dadb202249275e9a5e4bd23 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:52:25 +0545 Subject: [PATCH 1/6] 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 2/6] 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 3/6] 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 4/6] 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 5/6] 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 6/6] 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) }), })))