reject broken symlinks instead of skipping them as cycles

CHANGELOG.md

@@ -18,6 +18,7 @@ All versions prior to 1.0.0 are untracked.
 - Standardized CLI flags to use hyphens (e.g., `--trust-config` instead of `--trust_config`). Underscore variants are still accepted for backwards compatibility via token normalization.
 
 ### Fixed
+- Fixed serialization treating a broken symlink as a symlink cycle when `allow_symlinks` was enabled, silently leaving it out of the manifest. A dangling symlink added to a signed model passed verification unreported, while an added regular file was flagged as an extra file. Broken symlinks are now rejected with an error unless they are in the ignored paths.
 - Fixed verification of v0.2 signatures (the deprecated `https://model_signing/Digests/v0.1` predicate) following symlinks in the model. The manifest rebuilt from such a signature recorded `allow_symlinks` as the string `"false"`, which is truthy, so a signed file replaced by a symlink still verified. These signatures are now verified with symlinks refused.
 - Fixed serialization silently leaving out every file under a symlink to a directory when `allow_symlinks` was enabled, so those files were neither signed nor verified. Such symlinks are now rejected with an error unless they are in the ignored paths.
 - Fixed certificate verification accepting a signing certificate whose extended key usage does not permit code signing (for example a TLS `serverAuth` certificate), as long as the digitalSignature key usage bit was set. ([#648](https://github.com/sigstore/model-transparency/pull/648))

src/model_signing/_serialization/serialization.py

@@ -16,6 +16,7 @@
 
 import abc
 from collections.abc import Iterable
+import errno
 import logging
 import pathlib
 
@@ -71,12 +72,26 @@
 
     Returns:
         True if the symlink is usable, False if it should be skipped (cycle).
+
+    Raises:
+        ValueError: The symlink cannot be resolved (broken symlink).
     """
     try:
         target = path.resolve(strict=True)
-    except (OSError, RuntimeError):
+    except RuntimeError:
+        # Python 3.12 and earlier raise RuntimeError on symlink loops.
         logger.warning("Symlink cycle detected: '%s'", path)
         return False
+    except OSError as err:
+        if err.errno == errno.ELOOP:
+            logger.warning("Symlink cycle detected: '%s'", path)
+            return False
+        # A link that cannot be resolved is not a cycle. Skipping it would
+        # leave it out of the manifest at signing and leave it unreported at
+        # verification, so it must fail like any other unusable path.
+        raise ValueError(
+            f"Cannot use '{path}' because it is a broken symlink."
+        ) from err
 
     model_root = model_path.resolve()
     try:

tests/_serialization/file_shard_test.py

@@ -463,3 +463,12 @@ def test_symlink_outside_root_warns(
         names = {str(k) for k in result._item_to_digest}
         assert any("link_outside" in n for n in names)
         assert any("real_file" in n for n in names)
+
+    def test_broken_symlink_rejected(self, broken_symlink_model_folder):
+        serializer = file_shard.Serializer(
+            self._hasher_factory, allow_symlinks=True
+        )
+        with pytest.raises(
+            ValueError, match="Cannot use '.+' because it is a broken symlink."
+        ):
+            serializer.serialize(broken_symlink_model_folder)

tests/_serialization/file_test.py

@@ -381,6 +381,22 @@ def test_symlink_outside_root_warns(
         assert "link_outside" in names
         assert "real_file" in names
 
+    def test_broken_symlink_rejected(self, broken_symlink_model_folder):
+        serializer = file.Serializer(self._hasher_factory, allow_symlinks=True)
+        with pytest.raises(
+            ValueError, match="Cannot use '.+' because it is a broken symlink."
+        ):
+            serializer.serialize(broken_symlink_model_folder)
+
+    def test_ignored_broken_symlink_allowed(self, broken_symlink_model_folder):
+        link = broken_symlink_model_folder / "broken_link"
+        serializer = file.Serializer(self._hasher_factory, allow_symlinks=True)
+        manifest = serializer.serialize(
+            broken_symlink_model_folder, ignore_paths=[link]
+        )
+        names = {str(k) for k in manifest._item_to_digest}
+        assert names == {"real_file"}
+
 
 class TestUtilities:
     def test_check_file_or_directory_raises_on_pipes(self, sample_model_file):

tests/api_test.py

@@ -563,6 +563,31 @@ def test_recorded_policy_wins_over_the_callers_flag(self, tmp_path):
             .set_allow_symlinks(False)
         ).verify(model_path, signature)
 
+    def test_added_broken_symlink_fails_verification(self, tmp_path):
+        """A dangling symlink added after signing must not verify."""
+        # A broken symlink was skipped as if it were a symlink cycle, so one
+        # added to a signed model went unreported while an added regular file
+        # failed verification with "Extra files found".
+        model_path = self._model_with_symlink(tmp_path)
+        signature = tmp_path / "model.sig"
+        private_key = Path(TESTDATA / "keys/certificate/signing-key.pem")
+        public_key = Path(TESTDATA / "keys/certificate/signing-key-pub.pem")
+
+        signing.Config().use_elliptic_key_signer(
+            private_key=private_key, password=None
+        ).set_hashing_config(
+            hashing.Config()
+            .set_ignored_paths(paths=[signature], ignore_git_paths=False)
+            .set_allow_symlinks(True)
+        ).sign(model_path, signature)
+
+        (model_path / "added_link").symlink_to(model_path / "missing")
+
+        with pytest.raises(ValueError, match="because it is a broken symlink"):
+            verifying.Config().use_elliptic_key_verifier(
+                public_key=public_key
+            ).verify(model_path, signature)
+
     def test_policy_off_is_also_applied(self, tmp_path):
         """A recorded "off" survives a caller that did ask for symlinks."""
         # The other direction. Sign a model with no symlink and the policy

tests/conftest.py

@@ -161,6 +161,20 @@ def symlink_cycle_model_folder(
     return model_dir
 
 
+@pytest.fixture
+def broken_symlink_model_folder(
+    tmp_path_factory: pytest.TempPathFactory,
+) -> pathlib.Path:
+    """A model folder with a symlink whose target does not exist."""
+    model_dir = tmp_path_factory.mktemp("model") / "broken_symlink_model"
+    model_dir.mkdir()
+    (model_dir / "real_file").write_bytes(test_support.KNOWN_MODEL_TEXT)
+
+    link = model_dir / "broken_link"
+    os.symlink(model_dir.absolute() / "missing_target", link.absolute())
+    return model_dir
+
+
 @pytest.fixture
 def symlink_outside_root_model_folder(
     tmp_path_factory: pytest.TempPathFactory,