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

Allow setting filestorage umask #121

Closed
SuperQ opened this issue Mar 15, 2021 · 9 comments
Closed

Allow setting filestorage umask #121

SuperQ opened this issue Mar 15, 2021 · 9 comments
Labels
feature request Request for new feature or functionality

Comments

@SuperQ
Copy link

SuperQ commented Mar 15, 2021

What would you like to have changed?

The filestorage method currently hard codes the permissions to user-only access to the cert/key files generated.

Allow passing permission, or at a minimum "allow group read" to the cert/key dirs/files.

Why is this feature a useful, necessary, and/or important addition to this project?

This would make it possible for other services to use the TLS keys to be re-used.

What alternatives are there, or what are you doing in the meantime to work around the lack of this feature?

Please link to any relevant issues, pull requests, or other discussions.

@SuperQ SuperQ added the feature request Request for new feature or functionality label Mar 15, 2021
@francislavoie
Copy link
Member

The file writes happen here:

return ioutil.WriteFile(filename, value, 0600)

@SuperQ SuperQ changed the title Allow setting filestorage umsak Allow setting filestorage umask Mar 15, 2021
@SuperQ
Copy link
Author

SuperQ commented Mar 15, 2021

@francislavoie Yup, that's how I found it was hard coded. 😄

The question is, should we allow directly changing the mask, or have an opinionated boolena option for "allow group read"?

@mholt
Copy link
Member

mholt commented Apr 20, 2021

Changing that mask is a bit scary. I'd be more comfortable if something external did it, with intention.

@mholt mholt closed this as completed Apr 20, 2021
@francislavoie
Copy link
Member

francislavoie commented Apr 20, 2021

I think this might fall under "something an event system in Caddy could solve", i.e. on cert write, a event handler that triggers the file permission to be changed, if you need it to be... I guess. See caddyserver/caddy#3643

@francislavoie
Copy link
Member

francislavoie commented Apr 20, 2021

Alternatively, fork the filestorage module (maybe like unsafe_filestorage) with a wider default permission, or configurable permissions. 🤷‍♂️ It's all pluggable anyways.

@mholt
Copy link
Member

mholt commented Apr 20, 2021

Yeah, something like that. I just don't want people changing this without it being a part of another system that actually uses those permissions -- otherwise you get code or config that's copied and pasted that's insecure for no good reason. I think loosening restrictions needs to be done in the system that needs them loosened, rather than in the system that needs to be secure.

@SuperQ
Copy link
Author

SuperQ commented Apr 20, 2021

@mholt Changing the umask to allow group access is a completely standard way to do things, and not scary at all.

@SuperQ
Copy link
Author

SuperQ commented Apr 20, 2021

I think loosening restrictions needs to be done in the system that needs them loosened

The problem is, UNIX file mask permissions don't work this way. The file owner (Caddy/certmagic) needs to allow access. Without allowing things to change, there's no way for an external system to manage access.

I would be perfectly happy with hard-coding a change from 0600 to 0640, so that the managing system that creates the caddy user/group can then manage what users are allowed to read from the caddy group. I'd also be happy with a bool that says "Allow group read". We don't need to get fancy. Just enough such that group read permissions are allowed.

Here's an example of why having an "external system" is not viable.

I manage Caddy with caddy-ansible. I also manage a Postfix instance with another Ansible role.

Say Caddy writes out a new certificate, the permissions are now changed to be inaccessible. It means a change that requires a Postfix restart is now broken until I go and run my "fix caddy permissions" task. There's just too many chances for race conditions and breakage.

SuperQ added a commit to SuperQ/certmagic that referenced this issue Apr 20, 2021
Add a flag to FileStorage to allow group read access to the files
created. This allows for other systems to be granted group access to the
certs/keys created.

Fixes: caddyserver#121

Signed-off-by: SuperQ <[email protected]>
@SuperQ
Copy link
Author

SuperQ commented Apr 20, 2021

Ok, I implemented a simple boolean flag that allows safe read-only access to keys based on group permissions. This way the service owner can run Caddy server as group caddy, then do something like caddy:x:999: postfix in /etc/group. This is how things are typically handled in tools like certbot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for new feature or functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants