diff --git a/README.md b/README.md index 04a7ef6..b594f87 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; 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 8ab7985..9d019dc 100644 --- a/lua/resty/saml.lua +++ b/lua/resty/saml.lua @@ -258,6 +258,37 @@ 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. +-- +-- 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. 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 + 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 + ok = true + break + end + end + if not ok then + return false, issuer + end + end + return true +end + local function login_callback(self, opts) local sess = session.start(self.session_config) @@ -301,6 +332,12 @@ 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)) + if not allowed then + ngx.log(ngx.ERR, "unexpected issuer in response from IdP: ", tostring(unexpected)) + 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/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/lua_saml.c b/src/lua_saml.c index 80baa9a..fb314be 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, issuers[i] == NULL ? "" : (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..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); @@ -78,6 +79,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/sig.c b/src/sig.c index 1d8e33d..8800d76 100644 --- a/src/sig.c +++ b/src/sig.c @@ -339,28 +339,31 @@ 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; -} - - +// 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) { @@ -371,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 bbc1bfb..1390e8f 100644 --- a/src/xml.c +++ b/src/xml.c @@ -25,20 +25,137 @@ 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 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, name) == 1 && + child->ns != NULL && + 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. 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* 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); +} + + +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 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, +// 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); + 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; } @@ -49,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; } @@ -76,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 @@ -98,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; } @@ -144,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/login-callback.t b/t/login-callback.t new file mode 100644 index 0000000..d2a590f --- /dev/null +++ b/t/login-callback.t @@ -0,0 +1,300 @@ +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" }, + both = { IDP, "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 + + 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 + + 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', + 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 + -- 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 + + + +=== 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 / + + + +=== 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 diff --git a/t/signed-response.t b/t/signed-response.t index c4df4c9..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 @@ -496,3 +496,138 @@ 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 + + + +=== 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 + + + +=== 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