Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Response Destination Validation - Query Strings #525

Open
dlpetrie opened this issue Jun 22, 2023 · 1 comment · May be fixed by #577
Open

Response Destination Validation - Query Strings #525

dlpetrie opened this issue Jun 22, 2023 · 1 comment · May be fixed by #577

Comments

@dlpetrie
Copy link

We are porting an old SAML implementation from PHP over to Go, and so far this library has worked great. I have reused the middleware logic and mixed with our own to satisfy the multi-tenant setup we have.

The issue I'm running into now is our old setup used a few query strings in the ACS URL Location, and we need to maintain that for compatibility. With the library and go, unfortunately, it organizes the query string in alphabetical order and looks for an exact match URL with query strings, and if not matching, it fails. So even if the URL is the same, but the query string appear in a different order, the destination validation fails.

Would you be open to a PR that either:

  1. Removes the query string as part of the ACS Location / Destination validation and ignores the query string
  2. Removes the query string and verifies the rest of the url. Then additionally validates the query string, regardless of order

saml/service_provider.go

Lines 869 to 873 in 34930b2

if responseHasSignature || response.Destination != "" {
if response.Destination != sp.AcsURL.String() {
return nil, fmt.Errorf("`Destination` does not match AcsURL (expected %q, actual %q)", sp.AcsURL.String(), response.Destination)
}
}

@dlpetrie dlpetrie changed the title Response Destination Validation Response Destination Validation - Query Strings Jun 22, 2023
@studouglas
Copy link

studouglas commented Oct 4, 2024 •
edited
Loading

This is affecting us as well. The proper solution here would be to compare response.Destination against the actual URL the request was received at instead of the ACS URL.

This would correctly implement Section "3.4.5.2, Security Considerations" of the SAML spec:

If the message is signed, the Destination XML attribute in the root SAML element of the protocol
message MUST contain the URL to which the sender has instructed the user agent to deliver the
message. The recipient MUST then verify that the value matches the location at which the message has
been received.

This is how SAML-toolkits/java-saml implements the check here.

This can be done by adding a new param for the actual URL to ParseXMLResponse, based on the request object in-scope at the call-site in parseResponseHTTP.

@studouglas studouglas linked a pull request Oct 7, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: check Destination url against the request URL or the ACS url studouglas/saml
2 participants
@dlpetrie @studouglas