validateSignature: Support XML docs that contain multiple signed nodes

[vandernorth]

validateSignature: Support XML docs that contain multiple signed nodes. Only select the signatures which reference the currentNode.

I've come across a SAML-response that had multiple (different) parts of that XML signed. The current xpath selector finds all 's in the document, but what we are actually interested in are the signatures for the currentNode.

If there is more than 1 of such signature we should still fail, but overall this seems like a more compatible xpath selector.

Any thoughts on this?

[cjbarth]

I follow the logic of your PR, however, I've never seen such an XML document before, so I can't comment on its technical accuracy or broad applicability. Perhaps you could share with us a part from the SAML spec that addresses this.

In any case, some tests will be needed; please add them.

[vandernorth]

@cjbarth it took me a while to find the specs, but here it is.

This is the SAML Response I'm working with. Stripped most of the data, trying to only keep the relevant parts.

<?xml version="1.0" encoding="UTF-8"?>
<samlp:Response xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
                xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
                xmlns:ds="http://www.w3.org/2000/09/xmldsig#"
                ID="_A">
    <ds:Signature>sigature for _A</ds:Signature>
    <saml:Issuer>...</saml:Issuer>
    <samlp:Status>...</samlp:Status>
    <saml:Assertion ID="_B">
        <ds:Signature>signature for _B</ds:Signature>
        <saml:Issuer>...</saml:Issuer>
        <saml:Subject>...</saml:Subject>
        <saml:Conditions>...</saml:Conditions>
        <saml:Advice>
            <saml:Assertion ID="_C">
                <saml:Issuer>...</saml:Issuer>
                <ds:Signature>Signature for _C</ds:Signature>
                <saml:Subject>....</saml:Subject>
                <saml:Conditions>...</saml:Conditions>
                <saml:AuthnStatement>...</saml:AuthnStatement>
                <saml:AttributeStatement>...</saml:AttributeStatement>
            </saml:Assertion>
            <saml:Assertion ID="_D">
                <saml:Issuer>...</saml:Issuer>
                <ds:Signature>signature for _D</ds:Signature>
                <saml:Subject>...</saml:Subject>
                <saml:Conditions />
                <saml:Advice>...</saml:Advice>
                <saml:Statement>...</saml:Statement>
            </saml:Assertion>
        </saml:Advice>
        <saml:AuthnStatement>...</saml:AuthnStatement>
        <saml:AttributeStatement>...</saml:AttributeStatement>
    </saml:Assertion>
</samlp:Response>

Spec: https://docs.oasis-open.org/security/saml/v2.0/saml-core-2.0-os.pdf
I'll reference the line numbers they use.

<Reponse> has been signed which is optionally allowed on line 1569 (Reponse is ResponseType which is a StatusResponseType).
<Assertion>'s have been signed which is optionally allowed on line 564.
The <Assertion> contains the optional <Advice> on line 572.
<Advice> can contain 0 to unbounded amounts of <Assertion> see line 1026.

I'll try to write some tests. Any suggestions there on what you'd like to see? Testing the ValidateSignature function with a few differently signed XMLs?

[cjbarth]

Ideally we'd like to have test to cover the parts of the spec that you're referencing. So:

• A <Response> that is signed and one that is unsigned
• An <Assertion> that has been signed and unsigned without and <Advice>
• An <Assertion> with an <Advice> referencing 0, 1, and 2 <Assertion>

So I would like to see a minimum of 7 tests to cover this. Ideally, we'd cover all permutations of this just to catch any sneeky little bugs that might crop up with a mix-and-match of these things, though such bugs are unlikely in practice. Having said that, I haven't checked, but we probably have tests to check a <Response> object, so two of the tests may already be done.

[vandernorth]

@cjbarth / @markstos I've added tests for a few of the permutations you asked for. Let me know what you think.

[markstos]

@vandernorth Thank you. At the same time, we are working on converting the project to TypeScript (just merged).

I'm sorry that created a conflict with your most recent work. Would you mind looking at the conflict?

[cjbarth]

If you need help resolving this conflict, which seems pretty small to be honest, I might suggest that you engage with @gugu , since he is also actively working with this project and some refactoring.

Thanks again for your very thorough tests and continued work on this PR. It looks like it will be a very nice value-add when it lands.

[cjbarth]

These look great. I think it is good to merge as soon as the conflicts can be resolved.

[vandernorth]

Good work on the typescript! Fixed the conflicts.

[markstos]

QA Log

• Reviewed diff

[markstos]

@cjbarth merge if the final result looks good to you (let's "squash" to minimize commit noise).

[cjbarth]

@markstos @vandernorth I'm not sure what happened here. The build that happened after I merged failed. I reverted this commit and the build started working again. @vandernorth Can you please have a look at the failed built and see what can be done to rectify the issues?

[vandernorth]

Strange! Will check tomorrow. Note to self: https://github.com/node-saml/passport-saml/actions/runs/336666640

[vandernorth]

See #481