search

SAML-Toolkits / python3-saml

MIT License
704 stars 309 forks source link

Documentation for request_data? #273

Open akx opened 3 years ago

akx commented 3 years ago

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 https://github.com/onelogin/python3-saml/commit/15a331e22cb1fc732f3f305f126dc07b5e9b3287 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 commented 3 years ago

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 commented 3 years ago

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...)