Certbot should not touch pre-existing files unless needed

[hlieberman]

From downstream Debian bug 920565.

Currently, certbot will run mkdir on /etc/letsencrypt and /var/log/letsencrypt, whether or not the files currently exist, or whether any files are being changed. This causes the timestamps to be updated unnecessarily. It would be good to check whether or not the files exist rather than checking for the error condition on the mkdir.

[ohemorange]

It is more Pythonic and more threadsafe to do it this way; is it a problem to have the timestamps updated unnecessarily?

[ohemorange]

(Relevant code here: https://github.com/certbot/certbot/blob/master/certbot/util.py#L168)

[jwpemail]

Yes, it is problematic. Security systems that monitor file systems will repeatedly report directory modifcations for a directory that shouldn't be changing unecessarily.

[basak]

@ohemorange I understand the Pythonic principle you're applying here, and I prefer this style myself. However, I'd say that this preference should go out of the window if there are side effects, as there are here. "Pythonic-ness", in my view, only applies to the right way to structure code for a particular desired behaviour.

I don't think that code style preferences are valid as an argument for not changing undesired behaviour. The moment someone complains about undesired externally visible behaviour, and we agree that the behaviour is unnecessary, my view is that the desire for "Pythonicness" must immediately give way.

[basak]

Having said that, it might be argued that os.makedirs shouldn't be touching the directory, and that's a bug in stdlib. The docs don't define it to do that.

[adferrand]

For an example of how this could be implemented, with minimal overhead, one can check my ongoing PR #6497, on https://github.com/adferrand/certbot/blob/windows-files-permissions/certbot/compat/os.py.

It is to improve some os methods toward security on Windows for Certbot, and in particular os.makedirs, to apply proper permissions on created directory. With this kind of approach, another logic can be added to avoid to touch files that already exists.

[stale]

We've made a lot of changes to Certbot since this issue was opened. If you still have this issue with an up-to-date version of Certbot, can you please add a comment letting us know? This helps us to better see what issues are still affecting our users. If there is no activity in the next 30 days, this issue will be automatically closed.

[hlieberman]

yes.

…

On Thu, Feb 6, 2020 at 7:35 PM stale[bot] ***@***.***> wrote: We've made a lot of changes to Certbot since this issue was opened. If you still have this issue with an up-to-date version of Certbot, can you please add a comment letting us know? This helps us to better see what issues are still affecting our users. If there is no activity in the next 30 days, this issue will be automatically closed. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#6726?email_source=notifications&email_token=AAFNWXM66EBRWQYNSWUNP3TRBSUG3A5CNFSM4GTNKKO2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELBJZAI#issuecomment-583179393>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFNWXMQ3PEOE5JSBWVIT7LRBSUG3ANCNFSM4GTNKKOQ> .

-- Harlan Lieberman-Berg ~hlieberman

[jwpemail]

certbot v0.28.0-1~deb9u2 (Debian oldstable) is still affected by this

certbot v0.31.0-1 (debian stable) is also still affected by this.

To be clear, simply running 'certbot renew' will modify the timestamp of the directory /etc/letsencrypt to the current time.

[github-actions]

We've made a lot of changes to Certbot since this issue was opened. If you still have this issue with an up-to-date version of Certbot, can you please add a comment letting us know? This helps us to better see what issues are still affecting our users. If there is no activity in the next 30 days, this issue will be automatically closed.

[jwpemail]

Yes, the problem still exits. Here's how you can verify this on a system with certbot installed.

$  date
$  ls -ald /etc/letsencrypt  <- This will be the datetime of the last run of 'certbot renew'
$  certbot renew 
$  date
$  ls -ald /etc/letsencrypt  <- This will now be the same as the current time

This happens for me on Debian bullseye (certbot v1.12.0-2) and bookworm (certbot v2.1.0-4)