From 0e78d0e17ba87db963b70de05b02bfa83d5bb86f Mon Sep 17 00:00:00 2001 From: Ember Date: Tue, 21 Apr 2026 12:19:55 -0700 Subject: [PATCH] Make cert symlink updates atomic Addresses the TODO at storage.py:909. Uses create-temp-then-rename instead of unlink-then-symlink so the transition never shows an intermediate missing-link state. Today nothing exploits the race because live/ is 0o700, but the atomic pattern removes the dependence on that single check. --- certbot/src/certbot/_internal/storage.py | 17 ++++--- .../certbot/_internal/tests/storage_test.py | 48 +++++++++++++++++-- newsfragments/+symlink-atomic.changed | 1 + 3 files changed, 55 insertions(+), 11 deletions(-) create mode 100644 newsfragments/+symlink-atomic.changed diff --git a/certbot/src/certbot/_internal/storage.py b/certbot/src/certbot/_internal/storage.py index a7f19d0dc..e887f45ef 100644 --- a/certbot/src/certbot/_internal/storage.py +++ b/certbot/src/certbot/_internal/storage.py @@ -906,14 +906,19 @@ class RenewableCert(interfaces.RenewableCert): filename = "{0}{1}.pem".format(kind, version) # Relative rather than absolute target directory target_directory = os.path.dirname(filesystem.readlink(link)) - # TODO: it could be safer to make the link first under a temporary - # filename, then unlink the old link, then rename the new link - # to the old link; this ensures that this process is able to - # create symlinks. + new_target = os.path.join(target_directory, filename) + # TODO: we might also want to check consistency of related links # for the other corresponding items - os.unlink(link) - os.symlink(os.path.join(target_directory, filename), link) + # Create the new link under a temporary name and rename it over the + # old link so the transition is atomic. This avoids a window where + # the link does not exist, and removes the dependence on live/ being + # 0o700 to prevent an attacker-planted symlink from winning the race. + temp_link = link + ".new" + if os.path.lexists(temp_link): + os.unlink(temp_link) + os.symlink(new_target, temp_link) + filesystem.replace(temp_link, link) def update_all_links_to(self, version: int) -> None: """Change all member objects to point to the specified version. diff --git a/certbot/src/certbot/_internal/tests/storage_test.py b/certbot/src/certbot/_internal/tests/storage_test.py index da3bd25ce..eee729c70 100644 --- a/certbot/src/certbot/_internal/tests/storage_test.py +++ b/certbot/src/certbot/_internal/tests/storage_test.py @@ -366,6 +366,41 @@ class RenewableCertTests(BaseRenewableCertTest): assert os.path.basename(filesystem.readlink(self.test_rc.chain)) == \ "chain3000.pem" + def test_update_link_to_atomic_keeps_link_valid(self): + """If the rename is interrupted, the original link must still resolve.""" + for ver in range(1, 3): + for kind in ALL_FOUR: + self._write_out_kind(kind, ver) + link = self.test_rc.cert + original_target = filesystem.readlink(link) + + # Simulate an interruption of filesystem.replace after the new symlink + # is created under a temporary name. The original link must still + # exist and point at the original target. + with mock.patch( + "certbot._internal.storage.filesystem.replace", + side_effect=RuntimeError("simulated interrupt")): + with pytest.raises(RuntimeError): + self.test_rc._update_link_to("cert", 1) # pylint: disable=protected-access + + assert os.path.lexists(link) + assert filesystem.readlink(link) == original_target + + def test_update_link_to_cleans_stale_temp(self): + """A stale .new symlink from a previous interrupted run is cleaned up.""" + for ver in range(1, 3): + for kind in ALL_FOUR: + self._write_out_kind(kind, ver) + link = self.test_rc.cert + temp_link = link + ".new" + os.symlink("stale-target.pem", temp_link) + assert os.path.lexists(temp_link) + + self.test_rc._update_link_to("cert", 1) # pylint: disable=protected-access + + assert not os.path.lexists(temp_link) + assert os.path.basename(filesystem.readlink(link)) == "cert1.pem" + def test_version(self): self._write_out_kind("cert", 12) # TODO: We should probably test that the directory is still the @@ -404,15 +439,18 @@ class RenewableCertTests(BaseRenewableCertTest): assert self.test_rc.current_version(kind) == 12 def test_update_all_links_to_full_failure(self): - def unlink_or_raise(path, real_unlink=os.unlink): + # Symlink updates now go through filesystem.replace rather than a + # plain os.unlink. To simulate a mid-update failure, fail the replace + # for fullchain. + def replace_or_raise(src, dst, real_replace=filesystem.replace): # pylint: disable=missing-docstring - if "fullchain" in os.path.basename(path): + if "fullchain" in os.path.basename(dst): raise ValueError - real_unlink(path) + real_replace(src, dst) self._write_out_ex_kinds() - with mock.patch("certbot._internal.storage.os.unlink") as mock_unlink: - mock_unlink.side_effect = unlink_or_raise + with mock.patch("certbot._internal.storage.filesystem.replace") as mock_replace: + mock_replace.side_effect = replace_or_raise with pytest.raises(ValueError): self.test_rc.update_all_links_to(12) diff --git a/newsfragments/+symlink-atomic.changed b/newsfragments/+symlink-atomic.changed new file mode 100644 index 000000000..33b9150a6 --- /dev/null +++ b/newsfragments/+symlink-atomic.changed @@ -0,0 +1 @@ +Cert symlink updates in the live directory are now performed atomically via a temporary filename and rename, closing a small window where the link briefly did not exist.