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.

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

Destination value MUST be present in Response if binding is HTTP-REDIRECT or HTTP-POST #812

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

peppelinux
Copy link
Member

@peppelinux peppelinux commented Jul 20, 2021

A general review of #782

Destination MAY be omitted, because it's optional, BUT if present it MUST be validated upon a match on valid ones.
Destination MUST be present if the SAML Binding is async (HTTP-POST or HTTP-REDIRECT).

closes #770

All Submissions:

  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what problem you are trying to solve with this PR?
  • Have you added information on what your changes do and why you chose this as your solution?
  • Have you written new tests for your changes?
  • Does your submission pass tests?
  • This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?

src/saml2/response.py Outdated Show resolved Hide resolved
src/saml2/response.py Outdated Show resolved Hide resolved
@peppelinux peppelinux reopened this Jul 22, 2021
@c00kiemon5ter
Copy link
Member

Destination MUST be present if the SAML Binding is async (HTTP-POST or HTTP-REDIRECT).

@peppelinux where is this defined in the specification?


As a reference, the core specification says

Destination [Optional]

A URI reference indicating the address to which this request has been sent. This is useful to prevent
malicious forwarding of requests to unintended recipients, a protection that is required by some
protocol bindings. If it is present, the actual recipient MUST check that the URI reference identifies
the location at which the message was received. If it does not, the request MUST be discarded.
Some protocol bindings may require the use of this attribute (see [SAMLBind]).

What I see is the following in the SAML-bindings:

3.4.5.2 Security Considerations

...
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 does not say that the Destination XML attribute MUST be there; at least not in a clear way.
Based on the core specification that defines the Destination XML attribute as optional, I would interpret this as:

  • if "the Destination XML attribute is present"
  • and "the message is signed"
  • then "the protocol message MUST contain the URL ..."

I would expect a clear requirement for the Destination XML attribute to always be present given those bindings.

Additionally, the "message is signed" phrase makes the check even less strict, as in the cases when the message is not signed the destination could be present and point anywhere. This contradicts what the core specification says, so I think the "message is signed" is probably emphasis and not an actual rule.


In practice, I expect the destination attribute to be present. I cannot guarantee though that it always will and at this point I cannot make it required as this is not clear to me from the specs. I will try to ask the SAML authors what the intention was and whether the attribute is required for those bindings. In the meantime, if you have seen this clearly defined in the specs, let me know.

@peppelinux
Copy link
Member Author

peppelinux commented Jul 27, 2021

Thank you @c00kiemon5ter
This requirement Is not clear to me too, I Just pushed this contribution as a placeholder to motivate a better clearness

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing Destination in Response
3 participants