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 configuring root as the owner of acme certificate files #140709

Closed
poscat0x04 opened this issue Oct 6, 2021 · 10 comments · Fixed by #147784
Closed

Allow configuring root as the owner of acme certificate files #140709

poscat0x04 opened this issue Oct 6, 2021 · 10 comments · Fixed by #147784
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS

Comments

@poscat0x04
Copy link
Contributor

Issue description

Currently the acme service hard codes acme as the owner of the certificate files. This can cause problems for applications that require very restrict permission (either 600 or 640 with root as the owner) on the private key, such as PostgreSQL. It would be nice to allow configuring the owner of the certificate files, preferably on a per-domain basis.

cc @NixOS/acme

@veprbl veprbl added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Oct 6, 2021
@pennae
Copy link
Contributor

pennae commented Oct 6, 2021

sounds like in those extremely restrictive cases it might be better(/necessary) to copy the certificate and use the copy in the service? :/

@aanderse
Copy link
Member

aanderse commented Oct 6, 2021

sounds like in those extremely restrictive cases it might be better(/necessary) to copy the certificate and use the copy in the service? :/

Agree.

@m1cr0man
Copy link
Contributor

m1cr0man commented Oct 6, 2021

This has been discussed before actually. Some services like opensmtpd explicitly require certs to be owned by root. For a while, we couldn't solve this issue nicely without doing what @pennae recommends, and although the final solution of using LoadCredential still technically invokes a copy, it is at least managed by systemd and easy to implement. You could take a look at #123261 for some inspiration or the comment I linked at the start.

In terms of the original question - allowing configuring the user of the certs - this is not allowed because it complicates how certs can/should be shared across services. There is a lot of care taken to ensure that all files related to the module are owned + only readable by the acme user and the configured group with the goal of simplicity and security. Where root ownership is required LoadCredential is a feasible solution in my testing. Fwiw, before #91121 you actually could set the user, but somewhere in the discussions of #84633 and other tickets we found that there was no need for the extra complexity it induced.

@pennae
Copy link
Contributor

pennae commented Oct 6, 2021

Where root ownership is required LoadCredential is a feasible solution in my testing.

would it make sense to put a pointer to LoadCredential into the acme docs? next time we'll need we'll probably have forgotten about it again.

@m1cr0man
Copy link
Contributor

m1cr0man commented Oct 6, 2021

Indeed it would! The solution is very new and with #123261 I just never got around to doing so.

@poscat0x04
Copy link
Contributor Author

LoadCredential definitely solves my problem and I'm quite satisfied with this solution.

In terms of the original question - allowing configuring the user of the certs - this is not allowed because it complicates how certs can/should be shared across services.

Allow configuring anyone as the owner of the certificates might be a bad idea, but only allow configuring root as the owner shouldn't be a big deal, right?

@m1cr0man
Copy link
Contributor

Ok, I think we can add a useRoot option which is explicit in intention and prevents users setting whoever they want. It's really important that the same user is used across all parts of the acme module... and this is still going to take some testing to validate. I can't create a solid argument against this other than that it is a lot of complexity to maintain.

@aanderse
Copy link
Member

I can't create a solid argument against this other than that it is a lot of complexity to maintain.

Sure... but that's a really solid argument 😕 The module is overly complicated enough to a point where if you get hit by a truck I'm pretty sure the next update after that happens is going to break something. At this point we should really focus on simplifying the module instead of adding complication. It's not hard for users to write their own systemd service to do what they need our module doesn't fit their needs.

I'm not saying we definitely shouldn't accommodate this request, but we should think carefully what we gain vs what we lose if we do.

@m1cr0man
Copy link
Contributor

Yeah, the bus factor and the module complexity is a good way to put it. I'll take a look at the delta for this change tomorrow and then make a decision from that. My thought is that the group setting logic is robust enough to cover user too. Need to test it.

I'm preparing a bunch of updates this weekend and for once I did actually remove some complexity from the script which is nice.

@m1cr0man
Copy link
Contributor

m1cr0man commented Dec 4, 2021

Adding a useRoot option wasn't too bad in the end. I added a test for it too, and I put a big warning against it on the option's description :) It really is better to use LoadCredential as described in #101389 (comment) and I have added such steps to the manual now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants