Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add SSL proxy support #208

Closed
wants to merge 1 commit into from

Conversation

herbrechtsmeier
Copy link
Contributor

Add support for a SSL proxy that handles multiple domains via host name
as prefix in the request URI (https://ssl-proxy.tld/domain.tld/).

As the SSL proxy is transparent for the web server the REQUEST_URI and
SCRIPT_NAME need manual adjustments. This patch replace the direct use
of this _SERVER variables with function calls and extend this functions
to detect the proxy and to add the needed prefix. Additionally it adds
a Sabre request backend with extends the Sabre_HTTP_Request to use the
same functions.

http://bugs.owncloud.org/thebuggenie/owncloud/issues/oc-890

Add support for a SSL proxy that handles multiple domains via host name
as prefix in the request URI (https://ssl-proxy.tld/domain.tld/).

As the SSL proxy is transparent for the web server the REQUEST_URI and
SCRIPT_NAME need manual adjustments. This patch replace the direct use
of this _SERVER variables with function calls and extend this functions
to detect the proxy and to add the needed prefix. Additionally it adds
a Sabre request backend with extends the Sabre_HTTP_Request to use the
same functions.
@LukasReschke
Copy link
Member

@bartv2 This is related to #9 where you said you would like to get a ping :-)

@danimo
Copy link
Contributor

danimo commented Nov 1, 2012

While I applaud the enthusiasm of making this possible, I wonder: what could be a legitimate (i.e. secure) use case for this? All I can think of is that such proxies blatantly bypass the same origin policy, plus the traffic TO the such proxy sites is still unencrypted. So in this case there is little security to gain. You are trading a fuzzy warm feeling of having your wifi-end SSL-secured against same-origin attacks and the strange gut feeling that the other half of the connection is still not encrypted at all.

@danimo
Copy link
Contributor

danimo commented Nov 1, 2012

Ah, some ISPs seem to offer this, that's kind of legit, if you trust that their backbone is and always will be fully switched (yeah, right...)

@libasys
Copy link
Contributor

libasys commented Nov 8, 2012

I am more a fan to check the Servervariable $_SERVER['HTTP_X_FORWARDED_SERVER'], so you can make more automatically to check for ssl-proxy! So you need no more config values!

@karlitschek
Copy link
Contributor

Exactly. We should check and use the $_SERVER['HTTP_X_FORWARDED_SERVER'] variable as we already do with HTTP_X_FORWARDED_PROTO and HTTP_X_FORWARDED_HOST

@herbrechtsmeier
Copy link
Contributor Author

How do you want to distinguish between transparent proxy (https://ssl-domain.tld) and multiple domain proxy ((https://ssl-proxy.tld/domain.tld/) without a variable? Of course I can replace the prefix variable with a Boolean variable and generate the URL with the HTTP_X_FORWARDED_HOST and the HTTP_HOST.

But how do you want to generate the HTTPS URL without knowing the ssl-proxy host in checkSSL function during http access (forcesll=true)?

An additional problem is that HTTP_X_FORWARDED_PROTO is not available on my webhoster ssl-proxy and I use the ssl-proxy host variable to overcome this.

@crti
Copy link

crti commented Nov 9, 2012

I strongly welcome the SSL proxy support efforts. When a solution suitable to most user cases is not possible for the core, the integration in an app or the use of provider configuration files should be tried.
See also OC Forum: http://forum.owncloud.org/viewtopic.php?f=4&t=3079

@karlitschek
Copy link
Contributor

O.K. I will look into that issue

@LukasReschke
Copy link
Member

Sorry, I'm against this and fully support what @danimo said.
SSL proxies gives a wrong feeling of security, so -1.

@karlitschek
Copy link
Contributor

SSL terminating Proxys or Load Balancers are very common pieces in serious HA cluster setups. It´s even required if you want to use a sticky connection setup which you have to do with ownCloud because we don´t have a cluster enabled session management yet. It´s also needed if you want to use a SSL appliance in front of your application server. In fact ownCloud already supports SSL Proxys since 3.0 but it doesn´t work if the Proxy is not sending the right Header variables which this pull request tries to fix.
It´s a necessary feature if we want to have ownCloud running in bigger installations. I´m not super happy about the technical details of this pull request and I will try to do this a bit differently but we need to have that.

@danimo
Copy link
Contributor

danimo commented Nov 9, 2012

@karlitschek Obviously I agree to the support for HA clustering, but if the fix for this is the same as for dubious SSL proxy tunneling services, which is what a lot of people seem to want to use it with, we should educate people about the loss of privacy when using those services.

@herbrechtsmeier
Copy link
Contributor Author

A lot of web hosting services offer a free SSL Proxy and most of them decode the destination host in a prefix of the URI (https://ssl-proxy.tld/domain.tld/). Some decode the destination host via the customer number in a sub domain of the proxy (https://customer-number.ssl-proxy.tld). The later is already partly supported. The force of a SSL connection don't work, but this is mostly fixed by some .htaccess rewrite rules.

This pull request tries to fix the force of a SSL connection with a SSL Proxy employment, adds support for a SSL Proxy that doesn't set HTTP_X_FORWARDED_PROTO and allows the use of a SSL Proxy that decodes the destination host in the URI.

@danimo: Why is a dedicated SSL Proxy server in the web hosting computing centre dubious? You really want to educate people by forcing them to buy a SSL certificate or to use a patched (older) version of the software? I think it is much better to compare the remote port and the local port and show a warning if they differ and allow the admin to explicit disable the message via a configuration variable.

@karlitschek: Please let me know if I can help you.

@danimo
Copy link
Contributor

danimo commented Nov 10, 2012

@herbrechtsmeier Running multiple instances of a software via thehttps://ssl-proxy.tld/domain.tld/ scheme defeats the purpose of the same origin policy. I am not arguing against those kinds of setup (see my comment earlier), but by educating I mean exactly that: Empower users to understand as closely as possible what kind of security a certain setup will buy them. For instance, the https://customer-number.ssl-proxy.tld/ proxy scheme will protect them against same origin, but only as long as they are not trying to run several instances under the same customer number (unlikely, but unfortunately I have seen that happening).

Don't get me wrong, all I am saying is that security is a hard problem and under certain conditions, SSL certificates are even available for free today, and wether or not a dedicated SSL proxy server is dubious depends entirely on their setup, which we can not vouch for as a vendor.

PS: contributions to the administrators manual are welcome as well :-)

@LukasReschke
Copy link
Member

I´m not super happy about the technical details of this pull request and I will try to do this a bit differently but we need to have that.

Closing this for now. \cc @karlitschek

@karlitschek
Copy link
Contributor

Why are we closing that? Do we have a solution? This is the best starting-point that we have so far.

@karlitschek karlitschek reopened this Nov 15, 2012
@kpi4
Copy link

kpi4 commented Nov 30, 2012

I want to use an SSL proxy, so I implemented these suggested adjustment for SSL proxies which I already used under 4.5.2: http://forum.owncloud.org/viewtopic.php?f=3&t=3841 . This kind of works in 4.5.3, except the htaccess-adjustment doesn seem necessary. Will this still be needed in 4.5.3?

@herbrechtsmeier
Copy link
Contributor Author

@karlitschek Should I update my patch to use your overwrite* configure variables and add an addition prependuri configure variable to support a proxy with prefix in the URI?

@karlitschek
Copy link
Contributor

@herbrechtsmeier Yes. Would be great. I suggest to add an optional overwritewebroot parameter where you can set a custom path.
It would be great if you could sign the contributor agreement by the way: http://owncloud.org/about/contributor-agreement/ I can give you full access and merge your patch afterwards.
Does this work for you?

@karlitschek
Copy link
Contributor

@herbrechtsmeier Do you still work on that? Or do you wan't me to do it?

@herbrechtsmeier
Copy link
Contributor Author

@karlitschek Sorry for the delay. I currently test my rework. Is it okay to only prepend the overwritewebroot or you really want to overwrite the detected webroot?
With the usage of the overwrite variables the setup no longer works without the SSL-proxy!
Should I open a new branch and pull request as I have to rebase my work?

@karlitschek
Copy link
Contributor

Not sure if prepend is enough. There might be setups where the webroot is completely different.
But your decision. And the not working setup is a valid point of course :-)
Yes. A new branch an pull request is probably best. You can close this one afterwards.

@herbrechtsmeier
Copy link
Contributor Author

I have open a new pull request #1035.

In order to get a working setup I need more than one configuration variable. I stick with the webroot prefix instead of a real overwrite as this makes the configuration simpler. Additionally I use the configuration variable prefix reverseproxy as they don't belong to the other overwrite variables.

@kill-joy
Copy link

What about this solution from the forum: http://forum.owncloud.org/viewtopic.php?f=3&t=3841
It work fine for me in OC 4.5.5

@lock lock bot locked as resolved and limited conversation to collaborators Aug 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants