feat: add pypi attestation discovery #1067

benmss · 2025-04-24T03:58:09Z

Summary

This PR adds discovery of PyPI attestation. URLs to these attestation files are sought via the deps.dev API.

Description of changes

DepsDevRepoFinder was updated to use the DepsDevService, ensuring consistent and easily configurable use of the API
Tests were added for DepsDevRepoFinder functions (they were not added previously), including for the functions that PyPI attestation discovery relies upon.
PyPI attestations do not have a predicate. The pypi-attestation is used to extract information from the attestation certificate. This information is coerced into a predicate for use elsewhere within Macaron.
Addition of an integration test case using the ultralytics Python library as its target.

Related issues

Closes #947

Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai · 2025-04-29T05:54:24Z

src/macaron/provenance/provenance_extractor.py

+        logger.debug("No predicate in payload statement.")
+        return None, None
+
+    repo = json_extract(predicate, ["sourceUri"], str)


From the documentation for the predicate of https://docs.pypi.org/attestations/publish/v1/, currently its content is not defined. I wonder if the sourceUri and sourceDigest was copied from an existing implementation or it's purposely put here to catch these data if they are available in the future?
I have decoded the statement of https://pypi.org/integrity/ultralytics/8.3.119/ultralytics-8.3.119.tar.gz/provenance and got

{ "_type": "https://in-toto.io/Statement/v1", "subject": [ { "name": "ultralytics-8.3.119.tar.gz", "digest": { "sha256": "497bdcf3eb1beb082f451d42e5af2a6af944693a5991c78a9b9b0ce538593153" } } ], "predicateType": "https://docs.pypi.org/attestations/publish/v1", "predicate": null }

I may have confused things by converting the certificate information into a predicate and attaching it to the pypi spec that has none. The type and the related function could be renamed to better distinguish them from the actual spec. The actual field names within the substitute predicate were chosen for simplicity, and loosely based on existing predicates I suppose.

Oh okay, I can see the big picture now. I'm okay with some extra documentation here to indicate that we are relying on a Predicate that we built ourselves, and that it's not reflecting the real predicate format from Pypi (as there is None).

src/macaron/slsa_analyzer/package_registry/deps_dev.py

Signed-off-by: Ben Selwyn-Smith <[email protected]>

src/macaron/slsa_analyzer/provenance/loader.py

src/macaron/provenance/provenance_finder.py

Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai · 2025-05-02T04:37:13Z

src/macaron/slsa_analyzer/package_registry/deps_dev.py

+            If the service is misconfigured, the API is invalid, a network error happens,
+            or unexpected response is returned by the API.
+        """
+        if "%" in purl:


It would be good to have a comment or an example here to explain why we need to convert all "%xx" to their character equivalent.
If I understand it correctly, if we don't convert "%xx", the call to quote / encode below will convert % to "%25", which is not what we want.

tromai · 2025-05-02T04:44:31Z

src/macaron/slsa_analyzer/package_registry/deps_dev.py

+        """
+        if "%" in purl:
+            purl = decode(purl)
+        purl = encode(purl, safe="")


I wonder if we can use decode even if purl doesn't have any %xx?
If it's possible to do so it can be simplified to

convert_escape_char_purl = decode(purl) encoded_purl = encode(convert_escape_char_purl, safe="")

Alternatively, I'm happy to keep the if statement too, but perhaps it's better if we use a different variable to stored the mutated version of the original purl string. For example

convert_escape_char_purl = decode(purl) if "%" in purl else purl encoded_purl = encode(convert_escape_char_purl, safe="")

tromai · 2025-05-02T06:21:45Z

src/macaron/slsa_analyzer/package_registry/deps_dev.py

-        APIAccessError
-            If the service is misconfigured, the API is invalid, a network error happens,
-            or unexpected response is returned by the API.
+        Any


This doc string part should be updated with the new return type hint too.

tromai · 2025-05-02T06:33:00Z

src/macaron/slsa_analyzer/package_registry/deps_dev.py

@@ -20,28 +21,24 @@ class DepsDevService:
    """The deps.dev service class."""

    @staticmethod
-    def get_package_info(purl: str) -> dict | None:
-        """Check if the package identified by the PackageURL (PURL) exists and return its information.
+    def get_endpoint(purl: bool = True, path: str | None = None) -> urllib.parse.SplitResult:


I have a couple of comments about this function

Because we have two types of end point for different purposes, the PURL endpoint https://api.deps.dev/v3alpha/purl/ and the BASE endpoint https://api.deps.dev/v3alpha/. I wonder if we could separate them into 2 functions?

def get_base_endpoint(...) def get_purl_endpoint(...)

Could we add a small comment (may be as a doc string) to explicitly say that path is added directly to the end of the URL endpoint without any modification? - This is just to prevent using this method without percent encoding path before hand. Additional examples would be good to have too.

tromai · 2025-05-02T06:46:25Z

src/macaron/repo_finder/repo_finder_deps_dev.py

-            return ""
-
-        return response.text
+        api_endpoint = DepsDevService.get_endpoint(


Could we add a small example (as a comment) for what the api end point should look like here ?

https://api.deps.dev/v3alpha/systems/pypi/packages/ultralytics/versions/8.3.70

tromai · 2025-05-02T07:09:25Z

src/macaron/repo_finder/repo_finder_deps_dev.py

-
-        If a version is not specified, remote API calls will be used to try and find one.
+    @staticmethod
+    def get_attestation(purl: PackageURL) -> tuple[dict | None, bool]:


Is this function intended to support extracting the attestation for all type of PURL or only PYPI one ?
I think this function assumes that the link to the attestation provided by deps.dev (e.g. https://pypi.org/integrity/ultralytics/8.3.70/ultralytics-8.3.70-py3-none-any.whl/provenance) to a PYPI provenance.
If that is True, we might want to rename the function and modify the doc string to reflect this assumption.

It is currently only intended for PyPI. I have moved the PyPI specific extraction code to the related registry, and will report unsupported for other types.

tromai · 2025-05-02T07:11:51Z

src/macaron/repo_finder/repo_finder_deps_dev.py

        """
-        Parse the deps.dev json file and extract the repository links.
+        Extract the repository links from  the deps.dev json data.


I have a naive question. Does the BASE endpoint and the PURL endpoint returns the JSON data with similar schema and content ?

They return slightly different results in terms of the schema.
https://api.deps.dev/v3alpha/purl/pkg%3Anpm%2F%40colors%[email protected]
https://api.deps.dev/v3alpha/systems/npm/packages/%40colors%2Fcolors/versions/1.4.0

Ah, I see. Thanks

src/macaron/repo_finder/repo_finder_deps_dev.py

tromai · 2025-05-06T04:21:29Z

src/macaron/slsa_analyzer/provenance/loader.py

@@ -83,6 +103,13 @@ def _load_provenance_file_content(
        # Some provenances, such as Witness may not include the DSSE envelope `dsseEnvelope`
        # property but contain its value directly.
        provenance_payload = provenance.get("payload", None)
+    if not provenance_payload:
+        # GitHub Attestation.
+        # TODO Check if old method (above) actually works.


Could a doc string be added here - to explain the reason why there is a change in handling Github attestation?

Perhaps @behnazh-w could weigh in on this?

tromai · 2025-05-06T04:36:28Z

src/macaron/slsa_analyzer/provenance/loader.py

+    if json_payload["predicate"]:
+        return json_payload
+
+    # For provenance without a predicate (e.g. PyPI), try to use the provenance certificate instead.


I think it would be good to add an if statement to check for predicateType being "https://docs.pypi.org/attestations/publish/v1".
This is for recognizing if predicate being empty is expected (because it's a PYPI attestation) or not (e.g the content is malformed).

tromai · 2025-05-06T05:57:25Z

src/macaron/slsa_analyzer/provenance/loader.py

+
+    Parameters
+    ----------
+    decoded_certificate: bytes


We assume that the format of this certificate is Distinguished Encoding Rules (DER) following https://peps.python.org/pep-0740/#provenance-objects. I think we should mention this assumption in the function name and the doc string for extra information.
I also think we can rename decoded_certificate to x509_der_certificate, because the "decoded" part seems to not be in the scope of this function (it has been handled in the function _load_provenance_file_content)

tromai · 2025-05-06T06:03:47Z

src/macaron/slsa_analyzer/provenance/loader.py

+
+    Returns
+    -------
+    dict


Even though the return type of this function is a dictionary, it only contains a fix number of keys:

source_repo

source_digest

workflow

invocation
Do you think it would be worth capturing this in a TypedDict. Or instead we could return a instance of PyPICertificatePredicate from this function if the dictionary is not used elsewhere.

tromai · 2025-05-06T12:03:13Z

src/macaron/slsa_analyzer/provenance/loader.py

+            continue
+
+        claim_name = _OID_IDS[extension.oid.dotted_string]
+        certificate_claims[claim_name] = extension.value.value


Are we assuming that extension here is always of type cryptography.x509.UnrecognizedExtension because of the OIDs we are looking for are not part of the standard X.509 extension type.
If true, I think it's good to have a small comment with that link to api documentation of cryptography.x509.UnrecognizedExtension.
This is due to cryptography.x509.UnrecognizedExtension has the value attribute. Other type of Extension might not have them.

tromai · 2025-05-06T12:29:12Z

src/macaron/slsa_analyzer/provenance/loader.py

+    """
+    certificate = x509.load_der_x509_certificate(decoded_certificate)
+    certificate_claims = {}
+    for extension in certificate.extensions:


I think assessing certificate.extensions can raise Error - https://cryptography.io/en/latest/x509/reference/#cryptography.x509.Certificate.extensions

cryptography.x509.DuplicateExtension

cryptography.x509.UnsupportedGeneralNameType

tromai · 2025-05-06T12:31:36Z

src/macaron/slsa_analyzer/provenance/loader.py

+        claim_name = _OID_IDS[extension.oid.dotted_string]
+        certificate_claims[claim_name] = extension.value.value
+
+    for name in _OID_NAMES:


I wonder if _OID_NAMES is ever used as a dictionary? If not, we can turn it into a list or reuse _OID_IDS.values.

It was redundant and has been removed.

tromai · 2025-05-06T12:35:23Z

src/macaron/slsa_analyzer/provenance/loader.py

+            raise ValueError(f"Missing certificate value: {name}")
+
+        # Values are DER encoded UTF-8 strings. Removing the first two bytes seems to be sufficient.
+        value: str = certificate_claims[name][2:].decode("UTF-8")


Could we perform this operation in the first loop ?

for extension in certificate.extensions: ...

I think the purpose of this loop is for validating certificate_claims content, so in my opinion, it's best if we don't do extra mutation to certificate_claims.

I have combined the loops.

tromai · 2025-05-07T01:24:24Z

tests/repo_finder/test_repo_finder_deps_dev.py

+            "attestation_bundles": [{"attestations": [{"foo": "bar"}]}]
+        }
+    """
+    data = data.replace("*replace_url*", attestation_url)


I wonder if we can use a formatted multi-line string for data?

tromai · 2025-05-07T01:25:49Z

tests/repo_finder/test_repo_finder_deps_dev.py

+    purl = PackageURL.from_string("pkg:pypi/test@3")
+    target_url = (
+        f"/{deps_dev_service_mock['api']}/"
+        + f"{'/'.join(['systems', purl.type, 'packages', purl.name, 'versions', purl.version or ''])}"


I wonder why we have purl.version or '' here. I think purl is not modified at all in this test.

This has been removed as part of using PURL for attestation instead of the BASE endpoint.

tromai · 2025-05-07T01:49:25Z

tests/repo_finder/test_repo_finder_deps_dev.py

+def test_get_project_info_failures(
+    httpserver: HTTPServer, deps_dev_service_mock: dict, repo_url: str, server_url: str, data: str
+) -> None:
+    """Test get project info failures."""
+    if not repo_url:
+        repo_url = f"{deps_dev_service_mock['base_scheme']}://{deps_dev_service_mock['base_netloc']}{server_url}"
+
+    if server_url:
+        target_url = f"/{deps_dev_service_mock['api']}/projects/{deps_dev_service_mock['base_hostname']}{server_url}"
+        httpserver.expect_request(target_url).respond_with_data(data)
+
+    assert not DepsDevRepoFinder().get_project_info(repo_url)


I think there are 2 cases of failures we are testing in this function

The Repository URL is given, and not valid

The repository URL is available (from pytest-httpserver) but the result JSON is invalid

Could we have 2 separated test functions for each case, so that we can simplify the if conditions on the test inputs.

I have separated the two test cases.

tromai

I have finished my round of review. Thank you.

Signed-off-by: Ben Selwyn-Smith <[email protected]>

benmss self-assigned this Apr 24, 2025

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Apr 24, 2025

feat: add pypi attestation discovery

a362d7c

Signed-off-by: Ben Selwyn-Smith <[email protected]>

benmss force-pushed the 947-discover-pypi-attestation branch from b97a9cb to a362d7c Compare April 24, 2025 04:28

chore: keep encoding of purls consistent

6d7cf95

Signed-off-by: Ben Selwyn-Smith <[email protected]>

benmss force-pushed the 947-discover-pypi-attestation branch from 2df212b to 6d7cf95 Compare April 24, 2025 06:31

chore: add config to repo finder tests

3df9273

Signed-off-by: Ben Selwyn-Smith <[email protected]>

benmss marked this pull request as ready for review April 24, 2025 13:00

benmss requested review from behnazh-w and tromai as code owners April 24, 2025 13:00

tromai reviewed Apr 29, 2025

View reviewed changes

tromai reviewed Apr 30, 2025

View reviewed changes

src/macaron/slsa_analyzer/package_registry/deps_dev.py Outdated Show resolved Hide resolved

benmss added 2 commits May 1, 2025 09:22

chore: use SplitResult type hint

b96fbab

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: replace library used for x509 extraction

7e91ef3

Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai reviewed May 2, 2025

View reviewed changes

src/macaron/slsa_analyzer/provenance/loader.py Show resolved Hide resolved

tromai reviewed May 2, 2025

View reviewed changes

src/macaron/provenance/provenance_finder.py Outdated Show resolved Hide resolved

benmss added 2 commits May 2, 2025 14:08

chore: update pyproject.toml

37a8371

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: remove comment

539cb96

Signed-off-by: Ben Selwyn-Smith <[email protected]>

tromai reviewed May 2, 2025

View reviewed changes

src/macaron/repo_finder/repo_finder_deps_dev.py Show resolved Hide resolved

tromai reviewed May 6, 2025

View reviewed changes

tromai reviewed May 7, 2025

View reviewed changes

tromai requested changes May 7, 2025

View reviewed changes

benmss added 3 commits May 7, 2025 14:10

chore: address PR feedback

fdf5a42

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: minor fix

4f402a2

Signed-off-by: Ben Selwyn-Smith <[email protected]>

chore: minor fix

e9b73d7

Signed-off-by: Ben Selwyn-Smith <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add pypi attestation discovery #1067

feat: add pypi attestation discovery #1067

benmss commented Apr 24, 2025

tromai Apr 29, 2025

benmss Apr 29, 2025

tromai May 7, 2025

tromai May 2, 2025

tromai May 2, 2025 •

edited

Loading

tromai May 2, 2025

tromai May 2, 2025 •

edited

Loading

tromai May 2, 2025 •

edited

Loading

tromai May 2, 2025 •

edited

Loading

benmss May 7, 2025

tromai May 2, 2025

benmss May 7, 2025

tromai May 7, 2025

tromai May 6, 2025 •

edited

Loading

benmss May 7, 2025

tromai May 6, 2025 •

edited

Loading

tromai May 6, 2025 •

edited

Loading

tromai May 6, 2025

tromai May 6, 2025 •

edited

Loading

tromai May 6, 2025

tromai May 6, 2025

benmss May 7, 2025

tromai May 6, 2025 •

edited

Loading

benmss May 7, 2025

tromai May 7, 2025

tromai May 7, 2025

benmss May 7, 2025

tromai May 7, 2025

benmss May 7, 2025

tromai left a comment

feat: add pypi attestation discovery #1067

Are you sure you want to change the base?

feat: add pypi attestation discovery #1067

Conversation

benmss commented Apr 24, 2025

Summary

Description of changes

Related issues

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

tromai May 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromai May 2, 2025 • edited Loading

Choose a reason for hiding this comment

tromai May 2, 2025 • edited Loading

Choose a reason for hiding this comment

tromai May 2, 2025 • edited Loading

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

tromai May 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromai May 6, 2025 • edited Loading

Choose a reason for hiding this comment

tromai May 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromai May 6, 2025 • edited Loading

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

tromai May 6, 2025 • edited Loading

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

tromai left a comment

Choose a reason for hiding this comment

tromai May 2, 2025 •

edited

Loading

tromai May 2, 2025 •

edited

Loading

tromai May 2, 2025 •

edited

Loading

tromai May 2, 2025 •

edited

Loading

tromai May 6, 2025 •

edited

Loading

tromai May 6, 2025 •

edited

Loading

tromai May 6, 2025 •

edited

Loading

tromai May 6, 2025 •

edited

Loading

tromai May 6, 2025 •

edited

Loading