From b640cdb92941c1ca7dadb202249275e9a5e4bd23 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:52:25 +0545 Subject: [PATCH 01/11] 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 8144136a9c1b65ee94f68a58cdb10fd3ae8ab341 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 14:01:02 +0545 Subject: [PATCH 02/11] 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 03/11] 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: Wed, 19 Aug 2026 15:44:00 +0545 Subject: [PATCH 04/11] fix: a confirmation that states nothing confirms nothing A SubjectConfirmation carrying no SubjectConfirmationData names no endpoint, no request and no window. Every field read from it was nil, and nil reads the same here as a condition that holds, so the confirmation came out satisfied. Since one satisfiable confirmation is enough, a single empty element beside a confirmation binding the assertion elsewhere answered in its place and disarmed the Recipient check entirely. An assertion the IdP addressed to another endpoint was then accepted here. has_data carries the distinction from the reader, and a confirmation without it satisfies nothing. An assertion offering no confirmation at all is untouched, since there is nothing there to weigh. --- lua/resty/saml.lua | 7 +++++++ src/lua_saml.c | 1 + src/saml.h | 1 + src/xml.c | 3 +++ t/assertion-conditions.t | 25 +++++++++++++++++++++++++ 5 files changed, 37 insertions(+) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index fbe5814..6ec9988 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -333,6 +333,13 @@ end -- 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) + -- a confirmation carrying no SubjectConfirmationData states no condition, + -- and one that states nothing confirms nothing. Counting it as satisfied + -- would let it answer for a sibling that does bind the assertion, which + -- disarms every check below with one empty element. + if not confirmation.has_data then + return false + end if confirmation.recipient and confirmation.recipient ~= acs_url then return false end diff --git a/src/lua_saml.c b/src/lua_saml.c index 7412122..e54332f 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -599,6 +599,7 @@ static void push_subject_confirmations(lua_State* L, saml_assertion_t* a) { lua_pushinteger(L, i + 1); lua_newtable(L); set_str_field(L, "method", confirmation->method); + set_bool_field(L, "has_data", confirmation->has_data); 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); diff --git a/src/saml.h b/src/saml.h index ac77c57..9834728 100644 --- a/src/saml.h +++ b/src/saml.h @@ -49,6 +49,7 @@ typedef struct { typedef struct { xmlChar* method; + int has_data; xmlChar* recipient; xmlChar* not_before; xmlChar* not_on_or_after; diff --git a/src/xml.c b/src/xml.c index d854529..d8329da 100644 --- a/src/xml.c +++ b/src/xml.c @@ -360,6 +360,9 @@ static int read_subject_confirmations(xmlNode* subject, saml_assertion_t* a) { if (data == NULL) { continue; } + // reported, because a confirmation carrying no data states no condition, + // which reads the same as one whose every condition holds + confirmation->has_data = 1; 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"); diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index b9445ad..f943292 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -595,3 +595,28 @@ offers no subject confirmation this SP can satisfy } --- response_body 302 / + + + +=== TEST 20: a confirmation that states nothing confirms nothing +--- config + location /t { + content_by_lua_block { + -- the only confirmation carries no SubjectConfirmationData, so the + -- assertion names neither an endpoint nor a window + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ data = false }), + }))) + -- and an empty one cannot answer for a sibling that binds the + -- assertion somewhere else + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = "https://evil.example.com/acs" }) .. + confirmation({ data = false }), + }))) + } + } +--- response_body +401 nil +401 nil +--- error_log +offers no subject confirmation this SP can satisfy From 83c589bba53dc07bc7a84ce304b324ce0970233d Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 15:56:16 +0545 Subject: [PATCH 05/11] fix: fail the read when the unrecognised condition's name is lost xmlStrdup was the one allocation in the new reader left unchecked. A NULL from it leaves unknown_condition unset, the key is omitted from the table, and the caller's Indeterminate gate never fires, so an assertion carrying a condition this SP cannot satisfy is accepted rather than refused. Failing the read instead puts it with every other allocation here: the caller gets nil for the assertions and refuses the response. --- src/xml.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/xml.c b/src/xml.c index d8329da..ecb73ea 100644 --- a/src/xml.c +++ b/src/xml.c @@ -383,7 +383,12 @@ static int read_assertion(xmlDoc* doc, xmlNode* node, saml_assertion_t* a) { for (xmlNode* child = conditions->children; child != NULL; child = child->next) { if (child->type == XML_ELEMENT_NODE && !is_known_condition(child)) { + // the caller refuses the assertion on this name, so losing it would + // let the condition through rather than fail the read a->unknown_condition = xmlStrdup(child->name); + if (a->unknown_condition == NULL) { + return -1; + } break; } } From d0009e545523ead7fde3af66640fb238b03d4981 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 16:21:30 +0545 Subject: [PATCH 06/11] fix: log the session expiry as the UTC instant it now is The INFO line rendered the parsed expiry with os.date and no ! prefix, so a value this branch just redefined as a true UTC epoch came out in the machine's local time, which is the reading TEST 17 exists to rule out. It also ran ahead of the guard on the parse error beside it, and os.date falls back to the current time when handed nil, so a failed parse logged an expiry of right now before the error branch fired. TEST 21 covers the session lifetime that expiry decides, which nothing covered before: a session the IdP leaves ten minutes to run is still good on a worker fourteen hours ahead of UTC. --- lua/resty/saml.lua | 3 +- t/assertion-conditions.t | 69 ++++++++++++++++++++++++++++++++++++++-- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 6ec9988..a900066 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -468,11 +468,12 @@ local function login_callback(self, opts) local expires if session_expires then expires, err = parse_iso8601_utc_time(session_expires) - ngx.log(ngx.INFO, "login callback: session_expires=", os.date("%Y-%m-%d %T %z", expires)) if err then ngx.say(err) ngx.exit(500) end + ngx.log(ngx.INFO, "login callback: session_expires=", + os.date("!%Y-%m-%d %TZ", expires)) end diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index f943292..4267eda 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -165,15 +165,28 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== spec.method or BEARER, data) end - -- Conditions follows Subject, the order the schema prescribes + function authn_statement(session_expires) + if session_expires == nil then + return "" + end + return string.format('' .. + '' .. + 'urn:oasis:names:tc:SAML:2.0:ac:classes:Password' .. + '', + attr("SessionNotOnOrAfter", session_expires)) + end + + -- Subject, then Conditions, then the statements, the order the schema + -- prescribes function assertion(spec) spec = spec or {} return string.format('' .. '%s' .. - '%s%s%s', + '%s%s%s%s', spec.id or "a1", IDP, spec.name_id or "signed\@example.com", - spec.confirmations or "", spec.conditions or "") + spec.confirmations or "", spec.conditions or "", + authn_statement(spec.session_expires)) end function response(body, destination) @@ -222,6 +235,36 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== return res.status .. " " .. tostring(res.headers["Location"]) 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) + local httpc = require("resty.http").new() + local base = "http://127.0.0.1:1984" + local headers = { ["X-Test-SP"] = name } + + local res = assert(httpc:request_uri(base .. "/", { headers = headers })) + local cookie = res.headers["Set-Cookie"] + if type(cookie) == "table" then cookie = cookie[1] end + local state = res.headers["Location"]:match("RelayState=([^&]+)") + + res = assert(httpc:request_uri(base .. "/acs", { + method = "POST", + body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. + "&RelayState=" .. state, + headers = callback_headers(name, cookie), + })) + if res.status ~= 302 then return "callback: " .. res.status end + + local rotated = res.headers["Set-Cookie"] + if type(rotated) == "table" then rotated = rotated[1] end + if rotated then cookie = rotated end + + res = assert(httpc:request_uri(base .. "/", { + headers = { ["X-Test-SP"] = name, ["Cookie"] = cookie:match("^[^;]+") }, + })) + return res.status + end + function parse(xml) local key = assert(saml.key_read_memory(CERT_PEM, saml.KeyDataFormatCertPem)) local mngr = assert(saml.create_keys_manager({ key })) @@ -620,3 +663,23 @@ offers no subject confirmation this SP can satisfy 401 nil --- error_log offers no subject confirmation this SP can satisfy + + + +=== TEST 21: session lifetime follows the IdP's clock, not the machine's +--- config + location /t { + content_by_lua_block { + -- ten minutes of session left, with the worker fourteen hours + -- ahead of UTC: read as local time it would already be spent + ngx.say(session_after_login("plain", saml_response({ session_expires = at(600) }))) + -- and ten minutes past, which is spent either way + ngx.say(session_after_login("plain", saml_response({ session_expires = at(-600) }))) + } + } +--- main_config +env SAML_DATA_DIR=./; +env TZ=XXX-14; +--- response_body +200 +302 From 1b0fa845a160664d7306ae59a56e74265665e288 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 16:40:45 +0545 Subject: [PATCH 07/11] fix: keep a smuggled newline from splitting a log line Destination rides the Response wrapper, which no signature covers, so its value is whatever the sender typed. XML folds a literal newline inside an attribute to a space, and a character reference survives that folding, so reaches the parsed value as a real newline and validates against the bundled schema. One ngx.log call then wrote two lines, the second being text of the sender's choosing sitting in the error log as its own entry. Anyone able to reach the callback with a session of their own could plant them. Control characters are escaped now on the way into the log, for the reason string as well, whose audiences are the same anyURI shape. --- lua/resty/saml.lua | 16 ++++++++++++++-- t/assertion-conditions.t | 17 +++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index a900066..cc125e2 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -282,6 +282,18 @@ local function parse_iso8601_utc_time(str) end +-- Values lifted out of the IdP's document end up in the error log, which is +-- read a line at a time. XML folds a literal newline inside an attribute to a +-- space, but a character reference survives that, and the Response wrapper is +-- not covered by the signature, so its Destination is whatever the sender +-- typed. Escape rather than trust any of it to stay on one line. +local function loggable(value) + return (tostring(value):gsub("%c", function(c) + return string.format("\\x%02X", c:byte()) + end)) +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, @@ -436,7 +448,7 @@ local function login_callback(self, opts) 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.log(ngx.ERR, "response from IdP is addressed to ", loggable(destination)) ngx.exit(ngx.HTTP_UNAUTHORIZED) end @@ -448,7 +460,7 @@ local function login_callback(self, opts) 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.log(ngx.ERR, "response from IdP rejected: ", loggable(reason)) ngx.exit(ngx.HTTP_UNAUTHORIZED) end diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 4267eda..1a33ee0 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -683,3 +683,20 @@ env TZ=XXX-14; --- response_body 200 302 + + + +=== TEST 22: a newline smuggled into the response cannot split a log line +--- config + location /t { + content_by_lua_block { + -- Destination rides the unsigned wrapper, so it is the sender's to + -- write, and a character reference is not folded to a space the way + -- a literal newline would be + ngx.say(login_with("plain", saml_response({}, "https://x WARNING-forged-entry"))) + } + } +--- response_body +401 nil +--- error_log +addressed to https://x\x0AWARNING-forged-entry From c666d5036729a99ec9702b18848dea621fcae898 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 20 Aug 2026 11:34:08 +0545 Subject: [PATCH 08/11] fix: require a confirmation to name this SP, not merely avoid contradicting it has_data told the caller whether the IdP wrote a SubjectConfirmationData, which is a different question from whether the confirmation binds the assertion here. Every attribute on that element is optional, so an empty one is schema-valid and set the flag while stating exactly as much as the absent element it replaced: nothing. An empty element, and one carrying only a condition that happens to hold, were both satisfiable, and one satisfiable confirmation is enough, so either could still answer in place of a sibling binding the assertion elsewhere. Recipient is the only thing a confirmation says about where the assertion may be presented, so a confirmation satisfies this SP when it names it. That closes the family and leaves nothing for has_data to report. An assertion offering no confirmation at all is untouched. Three more values on this path reach the log unescaped: RelayState, which is a URL-decoded form field with no schema to squeeze through, the status code off the unsigned wrapper, and the name id, whose element text carries a literal newline without needing a character reference. --- README.md | 2 +- lua/resty/saml.lua | 20 +++++++++----------- src/lua_saml.c | 1 - src/saml.h | 1 - src/xml.c | 3 --- t/assertion-conditions.t | 40 ++++++++++++++++++++++++++++++++-------- 6 files changed, 42 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index c887cfd..d579789 100644 --- a/README.md +++ b/README.md @@ -79,7 +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_acs_url` | string | built from the request | Absolute URL of this SP's assertion consumer service. It is announced to the IdP, every `SubjectConfirmationData/@Recipient` has to name it, and a `Destination` has to name it on a response carrying one. Unset, it is assembled from the request's scheme and host, which is only as trustworthy as whatever sits in front: set it wherever the ingress does not normalise `Forwarded` and `X-Forwarded-*`, or terminates TLS without setting `X-Forwarded-Proto`. | | `sp_audiences` | array of strings | `{ sp_issuer }` | Audiences this SP answers to. An assertion carrying an `AudienceRestriction` has to name one of them; an assertion carrying none is unrestricted. | | `clock_skew` | number | `60` | Seconds of clock difference tolerated against the IdP when weighing `NotBefore` and `NotOnOrAfter`. | diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index cc125e2..6a047b1 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -345,14 +345,12 @@ end -- 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) - -- a confirmation carrying no SubjectConfirmationData states no condition, - -- and one that states nothing confirms nothing. Counting it as satisfied - -- would let it answer for a sibling that does bind the assertion, which - -- disarms every check below with one empty element. - if not confirmation.has_data then - return false - end - if confirmation.recipient and confirmation.recipient ~= acs_url then + -- Recipient is the only thing a confirmation says about where the assertion + -- may be presented, so it has to be there. An absent one, an empty + -- SubjectConfirmationData, and one carrying nothing but conditions that + -- happen to hold all say the same nothing, and any of them would otherwise + -- answer in place of a sibling that binds the assertion somewhere else. + if confirmation.recipient ~= acs_url then return false end return (time_bounds_ok(confirmation.not_before, confirmation.not_on_or_after, now, skew)) @@ -434,13 +432,13 @@ local function login_callback(self, opts) local status_code = saml.doc_status_code(doc) if status_code ~= saml.STATUS_SUCCESS then - ngx.log(ngx.ERR, "IdP returned non-success status: ", status_code) + ngx.log(ngx.ERR, "IdP returned non-success status: ", loggable(status_code)) ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) end local state = args.RelayState if state ~= saml_state then - ngx.log(ngx.ERR, "state different: args.state=", state, ", state=", saml_state) + ngx.log(ngx.ERR, "state different: args.state=", loggable(state), ", state=", saml_state) ngx.exit(ngx.HTTP_UNAUTHORIZED) end @@ -501,7 +499,7 @@ local function login_callback(self, opts) sess:set("request_uri", nil) sess:save() - ngx.log(ngx.INFO, "login finish: name_id=", name_id) + ngx.log(ngx.INFO, "login finish: name_id=", loggable(name_id)) return ngx.redirect(request_uri) end diff --git a/src/lua_saml.c b/src/lua_saml.c index e54332f..7412122 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -599,7 +599,6 @@ static void push_subject_confirmations(lua_State* L, saml_assertion_t* a) { lua_pushinteger(L, i + 1); lua_newtable(L); set_str_field(L, "method", confirmation->method); - set_bool_field(L, "has_data", confirmation->has_data); 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); diff --git a/src/saml.h b/src/saml.h index 9834728..ac77c57 100644 --- a/src/saml.h +++ b/src/saml.h @@ -49,7 +49,6 @@ typedef struct { typedef struct { xmlChar* method; - int has_data; xmlChar* recipient; xmlChar* not_before; xmlChar* not_on_or_after; diff --git a/src/xml.c b/src/xml.c index ecb73ea..14ea81e 100644 --- a/src/xml.c +++ b/src/xml.c @@ -360,9 +360,6 @@ static int read_subject_confirmations(xmlNode* subject, saml_assertion_t* a) { if (data == NULL) { continue; } - // reported, because a confirmation carrying no data states no condition, - // which reads the same as one whose every condition holds - confirmation->has_data = 1; 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"); diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 1a33ee0..358704c 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -614,14 +614,23 @@ env TZ=XXX-14; 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)) + + -- the same value settles Destination, which is read on a response + -- carrying no confirmation to weigh + local addressed = saml_response({}, "https://sp.example.com/acs") + ngx.say(login_with("plain", addressed, forged)) + ngx.say(login_with("acs", addressed, forged)) } } --- response_body 302 / 401 nil 302 / ---- error_log -offers no subject confirmation this SP can satisfy +302 / +401 nil +--- error_log eval +[qr/offers no subject confirmation this SP can satisfy/, +qr{addressed to https://sp\.example\.com/acs}] @@ -645,22 +654,37 @@ offers no subject confirmation this SP can satisfy --- config location /t { content_by_lua_block { - -- the only confirmation carries no SubjectConfirmationData, so the - -- assertion names neither an endpoint nor a window + local elsewhere = confirmation({ recipient = "https://evil.example.com/acs" }) + + -- no SubjectConfirmationData at all ngx.say(login_with("plain", saml_response({ confirmations = confirmation({ data = false }), }))) - -- and an empty one cannot answer for a sibling that binds the - -- assertion somewhere else + -- the element written out with nothing in it, which the schema + -- allows since every attribute on it is optional + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({}), + }))) + -- and neither spelling may answer in place of a sibling that binds + -- the assertion somewhere else + ngx.say(login_with("plain", saml_response({ + confirmations = elsewhere .. confirmation({ data = false }), + }))) + ngx.say(login_with("plain", saml_response({ + confirmations = elsewhere .. confirmation({}), + }))) + -- nor may one carrying only a condition that happens to hold ngx.say(login_with("plain", saml_response({ - confirmations = confirmation({ recipient = "https://evil.example.com/acs" }) .. - confirmation({ data = false }), + confirmations = elsewhere .. confirmation({ not_before = at(-600) }), }))) } } --- response_body 401 nil 401 nil +401 nil +401 nil +401 nil --- error_log offers no subject confirmation this SP can satisfy From 69c6d46e53405d9f7f33d34fe3321ced1fbe14f2 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 20 Aug 2026 12:31:34 +0545 Subject: [PATCH 09/11] fix: anchor the timestamp pattern so a year is read whole xs:dateTime allows a leading minus for BCE and the schema accepts it, so NotOnOrAfter="-9999-01-01T00:00:00Z" validates. Unanchored, the match started after the minus and read 9999 CE, turning a bound expired eight thousand years ago into one good for another eight thousand. Measured on c666d50 it logs in. A five-digit year matched from its second character for the same reason, where only the year floor below caught it. Anchored at both ends now, with the fractional second spelled out rather than swallowed by .*, so a shape this parser cannot hold is refused instead of read from part way in. Every timestamp it reads sits inside the signed assertion, so this needs the IdP to send it. --- lua/resty/saml.lua | 7 ++++++- t/assertion-conditions.t | 29 +++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 6a047b1..bd98e1d 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -250,7 +250,12 @@ 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') + -- Anchored at both ends, so a year the four-digit field cannot hold is + -- refused rather than read from part way in: xs:dateTime allows a leading + -- minus for BCE, and an unanchored match starts after it and turns 9999 BCE + -- into 9999 CE. A fractional second is read and truncated towards the past. + 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)%.?%d*Z$') if year_s == nil then return nil, 'invalid UTC time pattern unmatch' end diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 358704c..47996f7 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -724,3 +724,32 @@ env TZ=XXX-14; 401 nil --- error_log addressed to https://x\x0AWARNING-forged-entry + + + +=== TEST 23: a year the parser cannot hold is refused, not read from part way in +--- config + location /t { + content_by_lua_block { + -- 9999 BCE, which the schema accepts: unanchored, the match starts + -- after the minus and reads a far-future 9999 CE out of an + -- already-expired bound + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_on_or_after = "-9999-01-01T00:00:00Z" }), + }))) + -- five digits, where the match can start one character in + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_on_or_after = "20260-01-01T00:00:00Z" }), + }))) + -- a fractional second is still read + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_on_or_after = (at(600):gsub("Z", ".500Z")) }), + }))) + } + } +--- response_body +401 nil +401 nil +302 / +--- error_log +carries an unreadable NotOnOrAfter -9999-01-01T00:00:00Z From e63345bcfbebccd196631e12bc2be8250d3649ba Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 20 Aug 2026 14:49:50 +0545 Subject: [PATCH 10/11] fix: close the classes behind three review rounds of single sites An unreadable SessionNotOnOrAfter answered 200 with the parse error as the body. ngx.say commits the response, so the ngx.exit(500) beside it could no longer set a status, and nginx logged the attempt. It goes to the log now, with the offending value, and the status the branch always meant to send. The branch predates this PR and was already reachable through a numeric offset; anchoring the pattern added one more shape to it. Every value read out of a SAML message now goes through loggable on its way to a log, logout_callback's issuer, name id, session index and status code included, rather than the login path alone. The rule is where the value came from, not what its schema says it may hold today. read_attr separates an absent attribute from one whose value could not be copied, which xmlGetNoNsProp returns NULL for alike. Reading NULL as absent drops a bound or an endpoint rather than failing, so every attribute the reader takes goes through it, and doc_destination reports the difference to Lua instead of collapsing both into nil. --- lua/resty/saml.lua | 24 ++++++++++++++----- src/lua_saml.c | 14 +++++++---- src/saml.h | 1 + src/xml.c | 51 +++++++++++++++++++++++++++++++++------- t/assertion-conditions.t | 34 +++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 19 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index bd98e1d..0f2a26f 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -292,6 +292,10 @@ end -- space, but a character reference survives that, and the Response wrapper is -- not covered by the signature, so its Destination is whatever the sender -- typed. Escape rather than trust any of it to stay on one line. +-- Every value read out of a SAML message goes through here on its way to a log, +-- whether or not a signature covers it and whichever message it came from. The +-- rule is the value's origin, not its type: an attribute the schema constrains +-- today is one schema revision away from carrying anything. local function loggable(value) return (tostring(value):gsub("%c", function(c) return string.format("\\x%02X", c:byte()) @@ -449,7 +453,12 @@ local function login_callback(self, opts) local acs_url = sp_acs_url(opts) - local destination = saml.doc_destination(doc) + local destination, destination_err = saml.doc_destination(doc) + if destination_err then + ngx.log(ngx.ERR, "could not read the destination of the response from IdP: ", + destination_err) + ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) + end if destination and destination ~= acs_url then ngx.log(ngx.ERR, "response from IdP is addressed to ", loggable(destination)) ngx.exit(ngx.HTTP_UNAUTHORIZED) @@ -484,7 +493,10 @@ local function login_callback(self, opts) if session_expires then expires, err = parse_iso8601_utc_time(session_expires) if err then - ngx.say(err) + -- ngx.say would commit the response, leaving ngx.exit unable to set + -- a status and the caller a 200 carrying this string + ngx.log(ngx.ERR, "unreadable SessionNotOnOrAfter ", loggable(session_expires), + " in response from IdP: ", err) ngx.exit(500) end ngx.log(ngx.INFO, "login callback: session_expires=", @@ -576,20 +588,20 @@ local function logout_callback(self, opts) local saved_issuer = sess:get("issuer") if issuer ~= saved_issuer then - ngx.log(ngx.WARN, "issuer different: issuer=", issuer, + ngx.log(ngx.WARN, "issuer different: issuer=", loggable(issuer), ", data.issuer=", saved_issuer) end local saved_name_id = sess:get("name_id") if name_id ~= saved_name_id then - ngx.log(ngx.WARN, "name_id different: name_id=", name_id, + ngx.log(ngx.WARN, "name_id different: name_id=", loggable(name_id), ", data.name_id=", saved_name_id) end local saved_session_index = sess:get("session_index") if session_index ~= saved_session_index then ngx.log(ngx.WARN, "session_index different: session_index=", - session_index, ", data.session_index=", saved_session_index) + loggable(session_index), ", data.session_index=", saved_session_index) end sess:destroy() @@ -609,7 +621,7 @@ local function logout_callback(self, opts) else local status_code = saml.doc_status_code(doc) if status_code ~= saml.STATUS_SUCCESS then - ngx.log(ngx.ERR, "IdP returned non-success status: ", status_code) + ngx.log(ngx.ERR, "IdP returned non-success status: ", loggable(status_code)) ngx.exit(ngx.HTTP_INTERNAL_SERVER_ERROR) end diff --git a/src/lua_saml.c b/src/lua_saml.c index 7412122..817b9f2 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -524,26 +524,30 @@ Get the Destination attribute of the root message @function doc_destination @tparam xmlDoc* doc @treturn ?string destination +@treturn ?string error */ 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) { + // nil for an absent Destination and nil for one that could not be read would + // be the same answer to the caller, and it skips the check on the first + xmlChar* destination; + if (saml_doc_destination(doc, &destination) < 0) { lua_pushnil(L); - return 1; + lua_pushstring(L, "could not read Destination"); + return 2; } - xmlChar* destination = xmlGetNoNsProp(root, (const xmlChar*)"Destination"); if (destination == NULL) { lua_pushnil(L); } else { lua_pushstring(L, (char*)destination); xmlFree(destination); } - return 1; + lua_pushnil(L); + return 2; } diff --git a/src/saml.h b/src/saml.h index ac77c57..cc75506 100644 --- a/src/saml.h +++ b/src/saml.h @@ -110,6 +110,7 @@ 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); +int saml_doc_destination(xmlDoc* doc, xmlChar** destination); 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); diff --git a/src/xml.c b/src/xml.c index 14ea81e..37c971e 100644 --- a/src/xml.c +++ b/src/xml.c @@ -247,6 +247,19 @@ void saml_attrs_free(saml_attr_t* attrs, size_t attrs_len) { static int is_saml_assertion(xmlNode* node); +// An absent attribute and one whose value could not be copied both come back +// NULL from xmlGetNoNsProp, and every caller here reads NULL as "the IdP said +// nothing". For a bound or an endpoint that is a constraint quietly dropped, so +// tell the two apart and let the read fail rather than the check. +static int read_attr(xmlNode* node, const char* name, xmlChar** out) { + *out = xmlGetNoNsProp(node, (const xmlChar*)name); + if (*out == NULL && xmlHasNsProp(node, (const xmlChar*)name, NULL) != NULL) { + return -1; + } + return 0; +} + + // 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 && @@ -354,29 +367,37 @@ static int read_subject_confirmations(xmlNode* subject, saml_assertion_t* a) { } saml_subject_confirmation_t* confirmation = a->confirmations + i++; - confirmation->method = xmlGetNoNsProp(node, (const xmlChar*)"Method"); + if (read_attr(node, "Method", &confirmation->method) < 0) { + return -1; + } 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"); + if (read_attr(data, "Recipient", &confirmation->recipient) < 0 || + read_attr(data, "NotBefore", &confirmation->not_before) < 0 || + read_attr(data, "NotOnOrAfter", &confirmation->not_on_or_after) < 0 || + read_attr(data, "InResponseTo", &confirmation->in_response_to) < 0) { + return -1; + } } return 0; } static int read_assertion(xmlDoc* doc, xmlNode* node, saml_assertion_t* a) { - a->id = xmlGetNoNsProp(node, (const xmlChar*)"ID"); + if (read_attr(node, "ID", &a->id) < 0) { + return -1; + } 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"); + if (read_attr(conditions, "NotBefore", &a->not_before) < 0 || + read_attr(conditions, "NotOnOrAfter", &a->not_on_or_after) < 0) { + return -1; + } for (xmlNode* child = conditions->children; child != NULL; child = child->next) { if (child->type == XML_ELEMENT_NODE && !is_known_condition(child)) { @@ -450,6 +471,20 @@ int saml_doc_assertions(xmlDoc* doc, saml_assertion_t** assertions, size_t* asse } +// The Destination of the root message, through an out parameter so that a value +// which could not be read is distinguishable from one that is absent. The caller +// skips the check on absent, which is the wrong answer for the other. +int saml_doc_destination(xmlDoc* doc, xmlChar** destination) { + *destination = NULL; + + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { + return 0; + } + return read_attr(root, "Destination", destination); +} + + 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; diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index 47996f7..dee3b20 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -753,3 +753,37 @@ addressed to https://x\x0AWARNING-forged-entry 302 / --- error_log carries an unreadable NotOnOrAfter -9999-01-01T00:00:00Z + + + +=== TEST 24: an unreadable session expiry is an error, not a 200 carrying one +--- config + location /t { + content_by_lua_block { + local httpc = require("resty.http").new() + local base = "http://127.0.0.1:1984" + + local res = assert(httpc:request_uri(base .. "/", { headers = { ["X-Test-SP"] = "plain" } })) + local cookie = res.headers["Set-Cookie"] + if type(cookie) == "table" then cookie = cookie[1] end + local state = res.headers["Location"]:match("RelayState=([^&]+)") + + -- schema-valid xs:dateTime this parser will not read: a numeric + -- offset where SAML requires Z + local xml = saml_response({ session_expires = "2030-01-01T00:00:00+00:00" }) + res = assert(httpc:request_uri(base .. "/acs", { + method = "POST", + body = "SAMLResponse=" .. ngx.escape_uri(saml.base64_encode(xml)) .. + "&RelayState=" .. state, + headers = callback_headers("plain", cookie), + })) + -- the status the branch always meant to send, and the parse + -- error kept out of the body it used to be written into + ngx.say(res.status, " leaks=", + tostring(((res.body or ""):find("UTC time", 1, true)) ~= nil)) + } + } +--- response_body +500 leaks=false +--- error_log +unreadable SessionNotOnOrAfter 2030-01-01T00:00:00+00:00 From 8cacd7251fde017165f6710622d5b71c91b4a353 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 20 Aug 2026 16:52:56 +0545 Subject: [PATCH 11/11] fix: refuse a DTD, and a window that opens after it closes Canonicalisation drops the document type declaration before anything is hashed, so no signature covers one and none is disturbed by one being added. Its ATTLIST defaults are answered to every reader that asks a node for an attribute, and never written onto the node, so a prologue nobody signed supplies attributes the signed content never carried. Measured: an IdP-signed assertion whose confirmation names no endpoint is refused, and the same bytes behind ]> log in. Signing the whole Response rather than the assertion changes nothing, since the prologue is outside what canonicalisation keeps either way. SAML has no use for a DTD, so both parse paths refuse a message carrying one. Separately, each end of a validity window was weighed against now on its own and never against the other end, so the skew allowance let a window inverted by up to twice it pass while satisfying no instant at all. The ends are compared to each other first, where the allowance has no say. --- lua/resty/saml.lua | 33 +++++++++++++++------- src/binding.c | 20 +++++++++++++ src/saml.h | 1 + t/assertion-conditions.t | 61 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 105 insertions(+), 10 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 0f2a26f..9fba347 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -314,24 +314,37 @@ end local DEFAULT_CLOCK_SKEW = 60 local function time_bounds_ok(not_before, not_on_or_after, now, skew) + local opens, closes, err + if not_before then - local at, err = parse_iso8601_utc_time(not_before) - if not at then + opens, err = parse_iso8601_utc_time(not_before) + if not opens 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 + closes, err = parse_iso8601_utc_time(not_on_or_after) + if not closes 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 + + -- A window that opens after it closes is empty on every clock, so the skew + -- allowance has no say in it: without this, each end on its own looks + -- acceptable and an inversion of up to twice the allowance passes. Strictly + -- later rather than not earlier, since a fractional second is truncated away + -- and two instants inside one second read as equal. + if opens and closes and opens > closes then + return false, "opens at " .. not_before .. " and closes at " .. not_on_or_after + end + + if opens and now + skew < opens then + return false, "is not valid before " .. not_before + end + + if closes and now - skew >= closes then + return false, "is not valid on or after " .. not_on_or_after end return true diff --git a/src/binding.c b/src/binding.c index 69f89a5..0c63891 100644 --- a/src/binding.c +++ b/src/binding.c @@ -43,12 +43,24 @@ static char* ERRORS[] = { "document does not validate against schema", "invalid signature algorithm", "signature does not match", + "document carries a document type declaration", }; char* saml_binding_error_msg(saml_binding_status_t status) { return ERRORS[status - SAML_ZLIB_ERROR]; } + +// Canonicalisation drops the document type declaration before anything is +// hashed, so no signature covers one and none is disturbed by one being added. +// Its ATTLIST defaults are still answered to every reader that asks a node for +// an attribute, without ever being written onto the node, so a prologue nobody +// signed supplies attributes the signed content never carried. SAML has no use +// for a DTD, so a message carrying one is refused rather than read. +static int doc_has_dtd(xmlDoc* doc) { + return doc->intSubset != NULL || doc->extSubset != NULL; +} + static void redirect_concat_args(char* saml_type, char* content, char* sig_alg, char* relay_state, str_t* query) { char* content_uri = saml_uri_encode(content); char* sig_alg_uri = saml_uri_encode(sig_alg); @@ -176,6 +188,10 @@ saml_binding_status_t saml_binding_redirect_parse(char* content, char* sig_alg, return SAML_INVALID_XML; } + if (doc_has_dtd(*doc)) { + return SAML_HAS_DTD; + } + if (!saml_doc_validate(*doc)) { return SAML_INVALID_DOC; } @@ -290,6 +306,10 @@ saml_binding_status_t saml_binding_post_parse(char* content, xmlDoc** doc) { return SAML_INVALID_XML; } + if (doc_has_dtd(*doc)) { + return SAML_HAS_DTD; + } + if (!saml_doc_validate(*doc)) { return SAML_INVALID_DOC; } diff --git a/src/saml.h b/src/saml.h index cc75506..1737778 100644 --- a/src/saml.h +++ b/src/saml.h @@ -82,6 +82,7 @@ typedef enum { SAML_INVALID_DOC, SAML_INVALID_SIG_ALG, SAML_INVALID_SIGNATURE, + SAML_HAS_DTD, } saml_binding_status_t; char* saml_binding_error_msg(saml_binding_status_t status); diff --git a/t/assertion-conditions.t b/t/assertion-conditions.t index dee3b20..4d48cae 100644 --- a/t/assertion-conditions.t +++ b/t/assertion-conditions.t @@ -787,3 +787,64 @@ carries an unreadable NotOnOrAfter -9999-01-01T00:00:00Z 500 leaks=false --- error_log unreadable SessionNotOnOrAfter 2030-01-01T00:00:00+00:00 + + + +=== TEST 25: a window that opens after it closes is empty on every clock +--- config + location /t { + content_by_lua_block { + -- inverted by less than twice the skew allowance, so each end taken + -- on its own still looks acceptable + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_before = at(30), not_on_or_after = at(-30) }), + }))) + -- the same shape on a subject confirmation, which shares the check + ngx.say(login_with("plain", saml_response({ + confirmations = confirmation({ recipient = ACS, + not_before = at(30), not_on_or_after = at(-30) }), + }))) + -- both ends inside one second still reads as open, since the + -- fractional part is truncated away before they are compared + ngx.say(login_with("plain", saml_response({ + conditions = conditions({ not_before = at(0), not_on_or_after = at(0) }), + }))) + } + } +--- response_body +401 nil +401 nil +302 / +--- error_log eval +[qr/opens at .* and closes at /, +qr/offers no subject confirmation this SP can satisfy/] + + + +=== TEST 26: a document type declaration is refused, whatever it declares +--- config + location /t { + content_by_lua_block { + -- an ATTLIST default answers every reader that asks the node for an + -- attribute, without ever being written onto the node, and + -- canonicalisation drops the prologue before anything is hashed. So + -- this invents a Recipient inside signed content while leaving every + -- signature intact, whichever scope it covers + local doctype = ']>' + local body = assertion({ confirmations = confirmation({}) }) + + ngx.say(login_with("plain", doctype .. response(sign_doc(body)))) + ngx.say(login_with("plain", doctype .. sign_doc(response(body)))) + -- and the same documents without the prologue, which carry no + -- Recipient of their own + ngx.say(login_with("plain", response(sign_doc(body)))) + } + } +--- response_body +400 nil +400 nil +401 nil +--- error_log eval +[qr/parse post from IdP: document carries a document type declaration/, +qr/offers no subject confirmation this SP can satisfy/]