refactor: accept provenance data in artifact pipeline check #872

behnazh-w · 2024-09-27T07:41:29Z

Refactoring the artifact pipeline detection check

Renames mcn_infer_artifact_pipeline_1 to mcn_find_artifact_pipeline_1.
This check can support all the package registries now.
Modifies the check fact table schema by adding new columns and allowing some existing columns to be nullable. This change enables us to store the reasons for check failures, such as when a GitHub workflow run is deleted, which may result in some previous columns lacking values.
Improve the heusristics, e.g., if an artifact is published before the corresponding code is committed, there cannot be a CI pipeline that triggered the publishing.
This check depends on the deploy command identified by the mcn_build_as_code_1 check. If a deploy command is detected, this check will attempt to locate a successful CI pipeline that triggered the step containing the deploy command.
When a verifiable provenance is found for an artifact, we use it to obtain the pipeline trigger. Otherwise, we use heuristics to find the triggering pipeline.

Improvements to `mcn_build_as_code_1`

If a provenance is found, we obtain the workflow that has triggered the artifact release.
Add support for Reusable GitHub Actions that perform automatic deployment. Since we do not analyze the external Reusable GitHub Actions, we use an allow list of approved Actions.
A new function, infer_confidence_deploy_workflow is added to BaseBuildTool to infer the confidence for such Reusable workflows.

The `store_inferred_build_info_results` function

Renamed store_inferred_provenance to store_inferred_build_info_results.
To avoid confusion, we avoid using the term inferred provenance here and instead simplify store build related information in the context object provided to checks.
CIInfo["provenances"] is also renamed to CIInfo["build_info_results"].

Provenance Extractor

New abstractions added to the provenance extractor to reuse the logic for extracting information such as ProvenanceBuildDefinition and ProvenancePredicate. With these new abstractions, we don't need to hardcode the expected buildType value while processing a provenance.

`find_publish_timestamp`

Added an API that can obtain the artifact timestamp for all the supported package registries.
By default we use deps.dev to obtain the timestamp except for Maven artifacts because we have observed that Maven Central has more accurate results.
Decoupled the Maven Central search API from the repository, making the hostname fully configurable to enable offline testing with a localhost server.

Tutorial and integration tests

Changed the Detecting a malicious Java dependency uploaded manually to Maven Central tutorial to Detecting Java dependencies manually uploaded to Maven Central
Used log4j-core artifact instead of guava, which has an automated deployment workflow.
Fixed the integration tests and added a new one for log4j-core.

src/macaron/slsa_analyzer/checks/infer_artifact_pipeline_check.py

src/macaron/repo_finder/provenance_extractor.py

src/macaron/slsa_analyzer/build_tool/base_build_tool.py

src/macaron/slsa_analyzer/ci_service/github_actions/github_actions_ci.py

src/macaron/slsa_analyzer/package_registry/package_registry.py

tests/integration/cases/micronaut-projects_micronaut-core/config.ini

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

tromai · 2024-11-10T23:36:21Z

src/macaron/config/defaults.ini

+    actions/setup-java
+    # Parent project used in Maven-based projects of the Apache Logging Services.
+    apache/logging-parent/.github/workflows/build-reusable.yaml
+    # This action can be used to deploy artifacts to a JFrog artifactory server.


Should this entry belong to builder.maven.ci.deploy instead?

tromai · 2024-11-10T23:38:27Z

src/macaron/config/defaults.ini

@@ -494,7 +503,7 @@ artifact_extensions =
 # Package registries.
 [package_registry]
 # The allowed time range (in seconds) from a deploy workflow run start time to publish time.
-publish_time_range = 3600
+publish_time_range = 7200


I wonder why did we decide to increase the publish time range ? (e.g in what case the time range of 3600 is not enough)

tromai · 2024-11-11T02:08:26Z

src/macaron/slsa_analyzer/checks/infer_artifact_pipeline_check.py

Should we rename this file name to reflect the new name of the check ?

tromai · 2024-11-11T04:29:49Z

src/macaron/slsa_analyzer/package_registry/package_registry.py

+        request to fetch metadata about the package, and extracts the publication timestamp
+        from the response.
+
+        Note: The method expects the response to include a ``version`` field with a ``publishedAt``


Does deps.dev have any reference about the format of this version field?. It would be good to include that here if that page is available.

tromai · 2024-11-11T04:32:10Z

src/macaron/slsa_analyzer/package_registry/package_registry.py

+        # implemented at the beginning of the analyze command to ensure that the data
+        # is available for subsequent processing.
+
+        base_url_parsed = urllib.parse.urlparse(registry_url or "https://api.deps.dev")


If the default value of registry_url is https://api.devs.dev, could we make the type of this parameters registry_url: str = "https://api.devs.dev"?

tromai · 2024-11-11T04:43:23Z

src/macaron/slsa_analyzer/package_registry/package_registry.py

+        # is available for subsequent processing.
+
+        base_url_parsed = urllib.parse.urlparse(registry_url or "https://api.deps.dev")
+        path_params = "/".join(["v3alpha", "purl", encode(purl).replace("/", "%2F")])


The encode function here allows you to specify the set of safe characters that will not be encoded (see here). If we replace all / anyway, should we leverage this safe parameter?

tromai · 2024-11-11T04:59:55Z

src/macaron/slsa_analyzer/package_registry/package_registry.py

+            logger.debug("Found timestamp: %s.", timestamp)
+
+            try:
+                return datetime.fromisoformat(timestamp.replace("Z", "+00:00"))


If the timestamp value is already in ISO 8601 format, why do we need to perform extra modification on it before providing it to datetime.fromisoformat.

tromai · 2024-11-11T05:13:12Z

src/macaron/slsa_analyzer/package_registry/package_registry.py

+
+            try:
+                return datetime.fromisoformat(timestamp.replace("Z", "+00:00"))
+            except (OverflowError, OSError) as error:


I wonder in what scenario OverflowError and OSError happens.
As far as I know, datetime.fromisoformat raises:

TypeError if the input is not a string.

ValueError if the date is in correct.

tromai · 2024-11-12T01:32:23Z

src/macaron/slsa_analyzer/package_registry/package_registry.py

+        InvalidHTTPResponseError
+            If the URL construction fails, the HTTP response is invalid, or if the response
+            cannot be parsed correctly, or if the expected timestamp is missing or invalid.
+        NotImplementedError


nit: Should we document this exception if it's not raise in this implementation ? 🤔

tromai · 2024-11-12T01:47:20Z

src/macaron/slsa_analyzer/package_registry/maven_central_registry.py

I have some comments on this file that we don't need to fix it in this PR:

On line 249, we don't use the utility json extract method. We might need to consider replacing it.

On line 233, we can consider updating the if conditions similar to this discussion - refactor: accept provenance data in artifact pipeline check #872 (comment)

tromai · 2024-11-12T01:53:45Z

src/macaron/slsa_analyzer/package_registry/maven_central_registry.py

+            purl_object = PackageURL.from_string(purl)
+        except ValueError as error:
+            logger.debug("Could not parse PURL: %s", error)
+        query_params = [f"q=g:{purl_object.namespace}", f"a:{purl_object.name}", f"v:{purl_object.version}"]


purl_object.version has type str | None. If it's none the last query param would be 'v:None'. I don't think it has the same behavior as the old implementation. In the old implementation, if version is None, no extra query param is added. I wonder if this new behavior is intended?

tromai · 2024-11-12T02:07:16Z

src/macaron/slsa_analyzer/specs/ci_spec.py

    provenances: Sequence[DownloadedProvenanceData]
    """The provenances data."""
+
+    build_info_results: InTotoV01Payload
+    """The build information results computed for a build step. We use the in-toto 0.1 as the spec."""


It's mentioned in the PR description that CIInfo["provenances"] is also renamed to CIInfo["build_info_results"]. Are we planning to remove this provenances attribute here too?

tromai · 2024-11-12T05:14:43Z

src/macaron/slsa_analyzer/checks/build_as_code_check.py

+                                # or Reusable GitHub Action to be be a GitHubJobNode.
+                                if not isinstance(job, GitHubJobNode):
+                                    continue
+


Suggested change

# or Reusable GitHub Action to be be a GitHubJobNode.

if not isinstance(job, GitHubJobNode):

continue

# or Reusable GitHub Action to be a GitHubJobNode.

if not isinstance(job, GitHubJobNode):

continue

tromai · 2024-11-12T05:28:06Z

src/macaron/repo_finder/provenance_extractor.py

+        if build_type := json_extract(statement["predicate"], ["buildType"], str):
+            return build_type
+
+        return json_extract(statement["predicate"], ["buildDefinition", "buildType"], str)


It might be good to document why we have two different way to extract the build type

json_extract(statement["predicate"], ["buildType"], str)

json_extract(statement["predicate"], ["buildDefinition", "buildType"], str)

tromai · 2024-11-12T05:37:49Z

src/macaron/repo_finder/provenance_extractor.py

+        build_type = ProvenancePredicate.get_build_type(statement)
+        build_defs: list[ProvenanceBuildDefinition] = [
+            SLSAGithubGenericBuildDefinitionV01(),
+            SLSAGithubActionsBuildDefinitionV1(),
+            SLSANPMCLIBuildDefinitionV2(),
+            SLSAGCBBuildDefinitionV1(),
+            SLSAOCIBuildDefinitionV1(),
+            WitnessGitLabBuildDefinitionV01(),
+        ]
+
+        for build_def in build_defs:
+            if build_def.expected_build_type == build_type:
+                return build_def
+
+        raise ProvenanceError("Unable to find build definition in the provenance statement.")


A minor improvement could be that we differentiate between 2 cases that we raise an exception:

There is no build definition, happens when ProvenancePredicate.get_build_type(statement) returns None.

There is a build definition, but we don't have support for its value (the condition build_def.expected_build_type == build_type in the for loop never meet).

Right now we only raise one kind of exception message for both cases.

tromai · 2024-11-12T05:46:52Z

src/macaron/repo_finder/provenance_extractor.py

@@ -355,3 +358,354 @@ def check_if_repository_purl_and_url_match(url: str, repo_purl: PackageURL) -> b
        purl_path = f"{repo_purl.namespace}/{purl_path}"
    # Note that the urllib method includes the "/" before path while the PURL method does not.
    return f"{parsed_url.hostname}{parsed_url.path}".lower() == f"{expanded_purl_type or repo_purl.type}/{purl_path}"
+
+
+class ProvenanceBuildDefinition(ABC):


I wonder if these or some of these abstractions should be put within the package src/macaron/slsa_analyzer/provenance as they are closely related to the provenance format 🤔 ?
I think only find_build_def and get_build_type should remain here as a static function.

tromai · 2024-11-13T00:00:07Z

src/macaron/slsa_analyzer/checks/infer_artifact_pipeline_check.py

+                except NotImplementedError:
+                    continue


I think catching NotImplementedError here has a strong indication that the interface of find_publish_timestamp from subtype of PackageRegistry will raise NotImplementedError. I believe this is not the case because only Jfrog Maven registry will raise NotImplementedError. A better way to communicate this would be to explicitly skip running find_publish_timestamp if registry_info is of type JFrogMavenRegistry.

for registry_info in ctx.dynamic_data["package_registries"]: if isinstance(registry_info.package_registry, JfrogMavenRegistry): # We currently don't support this. continue if registry_info.build_tool.purl_type == ctx.component.type: try: artifact_published_date = registry_info.package_registry.find_publish_timestamp(ctx.component.purl) break except InvalidHTTPResponseError as error: logger.debug(error)

What do you think?

This is because I think NotImplementedError should only be raised when we mistakenly call a method that is not implemented (similar to an unexpected critical error).

tromai · 2024-11-13T00:49:25Z

src/macaron/repo_finder/provenance_extractor.py

+        This method is intended to be implemented by subclasses to extract
+        specific invocation details from a provenance statement.


Should we remove these 2 lines as this is a concrete method 🤔 ?
The same comment applies for SLSAGithubActionsBuildDefinitionV1.get_build_invocation

tromai · 2024-11-13T01:02:39Z

src/macaron/repo_finder/provenance_extractor.py

+        tuple[str | None, str | None]
+            A tuple containing two elements:
+            - The first element is the build invocation entry point (e.g., workflow name), or None if not found.
+            - The second element is the invocation URL or identifier (e.g., job URL), or None if not found.


I'm not clear about the difference between invocation URL and identifier. Does it mean that if the second element of the returned tuple is not None, it could be an URL or an arbitrary string as the "identifier" of the workflow run?

tromai · 2024-11-13T01:07:24Z

src/macaron/slsa_analyzer/ci_service/github_actions/github_actions_ci.py

+        # TODO: change this check if this issue is resolved:
+        # https://github.com/orgs/community/discussions/138249
+        if datetime.now(timezone.utc) - timedelta(days=400) > timestamp:
+            logger.debug("Artifact published at %s is older than 410 days.", timestamp)


Suggested change

logger.debug("Artifact published at %s is older than 410 days.", timestamp)

logger.debug("Artifact published at %s is older than 400 days.", timestamp)

tromai

I have finished my review. Thanks.
Overall, there isn't any major changes needs. Most of my comments are for minor improvements/nit picking.

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Sep 27, 2024

behnazh-w force-pushed the behnazh/refactor-infer-publish-check branch 5 times, most recently from ac6cbcd to 7eac146 Compare October 2, 2024 09:47

tromai reviewed Oct 6, 2024

View reviewed changes

src/macaron/slsa_analyzer/checks/infer_artifact_pipeline_check.py Outdated Show resolved Hide resolved

behnazh-w force-pushed the behnazh/refactor-infer-publish-check branch from 1586789 to e592c5d Compare October 29, 2024 05:29

behnazh-w marked this pull request as ready for review October 29, 2024 05:29

behnazh-w requested a review from benmss October 29, 2024 05:30

benmss reviewed Oct 31, 2024

View reviewed changes

src/macaron/repo_finder/provenance_extractor.py Outdated Show resolved Hide resolved

benmss reviewed Nov 5, 2024

View reviewed changes

src/macaron/slsa_analyzer/build_tool/base_build_tool.py Show resolved Hide resolved

benmss reviewed Nov 5, 2024

View reviewed changes

src/macaron/slsa_analyzer/ci_service/github_actions/github_actions_ci.py Outdated Show resolved Hide resolved

benmss reviewed Nov 5, 2024

View reviewed changes

src/macaron/slsa_analyzer/ci_service/github_actions/github_actions_ci.py Outdated Show resolved Hide resolved

benmss reviewed Nov 5, 2024

View reviewed changes

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

benmss reviewed Nov 5, 2024

View reviewed changes

tests/integration/cases/micronaut-projects_micronaut-core/config.ini Outdated Show resolved Hide resolved

behnazh-w added 7 commits November 6, 2024 16:01

refactor: accept provenance data in artifact pipeline check

ee6867a

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

test: update the integration tests

34534e2

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

chore: support more ecosystems by using deps.dev

7d59c32

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

chore: add the Maven Central timestamp fetching back

c0e8537

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

fix: make deploy_workflow and deploy_job columns nullable

1f659b3

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

docs: improve the docstring of ArtifactPipelineCheck

c72a2eb

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

chore: address PR feedback

c9761dc

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

behnazh-w force-pushed the behnazh/refactor-infer-publish-check branch from e592c5d to c9761dc Compare November 6, 2024 06:03

chore: move assignment out of try block

678abb1

Signed-off-by: behnazh-w <behnaz.hassanshahi@oracle.com>

benmss approved these changes Nov 8, 2024

View reviewed changes

tromai reviewed Nov 10, 2024

View reviewed changes

tromai reviewed Nov 11, 2024

View reviewed changes

src/macaron/slsa_analyzer/checks/infer_artifact_pipeline_check.py

Copy link

Member

tromai Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we rename this file name to reflect the new name of the check ?

tromai reviewed Nov 11, 2024

View reviewed changes

tromai reviewed Nov 12, 2024

View reviewed changes

tromai reviewed Nov 13, 2024

View reviewed changes

tromai approved these changes Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: accept provenance data in artifact pipeline check #872

refactor: accept provenance data in artifact pipeline check #872

behnazh-w commented Sep 27, 2024 •

edited

Loading

tromai Nov 10, 2024

tromai Nov 10, 2024 •

edited

Loading

tromai Nov 11, 2024

tromai Nov 11, 2024

tromai Nov 11, 2024

tromai Nov 11, 2024

tromai Nov 11, 2024

tromai Nov 11, 2024

tromai Nov 12, 2024

tromai Nov 12, 2024 •

edited

Loading

tromai Nov 12, 2024

tromai Nov 12, 2024 •

edited

Loading

tromai Nov 12, 2024

tromai Nov 12, 2024

tromai Nov 12, 2024

tromai Nov 12, 2024 •

edited

Loading

tromai Nov 13, 2024 •

edited

Loading

tromai Nov 13, 2024

tromai Nov 13, 2024 •

edited

Loading

tromai Nov 13, 2024

tromai left a comment

		This method is intended to be implemented by subclasses to extract
		specific invocation details from a provenance statement.

	logger.debug("Artifact published at %s is older than 410 days.", timestamp)
	logger.debug("Artifact published at %s is older than 400 days.", timestamp)

refactor: accept provenance data in artifact pipeline check #872

Are you sure you want to change the base?

refactor: accept provenance data in artifact pipeline check #872

Conversation

behnazh-w commented Sep 27, 2024 • edited Loading

Refactoring the artifact pipeline detection check

Improvements to mcn_build_as_code_1

The store_inferred_build_info_results function

Provenance Extractor

find_publish_timestamp

Tutorial and integration tests

Choose a reason for hiding this comment

tromai Nov 10, 2024 • 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

Choose a reason for hiding this comment

tromai Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromai Nov 12, 2024 • 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 Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

tromai Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tromai Nov 13, 2024 • edited Loading

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

behnazh-w commented Sep 27, 2024 •

edited

Loading

Improvements to `mcn_build_as_code_1`

The `store_inferred_build_info_results` function

`find_publish_timestamp`

tromai Nov 10, 2024 •

edited

Loading

tromai Nov 12, 2024 •

edited

Loading

tromai Nov 12, 2024 •

edited

Loading

tromai Nov 12, 2024 •

edited

Loading

tromai Nov 13, 2024 •

edited

Loading

tromai Nov 13, 2024 •

edited

Loading