Add support for uploading attestations in legacy API #15952

facutuesca · 2024-05-13T16:56:30Z

Description

Add support for uploading PEP 740 attestations along with distribution files. Part of #15871.
For now only GHA-signed attestations are supported, but the implementation allows easily adding other publishers by implementing the OIDCPublisherMixin.publisher_verification_policy() method in the corresponding subclass.

Implementation

The core logic is in legacy.py: we check an upload request to see if it includes any attestations. If it does, we:

Check if the session is authenticated using Trusted Publishing (and GHA). If not, fail.
Parse the attestations using pypi-attestation-models.
Get the verification policy corresponding to the current Trusted Publisher (for now, always GitHub) .
Verify the attestations using sigstore with the above verification policy, against the uploaded distribution file.

For now we only verify the attestations. Storing them will be implemented in a later PR.

The GHA verification policy (from step 3) is defined in GitHubPublisherMixin.publisher_verification_policy(), and it checks the certificate in the attestation against the following claims:

OIDCBuildConfigURI (e.g: https://github.com/org/repo/.github/workflows/workflow.yml@....)
OIDCSourceRepositoryURI (e.g: https://github.com/org/repo/)

See here for the definition of each claim.

TODO before merging:

Re-add repository-service-tuf to requirements/dev.txt once they release a new version. The current version pins tuf==3.1.0, which conflicts with sigstore who depends on tuf==4.0.0.

cc @woodruffw @di

woodruffw · 2024-05-13T17:19:27Z

warehouse/oidc/models/github.py

+        expected_build_configs = [
+            OIDCBuildConfigURI(f"https://github.com/{self.job_workflow_ref}@{claim}")
+            for claim in [ref, sha]
+            if claim is not None
+        ]


Nitpick: I might be mistaken, but in practice this should only be ref, not sha. With that being said, we can probably use OIDCSourceRepositoryDigest to enforce that the sha also matches 🙂

The trusted publisher claim verification attempts both though:

warehouse/warehouse/oidc/models/github.py

Lines 56 to 62 in 784f446

ref = all_signed_claims.get("ref")

sha = all_signed_claims.get("sha")

if not (ref or sha):

raise InvalidPublisherError("The ref and sha claims are empty")

expected = {f"{ground_truth}@{_ref}" for _ref in [ref, sha] if _ref}

if signed_claim not in expected:

Ah, hmm. Maybe we observed GitHub's IdP doing both at different points...

This is fine for now, then! IMO we could also add the OIDCSourceRepositoryDigest for good measure 🙂

Thinking about this further, the current claims (plus the Digest one you suggested above) are:

OIDCBuildConfigURI (e.g: https://github.com/org/repo/.github/workflows/workflow.yml@$REF/$SHA)

OIDCSourceRepositoryURI (e.g: https://github.com/org/repo/)

OIDCSourceRepositoryDigest (the commit SHA)

I think we could remove the repo URI (2), since it's included in the build config URI (1). WDYT?

Makes sense to me!

Just tying things together: #14335 is where checking both the ref and the sha was introduced.

Here's a PR that adds a comment explaining why we check for both: #15967

woodruffw · 2024-05-13T17:21:24Z

warehouse/oidc/models/github.py

@@ -235,6 +241,25 @@ def publisher_url(self, claims=None):
            return f"{base}/commit/{sha}"
        return base

+    def publisher_verification_policy(self, claims):


Nitpick: docstring explaining the exact inner policy being constructed would be good!

woodruffw · 2024-05-13T17:28:05Z

warehouse/forklift/legacy.py

+            except (ValidationError, InvalidAttestationError) as e:
+                raise _exc_with_message(
+                    HTTPBadRequest,
+                    f"Error while decoding the included attestation: {e}",
+                )
+            except VerificationError as e:
+                raise _exc_with_message(
+                    HTTPBadRequest,
+                    f"Could not verify the uploaded artifact using the included "
+                    f"attestation: {e}",
+                )
+            except Exception as e:
+                raise _exc_with_message(
+                    HTTPBadRequest,
+                    f"Unknown error while trying to verify included attestations: {e}",
+                )


It'd be good to have some metrics/Sentry handlers for these different cases, to help us debug/triage as this gets rolled out 🙂

(See metrics and sentry_sdk uses in the codebase for some examples)

Got it, I added metrics for warehouse.upload.attestations.ok (successful upload), warehouse.upload.attestations.malformed (error decoding/parsing the attestation) and warehouse.upload.attestations.failed_verify (error verifying the attestation).

I also added a Sentry message for unknown/unexpected errors

woodruffw · 2024-05-13T17:28:29Z

warehouse/forklift/legacy.py

+                    verification_policy = publisher.publisher_verification_policy(
+                        request.oidc_claims
+                    )


I believe this can be moved out of the loop, to avoid re-building the policy each time.

good catch, fixed!

woodruffw · 2024-05-13T17:29:36Z

warehouse/oidc/models/github.py

+        expected_build_configs = [
+            OIDCBuildConfigURI(f"https://github.com/{self.job_workflow_ref}@{claim}")
+            for claim in [ref, sha]
+            if claim is not None
+        ]


Ah, hmm. Maybe we observed GitHub's IdP doing both at different points...

This is fine for now, then! IMO we could also add the OIDCSourceRepositoryDigest for good measure 🙂

kairoaraujo · 2024-05-21T09:25:36Z

requirements/dev.txt

@@ -3,4 +3,6 @@ hupper>=1.9
 pip-tools>=1.0
 pyramid_debugtoolbar>=2.5
 pip-api
-repository-service-tuf


It is now solved with the new releases of RSTUF that support tuf 4.0.0 :)

The new release has another conflict with warehouse. See: #15958 (comment)

facutuesca requested a review from a team as a code owner May 13, 2024 16:56

facutuesca marked this pull request as draft May 13, 2024 16:57

woodruffw reviewed May 13, 2024

View reviewed changes

facutuesca force-pushed the upload-attestations branch from af6432f to 784f446 Compare May 13, 2024 17:24

woodruffw reviewed May 13, 2024

View reviewed changes

woodruffw mentioned this pull request May 13, 2024

[Roadmap] Implement PEP 740 #15871

Open

16 tasks

facutuesca added 2 commits May 15, 2024 15:07

Add support for uploading attestations in legacy API

8fc3533

Address review comments

e0b9a33

facutuesca force-pushed the upload-attestations branch from 784f446 to e0b9a33 Compare May 15, 2024 13:08

facutuesca mentioned this pull request May 15, 2024

Remove repository-service-tuf dependency from dev.txt #15958

Open

Update sigstore and pypi-attestation-models

23103a0

kairoaraujo reviewed May 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for uploading attestations in legacy API #15952

Add support for uploading attestations in legacy API #15952

facutuesca commented May 13, 2024

woodruffw May 13, 2024

facutuesca May 13, 2024

woodruffw May 13, 2024

facutuesca May 14, 2024

woodruffw May 14, 2024

facutuesca May 15, 2024

woodruffw May 15, 2024

facutuesca May 16, 2024

woodruffw May 13, 2024

facutuesca May 15, 2024

woodruffw May 13, 2024

facutuesca May 15, 2024

woodruffw May 13, 2024

facutuesca May 15, 2024

woodruffw May 13, 2024

kairoaraujo May 21, 2024

facutuesca May 25, 2024

	ref = all_signed_claims.get("ref")
	sha = all_signed_claims.get("sha")
	if not (ref or sha):
	raise InvalidPublisherError("The ref and sha claims are empty")

	expected = {f"{ground_truth}@{_ref}" for _ref in [ref, sha] if _ref}
	if signed_claim not in expected:

Add support for uploading attestations in legacy API #15952

Are you sure you want to change the base?

Add support for uploading attestations in legacy API #15952

Conversation

facutuesca commented May 13, 2024

Description

Implementation

TODO before merging:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment