1261538 - Bugzilla is unable to access attachment.cgi when ssl_redirect = true and using Plack

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Frédéric Buclin]

I have configured Apache to use Plack like this:

<Location /bugzilla/>
    ProxyPass "http://localhost:5000/"
    ProxyPassReverse "http://localhost:5000/"
</Location>

But when I try to access attachment.cgi, Bugzilla enters an infinite loop because attachment.cgi calls do_ssl_redirect_if_required() which itself calls attachment.cgi again, etc.... The reason is that ssl_redirect forces to use HTTPS, but Plack is configured to use HTTP only. I'm not even sure we can configure Plack to use SSL.

Comment 1

[Frédéric Buclin]

Another problem is that Bugzilla is unable to check the (attachment) host correctly, e.g. in url_is_attachment_base(). When it compares $cgi->url against the 'attachment_base' parameter, this check will always return false.

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

OK, I finally managed to address all bugs related to using Plack behind a reverse proxy. First of all, the configuration in httpd.conf should be:

<Location /bugzilla/>
    ProxyPreserveHost On
    ProxyPass "http://localhost:5000/"
    ProxyPassReverse "http://localhost:5000/"
    # Will pass either 'https' or 'http' (without quotes).
    RequestHeader set X-Forwarded-Proto %{REQUEST_SCHEME}s
    # Will pass the original path to the script, required to determine
    # if we are on the attachment host.
    RequestHeader set X-Forwarded-URI %{REQUEST_URI}s
</Location>

Then the patch addresses the following issues:
- If ssl_redirect = true, Bugzilla entered an infinite loop when trying to access attachment.cgi, because it was unable to determine the original protocol (HTTPS);
- Even with the first issue addresses thanks to X-Forwarded-Proto (RFC 7239), Bugzilla was unable to determine the original URL, and so entered an infinite loop again when trying to display attachments in "View" mode, because the URL never matched the attachmentbase URL.
- The ssl_redirect parameter was ignored outside attachment.cgi, because the redirection was done in Bugzilla::init_page(), which is before the CGI script is loaded by Plack, and so i_am_cgi() was always false (which means no redirection). Moving the redirection in Bugzilla::CGI->new() fixes this issue. This move is fine, because either the script is not CGI (i_am_cgi() = false) in which case the redirection was already ignored, or the script is CGI (i_am_cgi() = true) in which case Bugzilla->cgi is always called to access parameters and to set headers.

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

(this time, correctly formatted)

Comment 4

[Frédéric Buclin]

Attachment: patch, v1.1

Better patch. Only look at X-* headers if we are behind a proxy, else ignore them.

Comment 5

[Dylan Hardison [:dylan] (he/him)]

Comment on attachment 8739116 [details] [diff] [review]
patch, v1.1

Review of attachment 8739116 [details] [diff] [review]:
-----------------------------------------------------------------

r=dylan this works, although I have some reservations -- see below.

::: Bugzilla/CGI.pm
@@ +539,5 @@
> +    my $url = $self->url;
> +
> +    # If we are behind a reverse proxy, we need to determine the original
> +    # URL, else the comparison with the attachment_base URL will fail.
> +    if (Bugzilla->params->{'inbound_proxies'}) {

Do you think we should also compare the connection's IP against the inbound proxy setting, to protect against misconfiguration?

Actually, more broadly, perhaps if inbound proxies are configured we should fail to serve content to other ip addresses?

Comment 6

[Frédéric Buclin]

To ssh://[e-mail]/bugzilla/bugzilla.git
   60299ec..fa360be  master -> master


(In reply to Dylan William Hardison [:dylan] from comment #5)
> Actually, more broadly, perhaps if inbound proxies are configured we should
> fail to serve content to other ip addresses?

Feel free to file a separate bug, as enhancement ;). But if you are between the proxy and Bugzilla, this check would fail. So I'm not sure it's a good idea to throw an error in that case.