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 Traefik secuity settings, add security documentation #2854

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

Conversation

jameswilliams1
Copy link
Contributor

@jameswilliams1 jameswilliams1 commented Sep 23, 2020

Description

Update Traefik settings to increase TLS strength, hide server software and enforce the same security Django uses for Flower (when it is used). Also add new documentation on security and explain any caveats in compatibilty caused by security settings.

Checklist:

  • I've made sure that tests/test_cookiecutter_generation.py is updated accordingly (especially if adding or updating a template option)
  • I've updated the documentation or confirm that my change doesn't require any updates

Rationale

Fixes: #2841 fixes: #1992

The security settings used by Traefik were previously as default, this PR forces Traefik to use better TLS ciphers plus various other small security tweaks. When using docker, some HTTP secutity headers set by django are now set by Traefik so that they are also used by Flower (if celery is used) and any other parts of the site not served by Django.

Traefik has also been configured to run as non-root and to mount the traefik.yml rather than copy in the Dockerfile so that live reloading can be used when making changes.

Testing

I am running the exact same non-root Dockerfile in prod currently, no issues so far (I did see on the original issue about some kind of redirect issues in the logs but I am not seeing this?). Also am running the same TLS/headers settings, works as expected.

@jameswilliams1
Copy link
Contributor Author

@browniebroke I think given there is already a fair bit of diff, CSP makes sense as its own issue. That would also need a lot of manual testing to ensure it doesn't block needed site features.

@browniebroke
Copy link
Member

I haven't looked into details yet, but from a quick glance, it looks like some very solid work here @jameswilliams1!

Since this is touching with security settings, I don't want to rush the review and give it a good look.

One question I have (which might be a stupid one) is to know whether it makes sense to have some security settings both in Django and Traefik? You mention the Django deploy checks will raise false positives for example...

I think given there is already a fair bit of diff, CSP makes sense as its own issue.

Agree

@jameswilliams1
Copy link
Contributor Author

@browniebroke yeah thats something I was deliberating with. On one hand I guess it's good to have a fallback, say if someone messes with the traefik.yml and removes the security and then django still catches it. But on the other config in 2 places gets confusing (as traefik will completely overwrite the django settings regardless of changes to production.py). I guess an option is to add the django settings back with a NOTE saying these are a fallback and won't affect anything when traefik is in use?

Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Just a single concern.

docs/security.rst Outdated Show resolved Hide resolved
@jameswilliams1 jameswilliams1 force-pushed the bugfix/2841-traefik-uses-weak-security-settings-by-default branch 3 times, most recently from a9eb69a to 9adb35a Compare November 28, 2020 22:26
@jameswilliams1 jameswilliams1 force-pushed the bugfix/2841-traefik-uses-weak-security-settings-by-default branch from 1af7f90 to c9d67c3 Compare April 3, 2021 15:46
@jameswilliams1
Copy link
Contributor Author

@Andrew-Chen-Wang completely forgot about this PR, have just rebased and ci has passed now, could you review again?

Copy link
Contributor

@Andrew-Chen-Wang Andrew-Chen-Wang left a comment

Choose a reason for hiding this comment

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

This LGTM now! I haven't tested this code due to limited resources. @browniebroke opinion?

@foxwoods369
Copy link

Shouldn't production.yml have the ports remapped for traefik?
e.g. "0.0.0.0:80:8080"

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.

Traefik uses weak security settings by default SEC: Traefik should run as non-root
4 participants