From e29735318fa6251c0ea43283d0a017d3d60f00e4 Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 12 Nov 2024 11:52:07 -0700 Subject: [PATCH 1/3] legacy: split attestation handling phases Only persist attestations once the file is created. Signed-off-by: William Woodruff --- tests/unit/forklift/test_legacy.py | 3 +++ warehouse/forklift/legacy.py | 35 ++++++++++++++++++++---------- 2 files changed, 26 insertions(+), 12 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 8994f1496316..558e6168e739 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -1117,6 +1117,7 @@ def storage_service_store(path, file_path, *, meta): assert resp.status_code == 200 assert db_request.find_service.calls == [ pretend.call(IMetricsService, context=None), + pretend.call(IIntegrityService, context=None), pretend.call(IFileStorage, name="archive"), ] assert len(storage_service.store.calls) == 1 @@ -2627,6 +2628,7 @@ def storage_service_store(path, file_path, *, meta): assert resp.status_code == 200 assert db_request.find_service.calls == [ pretend.call(IMetricsService, context=None), + pretend.call(IIntegrityService, context=None), pretend.call(IFileStorage, name="archive"), ] assert storage_service.store.calls == [ @@ -2976,6 +2978,7 @@ def storage_service_store(path, file_path, *, meta): assert resp.status_code == 200 assert db_request.find_service.calls == [ pretend.call(IMetricsService, context=None), + pretend.call(IIntegrityService, context=None), pretend.call(IFileStorage, name="archive"), ] assert storage_service.store.calls == [ diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index 10748b12b725..0c733ca87919 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -1158,6 +1158,27 @@ def file_upload(request): k: h.hexdigest().lower() for k, h in metadata_file_hashes.items() } + # If the user provided attestations, verify them + # We persist these attestations subsequently, only after the + # release file is persisted. + integrity_service: IIntegrityService = request.find_service( + IIntegrityService, context=None + ) + attestations: list[Attestation] = [] + if "attestations" in request.POST and not request.flags.enabled( + AdminFlagValue.DISABLE_PEP740 + ): + try: + attestations = integrity_service.parse_attestations( + request, + Distribution(name=filename, digest=file_hashes["sha256"]), + ) + except AttestationUploadError as e: + raise _exc_with_message( + HTTPBadRequest, + f"Invalid attestations supplied during upload: {e}", + ) + # TODO: This should be handled by some sort of database trigger or a # SQLAlchemy hook or the like instead of doing it inline in this # view. @@ -1226,19 +1247,9 @@ def file_upload(request): ) ) - # If the user provided attestations, verify and store them - if "attestations" in request.POST and not request.flags.enabled( - AdminFlagValue.DISABLE_PEP740 - ): - integrity_service: IIntegrityService = request.find_service( - IIntegrityService, context=None - ) - + # If we have attestations from above, persist them. + if attestations: try: - attestations: list[Attestation] = integrity_service.parse_attestations( - request, - Distribution(name=filename, digest=file_hashes["sha256"]), - ) request.db.add( integrity_service.build_provenance(request, file_, attestations) ) From 5b1bb53b377194972ae9276f5becb89ae767fccd Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Tue, 12 Nov 2024 12:37:49 -0700 Subject: [PATCH 2/3] round out coverage Signed-off-by: William Woodruff --- tests/unit/forklift/test_legacy.py | 58 ++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 558e6168e739..28fadfc3ff05 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -3605,6 +3605,64 @@ def test_upload_fails_attestation_error( assert resp.status_code == 400 assert resp.status.startswith("400 Invalid attestations") + def test_upload_fails_attestations_unsupported_publisher( + self, + monkeypatch, + pyramid_config, + db_request, + dummy_attestation, + ): + from warehouse.events.models import HasEvents + + project = ProjectFactory.create() + version = "1.0" + publisher = pretend.stub( + publisher_name="ActiveState", + publisher_url=lambda *_a, **_kw: "https://fake.example.com", + supports_attestations=True, + ) + identity = PublisherTokenContext(publisher, SignedClaims({})) + db_request.oidc_publisher = identity.publisher + db_request.oidc_claims = identity.claims + + db_request.db.add(Classifier(classifier="Environment :: Other Environment")) + db_request.db.add(Classifier(classifier="Programming Language :: Python")) + + filename = "{}-{}.tar.gz".format(project.name, "1.0") + + pyramid_config.testing_securitypolicy(identity=identity) + db_request.user = None + db_request.user_agent = "warehouse-tests/6.6.6" + db_request.POST = MultiDict( + { + "metadata_version": "1.2", + "name": project.name, + "attestations": f"[{dummy_attestation.model_dump_json()}]", + "version": version, + "summary": "This is my summary!", + "filetype": "sdist", + "md5_digest": _TAR_GZ_PKG_MD5, + "content": pretend.stub( + filename=filename, + file=io.BytesIO(_TAR_GZ_PKG_TESTDATA), + type="application/tar", + ), + } + ) + + record_event = pretend.call_recorder( + lambda self, *, tag, request=None, additional: None + ) + monkeypatch.setattr(HasEvents, "record_event", record_event) + + with pytest.raises(HTTPBadRequest) as excinfo: + legacy.file_upload(db_request) + + resp = excinfo.value + + assert resp.status_code == 400 + assert "Unsupported publisher: ActiveState" in resp.status + @pytest.mark.parametrize( ("url", "expected"), [ From bcfeb24cd5bd87473be52b8dd7a24dd45873779f Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Fri, 22 Nov 2024 08:39:46 -0500 Subject: [PATCH 3/3] cleanup, remove defunct test Signed-off-by: William Woodruff --- tests/unit/forklift/test_legacy.py | 58 ------------------------------ warehouse/forklift/legacy.py | 13 ++----- 2 files changed, 3 insertions(+), 68 deletions(-) diff --git a/tests/unit/forklift/test_legacy.py b/tests/unit/forklift/test_legacy.py index 414630026439..577761f0d230 100644 --- a/tests/unit/forklift/test_legacy.py +++ b/tests/unit/forklift/test_legacy.py @@ -3803,64 +3803,6 @@ def test_upload_fails_attestation_error( assert resp.status_code == 400 assert resp.status.startswith("400 Invalid attestations") - def test_upload_fails_attestations_unsupported_publisher( - self, - monkeypatch, - pyramid_config, - db_request, - dummy_attestation, - ): - from warehouse.events.models import HasEvents - - project = ProjectFactory.create() - version = "1.0" - publisher = pretend.stub( - publisher_name="ActiveState", - publisher_url=lambda *_a, **_kw: "https://fake.example.com", - supports_attestations=True, - ) - identity = PublisherTokenContext(publisher, SignedClaims({})) - db_request.oidc_publisher = identity.publisher - db_request.oidc_claims = identity.claims - - db_request.db.add(Classifier(classifier="Environment :: Other Environment")) - db_request.db.add(Classifier(classifier="Programming Language :: Python")) - - filename = "{}-{}.tar.gz".format(project.name, "1.0") - - pyramid_config.testing_securitypolicy(identity=identity) - db_request.user = None - db_request.user_agent = "warehouse-tests/6.6.6" - db_request.POST = MultiDict( - { - "metadata_version": "1.2", - "name": project.name, - "attestations": f"[{dummy_attestation.model_dump_json()}]", - "version": version, - "summary": "This is my summary!", - "filetype": "sdist", - "md5_digest": _TAR_GZ_PKG_MD5, - "content": pretend.stub( - filename=filename, - file=io.BytesIO(_TAR_GZ_PKG_TESTDATA), - type="application/tar", - ), - } - ) - - record_event = pretend.call_recorder( - lambda self, *, tag, request=None, additional: None - ) - monkeypatch.setattr(HasEvents, "record_event", record_event) - - with pytest.raises(HTTPBadRequest) as excinfo: - legacy.file_upload(db_request) - - resp = excinfo.value - - assert resp.status_code == 400 - assert "Unsupported publisher: ActiveState" in resp.status - @pytest.mark.parametrize( ("url", "expected"), [ diff --git a/warehouse/forklift/legacy.py b/warehouse/forklift/legacy.py index f42e10d261cf..d1f1fe840b9b 100644 --- a/warehouse/forklift/legacy.py +++ b/warehouse/forklift/legacy.py @@ -1082,7 +1082,6 @@ def file_upload(request): # TODO: Remove sdist zip handling when #12245 is resolved # (PEP 625 – Filename of a Source Distribution) if form.filetype.data == "sdist" and filename.endswith(".zip"): - # PEP 625: Enforcement on filename extensions. Files ending with # .zip will not be permitted. send_pep625_extension_email( @@ -1398,15 +1397,9 @@ def file_upload(request): # If we have attestations from above, persist them. if attestations: - try: - request.db.add( - integrity_service.build_provenance(request, file_, attestations) - ) - except AttestationUploadError as e: - raise _exc_with_message( - HTTPBadRequest, - f"Invalid attestations supplied during upload: {e}", - ) + request.db.add( + integrity_service.build_provenance(request, file_, attestations) + ) # Log successful attestation upload metrics.increment("warehouse.upload.attestations.ok")