Certificate mismatch does not throw descriptive error

[DbCrWk]

A SAML response comes back with a signature and the certificate used to generate that signature. When invoking the library, we pass a cert in options that specifies the key that we expect to have been used in creating the signature.

If the certificate that comes back in the SAML response does not match the cert option that is passed, it would be nice to have separately error-handling that provides a descriptive error. Although the current behavior is not wrong (in that it throws an error because the signature is invalid), it would be nice to have a more descriptive error message in this particular case.

[DbCrWk]

Also this is a great library! Thanks for making it!

[mishawakerman]

Came to add the same issue. I am writing tests for the case where a rogue IdP in a multi-saml configuration tries to spoof a login response for a user associated with a different IdP. This gets correctly handled by passport-saml when the signatures don't match but it get's thrown as a 500 server error.

In our express app, I added the following to our global error handling:

app.use(function (err, req, res, next) {
    ... 
    if (req.originalUrl.startsWith(SAML_CALLBACK_URL) && err.message === "Invalid signature") {
        return res.status(HttpStatus.UNAUTHORIZED).send('Unauthorized');
    }
    ...

so the client sees this as 401 rather than 500 (as 500 errors trigger alerts for engineers to investigate the cause).

[mishawakerman]

What do maintainers think about a PR that replaces the Errors in SAML.prototype.validatePostResponse with SAMLValidationErrors so that in our error handling we can do something like:

if (err && err instanceof SAMLValidationError) {
    // handle error
}

[DbCrWk]

@markstos based on my understanding of the code, it seems like the signature validation functions return true or false depending on the validity of the signature. However, an actual error doesn't get thrown much later.

For reference, the error is thrown here:
https://github.com/bergie/passport-saml/blob/6f0876ed77815235d7d846ea145da733c3fa6b04/lib/passport-saml/saml.js#L629

which internally calls:
https://github.com/bergie/passport-saml/blob/6f0876ed77815235d7d846ea145da733c3fa6b04/lib/passport-saml/saml.js#L537

and the validation is performed here:
https://github.com/bergie/passport-saml/blob/6f0876ed77815235d7d846ea145da733c3fa6b04/lib/passport-saml/saml.js#L553

Are you okay with an error being thrown much deeper in the stack and being bubbled up, or did you want to keep it such that signature validation returns true or false?

@mishawakerman I'm not a maintainer, but I think throwing a SAMLValidationError is a reasonable approach. In my mind, there are two related, but distinct, issues:

• Checking the signing certificate in the response XML to see if it matches the expected certificate that was set in the options
• Ensuring the signature in the response XML matches our computed signature from the expected certificate that was set in the options

Throwing a SAMLValidationError in both cases and having some descriptive message (or subclassing) to distinguish these two cases would be nice.

Based on my understanding of the code, I think somewhere in this method would be a good place to put the error:
https://github.com/bergie/passport-saml/blob/6f0876ed77815235d7d846ea145da733c3fa6b04/lib/passport-saml/saml.js#L553

I think you can use the existing XML utilities to parse the signing certificate in the response body and add an initial check to ensure this matches the expected certificate passed in options (which is already in scope of the function as the cert parameter). If those don't match, an error could be thrown. Next, if the signatures don't match, a different error could be thrown here:
https://github.com/bergie/passport-saml/blob/6f0876ed77815235d7d846ea145da733c3fa6b04/lib/passport-saml/saml.js#L581

[markstos]

@DbCrWk I think your analysis and recommendation are spot-on. Throwing a custom error class is a great why to go. I think one error class with unique error messages will be sufficient. Would you be willing to submit a PR for this? Thanks.

[DbCrWk]

@markstos awesome, I can start working on that. I will be careful to avoid making it a breaking change, but if some internal interface changes, I will flag that explicitly. I don't know what your development priorities look like, but would this still be useful in, say, like a month or two?

[markstos]

No rush here. There is some talk of a big overhaul for a v2 (see pinned issue), but no work has started on that. It will be helpful to @ mention me in the pull request to bring it to my attention. Thanks.

[DbCrWk]

Awesome, will do!

[earllapura]

Any update on this?