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

fix(lego): Run lego.service to fix inconsistent ownership of cert dir #172

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jadyndev
Copy link
Contributor

@jadyndev jadyndev commented Aug 9, 2023

This fixes a bug that only occurs on initial deployments when lego_certificate_store_group is set to something other than lego. As the handler "Run lego" is not run with this in mind, all initially generated certificates are created with the permissions which leads to failures.
This is a temporary issue that gets resolved after lego.timer triggers the service for the first time, fixing the permissions.

This is a somewhat dirty but easy fix to this problem as it ensures the service is run after changes.

jdreichmann
jdreichmann previously approved these changes Sep 20, 2023
Copy link
Contributor

@jdreichmann jdreichmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok - changing the ownership in the handlers' command looks like more work. For the record, that means that the certificates are initially created with {{ lego_user }}:{{ lego_user }} by the "Run lego" handler (when the handlers are flushed) and then the permissions are rewritten to {{ lego_user }}:{{ lego_certificate_store_group }} when lego.service is run by either the systemd timers' activation or the "Enable lego.service" task from ansible?

@@ -206,6 +206,7 @@
- name: "Enable lego.service"
systemd:
name: "lego.service"
state: "started"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment above why we do this? It's not clear at the moment why this line is here if you only look at the code.

Alternatively, use ansible.builtin.file to fix the permissions instead, to make it more obvious what is happening here and why.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jdreichmann Could you incorporate this in #179?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcgruenhage see here: #172 (review) - that's the reason the unit is started here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, use ansible.builtin.file to fix the permissions instead, to make it more obvious what is happening here and why.

If one can trigger a handler after another handler when the first handler is already run? otherwise, one could modify the "Run Lego" handler to spawn a shell executing multiple commands. I don't think using ansible.builtin.file is the correct approach here tbh, as that only affects the time between a certificate being newly generated (not renewed!) and the first activation of the systemd timer for the renewal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know the reason, I'm just asking for that reason to be included in the ansible code, so that people don't have to go digging when they want to understand why we start the renewal unit right after we've gotten the certificates in the first place.

With regard to handler-after-handler: Why does it need to be a handler? We're flushing the handlers right before that, so we don't need to have it in a handler. We can just ensure the permissions are right after we've flushed the handlers, which will run after the run lego handler in every case.

Either way, I'm fine with both approaches ultimately, just if the approach without ansible.builtin.file is used, then I'd want a comment explaining why this happens.

@jadyndev
Copy link
Contributor Author

Seems ok - changing the ownership in the handlers' command looks like more work. For the record, that means that the certificates are initially created with {{ lego_user }}:{{ lego_user }} by the "Run lego" handler (when the handlers are flushed) and then the permissions are rewritten to {{ lego_user }}:{{ lego_certificate_store_group }} when lego.service is run by either the systemd timers' activation or the "Enable lego.service" task from ansible?

Yes exactly.

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

Successfully merging this pull request may close these issues.

3 participants