allows the signature to be embedded outside the assertion element

[bcoe]

Testing against a major SAML provider, I noticed that it's valid for the signature to live outside of the assertion.

This change, along with:

#47

and

https://github.com/yaronn/xml-crypto/pull/82/files

Adds support to saml2 for one of the bigger SAML providers out there -- we will need to also bump xml-crypto once 82 lands.

[jefff]

Thanks for the PR! Sorry about the delay in response. I noticed something strange in the test case here which lead to noticing a bug in how we verify signatures. I fixed that on master which unfortunately means you test case is no longer passing. I think the change is otherwise good so if you're able to fix the test we can get this merged.

[bcoe]

@jefff I actually found another issue testing today; I think even though there are multiple assertions each with a signature only the hash from the first assertion is checked -- I didn't dig too much, but I noticed saml20 simply takes the first signature if there are more than one; I wonder what the right call is?

[jefff]

Do you have an example of that case? I think we generally reject multiple assertions due to the complexity of combining and verifying them correctly (and also because none of the IdP's we tested with ever did that). I think the only correct way to handle that would be to verify each assertion is individually signed, and then return a result that represents a combination of all the individual assertions which needless to say would be difficult to get exactly right.

[bcoe]

@jefff this is the payload (from a major SAML IDP I can't name) that is giving all kinds of trouble:

https://gist.github.com/bcoe/4bfc7d35be1f6cde048b

It also has trouble with xml-crypto/xml-dom.

[jefff]

Ah, OK. In that example there is only one Assertion but two signatures; one that signs the Response and one that signs the contained Assertion. I think the library could be modified to accept the first one that contains the assertion, which in this case would be either of them. It's also likely that your IdP may have an option to turn one of the two off (I'd recommend turning off the one signing the Response if you have the choice).

[cozmo]

This PR has been quiet for quite some time - @bcoe where do we stand on this? Is this still relevant/needed?

[mohit]

Closing due to inactivity from author. @bcoe please comment if you want to repopen.