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

check permissions on acme.json during startup #1009

Merged
merged 1 commit into from
Jan 12, 2017

Conversation

bamarni
Copy link
Contributor

@bamarni bamarni commented Jan 1, 2017

Follow-up from #639. At the moment people that were affected by this security issue would still be vulnerable even after upgrading. This patch makes sure permissions are also checked for already existing files.

This might also happen with docker-compose, where it isn't uncommon to just touch a file so that it bind-mounts as a file instead of a directory, and some users might forget to put the permissions afterwards.

In case of perm issue it'd fail on startup like this :

ERRO[2017-01-01T14:45:55+01:00] Error creating TLS config: permissions 644 for acme.json are too open, please use 600
FATA[2017-01-01T14:45:55+01:00] Error preparing server: permissions 644 for acme.json are too open, please use 600

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks @bamarni
/cc @containous/traefik

@dtomcej
Copy link
Contributor

dtomcej commented Jan 11, 2017

LGTM. :shipit: although it looks like it needs a rebase/squash

@dtomcej dtomcej self-requested a review January 11, 2017 19:55
@bamarni
Copy link
Contributor Author

bamarni commented Jan 12, 2017

just rebased 😃

@bamarni
Copy link
Contributor Author

bamarni commented Jan 12, 2017

consul integration tests were failing somehow, just triggered another build, let's see..

Follow-up from traefik#639. At the moment people that were affected
by this security issue would still be vulnerable even after upgrading.

This patch makes sure permissions are also checked for already existing
files.

Signed-off-by: Bilal Amarni <bilal.amarni@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement a new or improved feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants