m1cr0man
commented
2 years ago Wrt Caddy, not only do your two points stand, but it is also naive to assume that the cert's group will be acme as it is configurable per-cert. At the very least, it should be iterating the certs its configured to use and pulling unique group names, or (alternatively to using SupplementaryGroups) doing the same thing Nginx and HTTPD does and setting a default for the cert's group to something it can access.
Wrt using SupplementaryGroups everywhere there's some issues that have come to mind.
Firstly, the point you made that "This may be undesirable for some users from a security standpoint" would still stand. There was in the past some concern raised around granting the acme group to other service accounts because it would grant them access to more certs than you may want. Thanks to the assertion you added, we're letting users know that the permissions are incorrect and they have to decide how to solve it, rather than us just blanket-granting access to certs.
However a valid counter argument here is what if an attack comes from the other direction AKA from the acme service/lego. In this case, the group assigned to the cert would be accessible to the attacker and this could be potentially worse than just losing some (renewable) certs (e.g., nginx group, and now all your web directories are compromised). As a result I think this concern cancels itself out, but that's not to say we can't keep both parties happy. All you would need to do is ensure SupplementaryGroups reads the appropriate group(s) from the cert configs that are being used by a service (e.g. the certs assigned to caddy).
Secondly I am wondering if SupplementaryGroups will increase or decrease complexity in a sensible way. If it increases complexity of the modules but decreases misconfigurations and errors for users, then it's probably worth it. The assertion is nice because it's stating that you will likely have an issue and it makes the user aware of the solution. With this proposal, it would either "just work" or you would otherwise have no idea how ACME certs are accessible to your service without reading the module source or generated service config. We've gone almost a year now with no issue reports about ACME which is amazing IMO 😄 but would be putting a spanner in user's understanding of the module if we did this?
Lastly there are still some cases where people/services want root as the cert owner. Before the useRoot option was added to acme, LoadCredential was the only solution here: https://github.com/NixOS/nixpkgs/pull/123261 (although I just noticed this hasn't been merged). I bring this up because LoadCredential would also be a valid solution instead of SupplementaryGroups, but because credentials are not re-read from disk when they change, it's not ideal for ACME usage (no hot reloading, need systemd restart).
So my conclusion here is yes, I agree that SupplementalGroups might be a better solution than our current rigmarole of group setting on the cert + service levels, but like all new changes to the ACME module, it comes with its own set of issues/drawbacks that need to be considered from an end user perspective.
emilylange
(Apologies for the terrible issue title.)
Currently, the Caddy module runs with the
acmegroup as a supplemental group if any ACME certs are used.This may be undesirable for some users from a security standpoint, as it would provide the Caddy service access to all certificates.
Additionally, I believe it breaks the assertions added in b52607f43b11319edb716d65bbecbfdbf2f5b92b (which is totally my bad).
I think we should make Caddy (and the other web servers) set their SupplementalGroups to include all the used certificate's (
useACMEHostonly probably, not ones created by the module directly) groups (and not theacmegroup), and remove the assertions surrounding thiscc @aanderse @m1cr0man for thoughts