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

nixos/acme: Fix allowKeysForGroup not applying immediately #72056

Merged
merged 1 commit into from Nov 13, 2019

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Oct 26, 2019

Motivation for this change

Previously setting allowKeysForGroup = true; group = "foo" would not
apply the group permission change of the certificates until the service
gets restarted. This commit fixes this by making systemd restart the
service every time it changes.

Note that applying this commit to a system with an already running acme
systemd service doesn't fix this immediately and you still need to wait
for the next refresh (or call systemctl restart acme-<domain>). Once
everybody's service has restarted once this should be a problem of the
past.

Fixes #48845, which I think has been broken since the introduction of the option in #12283

Ping @tmplt @arianvp @abbradar

Things done
  • Tested it by first using enableACME on a new domain without allowKeysForGroup. Then rebuilding with the option and checking that the directory has the correct group permissions.

Previously setting `allowKeysForGroup = true; group = "foo"` would not
apply the group permission change of the certificates until the service
gets restarted. This commit fixes this by making systemd restart the
service every time it changes.

Note that applying this commit to a system with an already running acme
systemd service doesn't fix this immediately and you still need to wait
for the next refresh (or call `systemctl restart acme-<domain>`). Once
everybody's service has restarted once this should be a problem of the
past.
@arianvp
Copy link
Member

arianvp commented Dec 20, 2019

Thanks! I just only noticed this PR. Please next time run the text suite (named acme) before merging though.
. This change seems to have broken automatic renewal due to the reload job for nginx not being able to retrigger due to RemainAfterExit

Fixed in #76052

@infinisil
Copy link
Member Author

Ahh sorry for that, wasn't aware of that test, will do next time

@arianvp arianvp mentioned this pull request Apr 8, 2020
4 tasks
@arianvp
Copy link
Member

arianvp commented Apr 15, 2020

That fix only fixed activation but I think renewal still sounds broken to me! if the unit is RemainAfterExit= then subsequent triggers by the .timer will not start the unit as it's already started! We should rever this ASAP

@infinisil
Copy link
Member Author

Was reverted in #85333

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixos/acme: allowKeysForGroup has no effect after cert is already created
2 participants