[Django] #33569: Add support for multiple values for the x-forwarded-proto header
Django
------------------------------------------------+------------------------
Reporter: heyts | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
When Django is deployed behind more than one proxy, the proxy behavior is
sometimes to list the protocol as a comma-separated list.
However, currently, Django expects only one value for the `x-forwarded-
proto` header, instead of parsing it as a list of values and setting the
protocol accordingly.
`x-forwarded-proto` is a non-standard header, so there isn't a
specification for its use, but different reverse-proxy vendors do use it
in different ways, and some append the protocol as a comma-separated value
from left-to-right (left being the furthermost proxy and rightmost being
the closest).
Similar issues have been raised and implemented in other projects, for
example:
Tornado:
* Issue: https://github.com/tornadoweb/tornado/issues/2161
* Implementation:
https://github.com/tornadoweb/tornado/blob/00c9e0ae31a5a0d12e09109fb77ffe391bfe1131/tornado/httpserver.py#L347-L350
Ruby:
* Issue: https://bugs.ruby-lang.org/issues/10789
* Implemenation:
https://github.com/ruby/ruby/blob/d92f09a5eea009fa28cd046e9d0eb698e3d94c5c/tool/lib/webrick/httprequest.rb#L614-L616
Reactor-Netty:
* https://github.com/reactor/reactor-netty/issues/976
* Implementation: https://github.com/reactor/reactor-
netty/commit/e190d5bbf65d88d3a0240cd60b81e1ee1907030e
Most implementation use the leftmost-value or rightmost value. I would
expect that provided that you are certain that the initial proxy can be
trusted, that the left-most value makes the most sense, since it represent
the original value at the entry-point for the HTTP request which is often
where TLS is being terminated.
Common example of this behavior is when using mulitple AWS proxies such as
API Gateway proxying to an elastic load balancer.
--
Ticket URL: <https://code.djangoproject.com/ticket/33569>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Thomas Schmidt | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Thomas Schmidt):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Claude Paroz):
[https://github.com/django/django/pull/15488 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Schmidt
Cleanup/optimization | Status: assigned
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Thomas Schmidt
* needs_better_patch: 0 => 1
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: Thomas
Type: | Schmidt
Component: HTTP handling | Version: dev
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* status: assigned => closed
* resolution: => wontfix
Comment:
As far as I'm aware, we cannot answer in the reliable way whether the
request was over HTTPS, when `X-Forwarded-Proto` contains multiple values.
Unfortunately it depends on which layer is trusted. We could check if all
values are "https", but this could be unacceptable with in-companies
proxies. It's definitely not enough to check if "https" is used somewhere
in the pipeline.
As a workaround you can set list of protocols in the
`SECURE_PROXY_SSL_HEADER`, e.g. `SECURE_PROXY_SSL_HEADER =
("HTTP_X_FORWARDED_PROTO", "https,http")`. I don't think it's worth
additional complexity. You can start a discussion on DevelopersMailingList
if you don't agree.
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: Thomas
Type: | Schmidt
Cleanup/optimization | Status: closed
Component: HTTP handling | Version: dev
Severity: Normal | Resolution: wontfix
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Florian Apolloner):
> As far as I'm aware, we cannot answer in the reliable way whether the
request was over HTTPS, when `X-Forwarded-Proto` contains multiple values.
Unfortunately it depends on which layer is trusted. We could check if all
values are "https", but this could be unacceptable with in-companies
proxies. It's definitely not enough to check if "https" is used somewhere
in the pipeline.
I do not think this is correct. First of all we have to make one decision
-- namely do we want to trust `X-Forwarded-Proto` at all. By setting
`SECURE_PROXY_SSL_HEADER` we do explicitly say that we trust the value to
be correct (see the big warning in
https://docs.djangoproject.com/en/4.0/ref/settings/).
Once we trust that header it is not required to check whether all items in
the list are `https` but solely whether the left one is `https`. Whether
the middle-items are `http` is irrelevant. Why? The goal of this header is
to check if the client connection to the first proxy was HTTPS, nothing
more nothing less. It is not an indicator on whether the connection is
secure but solely an indicator whether generated links etc should have
https:// or http://.
For example even if the header is set to the simple value of `https` there
is no gurantee that the connection from the proxy to the backend was https
(and more often than not it is simply http).
> As a workaround you can set list of protocols in the
`SECURE_PROXY_SSL_HEADER`, e.g. `SECURE_PROXY_SSL_HEADER =
("HTTP_X_FORWARDED_PROTO", "https,http")`.
I am not sure I would recommend that, this is highly implementation
dependent and assumes that the proxy doesn't start sneaking in a space at
some point. That said the worst case scenario here is then that the
request is determined as unsafe which is probably not the end of the
world… On the other hand this assumes that you actually know the number of
proxies involved as opposed to just knowing that you can trust the header
because the proxies to set it properly. Which might be hard in cloud
environments.
> I don't think it's worth additional complexity.
I would argue that the added complexity is relatively small; the biggest
task here is to update the docs to explain it nicely.
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: Thomas
Type: | Schmidt
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: closed => new
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* resolution: wontfix =>
* stage: Unreviewed => Accepted
Comment:
OK, we can prefer leftmost value, if that's enough.
[https://docs.djangoproject.com/en/4.0/ref/settings/#std:setting-
SECURE_PROXY_SSL_HEADER SECURE_PROXY_SSL_HEADER] and
[https://docs.djangoproject.com/en/4.0/ref/request-
response/#django.http.HttpRequest.is_secure HttpRequest.is_secure()] docs
must be updated.
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: Thomas
Type: | Schmidt
Cleanup/optimization | Status: new
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hmmm. I think adding complexity here (at all) will come back and bite us.
> Which might be hard in cloud environments.
The issue is that there's no easy "I only use HTTPS switch", which is most
deployments these days. That makes folks rely on
`SECURE_PROXY_SSL_HEADER`, which gets ever more tricky as deployments get
ever more baroque. Such a switch would solve most use-cases, I submit :)
Short of that, the lightest WSGI middleware setting `wsgi.url_scheme` in
the `environ` would be preferable to getting into parsing
`SECURE_PROXY_SSL_HEADER` (for me).
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: Thomas
Type: | Schmidt
Cleanup/optimization | Status: new
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Thomas Schmidt):
I can re-open the closed PR and update it to take the comments made here
into account, so we can discuss it.
Summary:
* leftmost value determine the protocol if multiple, comma-separated
values are present
* https://docs.djangoproject.com/en/4.0/ref/settings/#std:setting-
SECURE_PROXY_SSL_HEADER needs to be updated to document the change
* https://docs.djangoproject.com/en/4.0/ref/request-
response/#django.http.HttpRequest.is_secure may also need to be updated,
although I'm not certain it is necessary?
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: Thomas
Type: | Schmidt
Cleanup/optimization | Status: new
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/15497 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: Thomas
Type: | Schmidt
Component: HTTP handling | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Mariusz Felisiak):
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Thomas Schmidt | Owner: Thomas
Type: | Schmidt
Component: HTTP handling | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1cf60ce6017d904024ee132f7edae0b4b821a954" 1cf60ce]:
{{{
#!CommitTicketReference repository=""
revision="1cf60ce6017d904024ee132f7edae0b4b821a954"
Fixed #33569 -- Added SECURE_PROXY_SSL_HEADER support for list of
protocols in the header value.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/33569#comment:11>