Forum OpenACS Improvement Proposals (TIPs): TIP #75 (Approved) Add support for HTTPS proxy

Modify 3 procedures in OpenACS core:

  * (ad_restrict_to_https) in acs-tcl/tcl/admin-procs.tcl,
  * (ad_secure_conn_p) in acs-tcl/tcl/security-procs.tcl and
  * (util_current_location) in acs-tcl/tcl/utilities-procs.tcl

to accept proxied HTTPS requests forwarded by a proxy such as Pound,
Squid or Apache as regular HTTPS requests.

Most proxies forward incoming HTTPS requests as HTTP requests to the
backend. At least Pound does. Forwarded HTTPS requests are marked by
an additional HTTP header `X-SSL-Request: true'. Pound can be
configured to remove any X-SSL-Request headers part of the incoming
HTTP(s) request to the proxy itself (Pound) to avoid `Man in the
Middle' attacks.

Proposed modification adds checks to aforementioned procedures to
treat HTTP requests (as received by the OpenACS instance) on par with
HTTPS requests.

I have made these modifications a long time ago to ACS 4 and they have
been tested in production. In the past 2 days 2 community members
contacted me with requesting these patches. I think that is enough of
a basis to propose inclusion to OpenACS core.

For added security one could also add the IP address of the proxy as a
new parameter to OpenACS. One can then check the `X-Forwarded-For'
header in addition to the `X-SSL-Request' header. Only if the
X-Forwarded-For IP address matches the configured IP address will the
connection be treated as secure. However, this involves a more
substantial change and requires upgrade scripts to the data model.

Collapse
Posted by Jade Rubick on
I approve.
Collapse
Posted by Jeff Davis on
I approve.
Collapse
Posted by Dave Bauer on
I approve
Collapse
Posted by Rocael Hernández Rizzardini on
approved
Collapse
Posted by Andrew Grumet on
My only concern is that if you are not running a Pound proxy, you are not protected against bogus headers. That feels wrong, though I can't immediately think of a reason why someone would want to fake these.

How about a parameter for enabling or disabling the header checking? It should be off by default.

However, this involves a more substantial change and
requires upgrade scripts to the data model.

Are we talking about anything more than one new parameter (it could be a space-separated list)? Adding a new parameter is a simple change to the package info file, no upgrade scripts are needed.

Collapse
Posted by Andrew Grumet on
Sorry, in the first sentence above, that should be, "if you are not running a Pound proxy".
Collapse
Posted by Don Baccus on
Andrew ... I think you still have to ask the APM to run the upgrade even though it handles parameter changes itself without an explicit upgrade script, right? (just trying to clarify)

I think this feature should off by default, like Andrew, is there any reason it shouldn't be? Anyone smart enough to set up Pound can figure out how to set an OpenACS parameter.

And even if not strictly necessary, won't the ability to add a fixed IP (or list of IPs) from which such requests will be honored lead to better peace of mind for admins?

Obviously I approve of adding this feature just want to make sure we get it right (even if that means you guys prove me wrong).

Collapse
Posted by Bart Teeuwisse on
Andrew, you raised a good point. Which is easily addressed by checking the `ProxyIPs' parameter (let's call it that).

Specifically, in light of your concern, I propose to add a `ProxyIPs' parameter whose initial value is `null'. The `null' value disables checking for `X-SSL-Request' headers and thus avoids spoofing. Setting the `ProxyIPs' parameter -which should be a space separated list- enables acceptance of `X-SSL-Request' headers.

On a related note, AFAIK AOLserver will always log the `X-Forwarded-For' IP address, whether the `ProxyIPs' parameter has been set or not. There is little harm in this other than that it makes it harder to track mallicious visitors.

/Bart

Collapse
Posted by Don Baccus on
Bart ... sounds like a good plan to me.

Approve.

Collapse
Posted by Malte Sussdorff on
With the parameter included: APPROVED