From b640cdb92941c1ca7dadb202249275e9a5e4bd23 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:52:25 +0545 Subject: [PATCH 1/7] 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/7] 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 8144136a9c1b65ee94f68a58cdb10fd3ae8ab341 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 14:01:02 +0545 Subject: [PATCH 3/7] 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 4/7] 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 13:12:33 +0545 Subject: [PATCH 5/7] fix: require the assertion to name the request, and ride out the upgrade Checking InResponseTo only when it happens to be there left the binding skippable by whoever benefits from skipping it. The copy on the Response is unsigned, so a replay deletes it; the copy inside the assertion is covered by the signature, so a replay never has to, since an IdP that omits it produces the same nothing. A confirmation now has to name the request this SP issued, on the footing Recipient already stands on: profile 4.1.4.2 requires the value of an IdP answering an AuthnRequest, and this SP asks for nothing else. A response arriving with no login in progress is refused before any of this is reached, so the one case the value legitimately goes missing, IdP-initiated SSO, was already out. A session minted before the ID was kept has nothing to compare against. Refusing dead-ends a login that is genuinely the user's, so it starts the login again instead, which cannot repeat: the session it mints carries an ID. login takes the URI to return to, so the restart keeps the one the user was heading for. TEST 30 covers the confirmation that names no request, TEST 31 the restart. TESTs 10, 12 and 18 name the request they answer now, which means building the response after the SP has issued one. Two review points from the same round, both about the harness: - A block naming the error it expects got no other assertion about the log, so a block driving a rejection and then a success said nothing about the second half. The severities that never legitimately appear are asserted now, which is the part of it that can be said generically. - README covers the binding, what an IdP has to send, the upgrade window, and jit-uuid seeding once in the master when this module is loaded from init_by_lua, which now decides request IDs as well as RelayState. --- README.md | 27 ++++++++++ lua/resty/saml.lua | 29 +++++++--- t/assertion-conditions.t | 111 +++++++++++++++++++++++++++++++++++---- 3 files changed, 149 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 297ffb0..8c16f3a 100644 --- a/README.md +++ b/README.md @@ -84,6 +84,33 @@ 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`. | +#### Binding a response to the request + +An ID is minted for every `AuthnRequest` this SP sends and kept on the session as +`saml_request_id`. On the way back, a `SubjectConfirmationData` has to name that ID +in its `InResponseTo`, so an assertion captured from one login cannot be presented +in another. `Response/@InResponseTo` is weighed as well when it is there, though it +sits outside the signature, so it catches a misdirected answer rather than a +deliberate one. + +Two consequences worth knowing before upgrading: + +- An IdP that leaves `InResponseTo` off `SubjectConfirmationData` is refused. Profile + 4.1.4.2 requires the value of an IdP answering an `AuthnRequest`, and answering one + is the only thing this SP ever asks for: a response arriving with no login in + progress is refused whatever it carries. +- A session minted before this SP kept the ID has nothing for 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. + +#### Seeding the worker + +Request IDs and `RelayState` both come from `resty.jit-uuid`, which is seeded when +this module is first loaded, from the clock and the process ID. Loading `resty.saml` +from `init_by_lua` therefore seeds once in the master, and every worker forked after +it inherits that same sequence. Require this module from `init_worker_by_lua`, or +call `uuid.seed()` there yourself. + #### saml:authenticate() **syntax:** *data = saml:authenticate()* diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 96c4459..fe8ac8f 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -189,7 +189,10 @@ local function logout_request(opts, name_id, session_index) }) end -local function login(self, opts) +-- request_uri is where to return once the IdP answers. It defaults to what is +-- being asked for, and is passed in when a login is restarted from somewhere +-- else, where ngx.var.request_uri is that somewhere else. +local function login(self, opts, request_uri) local sess = session.start(self.session_config) local authenticated = sess:get("authenticated") @@ -213,7 +216,7 @@ local function login(self, opts) end local state = uuid.generate_v4() - local request_uri = ngx.var.request_uri + request_uri = request_uri or 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() @@ -379,10 +382,12 @@ local function confirmation_ok(confirmation, expected, now, skew) if confirmation.recipient ~= expected.acs_url then return false end - -- inside the signature, so this is the binding a replayed assertion cannot - -- be rewritten to satisfy. An IdP that leaves it out is left working, with - -- the Response-level check standing in until it arrives. - if confirmation.in_response_to and confirmation.in_response_to ~= expected.request_id then + -- The confirmation has to name the request this SP issued, on the same + -- footing as Recipient and for the same reason: one that names no request + -- binds the assertion to no login, which is the shape a replay arrives in. + -- Profile 4.1.4.2 requires the value of an IdP answering an AuthnRequest, + -- and answering one is the only thing this SP ever asks for. + if 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)) @@ -506,6 +511,16 @@ local function login_callback(self, opts) local request_uri = sess:get("request_uri") + -- A session minted before this SP kept the ID of the request it issued has + -- nothing for the assertion to name, and refusing dead-ends a login that is + -- genuinely this user's. Starting over gets a request that is remembered, + -- and cannot repeat, since the session it mints carries one. + local request_id = sess:get("saml_request_id") + if not request_id then + ngx.log(ngx.WARN, "session carries no request id, starting the login again") + return login(self, opts, request_uri) + end + local method = ngx.req.get_method() local doc, args, err if method == "POST" then @@ -535,7 +550,7 @@ local function login_callback(self, opts) local expected = { acs_url = sp_acs_url(opts), - request_id = sess:get("saml_request_id"), + request_id = request_id, } -- the Response is often left unsigned, so this only catches a stray answer; diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 6211c65..e402b0b 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -14,6 +14,11 @@ add_block_preprocessor(sub { if ((!defined $block->error_log) && (!defined $block->no_error_log)) { $block->set_value("no_error_log", "[error]"); + } elsif (!defined $block->no_error_log) { + # a block naming the error it expects gets no other assertion about the + # log, so a block that also drives a success asserts nothing about that + # half. This is the part of it that can be said generically. + $block->set_value("no_error_log", "[crit]\n[alert]\n[emerg]"); } if (!defined $block->request) { @@ -256,6 +261,26 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== return res.status .. " " .. tostring(res.headers["Location"]) end + -- hand a response to a session the old code would have left behind + function login_with_legacy(name, xml) + local httpc = require("resty.http").new() + local base = "http://127.0.0.1:1984" + + local res = assert(httpc:request_uri(base .. "/legacy", { + headers = { ["X-Test-SP"] = name }, + })) + local cookie = res.headers["Set-Cookie"] + if type(cookie) == "table" then cookie = cookie[1] end + + res = assert(httpc:request_uri(base .. "/acs", { + method = "POST", + body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. + "&RelayState=legacy-state", + headers = callback_headers(name, cookie), + })) + return res.status .. " " .. (res.headers["Location"] or ""):match("^[^?]*") + end + -- log in, then ask the app again carrying the session the callback -- handed out: a live session answers 200, an expired one starts over function session_after_login(name, xml) @@ -297,6 +322,19 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== server { listen 1984; + # a login in progress with no record of the request that started it, + # the shape a session minted before the binding existed has + location /legacy { + content_by_lua_block { + local name = ngx.var.http_x_test_sp or "plain" + local sess = require("resty.session").start(sp(name).session_config) + sess:set("saml_state", "legacy-state") + sess:set("request_uri", "/") + sess:save() + ngx.exit(200) + } + } + location / { access_by_lua_block { sp(ngx.var.http_x_test_sp or "plain"):authenticate() @@ -468,9 +506,13 @@ offers no subject confirmation this SP can satisfy --- config location /t { content_by_lua_block { - ngx.say(login_with("plain", saml_response({ - confirmations = confirmation({ recipient = ACS, not_on_or_after = at(300) }), - }))) + ngx.say(login_with("plain", function(request_id) + return saml_response({ + confirmations = confirmation({ + recipient = ACS, not_on_or_after = at(300), in_response_to = request_id, + }), + }) + end)) } } --- response_body @@ -498,10 +540,14 @@ offers no subject confirmation this SP can satisfy --- 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) }), - }))) + ngx.say(login_with("plain", function(request_id) + return saml_response({ + confirmations = confirmation({ recipient = "http://evil.example.com/acs" }) .. + confirmation({ + recipient = ACS, not_on_or_after = at(300), in_response_to = request_id, + }), + }) + end)) } } --- response_body @@ -620,10 +666,18 @@ env TZ=XXX-14; --- 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 function elsewhere(request_id) + return saml_response({ + confirmations = confirmation({ + recipient = "https://sp.example.com/acs", in_response_to = request_id, + }), + }) + end + local function here(request_id) + return saml_response({ + confirmations = confirmation({ recipient = ACS, in_response_to = request_id }), + }) + end local forged = { ["X-Forwarded-Proto"] = "https", ["X-Forwarded-Host"] = "sp.example.com", @@ -914,3 +968,38 @@ offers no subject confirmation this SP can satisfy } --- response_body 302 / + + +=== TEST 30: a confirmation naming no request confirms nothing +--- config + location /t { + content_by_lua_block { + -- an assertion that names no request is bound to no login, which is + -- the shape a captured one is replayed in + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = ACS }), + }))) + } + } +--- response_body +401 nil +--- error_log +offers no subject confirmation this SP can satisfy + + +=== TEST 31: a session minted before the binding starts the login again +--- config + location /t { + content_by_lua_block { + -- nothing to compare the assertion against, and the login is + -- genuinely this user's, so send them back to the IdP for one + -- that is remembered + ngx.say(login_with_legacy("plain", saml_response({ + confirmations = confirmation({ recipient = ACS, in_response_to = "ID_earlier" }), + }))) + } + } +--- response_body +302 http://127.0.0.1:1984/idp +--- error_log +session carries no request id, starting the login again From ff0edf848884a6bded0ed9932f1d809ccfc7b588 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Fri, 21 Aug 2026 15:31:13 +0545 Subject: [PATCH 6/7] fix: weigh InResponseTo when the IdP names a request, without demanding it Demanding it, as the previous commit did, buys protection against an IdP that is already out of spec and charges a working deployment for it. The only shape it refuses comes from an IdP omitting what profile 4.1.4.2 asks of it: an attacker cannot produce it, since the value sits inside the signature and cannot be stripped from a captured assertion. The one flow that legitimately omits it, IdP-initiated SSO, this SP already refuses for want of a login in progress. The binding is therefore worth what the IdP sends, and the README says so rather than promising a guarantee that holds for most IdPs and reads as holding for all. TESTs 10, 12 and 18 go back to the shape #42 wrote, and TEST 30 records the accepted case rather than a refusal. Restarting the login for a session minted before the ID was kept stays: it turns a dead end into a bounce back to the IdP and breaks nothing. --- README.md | 23 ++++++++++--------- lua/resty/saml.lua | 13 ++++++----- t/assertion-conditions.t | 49 +++++++++++++--------------------------- 3 files changed, 35 insertions(+), 50 deletions(-) diff --git a/README.md b/README.md index 8c16f3a..f1f1a09 100644 --- a/README.md +++ b/README.md @@ -87,21 +87,22 @@ local saml = resty_saml.new(opts) #### Binding a response to the request An ID is minted for every `AuthnRequest` this SP sends and kept on the session as -`saml_request_id`. On the way back, a `SubjectConfirmationData` has to name that ID -in its `InResponseTo`, so an assertion captured from one login cannot be presented -in another. `Response/@InResponseTo` is weighed as well when it is there, though it +`saml_request_id`. On the way back, a `SubjectConfirmationData` naming a different +request refuses the login, so an assertion captured from one login cannot be +presented in another. `Response/@InResponseTo` is weighed the same way, though it sits outside the signature, so it catches a misdirected answer rather than a deliberate one. -Two consequences worth knowing before upgrading: +That guarantee is worth what the IdP sends. Profile 4.1.4.2 asks an IdP answering an +`AuthnRequest` to name it, and every mainstream IdP does, but an IdP that leaves +`InResponseTo` out, or sends no `SubjectConfirmation` at all, keeps working and gets +no binding. Refusing it would trade a working login for protection against another +party's misconfiguration, and no attacker can produce the shape: the value sits +inside the signature, so it cannot be stripped from a captured assertion. -- An IdP that leaves `InResponseTo` off `SubjectConfirmationData` is refused. Profile - 4.1.4.2 requires the value of an IdP answering an `AuthnRequest`, and answering one - is the only thing this SP ever asks for: a response arriving with no login in - progress is refused whatever it carries. -- A session minted before this SP kept the ID has nothing for 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. +One note for upgrading. A session minted before this SP kept the ID has nothing for +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. #### Seeding the worker diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index fe8ac8f..a7f92d8 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -382,12 +382,13 @@ local function confirmation_ok(confirmation, expected, now, skew) if confirmation.recipient ~= expected.acs_url then return false end - -- The confirmation has to name the request this SP issued, on the same - -- footing as Recipient and for the same reason: one that names no request - -- binds the assertion to no login, which is the shape a replay arrives in. - -- Profile 4.1.4.2 requires the value of an IdP answering an AuthnRequest, - -- and answering one is the only thing this SP ever asks for. - if confirmation.in_response_to ~= expected.request_id then + -- Checked when the IdP names a request, and not demanded. Naming one is + -- what profile 4.1.4.2 asks of an IdP answering an AuthnRequest, so an IdP + -- that leaves it out is already out of spec, and refusing that trades a + -- working login for protection against somebody else's misconfiguration. + -- Nothing an attacker sends produces the shape: the value sits inside the + -- signature, so it cannot be stripped from a captured assertion. + 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)) diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index e402b0b..d730b32 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -506,13 +506,9 @@ offers no subject confirmation this SP can satisfy --- config location /t { content_by_lua_block { - ngx.say(login_with("plain", function(request_id) - return saml_response({ - confirmations = confirmation({ - recipient = ACS, not_on_or_after = at(300), in_response_to = request_id, - }), - }) - end)) + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = ACS, not_on_or_after = at(300) }), + }))) } } --- response_body @@ -540,14 +536,10 @@ offers no subject confirmation this SP can satisfy --- config location /t { content_by_lua_block { - ngx.say(login_with("plain", function(request_id) - return saml_response({ - confirmations = confirmation({ recipient = "http://evil.example.com/acs" }) .. - confirmation({ - recipient = ACS, not_on_or_after = at(300), in_response_to = request_id, - }), - }) - end)) + 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 @@ -666,18 +658,10 @@ env TZ=XXX-14; --- config location /t { content_by_lua_block { - local function elsewhere(request_id) - return saml_response({ - confirmations = confirmation({ - recipient = "https://sp.example.com/acs", in_response_to = request_id, - }), - }) - end - local function here(request_id) - return saml_response({ - confirmations = confirmation({ recipient = ACS, in_response_to = request_id }), - }) - end + 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", @@ -970,21 +954,20 @@ offers no subject confirmation this SP can satisfy 302 / -=== TEST 30: a confirmation naming no request confirms nothing +=== TEST 30: a confirmation naming no request keeps working --- config location /t { content_by_lua_block { - -- an assertion that names no request is bound to no login, which is - -- the shape a captured one is replayed in + -- naming one is what the profile asks of an IdP answering a + -- request, so an IdP that leaves it out is already out of spec. + -- The binding is worth what that IdP sends and no more. ngx.say(login_with("plain", saml_response({ confirmations = confirmation({ recipient = ACS }), }))) } } --- response_body -401 nil ---- error_log -offers no subject confirmation this SP can satisfy +302 / === TEST 31: a session minted before the binding starts the login again From 5a3b4eb17f9c65bad89e80d43000f84f621a7516 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Fri, 21 Aug 2026 15:49:43 +0545 Subject: [PATCH 7/7] docs: what the Response InResponseTo is worth depends on what the IdP signed Stated flatly that it sits outside the signature, which holds for the common shape, an assertion-level signature, and is wrong for an IdP signing the whole Response, which this library accepts and which covers the attribute. The advice underneath it is unchanged: the copy to rely on is the one inside the assertion. --- README.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index f1f1a09..d3e5bc0 100644 --- a/README.md +++ b/README.md @@ -89,9 +89,11 @@ local saml = resty_saml.new(opts) An ID is minted for every `AuthnRequest` this SP sends and kept on the session as `saml_request_id`. On the way back, a `SubjectConfirmationData` naming a different request refuses the login, so an assertion captured from one login cannot be -presented in another. `Response/@InResponseTo` is weighed the same way, though it -sits outside the signature, so it catches a misdirected answer rather than a -deliberate one. +presented in another. `Response/@InResponseTo` is weighed the same way, +though what it is worth depends on what the IdP signed: an IdP signing the whole +`Response` covers it, while one signing only the assertion, the common shape, leaves +it outside the signature, where the party replaying an assertion deletes it. Rely on +the copy inside the assertion, and read this one as catching a misdirected answer. That guarantee is worth what the IdP sends. Profile 4.1.4.2 asks an IdP answering an `AuthnRequest` to name it, and every mainstream IdP does, but an IdP that leaves