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

Documentation for request_data? #273

Open
akx opened this issue Jul 20, 2021 · 2 comments
Open

Documentation for request_data? #273

akx opened this issue Jul 20, 2021 · 2 comments

Comments

@akx
Copy link
Collaborator

akx commented Jul 20, 2021

Are the fields for the request_data dictionary documented anywhere? I was just bit by the fact that there's apparently an 'https' field you need to set to the string 'on' so using HTTPS works at all when a Destination is set in a SAML response document from an IdP.

This was "fixed for demos" in 15a331e but doesn't seem otherwise documented.

The validation chain for Destinations is:

  1. https://github.com/onelogin/python3-saml/blob/174ecfaf6f69c0ac13087b8233c293072b271742/src/onelogin/saml2/response.py#L191-L194
  2. where current_url is https://github.com/onelogin/python3-saml/blob/174ecfaf6f69c0ac13087b8233c293072b271742/src/onelogin/saml2/response.py#L122
  3. which calls https://github.com/onelogin/python3-saml/blob/174ecfaf6f69c0ac13087b8233c293072b271742/src/onelogin/saml2/utils.py#L329
  4. which calls https://github.com/onelogin/python3-saml/blob/174ecfaf6f69c0ac13087b8233c293072b271742/src/onelogin/saml2/utils.py#L258
  5. which calls https://github.com/onelogin/python3-saml/blob/174ecfaf6f69c0ac13087b8233c293072b271742/src/onelogin/saml2/utils.py#L303-L316 which checks for the https key, or alternatively whether the port is 443.
@pitbulk
Copy link
Contributor

pitbulk commented Jul 22, 2021

The request_data is not officially documented.

Each demo defines how request_data should look like: Ex. Django

Feel free to contribute with such documentation

@akx
Copy link
Collaborator Author

akx commented Jul 23, 2021

Right – the Django example, for one, is hopelessly incorrect in the common case where you have an application server running on e.g. port 8000, and reverse proxied & TLS terminated by e.g. Nginx at port 443 (let's say the user visits https://example.app).

In that case HTTP_HOST will be example.app, but SERVER_PORT is 8000. get_self_url_host will happily reconstruct a host https://example.app:8000, which is, of course, incorrect.

In cases where the app is accessed on a non-standard port (not 80 for HTTP or 443 for HTTPS), HTTP_HOST will already include the port number. (E.g. the Django dev server does 'HTTP_HOST': '127.0.0.1:8000'.

I think server_port is utterly unnecessary in the dict and should be just dropped. (That's maybe out of the scope for this issue, now that I think of it...)

EDIT: On the topic of ports, by the way, the port-splitting-and-checking-and-reinstating logic in get_self_host seems to be unequipped to deal with IPv6 hostnames (which can, in URLs, be of the form http://[FEDC:BA98:7654:3210:FEDC:BA98:7654:3210]:3251/) and also turns example.com:0x300 happily into example.com:768...)

akx added a commit to akx/python3-saml that referenced this issue Jul 23, 2021
`server_port` is unnecessary, since the HTTP Host header sent by the client
already includes any non-standard port.  In addition, when the Python
application server is sitting behind a reverse proxy/TLS terminator,
SERVER_PORT is likely to be wrong anyway (since it would be the server port
of the non-reverse-proxied server).

See SAML-Toolkits#273 (comment)
akx added a commit to akx/python3-saml that referenced this issue Jul 25, 2021
`server_port` is unnecessary, since the HTTP Host header sent by the client
already includes any non-standard port.  In addition, when the Python
application server is sitting behind a reverse proxy/TLS terminator,
SERVER_PORT is likely to be wrong anyway (since it would be the server port
of the non-reverse-proxied server).

See SAML-Toolkits#273 (comment)
akx added a commit to akx/python3-saml that referenced this issue Jul 25, 2021
`server_port` is unnecessary, since the HTTP Host header sent by the client
already includes any non-standard port.  In addition, when the Python
application server is sitting behind a reverse proxy/TLS terminator,
SERVER_PORT is likely to be wrong anyway (since it would be the server port
of the non-reverse-proxied server).

See SAML-Toolkits#273 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants