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

Update admin webserver config recommendations for well known handlers #5825

Merged
merged 1 commit into from
Dec 21, 2020

Conversation

ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Dec 16, 2020

For nextcloud/server#24702.

Basically well known has to go to Nextcloud, except for the hard-coded DAV routes.

I tested with nginx, both nextcloud hosted in the root as well as as sub directory. Ideally we'd have nginx answer to ./well-known directly without the index.php prefix but I'm not able to change our default config for this. My previous hand-crafted nginx config allowed this.

@daita please review and test

Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
Copy link
Member Author

On a side note this recommended nginx config really has some issues. I've reverted to my own version now. It's not idea, but works a lot better for development purposes (I don't care much about security headers there).

rewrite ^/\.well-known/host-meta /public.php?service=host-meta last;
rewrite ^/\.well-known/webfinger /public.php?service=webfinger last;
rewrite ^/\.well-known/nodeinfo /public.php?service=nodeinfo last;

location = /.well-known/carddav { return 301 /remote.php/dav/; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good to at some point then also migrate the *dav related well known endpoints

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely. @rullzer and I brainstormed about using the new API and having a simple handler in the dav app. Then installations that are hosted in the document root don't ever need to fiddle with the rewrites/redirects.

@ArtificialOwl
Copy link
Member

This need confirmation, as I might be wrong, but I don't think the current API can handle host-meta

https://tools.ietf.org/html/rfc6415#section-3.1 : The host-meta document root MUST be an "XRD" element.

@ChristophWurst
Copy link
Member Author

This need confirmation, as I might be wrong, but I don't think the current API can handle host-meta

https://tools.ietf.org/html/rfc6415#section-3.1 : The host-meta document root MUST be an "XRD" element.

This is doable today with the GenericResponse. See nextcloud/server#24702 (comment).

@MichaIng
Copy link
Member

Is it a great idea to redirect really all .well-known requests, including ACME and whatnot? If I'm not mistaken this breaks Let's Encrypt certificate renewal when the webroot authentication method is used.

location = /.well-known/carddav { return 301 /remote.php/dav/; }
location = /.well-known/caldav { return 301 /remote.php/dav/; }
# Anything else is dynamically handled by Nextcloud
location ^~ /.well-known { return 301 /index.php$uri; }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think $request_uri must be used here as $uri is the normalised/decided URI with query string and anchor removed, which is not what is wanted here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@daita could that explain why you sometimes didn't see query parameters?!

@ChristophWurst
Copy link
Member Author

Is it a great idea to redirect really all .well-known requests, including ACME and whatnot? If I'm not mistaken this breaks Let's Encrypt certificate renewal when the webroot authentication method is used.

My stack of thoughts ran into an overflow and I forgot our considerations but I seem to remember that we thought about this ACME challenge. Do you have a setup where you could test this?

@MichaIng
Copy link
Member

Do you have a setup where you could test this?

One with Nextcloud in /nextcloud sub directory and Apache2 at least. The issue should be the same when /.well-known/acme-challenge is redirected to /nextcloud/index.php/.well-known/acme-challenge and the question where the ACME client writes and the server reads files from. I'll run a quick test:

Redirect permanent /.well-known /nextcloud/index.php/.well-known
[Tue Feb 16 11:53:13 CET 2021] domain.org:Verify error:Invalid response from https://micha.gnoedi.org/nextcloud/login [123.456.789.123]:
total 4
[Tue Feb 16 11:53:13 CET 2021] Please add '--debug' or '--log' to check more details.
[Tue Feb 16 11:53:13 CET 2021] See: https://github.com/acmesh-official/acme.sh/wiki/How-to-debug-acme.sh
[Tue Feb 16 11:53:14 CET 2021] Error renew domain.org_ecc.

This is with acme.sh webroot authentication. The client writes to /.well-known/acme-challenge/<token> but the authentication server is redirected to /nextcloud/..., hence finally the login page.

.htaccess excludes it explicitly:

RewriteRule ^\.well-known/(?!acme-challenge|pki-validation) /index.php [QSA,L]

But it looks dangerous to go with the approach to redirect all .well-known requests and only exclude some, which are known to be not handled correctly by Nextcloud. I don't know how much services use this path (if a list can be maintained reasonably), but, while it makes the configs more complicated, to me it looks much safer to redirect only the service requests explicitly that are known to be handled correctly by Nextcloud. Especially with Nextcloud in a sub directory one cannot even assume that it is wanted that Nextcloud handles all of them, but probably other software is used to handle some of them. So for an example Nginx configuration, it would probably be more helpful to show how to redirect actually used services that shall be handled by Nextcloud explicitly instead of showing how to exclude those which shall not be handled by Nextcloud, isn't it?

@ChristophWurst
Copy link
Member Author

But it looks dangerous to go with the approach to redirect all .well-known requests and only exclude some, which are known to be not handled correctly by Nextcloud

If there is anything else that is never handled by Nextcloud then we can add it to this hard coded exclude pattern. But the idea was to let Nextcloud apps handle as much as they like dynamically. So from the PoV of Nextcloud we don't actually know what well known handlers are available as they are provided via apps.

@MichaIng
Copy link
Member

So from the PoV of Nextcloud we don't actually know what well known handlers are available as they are provided via apps.

Fair point as well. Practically I guess it doesn't make much difference whether one needs to add an extra include or an extra exclude.

However, ACME definitely needs to be excluded and likely pki-validation (whatever it is), to match the .htaccess.

@ChristophWurst
Copy link
Member Author

True. Would you mind sending the change as PR?

@MichaIng
Copy link
Member

Done: #6221

Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Feb 24, 2021
Please note that I didn't use the current nginx config from the
administration manual as this would've broken ACME challenges[1].

Also added a fix for Microsoft clients.

[1] nextcloud/documentation#5825 (comment)
matejc pushed a commit to matejc/nixpkgs that referenced this pull request Feb 25, 2021
Please note that I didn't use the current nginx config from the
administration manual as this would've broken ACME challenges[1].

Also added a fix for Microsoft clients.

[1] nextcloud/documentation#5825 (comment)
Ma27 added a commit to Ma27/nixpkgs that referenced this pull request Mar 4, 2021
Please note that I didn't use the current nginx config from the
administration manual as this would've broken ACME challenges[1].

Also added a fix for Microsoft clients.

[1] nextcloud/documentation#5825 (comment)

(cherry picked from commit 7977214)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants