On Tue, Apr 27, 2021 at 04:34:18PM -0400, Dave Voutila wrote: > > Stuart Henderson writes: > > > On 2021/04/27 10:40, Vincent Lee wrote: > >> > >> Hi all, > >> > >> Consider the following situation. A reverse proxy which performs TLS > >> termination is deployed in front of httpd, which listens unencrypted on > >> localhost. > >> > >> There is code in httpd to handle the case where a directory is accessed, > >> but the path named does not end with a slash. In this case, httpd > >> issues a 301 redirect to the path with a slash appended. > >> The logic here sets the protocol of the redirect path based on > >> whether the httpd virtual server is configured with TLS, but this isn't > >> enough because it will cause redirects to plain http when there is a > >> reverse proxy in front of httpd that performs TLS termination. > >> This will either cause an extra redirect round trip to get back to HTTPS, > >> or break the site if it's not publicly served on plain HTTP. > >> > >> Instead, we should be reading X-Forwarded-Proto, which most reverse proxies > >> add to inform the backing server what the original protocol was. > >> (relayd doesn't expose this to my knowledge, but I will look into doing so) > >> > >> The below attached diff does this for httpd. This is my first diff to the > >> project, so please give feedback on whether I've done everything right. > > > > How does this work with other web servers? For example, I don't see the > > string X-Forwarded-Proto in nginx or Apache httpd (and the use of other > > X-Forwarded headers in them are only for adding to requests when running > > as a proxy itself, or picking up the client IP from headers rather than > > TCP). > > I concur in what you're seeing in other codebases. > > On the proxy side, what I'm finding is any support for X-Forwarded-Proto > is runtime config and effectively manual intervention by whomever is > configuring the proxy. > > For Nginx, it seems they have an "autoindex" feature, but don't account > for X-Forwarded-Proto like in the proposed diff. > > What nginx *does* support is disabling absolute redirects changing the > way the Location header gets constructed in the redirect. (See [1].) > Maybe that makes more sense as a feature in httpd(8)? It should achieve > the correct end result. (Maybe it's already possible? Need to check.) > > -dv > > [1] https://nginx.org/en/docs/http/ngx_http_core_module.html#absolute_redirect
Another thing to consider is that X-Forwarded headers should only be accepted from trusted sources. I don't think this particular usage of X-Forwarded-Proto is probelmatic. In the end for this particular case of redirect using a relative URL seems to be a better choice since it will preserve the host, port and proto from the original requests without any configuration magic. -- :wq Claudio
