From 3339d251bcbbc99a40a7687fc4ad2b6f8d80e2b3 Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 2 Jul 2024 11:04:54 +0200 Subject: [PATCH 1/5] Prevent third party exceptions to leak from `Attestation.sign`. --- CHANGELOG.md | 4 +++ src/pypi_attestations/_impl.py | 62 ++++++++++++++++++++++------------ test/test_impl.py | 62 ++++++++++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9a419c..4bbafd3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `Attestation.sign` now only returns `AttestationError` when failing to sign a distribution file ([#XX]()) + ## [0.0.6] ### Added diff --git a/src/pypi_attestations/_impl.py b/src/pypi_attestations/_impl.py index d69ff8c..db6d379 100644 --- a/src/pypi_attestations/_impl.py +++ b/src/pypi_attestations/_impl.py @@ -12,13 +12,20 @@ from annotated_types import MinLen # noqa: TCH002 from cryptography import x509 from cryptography.hazmat.primitives import serialization -from packaging.utils import parse_sdist_filename, parse_wheel_filename +from packaging.utils import ( + InvalidSdistFilename, + InvalidWheelFilename, + parse_sdist_filename, + parse_wheel_filename, +) from pydantic import Base64Bytes, BaseModel from pydantic_core import ValidationError from sigstore._utils import _sha256_streaming from sigstore.dsse import Envelope as DsseEnvelope +from sigstore.dsse import Error as DsseError from sigstore.dsse import _DigestSet, _Statement, _StatementBuilder, _Subject from sigstore.models import Bundle, LogEntry +from sigstore.sign import ExpiredCertificate, ExpiredIdentity from sigstore_protobuf_specs.io.intoto import Envelope as _Envelope from sigstore_protobuf_specs.io.intoto import Signature as _Signature @@ -86,34 +93,47 @@ class Attestation(BaseModel): def sign(cls, signer: Signer, dist: Path) -> Attestation: """Create an envelope, with signature, from a distribution file. - On failure, raises `AttestationError` or an appropriate subclass. + On failure, raises `AttestationError`. """ - with dist.open(mode="rb", buffering=0) as io: - # Replace this with `hashlib.file_digest()` once - # our minimum supported Python is >=3.11 - digest = _sha256_streaming(io).hex() + try: + with dist.open(mode="rb", buffering=0) as io: + # Replace this with `hashlib.file_digest()` once + # our minimum supported Python is >=3.11 + digest = _sha256_streaming(io).hex() + except (PermissionError, FileNotFoundError) as e: + raise AttestationError(str(e)) try: name = _ultranormalize_dist_filename(dist.name) - except ValueError as e: + except (ValueError, InvalidWheelFilename, InvalidSdistFilename) as e: raise AttestationError(str(e)) - stmt = ( - _StatementBuilder() - .subjects( - [ - _Subject( - name=name, - digest=_DigestSet(root={"sha256": digest}), - ) - ] + try: + stmt = ( + _StatementBuilder() + .subjects( + [ + _Subject( + name=name, + digest=_DigestSet(root={"sha256": digest}), + ) + ] + ) + .predicate_type("https://docs.pypi.org/attestations/publish/v1") + .build() ) - .predicate_type("https://docs.pypi.org/attestations/publish/v1") - .build() - ) - bundle = signer.sign_dsse(stmt) + except DsseError as e: + raise AttestationError(str(e)) - return Attestation.from_bundle(bundle) + try: + bundle = signer.sign_dsse(stmt) + except (ExpiredCertificate, ExpiredIdentity) as e: + raise AttestationError(str(e)) + + try: + return Attestation.from_bundle(bundle) + except ConversionError as e: + raise AttestationError(str(e)) def verify( self, verifier: Verifier, policy: VerificationPolicy, dist: Path diff --git a/test/test_impl.py b/test/test_impl.py index d1b4a02..2a93c6d 100644 --- a/test/test_impl.py +++ b/test/test_impl.py @@ -6,6 +6,7 @@ import pretend import pypi_attestations._impl as impl import pytest +import sigstore from sigstore.dsse import _DigestSet, _StatementBuilder, _Subject from sigstore.models import Bundle from sigstore.oidc import IdentityToken @@ -57,6 +58,67 @@ def test_sign_invalid_dist_filename(self, tmp_path: Path) -> None: ): impl.Attestation.sign(pretend.stub(), bad_dist) + def test_sign_raises_attestation_exception( + self, id_token: IdentityToken, tmp_path: Path + ) -> None: + non_existing_file = tmp_path / "invalid-name.tar.gz" + with pytest.raises(impl.AttestationError, match="No such file"): + impl.Attestation.sign(pretend.stub(), non_existing_file) + + bad_wheel_filename = tmp_path / "invalid-name.whl" + bad_wheel_filename.write_bytes(b"junk") + + with pytest.raises(impl.AttestationError, match="Invalid wheel filename"): + impl.Attestation.sign(pretend.stub(), bad_wheel_filename) + + bad_sdist_filename = tmp_path / "invalid_name.tar.gz" + bad_sdist_filename.write_bytes(b"junk") + + with pytest.raises(impl.AttestationError, match="Invalid sdist filename"): + impl.Attestation.sign(pretend.stub(), bad_sdist_filename) + + def test_wrong_predicate_raises_exception( + self, id_token: IdentityToken, monkeypatch: pytest.MonkeyPatch + ) -> None: + def dummy_predicate(self_: _StatementBuilder, _: str) -> _StatementBuilder: + # wrong type here to have a validation error + self_._predicate_type = False + return self_ + + monkeypatch.setattr(sigstore.dsse._StatementBuilder, "predicate_type", dummy_predicate) + with pytest.raises(impl.AttestationError, match="invalid statement"): + impl.Attestation.sign(pretend.stub(), artifact_path) + + def test_expired_certificate( + self, id_token: IdentityToken, monkeypatch: pytest.MonkeyPatch + ) -> None: + def in_validity_period(_: IdentityToken) -> bool: + # wrong type here to have a validation error + return False + + monkeypatch.setattr(IdentityToken, "in_validity_period", in_validity_period) + + sign_ctx = SigningContext.staging() + with sign_ctx.signer(id_token, cache=False) as signer: + with pytest.raises(impl.AttestationError): + impl.Attestation.sign(signer, artifact_path) + + def test_multiple_signatures( + self, id_token: IdentityToken, monkeypatch: pytest.MonkeyPatch + ) -> None: + def get_bundle(*_) -> Bundle: # noqa: ANN002 + bundle = Bundle.from_json(gh_signed_bundle_path.read_bytes()) + bundle._inner.dsse_envelope.signatures.append(bundle._inner.dsse_envelope.signatures[0]) + return bundle + + monkeypatch.setattr(sigstore.sign.Signer, "sign_dsse", get_bundle) + + sign_ctx = SigningContext.staging() + + with pytest.raises(impl.AttestationError): + with sign_ctx.signer(id_token) as signer: + impl.Attestation.sign(signer, artifact_path) + def test_verify_github_attested(self) -> None: verifier = Verifier.production() pol = policy.AllOf( From 5140c3a5719375d77400aa3ef4ac8cb441acc326 Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 2 Jul 2024 11:05:43 +0200 Subject: [PATCH 2/5] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bbafd3..b5e3e9c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed -- `Attestation.sign` now only returns `AttestationError` when failing to sign a distribution file ([#XX]()) +- `Attestation.sign` now only returns `AttestationError` when failing to sign a distribution file ([#28](https://github.com/trailofbits/pypi-attestations/pull/28)) ## [0.0.6] From 607c27d0a21bf4a75c7ab5f7e4a821cae363d14b Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 2 Jul 2024 11:08:01 +0200 Subject: [PATCH 3/5] Better handling of OS related errors when opening a file. --- src/pypi_attestations/_impl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pypi_attestations/_impl.py b/src/pypi_attestations/_impl.py index db6d379..735494a 100644 --- a/src/pypi_attestations/_impl.py +++ b/src/pypi_attestations/_impl.py @@ -100,7 +100,7 @@ def sign(cls, signer: Signer, dist: Path) -> Attestation: # Replace this with `hashlib.file_digest()` once # our minimum supported Python is >=3.11 digest = _sha256_streaming(io).hex() - except (PermissionError, FileNotFoundError) as e: + except OSError as e: raise AttestationError(str(e)) try: From 2c567ae31c3896d048893e3cc53eddd60325e644 Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 2 Jul 2024 11:10:25 +0200 Subject: [PATCH 4/5] Update comments to better understand the tests. --- test/test_impl.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test_impl.py b/test/test_impl.py index 2a93c6d..5a8c832 100644 --- a/test/test_impl.py +++ b/test/test_impl.py @@ -93,7 +93,6 @@ def test_expired_certificate( self, id_token: IdentityToken, monkeypatch: pytest.MonkeyPatch ) -> None: def in_validity_period(_: IdentityToken) -> bool: - # wrong type here to have a validation error return False monkeypatch.setattr(IdentityToken, "in_validity_period", in_validity_period) @@ -107,6 +106,7 @@ def test_multiple_signatures( self, id_token: IdentityToken, monkeypatch: pytest.MonkeyPatch ) -> None: def get_bundle(*_) -> Bundle: # noqa: ANN002 + # Duplicate the signature to trigger a Conversion error bundle = Bundle.from_json(gh_signed_bundle_path.read_bytes()) bundle._inner.dsse_envelope.signatures.append(bundle._inner.dsse_envelope.signatures[0]) return bundle From b4069497a0c2430f2c692ff9fe6d9caf8d4589b2 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 2 Jul 2024 09:40:28 -0400 Subject: [PATCH 5/5] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b5e3e9c..d058e55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] -### Changed +### Fixed - `Attestation.sign` now only returns `AttestationError` when failing to sign a distribution file ([#28](https://github.com/trailofbits/pypi-attestations/pull/28))