Pass options with :get_params to signature validation

[pkarman]

The RubySaml lib supports signature validation via GET params as well as embedded within the XML document itself. This PR adds similar support, especially for Logout requests, where signature is necessary to prevent cross-site attacks.

[jphenow]

This seems slightly unnecessary as a separate method, or at least unnecessary as a public method

[pkarman]

Good point. Marked as private in 381640e

I made it a method to avoid having a really long line with mixed booleans.

[jphenow]

It seems we took a relatively single-entry interface and just added "options" - is there a way we could solve this without passing ambiguous options all the way down and changing the signature of a string of methods?

For instance, perhaps we accept options at the top, convert the inputs into something relatively consistent (perhaps even put a class in between to make things easier to access consistently) then return to business as usual

[pkarman]

I agree that it would have been preferable to not change so many method signatures. The dilemma is that the class that really requires a change is XMLSecurity -- ironically, a fork of the same class that ruby-saml also forked.

XMLSecurity is used only internally, mostly for validating signatures. In order to pass the signature pieces down to that level, I found it necessary to change all the methods that lead down to it.

I followed the pattern in ruby-saml:
https://github.com/onelogin/ruby-saml/blob/master/lib/onelogin/ruby-saml/logoutresponse.rb#L32
https://github.com/onelogin/ruby-saml/blob/master/lib/onelogin/ruby-saml/logoutresponse.rb#L204

[pkarman]

@jphenow I'd be happy to try a different approach for this; I'm just honestly not sure how. The architecture of the lib puts all the signature checking in an internal-only class. The ruby-saml lib architecture puts the signature checking in an external Utils class. Maybe that approach works better here?

[pkarman]

There's a related discussion here: SAML-Toolkits/python-saml#153 (comment)

Note that when using DEFLATE in a SAMLRequest (as SamlIdp.from_deflated_request assumes), then the request must pass the signature in the query parameters. So it makes sense to me that the SamlIdp.from_deflated_request method accept query parameters .

[jphenow]

request.GET

[jphenow]

Thinking on this one.. I have some ideas, I may pull the feature you're looking for and use it as an opportunity for a minor restructure. I'll come back with something soon

[jnardone]

@jphenow are you still thinking on this one? Or is this something that is going to see the light of day?

[jphenow]

I'm not actively overseeing this project at the moment and would rather not step on those that are now caring for it.

@mvastola how do you feel about these changes?

[pkarman]

assuming 18F fork is the relevant one now.