Merge 0e78d0e17b into 43f78f9c3c

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.

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)
 

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.