fix: read the issuer from signed content, and let it be pinned

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.       |

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

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;

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},

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);

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;
 }

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;
     }