Fixes #180: Signature validation will error if empty signature is pro…

[andrsnn]

…vided

[andrsnn]

There might be some discussion as to whether or not the cert property should be required. This pull request seems to address the main concern, without enforcing a breaking api change.

[markstos]

I agree about the problem, but not about the solution.

It could be helpful to validate that cert is not an empty string.

However this issue is a programmer error that could be caught earlier. So:

1. The error handling should be in SAML.prototype.intialize, where cert is set. It could throw immediately if cert is an empty string.
2. It's wrong to declare this an "Invalid Signature", because that's not the issue. A more helpful error message would be "cert: private key must not be empty".

[andrsnn]

Definitely makes sense, and thank you for the feedback. As error handling in the initialize / constructor function would be a first, I would argue throwing an error would be a breaking API change and would likely merit a major version increase. The above PR should be backwards compatible, which is why the validation occurs in the validatePostResponse and validatePostRequest.

I would also prefer an error message along the lines of "cert: private key must not be empty". For the above PR, I was assuming that some may be incorrectly passing this error directly back to the client passport.authenticate('saml', next), which might result in a security issue. Missing certificate might be better messaging without revealing too much.

Any thoughts to the above? I'd be glad to make the suggested changes if you do think they merit a major version bump.

[markstos]

I would argue throwing an error would be a breaking API change

In this case, it seems more like we would be fixing a "Garbage-In Garbage Out" bug. In what context would setting cert to an empty string even make sense? However, I was thinking of bumping the version to 1.0 very soon anyway.

Your refinements to the error message seem fine. For context, until recently we had 4 to 6 places in the code that returned the same "Invalid Signature" error, which was maddening to debug. Now all those places return unique error messages instead.

I don't want to optimize for developers are passing programmer-errors back to users. In my view, code that sets "cert" to empty string without validating it should not making to production.

Even if a developer is getting the 'cert' value dynamically, they have a responsibility to validate it and not blindly assuming it's OK.

[andrsnn]

Got it. Sounds good I will move the validation to the initialize method. 👍

[cjbarth]

@andrsnn I see this is the last outstanding PR that is tagged for our 1.0 release. Do you think you'll be able to clean it up so it can make that release?

[andrsnn]

@cjbarth Thank you for the reminder! I will look at wrapping up these changes this coming weekend.

[andrsnn]

@markstos @cjbarth Just pushed the above changes here. The above should also support an unsigned SAML Response with an unsigned Assertion, no signatures so no public key necessary. Any issues or comments let me know and I will address thanks!

[markstos]

This has been merged locally: https://github.com/bergie/passport-saml/commits/master So, closing this issue. Thanks for the contribution.