search

tngan / samlify

Node.js library for SAML SSO
https://samlify.js.org
MIT License
604 stars 213 forks source link

Signature verification - libsaml.ts throws ERR_FAILED_TO_VERIFY_SIGNATURE #389

Open dsimic93 opened 3 years ago

dsimic93 commented 3 years ago

Hello, i encountered a weird issue. When i'm receiving response for AuthnRequest ERR_FAILED_TO_VERIFY_SIGNATURE error is thrown. I experimented around package and found out that if i comment out a single line of code, the signature verification passes.

https://github.com/tngan/samlify/blob/06a8b9cb92221d03727c74ed1bc4a64b095d0b1d/src/libsaml.ts#L395

Also, i was looking at xml-crypto docs and in the section for signature verification didn't see that they use that approach with removing sig node.

Is this a bug or did i misconfigured something?

I will gladly provide more info if it's needed.

rkkatariya commented 3 years ago

I am running into the same issue. Commenting out the line of code is not helping me.

tngan commented 3 years ago

@dsimic-coco @rkkatariya Which IdP are you using ? I have confirmed a test case that if the response contains CRLF character, it will cause the failed signature verification.

d3simic commented 3 years ago

I am running into the same issue. Commenting out the line of code is not helping me.

@rkkatariya FYI, I referenced ts file for clarification. Try commenting out line in dist folder if you haven't.

d3simic commented 3 years ago

@dsimic-coco @rkkatariya Which IdP are you using ? I have confirmed a test case that if the response contains CRLF character, it will cause the failed signature verification.

@tngan I'm using third party IdP (croatian government - NIAS service), but i don't think that info will be helpful. From my knowledge, they usualy spin their apps on win instances with .net/java stack on backend, so your deduction might prove plausible.

rkkatariya commented 3 years ago

@tngan @dsimic-coco There was an issue with the certificate I was using. It is working now. I was using the Samlify as Idp and Sp.

tngan commented 3 years ago

@d3simic Yes, it's a known issue that the extra CRLF will cause the failed signature verification with using samlify, from the case that I have worked with someone in the community couple months ago, he eventually made a workaround for it by removing CRLF before the parsing process begins.

tngan commented 3 years ago

@rkkatariya Would you mind to share what kind of issue? It could be a hint for debugging that someone may encounter later on. :)

dsimic93 commented 3 years ago

@d3simic Yes, it's a known issue that the extra CRLF will cause the failed signature verification with using samlify, from the case that I have worked with someone in the community couple months ago, he eventually made a workaround for it by removing CRLF before the parsing process begins.

@tngan Since there are no \r\n in decoded string, I think something else might be the issue. What would you need to try to debug? Also, since you didn't mention it, what is the purpose of the line i referenced. If there is no use or if it's some kind of deprecated flow, maybe remove it?

netmiller commented 3 years ago

Any new ideas for that. I have also same issue: ERR_FAILED_TO_VERIFY_SIGNATURE at /Users/esa/devel/systaylor/iris3/server/node_modules/samlify/src/libsaml.ts:401:17

Is there some workaround for this ??

SongWeiC-inf commented 3 years ago

hi @netmiller , have you sorted out the issue? i'm having same issue after updating my nodeJS version from 10 to 12

netmiller commented 3 years ago

No, I have not. But this issue is going to be some-kind-of-blocker for my project very soon. I have to solve this some way. Any ideas are very appreciative !

smali-kazmi commented 3 years ago

@tngan can you please guide about extra CRLF in which file? because i am stuck on this thing as well

dsimic93 commented 3 years ago

@tngan Any news, ideas or plans to resolve this? If not, is it ok/safe to use forked library build with mentioned line removed?

slephant commented 2 years ago

Debugging this error while onboarding a new IDP led me to discover they weren't signing their responses with an exclusive canonicalization method.

When it's signed in a non-exclusive canonical form, the parent document of an XML node makes a big difference to the signature. Calling removeChild(signatureNode) manually strips away those effects, which causes a mismatch with the original signature. An exclusive canonical form would strip them away from both sides of the equation.

I'm not totally convinced this is a samlify bug since the SAML 2.0 spec admits implementations should use exclusive canonicalization with or without comments, but for what it's worth, removing the line in question doesn't seem to break validation for either method.