From b31c817d487ce0e7737efd6e9573fb231e793f5e Mon Sep 17 00:00:00 2001 From: Itecz Solution Date: Tue, 13 Oct 2026 23:44:20 +0530 Subject: [PATCH] reject broken symlinks instead of skipping them as cycles check_symlink() treats every failure of resolve(strict=True) as a symlink cycle and skips the entry with a warning. A dangling symlink also fails strict resolution, so with allow_symlinks enabled it is silently left out of the manifest when signing, and one added to a signed model afterwards passes verification unreported, while an added regular file fails with "Extra files found". The recorded allow_symlinks policy is applied automatically when verifying, so this holds for the plain CLI verify flow too. Narrow the cycle handling to actual loops (RuntimeError on Python 3.12 and earlier, OSError with ELOOP on 3.13 and later) and raise ValueError for a symlink that cannot be resolved, restoring the fail-closed behavior that existed before check_symlink was introduced. Signed-off-by: Itecz Solution --- CHANGELOG.md | 1 + .../_serialization/serialization.py | 17 ++++++++++++- tests/_serialization/file_shard_test.py | 9 +++++++ tests/_serialization/file_test.py | 16 ++++++++++++ tests/api_test.py | 25 +++++++++++++++++++ tests/conftest.py | 14 +++++++++++ 6 files changed, 81 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e8d36f92..40d0d8b0 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/src/model_signing/_serialization/serialization.py b/src/model_signing/_serialization/serialization.py index d57b6fa7..36ab7796 100644 --- a/src/model_signing/_serialization/serialization.py +++ b/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 @@ def check_symlink(path: pathlib.Path, model_path: pathlib.Path) -> bool: 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: diff --git a/tests/_serialization/file_shard_test.py b/tests/_serialization/file_shard_test.py index 7dd3b838..7e094ef2 100644 --- a/tests/_serialization/file_shard_test.py +++ b/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) diff --git a/tests/_serialization/file_test.py b/tests/_serialization/file_test.py index 33fe1525..d20531ef 100644 --- a/tests/_serialization/file_test.py +++ b/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): diff --git a/tests/api_test.py b/tests/api_test.py index d043a7a1..2cc5027d 100644 --- a/tests/api_test.py +++ b/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 diff --git a/tests/conftest.py b/tests/conftest.py index 916fde1c..863b729f 100644 --- a/tests/conftest.py +++ b/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,