Exposing signature validation errors?

[autopulated]

Currently whatever error causes the SAML response signature validation to fail, the same error message is returned.

Would you be open to exposing these somehow? (Either in more detailed error messages, or via a property on the SAML object and method on the strategy to read it), or is hiding the specific error a deliberate decision to make attacks harder?

[autopulated]

(specifically, would a pull request adding this be welcome?)

[markstos]

Yes, a PR is welcome. . Before I was a maintainer myself, I ran into this problem, and it was maddening that there were multiple cases that could trigger the same 'Invalid Signature' error, and I couldn't tell from the error which case had been triggered.

I tried to find the previous PR or Issue discussing this very issue, but I couldn't find it just now.

[markstos]

I'm in favor of more specific error messages.

You are right to be concerned that the more detailed error messages might be a security concern, but the developer is always welcome to catch the errors and throw a more generic error back to the user if they prefer.

[ccampanale]

I may be able to help with this; I've just spent all day with the Node debugger attached to a process in my production environment hunting down an invalid signature. Looking through the code, in most cases it should be as simple as just throwing an error instead of just returning false. In others - those which use xml-crypto to validate signatures - it may be as easy as check the return code then throwing sig.validationErrors (an array of error messages collected in xml-crypto when validating signatures) in a new error.

[markstos]

@ccampanale Just yesterday I realized a new version of passport-saml that dealt with this. Instead of returning "Invalid Signature" in 6 different places, each place now gets a unique error message. Does that meet your needs, o are their places where you want to pass along more detail as returned by the XML parser?

[ccampanale]

@markstos I didn't even realize that! Let me bump our package refs and take a look. I'll let you know. Thanks!

[autopulated]

👍 Sorry I never got around to making a PR 🙈

[ccampanale]

Bumped the version and tested. Still seeing our same issues but I'm pretty sure the issues are with our IDP. The new error messages are nice as they let you know where exactly in saml.js the issue is occurring but there are some errors from xml-crypto that could be relayed as well as I mentioned above. (Not necessarily parser level errors, though that may be helpful in some cases?) It's not a huge deal but I could see how it may save folks from needing to do a deep dive with the debugger as I have the past couple of days.

[markstos]

@ccampanale If you'd like to make a PR to pass along the detailed failures from xml-crypto, I'm up for merging that as well. XML parsing has historically had some security flaws, so if XML is parsing sometimes fails in strange ways, it could be useful to log details for forensics as well as to debug IdP connections.

[ccampanale]

Yeah, since I'm still fighting the good fight with my organization's IDP and can't get our production deployment to work I may very well do that. As you said, if nothing else it may save others a good deal of time troubleshooting strange failures and/or from a forensics collection perspective.

[markstos]

I just about pulled my hair out with a recent integration, only to eventually realize that the profile field mapping between them and us wasn't as expected. It's increasingly tempting to consider using Okta or OneLogin for their "Service Provider" offering.