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

Improve headers for security #4757

Open
wants to merge 7 commits into
base: staging
Choose a base branch
from

Conversation

TommyTran732
Copy link

@TommyTran732 TommyTran732 commented Sep 16, 2022

I added Permissions-Policy and Content-Security-Policy. I have tested these with the admin panel and FIDO2.
XSS filtering should be set to 0 as it could cause issues by itself according to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection.

HSTS should be preloaded and include subdomains. I don't see a reason not to.

@mkuron
Copy link
Member

mkuron commented Sep 17, 2022

We had previously decided to not do HSTS for subdomains because it would affect services on your domain other than Mailcow. I think that still holds, especially since it's near impossible to turn off HSTS later.

@TommyTran732
Copy link
Author

Mkay I removed the includeSubdomains part.

@DerLinkman DerLinkman changed the base branch from master to staging September 18, 2022 19:02
@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Nov 17, 2022
@TommyTran732
Copy link
Author

Please do not mark it as stale.

@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Nov 18, 2022
@DerLinkman
Copy link
Member

How does reverse Proxies behave on that?

@TommyTran732
Copy link
Author

How does reverse Proxies behave on that?

I don't understand the question 0.0

@DerLinkman
Copy link
Member

How does reverse Proxies behave on that?

I don't understand the question 0.0

No problem. Will Reverse Proxies still work with that changes? As these are changed directly at the root webserver.

I´m not that fit with those security headers that´s why i ask + to ensure the functionality with reverse proxies.

@TommyTran732
Copy link
Author

No problem. Will Reverse Proxies still work with that changes? As these are changed directly at the root webserver.

It works fine with Mailcow on the stable branch. I have not tested with SOGo yet, as I do not use it. I am using it on my own server as you can see here: https://mail.tommytran.io

@TommyTran732
Copy link
Author

It should work with the bootstrap theme now

@TommyTran732
Copy link
Author

TommyTran732 commented Dec 26, 2022

@DerLinkman These changes work fine with SOGo, I have tested them. They probably won't work with stuff like Gitea etc that you have in the docs. Maybe we can move them down to the location blocks to make it work. The other approach (and IMO the better approach) is to leave it like this and give instructions to overwrite the headers set at the root inside of the location blocks for those reverse proxies.

I have my CSP policy for Gitea listed here https://github.com/tommytran732/Gitea-Docker-Compose/blob/main/swag/nginx/ssl.conf#L32... I am not sure about the other stuff. Someone gotta make the CSP policies for them.

@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Feb 24, 2023
@TommyTran732
Copy link
Author

Do not close. It is not stale.

@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Feb 25, 2023
@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Apr 26, 2023
@TommyTran732
Copy link
Author

Do not close.

@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Apr 28, 2023
@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Jun 27, 2023
@TommyTran732
Copy link
Author

Do not close.

@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Jun 28, 2023
@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Oct 16, 2023
@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Oct 16, 2023
@@ -13,14 +13,18 @@
ssl_session_timeout 1d;
ssl_session_tickets off;

add_header Strict-Transport-Security "max-age=15768000;";
add_header Strict-Transport-Security "max-age=15768000; preload";
Copy link
Member

Choose a reason for hiding this comment

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

According to https://hstspreload.org, preload has no effect when includeSubDomains is not specified. And we definitely don't want that option as it has an effect outside of Mailcow.

Copy link

Choose a reason for hiding this comment

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

I agree that it should be removed, a quote also from hstspreload.org:

If you maintain a project that provides HTTPS configuration advice or provides an option to enable HSTS, do not include the preload directive by default.

add_header X-Content-Type-Options nosniff;
add_header X-XSS-Protection "1; mode=block";
add_header X-XSS-Protection "0";
Copy link
Member

Choose a reason for hiding this comment

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

Remove this option entirely. As per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-XSS-Protection, it is superseded by the Content-Security-Policy and not supported by any browser anymore.

add_header X-Robots-Tag none;
add_header X-Download-Options noopen;
add_header X-Frame-Options "SAMEORIGIN" always;
add_header X-Permitted-Cross-Domain-Policies none;
add_header Referrer-Policy strict-origin;
add_header Permissions-Policy "accelerometer=(), ambient-light-sensor=(), autoplay=(), battery=(), camera=(), clipboard-read=(), display-capture=(), document-domain=(), encrypted-media=(), fullscreen=(), geolocation=(), gyroscope=(), hid=(), idle-detection=(), interest-cohort=(), magnetometer=(), microphone=(), midi=(), payment=(), picture-in-picture=(), screen-wake-lock=(), serial=(), usb=(), sync-xhr=(), xr-spatial-tracking=()";
add_header Content-Security-Policy "default-src 'none'; connect-src 'self' https://api.github.com https://www.gravatar.com; font-src 'self' https://fonts.gstatic.com; img-src 'self' data: https://www.gravatar.com; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; frame-ancestors 'none'; upgrade-insecure-requests; block-all-mixed-content; base-uri 'none'";
Copy link
Member

Choose a reason for hiding this comment

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

Is this list still complete? The pull request is a year old, perhaps we now have different external dependencies?

Copy link
Author

Choose a reason for hiding this comment

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

I am still using this on my mailcow instance and have not had any issues. I can check later but I'd still expect it to work.

@DerLinkman DerLinkman added this to the 2023-11 milestone Nov 8, 2023
@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Jan 7, 2024
@DerLinkman DerLinkman modified the milestones: 2023-11, 2024 Jan 10, 2024
@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Jan 10, 2024
@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Apr 9, 2024
@TommyTran732
Copy link
Author

Do not close

@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Apr 9, 2024
@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Jun 9, 2024
@TommyTran732
Copy link
Author

Do not close

@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Jun 9, 2024
@milkmaker
Copy link
Collaborator

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@milkmaker milkmaker added the stale Please update the issue with current status, unclear if it's still open/needed. label Nov 4, 2024
@TommyTran732
Copy link
Author

Do not close

@milkmaker milkmaker removed the stale Please update the issue with current status, unclear if it's still open/needed. label Nov 7, 2024
@DerLinkman DerLinkman added the neverstale Bot doesn't mark the issue or PR as stale label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs testing neverstale Bot doesn't mark the issue or PR as stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants