Fix verification of SAML message signatures

[jeffparsons]

Status

?? READY/IN DEVELOPMENT/HOLD

Description

Current implementation of validate_signature incorrectly assumes it can reconstruct the relevant URI parts (RelayState, SAMLRequest, SigAlg) from the decoded values it receives. This contradicts the SAML specification, which requires computing the signature based on the URI components as sent by the Identity Provider.

The spec actually explicitly remarks that trying to reconstruct the URI parts is a bad idea because there is no single canonical URI encoding of a given value:

Further, note that URL-encoding is not canonical; that is, there are multiple legal encodings for a given value. The relying party MUST therefore perform the verification step using the original URL-encoded values it received on the query string. It is not sufficient to re-encode the parameters after they have been processed by software because the resulting encoding may not match the signer's encoding.

I've preserved the old behaviour in the case that the caller doesn't provide the the correct (raw URI-encoded) parts, for backward compatibility.

This could probably use a couple of tests, but I figured I'd better see if you're ok with the general approach first.

Todos

• Tests
• Documentation

Steps to Test or Reproduce

• Set up a Service Provider using ruby-saml, implementing IDP-initiated "Single Logout" with HTTP-Redirect binding.
• Register your app in Microsoft ADFS as a Relying Party.
• Try to log out from Microsoft's ADFS (IDP-initiated SLO).
• Observe that ruby-saml will erroneously consider the signature on the logout request to be incorrect.

[jeffparsons]

Those test failures on some configs appear to have nothing to do with the code changes.

[pitbulk]

Yes, it seems not related, we will need to apply the same at the validate_signature of the Logout Response.

As well as provide tests and documentation on the README.

[jeffparsons]

@pitbulk I've written some tests, and made the equivalent change for Logoutresponse.

I'm not sure what kind of change you'd like to the README; I can't see any existing documentation about using get_params, so it's not as straightforward as just updating what's already there. Can you give me some pointers on where you'd like the documentation added, and how much detail you'd like?

[pitbulk]

I think we may write an "Update from 1.5.0 to 1.6.0 version" and we ahould describe there that previously we used in LogoutRequest and LogoutResponse constructor the optioms [get_params] but now we recommend to use raw_get_params... and maybe and example of code of the new use.

[jeffparsons]

@pitbulk I've pushed some documentation in README.md. Does that look ok to you?

[jeffparsons]

Cheers. 😎

[jeffparsons]

Is there anything I can do to help move this (and #420) closer to landing?

[pitbulk]

Sorry for the delay, I will merge and do a new release today.

[jeffparsons]

No drama whatsoever. 😃 Cheers!