Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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

Open
peppelinux wants to merge 3 commits into IdentityPython:master
base: master
Choose a base branch
Loading
from peppelinux:destination

Conversation

@peppelinux
Copy link
Member

@peppelinux peppelinux commented Jul 20, 2021
edited by c00kiemon5ter
Loading

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?

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.

Copy link
Member Author

peppelinux commented Jul 27, 2021
edited
Loading

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

c00kiemon5ter reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@spaceone spaceone spaceone left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Missing Destination in Response

AltStyle によって変換されたページ (->オリジナル) /