fix: bind the read identity to the verified signature - #32
fix: bind the read identity to the verified signature#32shreemaan-abhishek wants to merge 8 commits into
Conversation
Identity is read from the first Assertion in document order while a single Signature is verified. If a Response carried more than one Assertion the extracted identity need not be the signed one. Reject a verified Response that does not carry exactly one Assertion, and add a regression test. Cut as 0.2.6.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change validates that successful SAML response identities are covered by signatures, rejects unsigned identities during POST binding, scopes status parsing to direct protocol elements, and adds end-to-end coverage for nested and malformed signature scenarios. ChangesSAML identity signature validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant POSTBinding
participant saml_verified_identity_is_signed
participant SignatureReferenceResolution
participant SAMLResponse
POSTBinding->>saml_verified_identity_is_signed: validate identity coverage
saml_verified_identity_is_signed->>SignatureReferenceResolution: resolve signature reference
SignatureReferenceResolution->>SAMLResponse: locate response or assertion target
SAMLResponse-->>SignatureReferenceResolution: return target
SignatureReferenceResolution-->>saml_verified_identity_is_signed: return coverage result
saml_verified_identity_is_signed-->>POSTBinding: return validation status
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Hardens SAML POST response handling against assertion confusion.
Changes:
- Adds exact assertion-count validation and a new error status.
- Adds regression coverage for one versus multiple assertions.
- Adds the 0.2.6 LuaRocks specification.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/xml.c |
Counts assertions in SAML responses. |
src/binding.c |
Enforces assertion count after signature verification. |
src/saml.h |
Exposes the helper and status code. |
t/assertion-count.t |
Tests single and multiple assertions. |
rockspec/lua-resty-saml-0.2.6-0.rockspec |
Defines release metadata. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Requiring exactly one assertion did not cover every case: identity is read from an Assertion that is a direct child of the Response, but the verified Signature was never tied to that element, and a signed but assertion-free Response nested in saml:Advice could satisfy a bare count. Instead resolve the element the verified signature covers and require it to be either the whole Response or the single read Assertion. Apply this to successful responses only, so a signed non-success response with no assertion is no longer rejected. Expand the test to cover assertion-level and whole-response signatures, the nested-Advice case, and a signed failure response.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/saml.h (1)
81-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the return contract on the public declaration.
Other exported functions in this codebase return
0for success and non-zero for failure.saml_verified_identity_is_signedinverts that convention and returns1when the identity is covered. Add a short comment so callers do not invert the check.📝 Proposed doc comment
+// Returns 1 if the identity read from the document is covered by the verified +// signature, 0 otherwise. Note the inverted convention: 0 means "not covered". int saml_verified_identity_is_signed(xmlDoc* doc);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/saml.h` at line 81, Add a concise documentation comment to the public declaration of saml_verified_identity_is_signed stating that it returns 1 when the identity is covered/signed and 0 otherwise, so callers use the inverted return convention correctly.src/sig.c (1)
287-308: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHandle signatures that carry more than one
ds:Reference.
xmlSecFindNodereturns only the firstds:Reference. XML-DSig permits several, and xmlsec verifies all of them. If the assertion is covered by the secondds:Reference, this helper returns the first target, the identity check fails, and a validly signed response is rejected.The failure direction is safe, so this is not a security hole. It is an interoperability gap with IdPs that sign the response and the assertion in one signature. Consider collecting every
ds:Referencetarget and accepting the document if any target is the root or the sole direct-child assertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/sig.c` around lines 287 - 308, Update signed_reference_target and its caller to iterate over every ds:Reference instead of using only xmlSecFindNode’s first result. Resolve each reference target and accept the document when any target is the document root or the sole direct-child assertion, while preserving rejection when none match.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sig.c`:
- Around line 358-360: In src/sig.c lines 358-360, update the assertion lookup
to set ctx->node to root and evaluate only direct saml:Assertion children,
preserving the existing count and identity checks. In t/signed-response.t lines
205-219, add a regression test after TEST 5 with a root Response lacking a
direct-child assertion and a nested Response containing one signed assertion,
and assert rejection with “response identity is not covered by the signature”.
---
Nitpick comments:
In `@src/saml.h`:
- Line 81: Add a concise documentation comment to the public declaration of
saml_verified_identity_is_signed stating that it returns 1 when the identity is
covered/signed and 0 otherwise, so callers use the inverted return convention
correctly.
In `@src/sig.c`:
- Around line 287-308: Update signed_reference_target and its caller to iterate
over every ds:Reference instead of using only xmlSecFindNode’s first result.
Resolve each reference target and accept the document when any target is the
document root or the sole direct-child assertion, while preserving rejection
when none match.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef7bd5b9-6c24-45dc-af26-92674b121fa2
📒 Files selected for processing (4)
src/binding.csrc/saml.hsrc/sig.ct/signed-response.t
| xmlChar* status = saml_doc_status_code(doc); | ||
| int success = status != NULL && xmlStrEqual(status, (const xmlChar*)SAML_STATUS_SUCCESS) == 1; | ||
| if (status != NULL) { | ||
| xmlFree(status); | ||
| } | ||
| if (!success) { | ||
| return 1; |
There was a problem hiding this comment.
Fixed in aa602ba. saml_doc_status_code now resolves the root message's direct Status/StatusCode instead of a document-wide match, so a nested non-success status can no longer be read as the root status. Confirmed the Extensions-position shape bypassed the previous commit, and added TEST 7 covering it.
| if (target == root) { | ||
| return 1; | ||
| } | ||
|
|
||
| xmlXPathContext* ctx = xmlXPathNewContext(doc); | ||
| if (ctx == NULL) { | ||
| return 0; | ||
| } | ||
| if (xmlXPathRegisterNs(ctx, (const xmlChar*)"saml", (const xmlChar*)SAML_XMLNS_ASSERTION) < 0 || | ||
| xmlXPathRegisterNs(ctx, (const xmlChar*)"samlp", (const xmlChar*)SAML_XMLNS_PROTOCOL) < 0) { | ||
| xmlXPathFreeContext(ctx); | ||
| return 0; | ||
| } | ||
|
|
||
| xmlXPathObject* obj = xmlXPathEvalExpression((const xmlChar*)"//samlp:Response/saml:Assertion", ctx); | ||
| int safe = obj != NULL && !xmlXPathNodeSetIsEmpty(obj->nodesetval) && | ||
| obj->nodesetval->nodeNr == 1 && obj->nodesetval->nodeTab[0] == target; |
There was a problem hiding this comment.
Fixed in aa602ba. The whole-response shortcut is gone: the cardinality check now runs for root-covered signatures too, requiring exactly one direct-child assertion. A signed success response with zero assertions is rejected (added TEST 6), so no authenticated session is created without an identity.
Follow-ups on the reference-binding check. Read the response status from the root message directly rather than a document-wide match, so a nested Response earlier in document order cannot relax the check. Require exactly one Assertion that is a direct child of the root Response and drop the whole-response shortcut, so a signed response with no assertion, or with assertions only under a nested Response, is no longer accepted. Add regression tests for a signed assertion-free success response and a nested status placed in Extensions.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/xml.c`:
- Line 109: Update the StatusCode/@Value lookup in doc_status_code to use
xmlGetNoNsProp(code, (const xmlChar*)"Value") instead of xmlGetProp, ensuring
only an attribute with no namespace is accepted. Add a regression test covering
an unvalidated document reaching saml_binding_post_verify and doc_status_code
with a namespaced Value attribute.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a7082e6-e438-456e-be84-7790afe24b2f
📒 Files selected for processing (3)
src/sig.csrc/xml.ct/signed-response.t
| int safe = 0; | ||
| if (obj != NULL && !xmlXPathNodeSetIsEmpty(obj->nodesetval) && obj->nodesetval->nodeNr == 1) { | ||
| xmlNode* assertion = obj->nodesetval->nodeTab[0]; | ||
| safe = target == root || target == assertion; |
There was a problem hiding this comment.
P1: This still permits unsigned identity injection through a schema-valid nested Response. With assertion-level signing, an attacker can place Extensions -> Assertion -> Advice -> Response -> Assertion before the signed direct-child assertion. ./saml:Assertion only counts the root response’s direct children, so target == assertion succeeds, but XPATH_NAME_ID, XPATH_ATTRIBUTES, XPATH_SESSION_INDEX, and XPATH_SESSION_EXPIRES remain document-wide //samlp:Response/saml:Assertion... queries and read the earlier nested, unsigned assertion. Please bind every identity/session lookup to this exact root assertion, or reject nested Response elements.
There was a problem hiding this comment.
Confirmed and fixed in 9eb980f. Reproduced the bypass (a forged assertion under a nested Response in Extensions was read by the document-wide //samlp:Response/saml:Assertion while the root-scoped count saw only the signed assertion). The check now counts assertions document-wide (//saml:Assertion), requires exactly one, requires it to be a direct child of this Response, and requires the signature to cover the Response or that assertion. Added TEST 8 for the nested-Response shape.
| @@ -0,0 +1,38 @@ | |||
| package = "lua-resty-saml" | |||
| version = "0.2.6-0" | |||
There was a problem hiding this comment.
P1: Adding this rockspec to this PR breaks the repository’s release workflow under its configured merge policy. The repository only permits squash merges and uses the PR title for a multi-commit squash; this PR is titled fix: .... A change under rockspec/** triggers release.yml, whose Extract release name step only accepts a head commit message beginning with feat: release v..., so the merge will fail before creating v0.2.6 or uploading it to LuaRocks. Please move this rockspec to a dedicated PR titled feat: release v0.2.6, or ensure the final squash title matches that format.
There was a problem hiding this comment.
Good catch. Dropped rockspec/lua-resty-saml-0.2.6-0.rockspec from this PR in 9eb980f. The version will be cut in a dedicated 'feat: release v0.2.6' change so release.yml's head-commit check is satisfied.
| xmlXPathObject* obj = xmlXPathEvalExpression((const xmlChar*)"./saml:Assertion", ctx); | ||
| int safe = 0; | ||
| if (obj != NULL && !xmlXPathNodeSetIsEmpty(obj->nodesetval) && obj->nodesetval->nodeNr == 1) { | ||
| xmlNode* assertion = obj->nodesetval->nodeTab[0]; | ||
| safe = target == root || target == assertion; | ||
| } |
There was a problem hiding this comment.
Fixed in 9eb980f, same root cause you flagged. Now counting //saml:Assertion document-wide and requiring the single result to be the root's direct child (and signed), so the count and the document-wide readers can no longer disagree. Nested-Response regression test added (TEST 8).
| if (sig == NULL) { | ||
| return 0; | ||
| } | ||
| xmlNode* target = signed_reference_target(doc, sig); |
There was a problem hiding this comment.
Valid SAML signature-profile point (Core 5.4 restricts reference transforms to enveloped-signature plus canonicalization). I'm treating it as a separate hardening change rather than folding it in here: it is not reachable by an external attacker in this flow, since live inside the signed and cannot be altered or introduced without the IdP key, so it only matters if the IdP itself signs with a disallowed transform. A too-strict allow-list also risks rejecting legitimate IdP signatures, so I'd rather enforce enabledReferenceTransforms with tests against real IdP signatures in its own PR. Tracking it as a follow-up.
The identity and attribute readers query the whole document, so counting only the root's direct children left a gap: an assertion placed under a nested Response (reachable through Extensions or Advice) is read by those queries but was not counted. Require the document to hold exactly one Assertion, that it be a direct child of this Response, and that the verified signature cover the Response or that assertion. Read StatusCode Value as an unqualified attribute. Add a nested-Response regression test. The release rockspec is dropped from this branch and will ship in a dedicated release change.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/saml.h:60
- The human-authored Changes section documents a new
SAML_INVALID_ASSERTIONSstatus andsaml_doc_single_assertionhelper, but the public API now exposesSAML_UNVERIFIED_IDENTITYand the implementation resolves signature targets instead. Please update the PR description so its API and implementation notes match the proposed code.
SAML_UNVERIFIED_IDENTITY,
Counting every Assertion in the document was both too strict and aimed at the wrong set. The readers select //samlp:Response/saml:Assertion, so an Assertion under Advice or Extensions is unreachable and harmless, while a Response signed at the root covers however many assertions it carries. Requiring exactly one rejected both shapes, and each is schema valid. Check that set instead: every assertion a reader can reach must be a direct child of this Response, and the verified signature must cover the Response or, for an assertion-level signature, that one assertion. Apply this whenever the readers see an assertion rather than only on success, so the guarantee no longer depends on the caller checking status first; status is still read to reject a successful response with no identity. Compile the selector next to the readers it mirrors, drop the ID scan that could resolve a different element than the one verification used, and cover multiple assertions, Advice, a signed assertion parked in Extensions, and a LogoutResponse status read.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/sig.c`:
- Around line 337-346: Require count == 1 in the target == root branch of the
signature coverage logic, matching the existing assertion-level check; keep
target == NULL behavior unchanged. Update TEST 9 in t/signed-response.t (lines
302-321) to expect “response identity is not covered by the signature” for two
direct-child assertions. The src/sig.c site requires the implementation change;
the test site requires the expectation update.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c27426d9-5985-49d0-8e49-9df6c7dd1e9a
📒 Files selected for processing (3)
src/saml.csrc/sig.ct/signed-response.t
openresty pulls in openresty-openssl3 already, so asking for openresty-openssl111-dev fetches a second OpenSSL that nothing else needs. Build against the one that ships with the server. The Makefile reads OPENSSL_DIR with ?=, so the job env selects it.
openresty-openssl3-dev is currently unfetchable, so building against it traded one broken download for a worse one. Go back to openresty-openssl111-dev and give apt room to get through the 500s openresty.org is returning.
openresty.org returns 500 for the -dev packages on every request from GitHub runners, so apt retries cannot get through. The openresty image carries the same 1.31.1.1 build with the OpenSSL headers the module needs, so copy the install tree out of it and skip the apt repo. That OpenSSL is 3.5.7, the one its nginx links against, so point OPENSSL_DIR at it rather than building against 1.1.1 headers.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
src/saml.c:71
- This selector does not implement the stated requirement of exactly one
<Assertion>anywhere in the document; it only counts assertions that are direct children of aResponse. For example, TEST 11 includes a second schema-valid assertion underAdviceand expects acceptance. Either count//saml:Assertionand reject that case, or update the PR title/description to explicitly scope the guarantee to identity-readable response assertions.
// Every assertion the readers above can reach. saml_verified_identity_is_signed
// requires the signature to cover all of them, so keep this selector in step
// with theirs.
XPATH_ASSERTIONS = xmlXPathCompile((const xmlChar*)"//samlp:Response/saml:Assertion");
What
saml_verify_docverifies one<Signature>and reports success. The identity readers (doc_name_id,doc_attrs,doc_session_index,doc_session_expires) then run//samlp:Response/saml:Assertion/...over the whole document. Nothing tied the two together, so the assertion the identity is read from need not be the one the signature covered.That is signature wrapping. Present a
Responsecarrying one genuinely IdP-signed assertion alongside a forged one, and verification passes while the readers pick the forged assertion.What this changes
saml_verified_identity_is_signedruns after verification succeeds and enforces one invariant: every assertion the readers can reach must be covered by the signature that was verified.XPATH_ASSERTIONSselects//samlp:Response/saml:Assertion, the exact prefix the four readers share, so the check and the readers cannot drift apart.Response, which rules out one smuggled into a nestedResponseunderExtensionsorAdvice.Referencemust resolve to the rootResponse, or, for an assertion-level signature, to the single reachable assertion.saml_doc_status_codenow reads the root message's directStatus/StatusCodeinstead of a document-wide match, so a nestedResponse's status cannot be mistaken for the root's, and reads@ValuewithxmlGetNoNsPropso a namespaced look-alike does not match.This is not an exactly-one-assertion rule
Earlier revisions of this branch required exactly one assertion, and the title said so. That requirement is dropped deliberately, because it was both too strict and aimed at the wrong set:
Response, so a multi-assertion signed response is authentic in full. Rejecting it costs interop for no security gain (TEST 9).saml:Adviceis not reader-visible, so it cannot stand in for an identity and needs no rule (TEST 11).Exactly-one survives only where it is load-bearing: an assertion-level signature covers one assertion, so one is all a reader may reach (TEST 10).
Changes
saml_verified_identity_is_signedandsigned_reference_targetinsrc/sig.c, wired intosaml_binding_post_verify, returning a newSAML_UNVERIFIED_IDENTITYstatus.saml_doc_status_codeinsrc/xml.c.XPATH_ASSERTIONSreplacesXPATH_STATUS_CODEinsrc/saml.c.t/signed-response.t, 13 cases covering assertion-level and response-level signatures, the multi-assertion and nested-Responseshapes,Advice,Extensions, an assertion-free success response, and aLogoutResponsestatus read.-devpackages from GitHub runners.Testing
prove -r t/, plus the existing Keycloak end-to-end suite in CI.Deliberately out of scope
Reference transform restrictions (SAML Core 5.4).
<Transforms>sits inside<SignedInfo>, so a node-set-filtering transform cannot be introduced without the IdP key, and a strict allow-list risks rejecting legitimate IdP signatures. Worth its own change with tests against real IdP output.Filed separately while reviewing this: #33 (
doc_issueroutside assertion-level coverage), #34 (anchoring the reader XPaths, which would make this check's direct-child rule redundant), #35 (verify_docexported without the check), #36 (LogoutRequestidentity unbound), #37 (assertionConditionsnever validated).The 0.2.6 rockspec was dropped from this branch and will ship as a dedicated
feat: release v0.2.6change, sorelease.yml's head-commit check is satisfied.