Implement query signature verification

[Jguer]

Hey,

This patch implements:

• Query signature validation for SLO redirect bindings.

• Query signing was also slightly refactored to fix the query getting fully signed instead of only the expected req+relay+alg format (also made it easier to implement tests for it)

Section 3.4.4.1 Oasis

2. To construct the signature, a string consisting of the concatenation of the RelayState (if present),
   SigAlg, and SAMLRequest (or SAMLResponse) query string parameters (each one URL-
   encoded) is constructed in one of the following ways (ordered as below):

SAMLRequest=value&RelayState=value&SigAlg=value
SAMLResponse=value&RelayState=value&SigAlg=value

3. The resulting string of bytes is the octet string to be fed into the signature algorithm. Any other
   content in the original query string is not included and not signed.

---

It can probably be adapted for validations in other scenarios.

Please advise on style, structure or other modifications that would help the project

[Jguer]

Hey, @crewjam any blocker into having this merged? I have some time to fix this up if needed

We've been running it in production for a few months now

[omerkarj]

I can confirm this does work properly.
Also passes verification with this tool (which master does not):

• sign a login request with the library and validate here: https://www.samltool.com/validate_authn_req.php
• generate a signed response and have it validated with the library: https://www.samltool.com/sign_response.php

[omerkarj]

Suggested change

	query += "&RelayState=" + relayState
	query += "&RelayState=" + url.QueryEscape(relayState)

[andreas-kupries]

I see here that the PR validates a signature coming via query elements.

However, in line 1547 the code still calls sp.validateSignature(doc.Root()), which
expects a signature element in the XML document, and wants to verify it.

So, this PR seems to expect a signature in two places, required in the XML document, and optional in the query.

With keycloak I have a signature in the query element, and nothing in the XML document.
I expect this to fail.

I am missing something ?

[andreas-kupries]

Added a querySig flag for myself, recording when a proper query sig is seen.
Check this flag if xml sign errors out with not present to avoid passing on that error

IOW

	querySig := false
	if query.Get("Signature") != "" && query.Get("SigAlg") != "" {
		if err := sp.validateQuerySig(query); err != nil {
			retErr.PrivateErr = err
			return retErr
		}
		querySig = true
	}

and

	if err := sp.validateSignature(doc.Root()); err != nil {
		if err != errSignatureElementNotPresent || !querySig {
			retErr.PrivateErr = err
			return retErr
		}
 	}

[andreas-kupries]

Tested the modification against Keycloak and works. As expected it verifies the detached sig, and then ignores the missing embedded sig.

Also extended further to generate a proper detached sig for logout requests. That solved issues with OKTA.

References:

• rancher@e1e76fd
• rancher@a10be19

[gsaandy]

@andreas-kupries - have you verified both IDP initiated and SP initiated single logouts? I cannot see any reference to handle LogoutRequest?

[andreas-kupries]

Hi. According to our QA (First two points at rancher/rancher#38494 (comment)) both IDP initiated (logout of Okta) and SP initiated (logout of Rancher) were tested.

[gsaandy]

Nice, I was thinking is there a way we can bring the tested fixes to this repo?
cc @crewjam

[gsaandy]

I believe the following signature verification checking is not needed for HTTP-Redirect binding SAMLResponse as it is already done in the previous step by this PR, looks like ValidateLogoutResponseRequest handle the HTTP-Redirect binding and ValidateLogoutResponseForm handles HTTP-POST Binding

if err := sp.validateSignature(doc.Root()); err != nil {

[crewjam]

LGTM generally. I want to take one more quick pass through and then will merge. Thank you all. :)

[gsaandy]

LGTM generally. I want to take one more quick pass through and then will merge. Thank you all. :)

Thanks @crewjam, really appreciated it. Looks like there are conflicts with this PR. I can help to resolve the conflicts if that speed things up, but looks like I cannot at the moment due to permission.
cc @andreas-kupries @Jguer

[gsaandy]

LGTM generally. I want to take one more quick pass through and then will merge. Thank you all. :)

Thanks @crewjam, really appreciated it. Looks like there are conflicts with this PR. I can help to resolve the conflicts if that speed things up, but looks like I cannot at the moment due to permission. cc @andreas-kupries @Jguer

I've resolved this PR conflicts in this new PR #621, which also fixes the Single Logout with ADFS.
Please review the changes @crewjam @andreas-kupries @Jguer @omerkarj

[gsaandy]

@crewjam - Do we have any updates here?