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

Small fix for .htaccess so that Apache will be aware of the ... #22084

Closed
wants to merge 1 commit into from
Closed

Small fix for .htaccess so that Apache will be aware of the ... #22084

wants to merge 1 commit into from

Conversation

gjoliver
Copy link

@gjoliver gjoliver commented Aug 2, 2020

X-Forwarded-Proto header, and redirect reverse proxied https requets to corresponding https
destinations.

As written right now, if a user uses their own reverse proxy to terminate and forward https requests and goes to settings->overview page, the 2 async https calls to .well-known/ URLs will get redirected to http://.../remote.php/dav.

And that is a security violation in Chrome, and the redirections will get blocked.
As a result, there will be 2 warning messages about these .well-known URLs not being properly forwarded, which are actually false positive.

…ed-Proto

header, and redirect reverse proxied https requets to corresponding https
destinations.

Signed-off-by: Jun Gong <jungong@mbpro.local>
@kesselb
Copy link
Contributor

kesselb commented Aug 2, 2020

Thanks for your pull request 👍

If Nextcloud generates invalid urls (http instead of https) the configuration is wrong.

Either add the reverse proxy to trusted proxies (Nextcloud will respect x-forwarded-proto) or use overwriteprotocol.

Check https://docs.nextcloud.com/server/19/admin_manual/configuration_server/reverse_proxy_configuration.html and https://github.com/nextcloud/docker#using-the-apache-image-behind-a-reverse-proxy-and-auto-configure-server-host-and-protocol

@gjoliver
Copy link
Author

gjoliver commented Aug 2, 2020

Hey Brian,

Thank you very much for the prompt reply. This is my first pull request to NextCloud, and I really appreciate it :)

Now I see that in the NextCloud documentation you pointed out, it says:

"The redirects for CalDAV or CardDAV does not work if Nextcloud is running behind a reverse proxy. The recommended solution is that your reverse proxy does the redirects."

I don't really know why the team took this stance. Because as I demo-ed with this commit, we can simply handle both http and https redirects in Apache with simple RewriteCond rules. I tested NextCloud in both http and https setups after this change. The redirects worked fine, and those warning messages were gone.

The reason I want to push a little bit on this direction is because it will be much cleaner if the NextCloud image can work well by itself, and we don't have to make such fixes in all possible reverse proxy implementations there may be.

Let me know what you think.
Thanks again.

@kesselb
Copy link
Contributor

kesselb commented Aug 3, 2020

Hey Brian,

I'm not Brian ;)

"The redirects for CalDAV or CardDAV does not work if Nextcloud is running behind a reverse proxy. The recommended solution is that your reverse proxy does the redirects."

Is a simplification. For most setups it's the fastest and cleanest way to solve the warnings.

As written right now, if a user uses their own reverse proxy to terminate and forward https requests and goes to settings->overview page, the 2 async https calls to .well-known/ URLs will get redirected to http://.../remote.php/dav.

Does your fix work for https://mydomain.com/nextcloud?

@gjoliver
Copy link
Author

gjoliver commented Aug 4, 2020

really sorry for mistaking your name ... apologize.
That's a good point, let me think about the non-root case a little bit :)
Thanks.

@gjoliver
Copy link
Author

gjoliver commented Aug 8, 2020

Hi There,

I did a bunch of testing, and felt like handling the case for https://mydomain.com/nextcloud is a bit out of the scope.

  • If you look at the current .htaccess setup, all these redirects are matched against "^.well-known/...", which don't seem to handle sub-folders either?
  • If you look at core/js/tests/specs/setupchecksSpec.js, these checks seem to be hardcoded to fetch .well-known URLs off the root of the hostname. These links are basically broken once I moved nextcloud to a subfolder.
  • I couldn't find good documentations about how to deploy NextCloud in a sub folder. Made me feel like this is a use case that is not officially supported.

My small fix is trying to make whatever is current situation a bit better (false security warnings look werid). I'd appreciate your input to whether this is useful or a potential waste of time.
If you can point me to better documentations about how to properly set up NextCloud in a subfolder, I can definitely do some more experimenting to see if there is some kind of regexp magic to handle both cases well.

Thanks again.

@kesselb
Copy link
Contributor

kesselb commented Aug 9, 2020

If you look at the current .htaccess setup, all these redirects are matched against "^.well-known/...", which don't seem to handle sub-folders either?

Good.

I couldn't find good documentations about how to deploy NextCloud in a sub folder. Made me feel like this is a use case that is not officially supported.

It's a common setup.

My small fix is trying to make whatever is current situation a bit better (false security warnings look werid).

Fine by me. I still think the reverse proxy is the best place to setup such a redirect because it's the first component and knows where other services are located.

#11850
#14132
nextcloud/docker#577
nextcloud/documentation#899

@kesselb
Copy link
Contributor

kesselb commented Aug 9, 2020

cc @nextcloud/docker @nextcloud/server-triage

@kesselb kesselb added 3. to review Waiting for reviews enhancement labels Aug 9, 2020
@kesselb kesselb added this to the Nextcloud 20 milestone Aug 9, 2020
@MichaIng
Copy link
Member

Nextcloud in a subdir is a very common setup, so that must not be broken. But /.well-known requests are not affected by /nextcloud/.htaccess and the /.well-known rewrites/redirects simply do not apply, hence this cannot break anything.

While the other rules are great in .htaccess for shared hosting setups, I agree that proxy-related rules are better done at the proxy. But there might be cases where this is also not easily possible?

@gjoliver
Copy link
Author

Thanks for the inputs guys.

I think there is this philosophical question of where we should fix our Apache configuration. In the container where Apache runs, or somewhere random depending on specific users' setups.

Aside from that, one of my colleagues said, and I quote " ... I want to host nextcloud on heroku, and I do not even control the reverse proxy ...". So yeah, what Michal said is definitely true.

It's good to see many of the issues with these redirections are known. But I want to point out a difference here.
We are experiencing a false positive. the redirections are actually working fine. it's how we are doing these security checks via ajax that is causing the warning messages to show up.
I am just trying to fix this quickly so nobody is getting confused. I think this is quite different from some of the issues where these redirections are not working, and the service discoveries are broken.

@MorrisJobke
Copy link
Member

Fine by me. I still think the reverse proxy is the best place to setup such a redirect because it's the first component and knows where other services are located.

I just talked with @rullzer about this and we also think that this is best suited in the proxy. We want to keep the .htaccess file simple because it is only a solution for part of the setups and making it more and more specific makes it also harder to debug in environments that doesn't fit in there.

We are experiencing a false positive. the redirections are actually working fine. it's how we are doing these security checks via ajax that is causing the warning messages to show up.

In this case we should fix the security checks in the JS side and not the setup itself.

@gjoliver Nevertheless thank you very much for your help with this. Do you mind to give the JS check a try? If not feel free to open a ticket with a reference to this discussion here so that somebody else could pick it up later on.

I would close this PR now here as it's not the perfect solution we are aiming for. Sorry for that.

@MorrisJobke MorrisJobke removed this from the Nextcloud 20 milestone Aug 20, 2020
@h8liu
Copy link

h8liu commented Aug 21, 2020

It cannot be resolved on the JS side as long as it still uses ajax for the check. The JS check is issuing requests to https, then the server running behind a reverse proxy redirects it to http, which chrome nowadays treats as a security violation (ajax security downgrade from https to http), and blocks it by default.

And it is a security issue imho. if a user visits https://mydomain/.well-known/carddav, it will be redirected to http://mydomain/remote.php/dav/ because nextcloud docker is not respecting x-forwarded-proto

Today's popular solution to this would be to use a redirect without scheme and host parts but just the (absolute) path, so the browser can fill in them properly (see https://en.wikipedia.org/wiki/HTTP_location ) Unfortunately mod_rewrite from the old days does not support that.

@MichaIng
Copy link
Member

RewriteRule ^\.well-known/carddav /remote.php/dav/ [R=301,L] exactly works like this, doesn't it?
But I have no idea how this is done in Docker, address the issue to the Docker repo: https://github.com/nextcloud/docker/issues

@gjoliver
Copy link
Author

RewriteRule ^\.well-known/carddav /remote.php/dav/ [R=301,L] exactly works like this, doesn't it?
But I have no idea how this is done in Docker, address the issue to the Docker repo: https://github.com/nextcloud/docker/issues

With a reverse proxy at the front, all requests to the Apache server are http requests. It is documented that NextCloud honors the x-forwarded-proto header, which tells a server the real protocol of a request being forwarded. However the config of the Apache server clearly doesn't do that.

We can't fix it in the js code. There's nothing wrong with the js code. It basically requests an https url and got a http redirect back. This needs to be fixed on the server side.

I will open a bug thread.

@kesselb
Copy link
Contributor

kesselb commented Aug 21, 2020

However the config of the Apache server clearly doesn't do that.

Is documented here: https://docs.nextcloud.com/server/19/admin_manual/configuration_server/reverse_proxy_configuration.html#service-discovery

We can't fix it in the js code. There's nothing wrong with the js code. It basically requests an https url and got a http redirect back. This needs to be fixed on the server side.

Setup the redirects with your reverse proxy.

I will open a bug thread.

Don't forget to use the search. I recall some reports here and at https://github.com/nextcloud/docker. Not sure if they are still open.

About those redirects in general: The redirects are not mission critical. Their purpose is to help caldav or cardav clients to find the right endpoint. If you don't need the feature feel free to ignore the warning.

It seems that some clients also supports a dns based lookup: https://sabre.io/dav/service-discovery/ (you still see the warning but the clients should work - for those who cannot setup the redirects with the reverse proxy).

@h8liu
Copy link

h8liu commented Aug 21, 2020

RewriteRule ^\.well-known/carddav /remote.php/dav/ [R=301,L] exactly works like this, doesn't it?

Nope. mod_rewrite always generates the full URL.

See R|redirect section in :
https://httpd.apache.org/docs/2.4/rewrite/flags.html

@MichaIng
Copy link
Member

Otherwise, the current protocol, servername, and port number will be used to generate the URL sent with the redirect.

Got it, does mod_alias with Redirect /foo /bar leave the choice to browser?

With a reverse proxy at the front, all requests to the Apache server are http requests.

Now I understand the issue. Cannot the proxy be configured to connect only via HTTPS to that certain webserver/host, especially when connection to the proxy has been made via HTTPS and probably HSTS on the target host? However indeed then X-Forwarded-Proto makes sense to respect, if present.

The redirects are not mission critical.

That is true, one can always use the final URL given by calendar/contact UI to connect to the endpoint directly. But comfortable when its possible to skip the path on 3rd party clients, especially on a mobile phone when having clumsy fingers like me 😄.

@h8liu
Copy link

h8liu commented Aug 21, 2020

does mod_alias with Redirect /foo /bar leave the choice to browser?

Nope (unfortunately). See https://httpd.apache.org/docs/2.4/mod/mod_alias.html , quote:

The new URL may be either an absolute URL beginning with a scheme and hostname, or a URL-path beginning with a slash. In this latter case the scheme and hostname of the current server will be added.

@MichaIng
Copy link
Member

Because you said "mod_rewrite from the old days does not support that", I'm wondering which method does support it. Regardless, X-Forwarded-Proto seems to be the de-facto solution for this, otherwise I think you guys will find the best one when discussing in a separate issue.

@h8liu
Copy link

h8liu commented Aug 22, 2020

This PR is respecting X-Forwarded-Proto and might be the only way to do this with Apache modules. I understand it adds complexity to .htaccess, but if it wants to respect X-Forwarded-Proto, this is the complexity it has to bear in my humble opinion, and it is introduced by Apache and its modules "from the old days", not supporting generating redirect paths that are not full URLs. If Apache modules supported omitting schemes and hosts in redirects, there would be a much cleaner solution here.

It might be the choice that code in this repo does not respect X-Forwarded-Proto (because it is too complicated), and claims that in the specification, and I think that is totally fine. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants