From 0cbf07919051586f20ed7351781ec21999c70475 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Mon, 17 Aug 2026 23:07:33 +0545 Subject: [PATCH 1/6] fix: read the issuer from the assertion the signature covers saml_doc_issuer returned the first Issuer under the document root, which for a Response is the Response's own. SAML lets the IdP sign the assertion rather than the whole response, and that Issuer then sits outside the signature: an attacker holding one signed assertion can rewrite it and the signature still verifies, so the value stored on the session was never attested. Read it from the assertion instead, the element the identity itself comes from and the one every other accessor already reads. Messages that carry no assertion are signed whole, so they keep reading their own Issuer. A Response left with no assertion after verification now yields no issuer rather than an unverified one. --- src/sig.c | 8 ------ src/xml.c | 47 ++++++++++++++++++++++++++++------ t/signed-response.t | 61 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 100 insertions(+), 16 deletions(-) diff --git a/src/sig.c b/src/sig.c index 1d8e33d..08a6d04 100644 --- a/src/sig.c +++ b/src/sig.c @@ -339,14 +339,6 @@ static int signature_covers(xmlDoc* doc, xmlNode* sig, xmlNode* node) { } -static int is_saml_assertion(xmlNode* node) { - return node->type == XML_ELEMENT_NODE && - xmlStrEqual(node->name, (const xmlChar*)"Assertion") == 1 && - node->ns != NULL && - xmlStrEqual(node->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1; -} - - // Identity is read from /samlp:Response/saml:Assertion, i.e. only from an // assertion that is a direct child of the verified root message. saml_verify_doc // checks one Signature but not that it covers the assertion a reader will pick, diff --git a/src/xml.c b/src/xml.c index bbc1bfb..65162b1 100644 --- a/src/xml.c +++ b/src/xml.c @@ -25,20 +25,51 @@ static xmlXPathObject* eval_xpath(xmlDoc* doc, xmlXPathCompExpr* xpath) { } +static int is_saml_assertion(xmlNode* node) { + return node->type == XML_ELEMENT_NODE && + xmlStrEqual(node->name, (const xmlChar*)"Assertion") == 1 && + node->ns != NULL && + xmlStrEqual(node->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1; +} + + +// The text of node's own Issuer child, or NULL. Issuer is in the assertion +// namespace wherever it appears, so a look-alike in another one is not it. +static xmlChar* issuer_of(xmlDoc* doc, xmlNode* node) { + for (xmlNode* child = node->children; child != NULL; child = child->next) { + if (child->type == XML_ELEMENT_NODE && + xmlStrEqual(child->name, (const xmlChar*)"Issuer") == 1 && + child->ns != NULL && + xmlStrEqual(child->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1) { + return xmlNodeListGetString(doc, child->children, 1); + } + } + return NULL; +} + + +// A Response's issuer is read from its assertion, the element the identity +// itself comes from. The Response's own Issuer sits outside an assertion-level +// signature and can be rewritten without breaking it, while every top-level +// assertion still in the document is one the signature covers. Other messages +// carry no assertion and are signed whole, so their own Issuer is the one to +// read. xmlChar* saml_doc_issuer(xmlDoc* doc) { - xmlNode* node = xmlDocGetRootElement(doc); - if (node == NULL) { + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { return NULL; } - node = node->children; - while (node != NULL) { - if (xmlStrEqual(node->name, (xmlChar*)"Issuer") == 1) { - return xmlNodeListGetString(doc, node->children, 1); + if (xmlStrEqual(root->name, (const xmlChar*)"Response") == 1) { + for (xmlNode* child = root->children; child != NULL; child = child->next) { + if (is_saml_assertion(child)) { + return issuer_of(doc, child); + } } - node = node->next; + return NULL; } - return NULL; + + return issuer_of(doc, root); } diff --git a/t/signed-response.t b/t/signed-response.t index c4df4c9..a2fd92d 100644 --- a/t/signed-response.t +++ b/t/signed-response.t @@ -496,3 +496,64 @@ name_id: first@example.com, dept: eng, role: ops } --- response_body name_id: signed@example.com + + + +=== TEST 18: the issuer comes from the assertion, not the unsigned Response +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + -- the signature covers the assertion only, so the Response around + -- it, its own Issuer included, is the attacker's to write + local signed = sign(key, transform, assertion("a1", "signed@example.com")) + local outer = '' .. + 'https://attacker.example.com' .. + '' .. + signed .. '' + local doc, err = submit(mngr, outer) + if err then ngx.say("err: ", err) else ngx.say("issuer: ", tostring(saml.doc_issuer(doc))) end + } + } +--- response_body +issuer: https://idp.example.com + + + +=== TEST 19: a whole-response signature reads the same issuer +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + local resp = response(SUCCESS, "resp-1", assertion("a1", "signed@example.com")) + local doc, err = submit(mngr, sign(key, transform, resp)) + if err then ngx.say("err: ", err) else ngx.say("issuer: ", tostring(saml.doc_issuer(doc))) end + } + } +--- response_body +issuer: https://idp.example.com + + + +=== TEST 20: a message carrying no assertion reads its own issuer +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + local logout = 'https://idp.example.com' .. + '' .. + '' + local doc, err = submit(mngr, sign(key, transform, logout)) + if err then + ngx.say("err: ", err) + else + ngx.say(saml.doc_root_name(doc), " issuer: ", tostring(saml.doc_issuer(doc))) + end + } + } +--- response_body +LogoutResponse issuer: https://idp.example.com From 92c511cedb31303b3281f8f0195547130180f928 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Mon, 17 Aug 2026 23:07:33 +0545 Subject: [PATCH 2/6] feat: pin the issuers accepted on a login response A valid signature says the response came from the configured idp_cert. It does not say which IdP that key speaks for, which matters when one key signs for several issuers, or when the certificate is a shared or intermediate issued one. idp_issuers names the issuers a deployment expects and the login callback rejects anything else; leaving it unset keeps current behaviour. --- README.md | 1 + lua/resty/saml.lua | 21 ++++ t/login-callback.t | 244 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 266 insertions(+) create mode 100644 t/login-callback.t diff --git a/README.md b/README.md index 04a7ef6..3866358 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,7 @@ local saml = resty_saml.new(opts) | `sp_issuer` | string | None | SP name to access IdP. | | `idp_uri` | string | None | URI of IdP. | | `idp_cert` | string | None | IdP Certificate, used to verify saml response. | +| `idp_issuers` | array of strings | None | Issuers accepted on a login response. Unset accepts any issuer the `idp_cert` signs for. | | `login_callback_uri` | string | None | redirect uri used to callback the SP from IdP after login. | | `logout_uri` | string | None | logout uri to trigger logout. | | `logout_callback_uri` | string | None | redirect uri used to callback the SP from IdP after logout. | diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 8ab7985..8673e39 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -258,6 +258,22 @@ local function parse_iso8601_utc_time(str) return os.time{year=year, month=month, day=day, hour=hour, min=min, sec=sec} end +-- A valid signature says the message came from the configured key. It does not +-- say which IdP that key speaks for, so pin the issuer when the caller names +-- the ones it expects. No list keeps the previous behaviour; a list nothing +-- matches, an empty one included, admits nobody. +local function issuer_allowed(allowed, issuer) + if allowed == nil then + return true + end + for _, expected in ipairs(allowed) do + if expected == issuer then + return true + end + end + return false +end + local function login_callback(self, opts) local sess = session.start(self.session_config) @@ -301,6 +317,11 @@ local function login_callback(self, opts) local name_id = saml.doc_name_id(doc) local session_index = saml.doc_session_index(doc) + if not issuer_allowed(opts.idp_issuers, issuer) then + ngx.log(ngx.ERR, "unexpected issuer in response from IdP: ", tostring(issuer)) + ngx.exit(ngx.HTTP_UNAUTHORIZED) + end + -- a success response the signature leaves without a readable assertion -- carries no identity, so there is nobody to authenticate as if not name_id then diff --git a/t/login-callback.t b/t/login-callback.t new file mode 100644 index 0000000..4c71f9c --- /dev/null +++ b/t/login-callback.t @@ -0,0 +1,244 @@ +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" + + 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 allow-list under test, picked by request header + ALLOW_LISTS = { + none = nil, + exact = { IDP }, + other = { "https://other.example.com" }, + } + SPS = {} + + function sp(name) + if SPS[name] == nil then + SPS[name] = require("resty.saml").new({ + 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!", + idp_issuers = ALLOW_LISTS[name], + }) + end + return SPS[name] + end + + -- an assertion signed by the IdP key, so the wrapping Response below is + -- outside the signature exactly as it is in the wild + function signed_assertion(issuer, name_id) + 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 xml = string.format('' .. + '%s' .. + '%s', + issuer, name_id) + local out = assert(saml.sign_xml(key, transform, xml, + { id_attr = "ID", insert_after = { saml.XMLNS_ASSERTION, "Issuer" } })) + return (out:gsub("<%?xml.-%?>%s*", "")) + end + + function saml_response(response_issuer, assertion_issuer, name_id) + return string.format('%s' .. + '%s', + response_issuer, SUCCESS, signed_assertion(assertion_issuer, name_id)) + 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 + } + + server { + listen 1984; + + location / { + access_by_lua_block { + sp(ngx.var.http_x_test_sp or "none"):authenticate() + } + + content_by_lua_block { + ngx.exit(200) + } + } + } +_EOC_ + + $block->set_value("http_config", $http_config); +}); + +run_tests(); + +__DATA__ + +=== TEST 1: no allow-list accepts the issuer the IdP key signs for +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("none", saml_response(IDP, IDP, "signed@example.com"))) + } + } +--- response_body +302 / + + + +=== TEST 2: an allow-listed issuer is accepted +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("exact", saml_response(IDP, IDP, "signed@example.com"))) + } + } +--- response_body +302 / + + + +=== TEST 3: an issuer outside the allow-list is rejected +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("exact", saml_response("https://elsewhere.example.com", + "https://elsewhere.example.com", "signed@example.com"))) + } + } +--- response_body +401 nil +--- error_log +unexpected issuer in response from IdP: https://elsewhere.example.com + + + +=== TEST 4: an allow-listed Issuer on the unsigned Response does not admit a foreign assertion +--- config + location /t { + content_by_lua_block { + -- the assertion is signed by the same key but issued by another + -- IdP, and the Response around it claims the allow-listed one + ngx.say(login_with("exact", saml_response(IDP, + "https://other.example.com", "attacker@example.com"))) + } + } +--- response_body +401 nil +--- error_log +unexpected issuer in response from IdP: https://other.example.com From 041bb56407320d3f7f07f73b420e427113f174b4 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:21:50 +0545 Subject: [PATCH 3/6] fix: check the issuer of every assertion the readers consume A response signed as a whole may carry several assertions, and the readers do not confine themselves to one: doc_attrs collects from all of them and doc_name_id takes the first carrying a subject. Matching only the issuer doc_issuer returns therefore let an allow-listed first assertion carry a second one from an issuer nobody approved. doc_issuers lists the issuer of every top-level assertion, and the login callback requires all of them to be allow-listed. --- README.md | 2 +- lua/resty/saml.lua | 26 +++++++++++++----- src/lua_saml.c | 30 +++++++++++++++++++++ src/saml.h | 2 ++ src/xml.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ t/login-callback.t | 62 +++++++++++++++++++++++++++++++++++------- t/signed-response.t | 21 +++++++++++++++ 7 files changed, 191 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 3866358..b594f87 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ local saml = resty_saml.new(opts) | `sp_issuer` | string | None | SP name to access IdP. | | `idp_uri` | string | None | URI of IdP. | | `idp_cert` | string | None | IdP Certificate, used to verify saml response. | -| `idp_issuers` | array of strings | None | Issuers accepted on a login response. Unset accepts any issuer the `idp_cert` signs for. | +| `idp_issuers` | array of strings | None | Issuers accepted on a login response; every assertion it carries has to name one. Unset accepts any issuer the `idp_cert` signs for. | | `login_callback_uri` | string | None | redirect uri used to callback the SP from IdP after login. | | `logout_uri` | string | None | logout uri to trigger logout. | | `logout_callback_uri` | string | None | redirect uri used to callback the SP from IdP after logout. | diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 8673e39..4d636c7 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -262,16 +262,27 @@ end -- say which IdP that key speaks for, so pin the issuer when the caller names -- the ones it expects. No list keeps the previous behaviour; a list nothing -- matches, an empty one included, admits nobody. -local function issuer_allowed(allowed, issuer) +-- +-- Every assertion is weighed, not just the one the issuer is taken from: a +-- response may legitimately carry several, and attributes are read from all of +-- them. Returns the offending issuer alongside a refusal. +local function issuers_allowed(allowed, issuers) if allowed == nil then return true end - for _, expected in ipairs(allowed) do - if expected == issuer then - return true + for _, issuer in ipairs(issuers or {}) do + local ok = false + for _, expected in ipairs(allowed) do + if expected == issuer then + ok = true + break + end + end + if not ok then + return false, issuer end end - return false + return true end local function login_callback(self, opts) @@ -317,8 +328,9 @@ local function login_callback(self, opts) local name_id = saml.doc_name_id(doc) local session_index = saml.doc_session_index(doc) - if not issuer_allowed(opts.idp_issuers, issuer) then - ngx.log(ngx.ERR, "unexpected issuer in response from IdP: ", tostring(issuer)) + local allowed, unexpected = issuers_allowed(opts.idp_issuers, saml.doc_issuers(doc)) + if not allowed then + ngx.log(ngx.ERR, "unexpected issuer in response from IdP: ", tostring(unexpected)) ngx.exit(ngx.HTTP_UNAUTHORIZED) end diff --git a/src/lua_saml.c b/src/lua_saml.c index 80baa9a..0d7adde 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -407,6 +407,35 @@ static int doc_issuer(lua_State* L) { } +/*** +Get the issuer of every assertion whose content the document's readers consume +@function doc_issuers +@tparam xmlDoc* doc +@treturn table issuers +*/ +static int doc_issuers(lua_State* L) { + lua_settop(L, 1); + xmlDoc* doc = doc_check(L, 1); + lua_pop(L, 1); + + xmlChar** issuers; + size_t issuers_len; + if (saml_doc_issuers(doc, &issuers, &issuers_len) < 0) { + lua_pushnil(L); + return 1; + } + + lua_newtable(L); + for (size_t i = 0; i < issuers_len; i++) { + lua_pushinteger(L, i + 1); + lua_pushstring(L, (char*)issuers[i]); + lua_settable(L, -3); + } + saml_issuers_free(issuers, issuers_len); + return 1; +} + + /*** Get the value of the StatusCode[Value] attribute in the document @function doc_status_code @@ -1160,6 +1189,7 @@ static const struct luaL_Reg saml_funcs[] = { {"doc_root_name", doc_root_name}, {"doc_id", doc_id}, {"doc_issuer", doc_issuer}, + {"doc_issuers", doc_issuers}, {"doc_name_id", doc_name_id}, {"doc_status_code", doc_status_code}, {"doc_session_index", doc_session_index}, diff --git a/src/saml.h b/src/saml.h index 7df4bfd..d582023 100644 --- a/src/saml.h +++ b/src/saml.h @@ -78,6 +78,8 @@ void saml_shutdown(); int saml_doc_validate(xmlDoc* doc); xmlChar* saml_doc_issuer(xmlDoc* doc); +int saml_doc_issuers(xmlDoc* doc, xmlChar*** issuers, size_t* issuers_len); +void saml_issuers_free(xmlChar** issuers, size_t issuers_len); xmlChar* saml_doc_name_id(xmlDoc* doc); xmlChar* saml_doc_status_code(xmlDoc* doc); xmlChar* saml_doc_session_index(xmlDoc* doc); diff --git a/src/xml.c b/src/xml.c index 65162b1..bc48190 100644 --- a/src/xml.c +++ b/src/xml.c @@ -73,6 +73,72 @@ xmlChar* saml_doc_issuer(xmlDoc* doc) { } +// Every issuer the message attributes content to: one per top-level assertion +// of a Response, or its own for a message that carries none. A caller matching +// the issuer against a policy has to weigh all of them, because doc_attrs reads +// every top-level assertion and doc_name_id the first one carrying a subject. +// An assertion with no Issuer is invalid SAML; it is listed as an empty string, +// which no configured issuer matches. +int saml_doc_issuers(xmlDoc* doc, xmlChar*** issuers, size_t* issuers_len) { + *issuers = NULL; + *issuers_len = 0; + + xmlNode* root = xmlDocGetRootElement(doc); + if (root == NULL) { + return 0; + } + + if (xmlStrEqual(root->name, (const xmlChar*)"Response") != 1) { + xmlChar* issuer = issuer_of(doc, root); + if (issuer == NULL) { + return 0; + } + *issuers = malloc(sizeof(xmlChar*)); + if (*issuers == NULL) { + xmlFree(issuer); + return -1; + } + (*issuers)[0] = issuer; + *issuers_len = 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; + } + + *issuers = malloc(count * sizeof(xmlChar*)); + if (*issuers == NULL) { + return -1; + } + + size_t i = 0; + for (xmlNode* child = root->children; child != NULL && i < count; child = child->next) { + if (!is_saml_assertion(child)) { + continue; + } + xmlChar* issuer = issuer_of(doc, child); + (*issuers)[i++] = issuer == NULL ? xmlStrdup((const xmlChar*)"") : issuer; + } + *issuers_len = i; + return 0; +} + + +void saml_issuers_free(xmlChar** issuers, size_t issuers_len) { + for (size_t i = 0; i < issuers_len; i++) { + xmlFree(issuers[i]); + } + free(issuers); +} + + xmlChar* saml_doc_name_id(xmlDoc* doc) { xmlNode* node = xmlDocGetRootElement(doc); if (node == NULL) { diff --git a/t/login-callback.t b/t/login-callback.t index 4c71f9c..a17f144 100644 --- a/t/login-callback.t +++ b/t/login-callback.t @@ -92,6 +92,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== none = nil, exact = { IDP }, other = { "https://other.example.com" }, + both = { IDP, "https://other.example.com" }, } SPS = {} @@ -114,29 +115,36 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== return SPS[name] end - -- an assertion signed by the IdP key, so the wrapping Response below is - -- outside the signature exactly as it is in the wild - function signed_assertion(issuer, name_id) + 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 xml = string.format('' .. - '%s' .. - '%s', - issuer, name_id) local out = assert(saml.sign_xml(key, transform, xml, { id_attr = "ID", insert_after = { saml.XMLNS_ASSERTION, "Issuer" } })) return (out:gsub("<%?xml.-%?>%s*", "")) end - function saml_response(response_issuer, assertion_issuer, name_id) + function assertion(issuer, id, name_id) + return string.format('' .. + '%s' .. + '%s', + id, issuer, name_id) + end + + function response(issuer, body) return string.format('%s' .. '%s', - response_issuer, SUCCESS, signed_assertion(assertion_issuer, name_id)) + issuer, SUCCESS, body) + end + + -- only the assertion is signed, so the Response around it, its own + -- Issuer included, is whatever the sender wants + function saml_response(response_issuer, assertion_issuer, name_id) + return response(response_issuer, sign_doc(assertion(assertion_issuer, "a1", name_id))) end -- start a login, then hand the crafted response back to the callback @@ -242,3 +250,37 @@ unexpected issuer in response from IdP: https://elsewhere.example.com 401 nil --- error_log unexpected issuer in response from IdP: https://other.example.com + + + +=== TEST 5: a second assertion the allow-list does not name is rejected +--- config + location /t { + content_by_lua_block { + -- the whole response is signed, so both assertions are covered and + -- both are read from, but only the first names an expected issuer + local xml = sign_doc(response(IDP, + assertion(IDP, "a1", "signed@example.com") .. + assertion("https://other.example.com", "a2", "other@example.com"))) + ngx.say(login_with("exact", xml)) + } + } +--- response_body +401 nil +--- error_log +unexpected issuer in response from IdP: https://other.example.com + + + +=== TEST 6: two assertions are accepted when the allow-list names both +--- config + location /t { + content_by_lua_block { + local xml = sign_doc(response(IDP, + assertion(IDP, "a1", "signed@example.com") .. + assertion("https://other.example.com", "a2", "other@example.com"))) + ngx.say(login_with("both", xml)) + } + } +--- response_body +302 / diff --git a/t/signed-response.t b/t/signed-response.t index a2fd92d..793f328 100644 --- a/t/signed-response.t +++ b/t/signed-response.t @@ -557,3 +557,24 @@ issuer: https://idp.example.com } --- response_body LogoutResponse issuer: https://idp.example.com + + + +=== TEST 21: every assertion the readers consume reports its issuer +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + local a1 = assertion("a1", "first@example.com") + local a2 = (assertion("a2", "second@example.com") + :gsub("https://idp.example.com", "https://other.example.com")) + local doc, err = submit(mngr, sign(key, transform, response(SUCCESS, "resp-1", a1 .. a2))) + if err then + ngx.say("err: ", err) + else + ngx.say(table.concat(saml.doc_issuers(doc), ", ")) + end + } + } +--- response_body +https://idp.example.com, https://other.example.com From 7bbea1e28f4704d6952bf3c6fb46a08af4c6f091 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Tue, 18 Aug 2026 16:28:46 +0545 Subject: [PATCH 4/6] fix: fail closed when the issuers cannot be read A short or missing issuer list read as fewer assertions to vouch for than the document holds, and the callback let it through. saml_doc_issuers now reports an allocation failure instead of returning a partial list, and a configured allow-list refuses a response whose issuers come back empty or unreadable. --- lua/resty/saml.lua | 8 ++++++-- src/lua_saml.c | 2 +- src/xml.c | 28 +++++++++++++++++++--------- t/login-callback.t | 14 ++++++++++++++ 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 4d636c7..9d019dc 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -265,12 +265,16 @@ end -- -- Every assertion is weighed, not just the one the issuer is taken from: a -- response may legitimately carry several, and attributes are read from all of --- them. Returns the offending issuer alongside a refusal. +-- them. A response whose issuers cannot be read vouches for nobody. Returns +-- what to name in the log alongside a refusal. local function issuers_allowed(allowed, issuers) if allowed == nil then return true end - for _, issuer in ipairs(issuers or {}) do + if type(issuers) ~= "table" or #issuers == 0 then + return false, "none readable" + end + for _, issuer in ipairs(issuers) do local ok = false for _, expected in ipairs(allowed) do if expected == issuer then diff --git a/src/lua_saml.c b/src/lua_saml.c index 0d7adde..fb314be 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -428,7 +428,7 @@ static int doc_issuers(lua_State* L) { lua_newtable(L); for (size_t i = 0; i < issuers_len; i++) { lua_pushinteger(L, i + 1); - lua_pushstring(L, (char*)issuers[i]); + lua_pushstring(L, issuers[i] == NULL ? "" : (char*)issuers[i]); lua_settable(L, -3); } saml_issuers_free(issuers, issuers_len); diff --git a/src/xml.c b/src/xml.c index bc48190..47f71f0 100644 --- a/src/xml.c +++ b/src/xml.c @@ -73,6 +73,14 @@ xmlChar* saml_doc_issuer(xmlDoc* doc) { } +void saml_issuers_free(xmlChar** issuers, size_t issuers_len) { + for (size_t i = 0; i < issuers_len; i++) { + xmlFree(issuers[i]); + } + free(issuers); +} + + // Every issuer the message attributes content to: one per top-level assertion // of a Response, or its own for a message that carries none. A caller matching // the issuer against a policy has to weigh all of them, because doc_attrs reads @@ -124,21 +132,23 @@ int saml_doc_issuers(xmlDoc* doc, xmlChar*** issuers, size_t* issuers_len) { continue; } xmlChar* issuer = issuer_of(doc, child); - (*issuers)[i++] = issuer == NULL ? xmlStrdup((const xmlChar*)"") : issuer; + if (issuer == NULL) { + issuer = xmlStrdup((const xmlChar*)""); + } + if (issuer == NULL) { + // a short list would read as fewer assertions to vouch for than the + // document holds, so report the failure rather than an incomplete answer + saml_issuers_free(*issuers, i); + *issuers = NULL; + return -1; + } + (*issuers)[i++] = issuer; } *issuers_len = i; return 0; } -void saml_issuers_free(xmlChar** issuers, size_t issuers_len) { - for (size_t i = 0; i < issuers_len; i++) { - xmlFree(issuers[i]); - } - free(issuers); -} - - xmlChar* saml_doc_name_id(xmlDoc* doc) { xmlNode* node = xmlDocGetRootElement(doc); if (node == NULL) { diff --git a/t/login-callback.t b/t/login-callback.t index a17f144..d2a590f 100644 --- a/t/login-callback.t +++ b/t/login-callback.t @@ -284,3 +284,17 @@ unexpected issuer in response from IdP: https://other.example.com } --- response_body 302 / + + + +=== TEST 7: a response with no readable issuer is rejected +--- config + location /t { + content_by_lua_block { + ngx.say(login_with("exact", sign_doc(response(IDP, "")))) + } + } +--- response_body +401 nil +--- error_log +unexpected issuer in response from IdP: none readable From 39392632276c3d3f16da2c31736ea817844d635d Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Wed, 19 Aug 2026 19:48:53 +0545 Subject: [PATCH 5/6] fix: refuse a message the verified signature does not cover samlp:Extensions takes elements of any other namespace, so a LogoutRequest carrying an IdP-signed assertion there satisfies saml_verify_doc while the message around it stays the sender's to write. Nothing confined the readers in that case: the assertion is not a direct child, so the sweep a Response gets never reached it, and doc_name_id searched the whole document. Verification now requires the signature to cover the root of any message that carries no assertion to confine, and the logout readers take NameID and SessionIndex from the message itself rather than from wherever they appear first. This is what the issuer branch added here already assumed. Closes #36. An ArtifactResponse whose only signature sits on a nested assertion is refused outright now rather than read as empty (TEST 14). --- src/binding.c | 5 ++++- src/saml.h | 1 + src/sig.c | 26 +++++++++++++++------ src/xml.c | 53 +++++++++++++++++++++---------------------- t/signed-response.t | 55 ++++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 103 insertions(+), 37 deletions(-) diff --git a/src/binding.c b/src/binding.c index 69f89a5..0f77e6e 100644 --- a/src/binding.c +++ b/src/binding.c @@ -43,6 +43,7 @@ static char* ERRORS[] = { "document does not validate against schema", "invalid signature algorithm", "signature does not match", + "signature does not cover the message", }; char* saml_binding_error_msg(saml_binding_status_t status) { @@ -303,7 +304,9 @@ saml_binding_status_t saml_binding_post_verify(xmlSecKeysMngr* mngr, xmlDoc* doc if (res < 0) { return SAML_XMLSEC_ERROR; } else if (res == 0) { - confine_identity_to_signature(doc); + if (!bind_identity_to_signature(doc)) { + return SAML_UNSIGNED_IDENTITY; + } return SAML_OK; } else { return SAML_INVALID_SIGNATURE; diff --git a/src/saml.h b/src/saml.h index d582023..d70aa3a 100644 --- a/src/saml.h +++ b/src/saml.h @@ -57,6 +57,7 @@ typedef enum { SAML_INVALID_DOC, SAML_INVALID_SIG_ALG, SAML_INVALID_SIGNATURE, + SAML_UNSIGNED_IDENTITY, } saml_binding_status_t; char* saml_binding_error_msg(saml_binding_status_t status); diff --git a/src/sig.c b/src/sig.c index 08a6d04..8800d76 100644 --- a/src/sig.c +++ b/src/sig.c @@ -339,20 +339,31 @@ static int signature_covers(xmlDoc* doc, xmlNode* sig, xmlNode* node) { } +// Leave the document with nothing a reader can read that the verified signature +// does not cover, and say whether that was possible at all. +// // Identity is read from /samlp:Response/saml:Assertion, i.e. only from an -// assertion that is a direct child of the verified root message. saml_verify_doc -// checks one Signature but not that it covers the assertion a reader will pick, -// so remove every top-level assertion that signature leaves out. A signature -// over the whole message covers all of them. The removed nodes are siblings, so +// assertion that is a direct child of the root message. saml_verify_doc checks +// one Signature but not that it covers the assertion a reader will pick, so +// remove every top-level assertion that signature leaves out. A signature over +// the whole message covers all of them. The removed nodes are siblings, so // freeing one never dangles another. -static void confine_identity_to_signature(xmlDoc* doc) { +// +// A message that carries no assertion has nothing to confine this way, and +// samlp:Extensions takes elements of any other namespace, so a signed assertion +// parked there satisfies saml_verify_doc while the message around it stays the +// sender's to write. Such a message is only trustworthy signed whole. +static int bind_identity_to_signature(xmlDoc* doc) { xmlNode* root = xmlDocGetRootElement(doc); if (root == NULL) { - return; + return 0; } xmlNode* sig = xmlSecFindNode(root, xmlSecNodeSignature, xmlSecDSigNs); if (sig != NULL && signature_covers(doc, sig, root)) { - return; + return 1; + } + if (xmlStrEqual(root->name, (const xmlChar*)"Response") != 1) { + return 0; } xmlNode* child = root->children; while (child != NULL) { @@ -363,4 +374,5 @@ static void confine_identity_to_signature(xmlDoc* doc) { } child = next; } + return 1; } diff --git a/src/xml.c b/src/xml.c index 47f71f0..1390e8f 100644 --- a/src/xml.c +++ b/src/xml.c @@ -33,27 +33,36 @@ static int is_saml_assertion(xmlNode* node) { } -// The text of node's own Issuer child, or NULL. Issuer is in the assertion -// namespace wherever it appears, so a look-alike in another one is not it. -static xmlChar* issuer_of(xmlDoc* doc, xmlNode* node) { +// The direct child of node named name in namespace ns, or NULL. Only direct +// children: an element the message itself declares is not the same as one a +// document-wide search happens to reach first. +static xmlNode* ns_child(xmlNode* node, const xmlChar* name, const char* ns) { for (xmlNode* child = node->children; child != NULL; child = child->next) { if (child->type == XML_ELEMENT_NODE && - xmlStrEqual(child->name, (const xmlChar*)"Issuer") == 1 && + xmlStrEqual(child->name, name) == 1 && child->ns != NULL && - xmlStrEqual(child->ns->href, (const xmlChar*)SAML_XMLNS_ASSERTION) == 1) { - return xmlNodeListGetString(doc, child->children, 1); + xmlStrEqual(child->ns->href, (const xmlChar*)ns) == 1) { + return child; } } return NULL; } +// The text of node's own Issuer child, or NULL. Issuer is in the assertion +// namespace wherever it appears, so a look-alike in another one is not it. +static xmlChar* issuer_of(xmlDoc* doc, xmlNode* node) { + xmlNode* issuer = ns_child(node, (const xmlChar*)"Issuer", SAML_XMLNS_ASSERTION); + return issuer == NULL ? NULL : xmlNodeListGetString(doc, issuer->children, 1); +} + + // A Response's issuer is read from its assertion, the element the identity // itself comes from. The Response's own Issuer sits outside an assertion-level // signature and can be rewritten without breaking it, while every top-level -// assertion still in the document is one the signature covers. Other messages -// carry no assertion and are signed whole, so their own Issuer is the one to -// read. +// assertion still in the document is one the signature covers. A message that +// carries no assertion is only accepted signed whole, so its own Issuer is the +// one to read. xmlChar* saml_doc_issuer(xmlDoc* doc) { xmlNode* root = xmlDocGetRootElement(doc); if (root == NULL) { @@ -82,7 +91,8 @@ void saml_issuers_free(xmlChar** issuers, size_t issuers_len) { // Every issuer the message attributes content to: one per top-level assertion -// of a Response, or its own for a message that carries none. A caller matching +// of a Response, or its own for a message that carries none and is therefore +// only accepted signed whole. A caller matching // the issuer against a policy has to weigh all of them, because doc_attrs reads // every top-level assertion and doc_name_id the first one carrying a subject. // An assertion with no Issuer is invalid SAML; it is listed as an empty string, @@ -156,7 +166,8 @@ xmlChar* saml_doc_name_id(xmlDoc* doc) { } if (xmlStrEqual(node->name, (xmlChar*)"LogoutRequest") == 1) { - node = xmlSecFindNode(node, (xmlChar*)"NameID", (xmlChar*)SAML_XMLNS_ASSERTION); + // the subject the request names, which the schema puts directly under it + node = ns_child(node, (const xmlChar*)"NameID", SAML_XMLNS_ASSERTION); if (node == NULL) { return NULL; } @@ -183,20 +194,6 @@ xmlChar* saml_doc_name_id(xmlDoc* doc) { } -// The direct child of node named name in the protocol namespace, or NULL. -static xmlNode* protocol_child(xmlNode* node, const xmlChar* name) { - for (xmlNode* child = node->children; child != NULL; child = child->next) { - if (child->type == XML_ELEMENT_NODE && - xmlStrEqual(child->name, name) == 1 && - child->ns != NULL && - xmlStrEqual(child->ns->href, (const xmlChar*)SAML_XMLNS_PROTOCOL) == 1) { - return child; - } - } - return NULL; -} - - xmlChar* saml_doc_status_code(xmlDoc* doc) { // Read the top-level message's status directly, not a document-wide match: // a nested Response (for example inside saml:Advice) can precede the root @@ -205,11 +202,11 @@ xmlChar* saml_doc_status_code(xmlDoc* doc) { if (root == NULL) { return NULL; } - xmlNode* status = protocol_child(root, (const xmlChar*)"Status"); + xmlNode* status = ns_child(root, (const xmlChar*)"Status", SAML_XMLNS_PROTOCOL); if (status == NULL) { return NULL; } - xmlNode* code = protocol_child(status, (const xmlChar*)"StatusCode"); + xmlNode* code = ns_child(status, (const xmlChar*)"StatusCode", SAML_XMLNS_PROTOCOL); if (code == NULL) { return NULL; } @@ -251,7 +248,7 @@ xmlChar* saml_doc_session_index(xmlDoc* doc) { } if (xmlStrEqual(node->name, (xmlChar*)"LogoutRequest") == 1) { - node = xmlSecFindNode(node, (xmlChar*)"SessionIndex", (xmlChar*)SAML_XMLNS_PROTOCOL); + node = ns_child(node, (const xmlChar*)"SessionIndex", SAML_XMLNS_PROTOCOL); if (node == NULL) { return NULL; } diff --git a/t/signed-response.t b/t/signed-response.t index 793f328..ee50524 100644 --- a/t/signed-response.t +++ b/t/signed-response.t @@ -436,7 +436,7 @@ LogoutResponse: urn:oasis:names:tc:SAML:2.0:status:Success } } --- response_body -root: ArtifactResponse, name_id: nil, role: nil +err: signature does not cover the message @@ -578,3 +578,56 @@ LogoutResponse issuer: https://idp.example.com } --- response_body https://idp.example.com, https://other.example.com + + + +=== TEST 22: a message with no assertion of its own must be signed whole +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + -- the request carries no signature; the only one in the document + -- belongs to an assertion parked in Extensions, which samlp + -- accepts because it takes any other namespace + local stolen = sign(key, transform, assertion("stolen", "attacker@example.com")) + local logout = 'https://attacker.example.com' .. + '' .. stolen .. '' .. + 'victim@example.com' + local doc, err = submit(mngr, logout) + if err then + ngx.say("err: ", err) + else + ngx.say("issuer: ", tostring(saml.doc_issuer(doc)), + ", name_id: ", tostring(saml.doc_name_id(doc))) + end + } + } +--- response_body +err: signature does not cover the message + + + +=== TEST 23: a logout request names its own subject, not one parked in Extensions +--- config + location /t { + content_by_lua_block { + local key, mngr, transform = saml_ctx() + local logout = 'https://idp.example.com' .. + 'elsewhere@example.com' .. + 'victim@example.com' .. + 's-1' + local doc, err = submit(mngr, sign(key, transform, logout)) + if err then + ngx.say("err: ", err) + else + ngx.say("name_id: ", tostring(saml.doc_name_id(doc)), + ", session_index: ", tostring(saml.doc_session_index(doc))) + end + } + } +--- response_body +name_id: victim@example.com, session_index: s-1 From 18d767801b4610f13a660e194eb013d99868f82e Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Thu, 20 Aug 2026 16:21:36 +0545 Subject: [PATCH 6/6] fix: read idp_issuers once, and compare what the values mean Raised in review. The option was walked per request with ipairs and compared exactly, so ngx.null from a JSON null and a bare string took down every ACS callback with an error naming ipairs, while a map or a list with a gap refused everyone and blamed the IdP. It is read once in new() now, into a set, and a shape the callback cannot walk fails there with the option named. An empty list stays legal and still admits nobody. Issuer is a string in the schema, so libxml2 hands back the element text as written and a pretty-printed Issuer never equalled the configured value. Both sides are trimmed. Also from review: the README did not say an empty list differs from no list, the luadoc for doc_issuer described what it did before this PR, the comment on saml_doc_issuer named only one of the two verify paths that keep its invariant, and bind_identity_to_signature matches the root name without a namespace because schema validation refuses a foreign root before it runs. Tests: TEST 19 gave the Response and the assertion the same Issuer, so it passed whichever one was read; TEST 23 carried the only SessionIndex in the document, so it passed whichever lookup was used. Both discriminate now. The harness took an unknown allow-list name as no allow-list, which is the one result an acceptance test must not reach by accident. --- README.md | 2 +- lua/resty/saml.lua | 47 ++++++++++++++++++++++++++++++++-------- src/lua_saml.c | 2 +- src/sig.c | 2 ++ src/xml.c | 6 +++++ t/login-callback.t | 53 +++++++++++++++++++++++++++++++++++++++++++++ t/signed-response.t | 19 ++++++++++++---- 7 files changed, 116 insertions(+), 15 deletions(-) diff --git a/README.md b/README.md index b594f87..107ee53 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,7 @@ local saml = resty_saml.new(opts) | `sp_issuer` | string | None | SP name to access IdP. | | `idp_uri` | string | None | URI of IdP. | | `idp_cert` | string | None | IdP Certificate, used to verify saml response. | -| `idp_issuers` | array of strings | None | Issuers accepted on a login response; every assertion it carries has to name one. Unset accepts any issuer the `idp_cert` signs for. | +| `idp_issuers` | array of strings | None | Issuers accepted on a login response; every assertion it carries has to name one. Unset accepts any issuer the `idp_cert` signs for, which is not the same as an empty list: that one accepts nobody. | | `login_callback_uri` | string | None | redirect uri used to callback the SP from IdP after login. | | `logout_uri` | string | None | logout uri to trigger logout. | | `logout_callback_uri` | string | None | redirect uri used to callback the SP from IdP after logout. | diff --git a/lua/resty/saml.lua b/lua/resty/saml.lua index 9d019dc..8f7469a 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -258,6 +258,41 @@ local function parse_iso8601_utc_time(str) return os.time{year=year, month=month, day=day, hour=hour, min=min, sec=sec} end +-- An Issuer is a string in the XML schema, so libxml2 hands back the element +-- text as written, indentation included. Compare what the two sides mean. +local function trim(s) + return (s:gsub("^%s*(.-)%s*$", "%1")) +end + +-- Read idp_issuers once, into a set. A shape the callback cannot walk is a +-- configuration mistake, and finding out at construction names the option, +-- where finding out per request is a 500 or a blanket refusal that blames the +-- IdP. An empty list stays legal and means what it says: nobody is expected. +local function issuer_set(issuers) + if issuers == nil then + return nil + end + + local invalid = "idp_issuers must be a list of strings" + if type(issuers) ~= "table" then + error(invalid, 3) + end + + local set, count = {}, 0 + for i, issuer in pairs(issuers) do + if type(i) ~= "number" or i % 1 ~= 0 or i < 1 or type(issuer) ~= "string" then + error(invalid, 3) + end + set[trim(issuer)] = true + count = count + 1 + end + -- a gap would leave the entries past it unreachable to ipairs + if count ~= #issuers then + error(invalid, 3) + end + return set +end + -- A valid signature says the message came from the configured key. It does not -- say which IdP that key speaks for, so pin the issuer when the caller names -- the ones it expects. No list keeps the previous behaviour; a list nothing @@ -275,14 +310,7 @@ local function issuers_allowed(allowed, issuers) return false, "none readable" end for _, issuer in ipairs(issuers) do - local ok = false - for _, expected in ipairs(allowed) do - if expected == issuer then - ok = true - break - end - end - if not ok then + if not allowed[trim(issuer)] then return false, issuer end end @@ -332,7 +360,7 @@ local function login_callback(self, opts) local name_id = saml.doc_name_id(doc) local session_index = saml.doc_session_index(doc) - local allowed, unexpected = issuers_allowed(opts.idp_issuers, saml.doc_issuers(doc)) + local allowed, unexpected = issuers_allowed(self.idp_issuers, saml.doc_issuers(doc)) if not allowed then ngx.log(ngx.ERR, "unexpected issuer in response from IdP: ", tostring(unexpected)) ngx.exit(ngx.HTTP_UNAUTHORIZED) @@ -531,6 +559,7 @@ function _M.new(opts) obj.key_mngr_from_doc = function(doc) return obj.idp_cert_manager end obj.idp_cert_func = function(doc) return idp_cert end obj.auth_protocol_binding_method = opts.auth_protocol_binding_method + obj.idp_issuers = issuer_set(opts.idp_issuers) local cookie_secure, cookie_same_site if opts.auth_protocol_binding_method == "HTTP-POST" then cookie_secure = true diff --git a/src/lua_saml.c b/src/lua_saml.c index fb314be..e65a1d2 100644 --- a/src/lua_saml.c +++ b/src/lua_saml.c @@ -386,7 +386,7 @@ static int doc_name_id(lua_State* L) { /*** -Get the text of the issuer node +Get the issuer of the assertion a Response carries, or of the message itself @function doc_issuer @tparam xmlDoc* doc @treturn ?string issuer diff --git a/src/sig.c b/src/sig.c index 8800d76..827e7e0 100644 --- a/src/sig.c +++ b/src/sig.c @@ -362,6 +362,8 @@ static int bind_identity_to_signature(xmlDoc* doc) { if (sig != NULL && signature_covers(doc, sig, root)) { return 1; } + // matching the name alone is enough because schema validation, which runs + // before any of this, has already refused a root in another namespace if (xmlStrEqual(root->name, (const xmlChar*)"Response") != 1) { return 0; } diff --git a/src/xml.c b/src/xml.c index 1390e8f..381980e 100644 --- a/src/xml.c +++ b/src/xml.c @@ -63,6 +63,12 @@ static xmlChar* issuer_of(xmlDoc* doc, xmlNode* node) { // assertion still in the document is one the signature covers. A message that // carries no assertion is only accepted signed whole, so its own Issuer is the // one to read. +// +// Like the other accessors, this expects a document one of the two verify paths +// let through, and each path keeps that property its own way: the POST binding +// by pruning what the signature leaves out, the redirect binding by signing the +// encoded message whole and never pruning at all. Narrowing either one is what +// would cost this its footing. xmlChar* saml_doc_issuer(xmlDoc* doc) { xmlNode* root = xmlDocGetRootElement(doc); if (root == NULL) { diff --git a/t/login-callback.t b/t/login-callback.t index d2a590f..3d6eeee 100644 --- a/t/login-callback.t +++ b/t/login-callback.t @@ -97,6 +97,7 @@ GnHKA3uj9HpsS6fAxHNPPvWxRjO67Xj8Yw== SPS = {} function sp(name) + assert(name == "none" or ALLOW_LISTS[name] ~= nil, "no allow-list named " .. name) if SPS[name] == nil then SPS[name] = require("resty.saml").new({ sp_issuer = "sp", @@ -298,3 +299,55 @@ unexpected issuer in response from IdP: https://other.example.com 401 nil --- error_log unexpected issuer in response from IdP: none readable + + + +=== TEST 8: an issuer the IdP indented still matches the configured one +--- config + location /t { + content_by_lua_block { + local padded = "\n " .. IDP .. "\n " + ngx.say(login_with("exact", response(IDP, sign_doc(assertion(padded, "a1", "signed@example.com"))))) + } + } +--- response_body +302 / + + + +=== TEST 9: a shape the allow-list cannot be read from is a construction error +--- config + location /t { + content_by_lua_block { + local cases = { + ["a bare string"] = "https://idp.example.com", + ["a map"] = { ["https://idp.example.com"] = true }, + ["a list with a gap"] = { [1] = "https://idp.example.com", [3] = "https://other.example.com" }, + ["ngx.null"] = ngx.null, + } + local lines = {} + for name, value in pairs(cases) do + local ok, err = pcall(function() + return require("resty.saml").new({ + sp_issuer = "sp", + idp_uri = "http://127.0.0.1:1984/idp", + login_callback_uri = "/acs", + sp_cert = CERT_PEM, + sp_private_key = KEY_PEM, + idp_cert = CERT_PEM, + idp_issuers = value, + }) + end) + -- pairs order is not fixed, so sort rather than assert on it + lines[#lines + 1] = name .. ": " .. + (ok and "accepted" or (tostring(err):gsub(".*: ", ""))) + end + table.sort(lines) + ngx.say(table.concat(lines, "\n")) + } + } +--- response_body +a bare string: idp_issuers must be a list of strings +a list with a gap: idp_issuers must be a list of strings +a map: idp_issuers must be a list of strings +ngx.null: idp_issuers must be a list of strings diff --git a/t/signed-response.t b/t/signed-response.t index ee50524..5ad9480 100644 --- a/t/signed-response.t +++ b/t/signed-response.t @@ -522,13 +522,21 @@ issuer: https://idp.example.com -=== TEST 19: a whole-response signature reads the same issuer +=== TEST 19: a whole-response signature still reads the assertion's issuer --- config location /t { content_by_lua_block { local key, mngr, transform = saml_ctx() - local resp = response(SUCCESS, "resp-1", assertion("a1", "signed@example.com")) - local doc, err = submit(mngr, sign(key, transform, resp)) + -- both elements are covered here, so the choice between them is the + -- library's rather than the signature's: it is the assertion that + -- names the authority the identity comes from + local outer = '' .. + 'https://broker.example.com' .. + '' .. + assertion("a1", "signed@example.com") .. '' + local doc, err = submit(mngr, sign(key, transform, outer)) if err then ngx.say("err: ", err) else ngx.say("issuer: ", tostring(saml.doc_issuer(doc))) end } } @@ -617,7 +625,10 @@ err: signature does not cover the message local logout = 'https://idp.example.com' .. - 'elsewhere@example.com' .. + 'elsewhere@example.com' .. + assertion("ext", "elsewhere@example.com", + "elsewhere") .. + '' .. 'victim@example.com' .. 's-1' local doc, err = submit(mngr, sign(key, transform, logout))