Skip to content

Tweak nginx config#52

Open
J0WI wants to merge 1 commit intomatomo-org:masterfrom
J0WI:tweaks
Open

Tweak nginx config#52
J0WI wants to merge 1 commit intomatomo-org:masterfrom
J0WI:tweaks

Conversation

@J0WI
Copy link
Copy Markdown

@J0WI J0WI commented Jun 20, 2019

No description provided.

@Findus23
Copy link
Copy Markdown
Collaborator

I hope it is okay if I apply your changes speratly as I'd like to keep an overview over all lines and am not sure about all lines:

  • X-Frame-Options is already sent by Matomo
  • Any reason for removing the /\.ht section?
  • merging the plugins/HeatmapSessionRecording/configs.php is a great idea, thanks
  • the separation between config|tmp|core|lang and libs|vendor|plugins|misc/user is intentional. This way js, css, etc. are allowed from the plugins but not from the tmp directory.
  • Any reason why the .well-known is allowed explicitly? Matomo doesn't really use it and for let's encrypt I think it needs to be in the HTTP section

Findus23 added a commit that referenced this pull request Jun 20, 2019
Findus23 added a commit that referenced this pull request Jun 20, 2019
Findus23 added a commit that referenced this pull request Jun 20, 2019
Findus23 added a commit that referenced this pull request Jun 20, 2019
@J0WI
Copy link
Copy Markdown
Author

J0WI commented Jun 20, 2019

I hope it is okay if I apply your changes speratly as I'd like to keep an overview over all lines

Sure, that was the idea.

X-Frame-Options is already sent by Matomo

It's more secure to add this in the server config, because this is harder to compromise and it doesn't depend on PHP. fastcgi_hide_header X-Content-Type-Options; can be used to avoid duplicates.

Any reason for removing the /.ht section?

This is now covered by \. which denies access to all dotfiles.

Any reason why the .well-known is allowed explicitly? Matomo doesn't really use it and for let's encrypt I think it needs to be in the HTTP section

All HTTP requests are rewritten to HTTPS, so acme challenge would be blocked.
You can also merge it with the default_type text/plain section if you like.

return 301 https://$host$request_uri;
}
# Redirect all HTTP requests to HTTPS with a 301 Moved Permanently response.
return 301 https://$server_name$request_uri;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just for your information, I applied this change in b74e721, but reverted it in 7035d4f as it causes troubles when one adds a second location block there (e.g. for let's encrypt) and doesn't notice that this is outside a block.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the hint, that makes sense. Maybe a comment on this would be helpful to other users, that $host have security issues and $server_name should be used when only one URL is in use ?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ah, thanks. I didn't notice that this was also changed.
Can you quickly describe me a use case where this would make it more secure?
If I understand it correctly $host takes the Host: header and $server_name the first domain specified in the nginx block.

So as long people don't use Matomo as the default_server it shouldn't matter as the Host as to match anyway. But if they do (which they shouldn't), an attacker could fake the $host ,right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants