-
Notifications
You must be signed in to change notification settings - Fork 430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement query signature verification #449
base: main
Are you sure you want to change the base?
Conversation
b7db51f
to
0002d15
Compare
add support for sha1 & sha512 add tests use query sign in redirect implement review feedback - Return error if signature is unsupported - wrap errors Co-authored-by: Ieva <ieva.vasiljeva@grafana.com> Co-authored-by: Orgad Shaneh <orgads@gmail.com>
0002d15
to
ea6eee2
Compare
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 |
I can confirm this does work properly.
|
} else { | ||
query += "SAMLRequest=" + url.QueryEscape(string(w.Bytes())) | ||
query += string(samlRequest) + "=" + url.QueryEscape(reqString) | ||
} | ||
|
||
if relayState != "" { | ||
query += "&RelayState=" + relayState |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
query += "&RelayState=" + relayState | |
query += "&RelayState=" + url.QueryEscape(relayState) |
@@ -1534,6 +1531,13 @@ func (sp *ServiceProvider) ValidateLogoutResponseRedirect(queryParameterData str | |||
return err | |||
} | |||
|
|||
if query.Get("Signature") != "" && query.Get("SigAlg") != "" { | |||
if err := sp.validateQuerySig(query); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Applied long-open PR crewjam#449 plus https://github.com/crewjam/saml/pull/449/files#r1654477177 to make embedded signature optional when a good detached signature is seen.
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
SigAlg, and SAMLRequest (or SAMLResponse) query string parameters (each one URL-
encoded) is constructed in one of the following ways (ordered as below):
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