[BUG] After an upgrade from passport-saml to @node-saml/passport-saml, assertion validation stopped working with HTTP-POST binding with an error: SAML.validatePostResponseAsync, Invalid document signature

v0idmaster commented 1 year ago

Hello, I upgraded from passport-saml to @node-saml/passport-saml and my logins stopped working diring assertion validation. I am using HTTP-POST binding and a PingFederate IDP. The error I am getting is:

Error: Invalid document signature
    at SAML.validatePostResponseAsync (C:\Develop\test\node_modules\@node-saml\node-saml\lib\saml.js:510:23)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

After debugging the code, I found out that it fails because it searches for a ds:Signature element having a ds:Reference descendant with an URI attr that is equal to the ID of the samlp:Response node, which is not the case, and fails because it cannot find a signatue that way.

  const xpathTransformQuery =
    ".//*[" +
    "local-name(.)='Transform' and " +
    "namespace-uri(.)='http://www.w3.org/2000/09/xmldsig#' and " +
    "ancestor::*[local-name(.)='Reference' and @URI='#" +
    currentNode.getAttribute("ID") +
    "']" +
    "]";

Here, currentNode is the samlp:Response element passed from the validatePostResponseAsync function. However, the Reference URL attr in my case is actually equal to the ID of the saml:Assertion element.

Example of the assertion message:

<samlp:Response Version="2.0" ID="ZHO2f87e-GRdaDjH0XdJJGD3pHT" IssueInstant="2023-01-16T20:28:51.587Z"
    xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol">
    <saml:Issuer
        xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">https://solvelocalassertion2
    </saml:Issuer>
    <samlp:Status>
        <samlp:StatusCode Value="urn:oasis:names:tc:SAML:2.0:status:Success"/>
    </samlp:Status>
    <saml:Assertion ID="w82kePprRVPi.hkkZ5QlwHD_DW_" IssueInstant="2023-01-16T20:28:51.587Z" Version="2.0"
        xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">
        <saml:Issuer>https://solvelocalassertion2</saml:Issuer>
        <ds:Signature
            xmlns:ds="http://www.w3.org/2000/09/xmldsig#">
            <ds:SignedInfo>
                <ds:CanonicalizationMethod Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
                <ds:SignatureMethod Algorithm="http://www.w3.org/2001/04/xmldsig-more#rsa-sha256"/>
                <ds:Reference URI="#w82kePprRVPi.hkkZ5QlwHD_DW_">
                    <ds:Transforms>
                        <ds:Transform Algorithm="http://www.w3.org/2000/09/xmldsig#enveloped-signature"/>
                        <ds:Transform Algorithm="http://www.w3.org/2001/10/xml-exc-c14n#"/>
                    </ds:Transforms>
                    <ds:DigestMethod Algorithm="http://www.w3.org/2001/04/xmlenc#sha256"/>
                    <ds:DigestValue>E5JSAPhCqiA37HD3kYPlRkJjD9aYgiSOjX4aaWA528M=</ds:DigestValue>
                </ds:Reference>
            </ds:SignedInfo>
            <ds:SignatureValue>CzTZoSBdNrgVbxIqlV7VIw43s4o/omuznW9mrt6smxzKhbsvFO9SIPnkIzQ49Y52BmhmR7FzpFJboXOJseMnuRraHrOOB9Mrh6M/aj+wKh97kRdbqjlV0XozRKjTwBteFr7Fktw5Y0yGPk4vpMIMoQyLRur4lh3+A0fJEUyl+BphCkHw0ZR4Qz34veswpv3s7sXFRagVSHUbscSVoN/l8zPyKxkMXh74WVd9X5tgmglpDSCVFQXIXQdrZCJMHU0R0fon7w/CWgQSo3y4YSQpF/0JzaOzMVYDK0bmM+qjABSdJf9yU+kOn0BIR0VuoPtNPIqYEZ3biajq1BIEwLZq0g==</ds:SignatureValue>
        </ds:Signature>
        <saml:Subject>
            <saml:NameID Format="urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified">cuVMW@controlUp.demo</saml:NameID>
            <saml:SubjectConfirmation Method="urn:oasis:names:tc:SAML:2.0:cm:bearer">
                <saml:SubjectConfirmationData Recipient="http://localhost:8000/api/user/sso/assertion" NotOnOrAfter="2023-01-16T20:33:51.587Z"/>
            </saml:SubjectConfirmation>
        </saml:Subject>
        <saml:Conditions NotBefore="2023-01-16T20:23:51.587Z" NotOnOrAfter="2023-01-16T20:33:51.587Z">
            <saml:AudienceRestriction>
                <saml:Audience>Solve_LocalAssertion2</saml:Audience>
            </saml:AudienceRestriction>
        </saml:Conditions>
        <saml:AuthnStatement SessionIndex="w82kePprRVPi.hkkZ5QlwHD_DW_" AuthnInstant="2023-01-16T20:27:42.053Z">
            <saml:AuthnContext>
                <saml:AuthnContextClassRef>urn:oasis:names:tc:SAML:2.0:ac:classes:unspecified</saml:AuthnContextClassRef>
            </saml:AuthnContext>
        </saml:AuthnStatement>
        <saml:AttributeStatement>
            <saml:Attribute Name="userPrincipalName" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
                <saml:AttributeValue xsi:type="xs:string"
                    xmlns:xs="http://www.w3.org/2001/XMLSchema"
                    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">cuVMW@controlUp.demo
                </saml:AttributeValue>
            </saml:Attribute>
            <saml:Attribute Name="username" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:basic">
                <saml:AttributeValue xsi:type="xs:string"
                    xmlns:xs="http://www.w3.org/2001/XMLSchema"
                    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance">cuvmw
                </saml:AttributeValue>
            </saml:Attribute>
        </saml:AttributeStatement>
    </saml:Assertion>
</samlp:Response>

This is the xml that is generated by PingFederate. Please assist me with resolving that issue. Thanks!

cjbarth commented 1 year ago

Please edit your post to correctly format code blocks.

v0idmaster commented 1 year ago

I Updated the post.

cjbarth commented 1 year ago

So it seems that you have a signed assertion and not a singed response. You should adjust your config to say so. By default it is assumed that you have both response and assertions signed, which is clearly not the case. If I'm misunderstanding, please post a reference to the SAML spec that indicates that passport-saml is doing something wrong.

srd90 commented 1 year ago

Root cause seems to be that changelog is not read prior/during/after migration to new major version.

This issue looks like exact duplicate of

https://github.com/node-saml/node-saml/issues/211

First see the referenced issue's replys for background information and alter value of wantAuthnResponseSigned configuration option.

Most up to date documentation of configuration options are available at @node-saml/node-saml because documentation updates at this side are not quite there yet.

v0idmaster commented 1 year ago

Thanks for the response, How should I adjust a config? Is it configurable from the passport-saml configuration or is it the IDP that should be reconfigured, because the latter is a problem.

v0idmaster commented 1 year ago

Root cause seems to be that changelog is not read prior/during/after migration to new major version.

This issue looks like exact duplicate of

[BUG] When validating the SAML Response, it is assumed that the SAML Response ID attribute is the same as the SAML Assertion ID attribute node-saml#211

First see the referenced issue's replys for background information and alter value of wantAuthnResponseSigned configuration option.

Most up to date documentation of configuration options are available at @node-saml/node-saml because documentation updates at this side are not quite there yet.

Thank tou for the answer.

markstos commented 1 year ago

@srd90 @cjbarth I think it would help if in our Changelog we "rolled up" all the "Major Changes" from the 4.0.0 beta series so that they were all mentioned under the 4.0.0 Release heading in one place.

Also, the bullet point "Add option to require a document signature" could be clearer, because on it's face, adding an option is backwards compatible. A better phasing of this major change is:

Document signatures are now required by default. Setting wantAuthenResponseSigned=false disables this feature and restores the prior, less secure behavior.

The other signature-related bullet point could also be more helpful as:

Require all assertions be signed; new option wantAssertionsSigned can be set to false to enabled the older, less secure behavior.

dangtony98 commented 1 year ago

Hey @markstos is the wantAuthenResponseSigned even available? I didn't see it in the docs... Currently stuck on this issue as JumpCloud IdP doesn't seem to sign responses.

srd90 commented 1 year ago

Hey @markstos is the wantAuthenResponseSigned even available? I didn't see it in the docs...

@dangtony98 as of now @node-saml/passport-saml project's configuration options documentation is not up to date. You should consult also @node-saml/node-saml project's documentation. passport-saml was splitted at 4.0.0 so that @node-saml/node-saml contains core SAML functionality and @node-saml/passport-saml uses internally that library to implement passportjs module for SAML (big bigture is that @node-saml/node-saml configuration options are used as via @node-saml/passport-saml )

node-saml / passport-saml

[BUG] After an upgrade from passport-saml to @node-saml/passport-saml, assertion validation stopped working with HTTP-POST binding with an error: SAML.validatePostResponseAsync, Invalid document signature #839