Skip to content

Conversation

buchdag
Copy link
Member

@buchdag buchdag commented Sep 4, 2018

This PR is a follow up to #412. It add checks and adjustment of ownership / permissions of sensitive files (private keys and ACME account keys) and enclosing folders + corresponding test unit.

The following files and folders are are currently adjusted by the PR:

[drwx------]  /etc/nginx/certs
├── [-rw-------]  default.key
├── [drwx------]  accounts
│   └── [drwx------]  acme-v01.api.letsencrypt.org
│       └── [drwx------]  directory
│           └── [-rw-------]  default.json
├── [drwx------]  domain.tld
│   └── [-rw-------]  key.pem
└── [drwx------]  subdomain.domain.tld
    └── [-rw-------]  key.pem

I'm not sure yet about changing /etc/nginx/certs ownership and permissions, I suspect it might break things for people using host volumes.

In preparation for upcoming permissions and account_keys test units
@buchdag buchdag requested a review from JrCs September 4, 2018 16:59
JrCs
JrCs previously approved these changes Sep 5, 2018
@buchdag
Copy link
Member Author

buchdag commented Sep 5, 2018

@JrCs any advice on changing /etc/nginx/certs permissions vs leaving them as they are ?

It works in both case with docker volumes, but it might be an inconvenience for people using host volumes (ie -v /path/to/certs:/etc/nginx/certs ).

@buchdag
Copy link
Member Author

buchdag commented Sep 16, 2018

I've decided not to touch /etc/nginx/certs ownership and permissions.

@JrCs can you review and approve ?

@buchdag
Copy link
Member Author

buchdag commented Sep 21, 2018

ping @JrCs

@buchdag buchdag merged commit 212e251 into nginx-proxy:master Sep 23, 2018
@buchdag buchdag deleted the fix-perms branch September 24, 2018 07:29
@buchdag buchdag added the kind/feature-request Issue requesting a new feature label Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature-request Issue requesting a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants