Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add allowed package sources policy #1212

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kosciCZ
Copy link

@kosciCZ kosciCZ commented Nov 5, 2024

This adds a new allowed_package_sources policy that can gate content fetched by cachi2. This rule take a lot from the allowed_external_references, but makes it specific to content fetched by cachi2, because it is not uncommon for dependencies fetched by other means to define externalReferences of type distribution. Cachi2 currently produces the mentioned external reference only for packages fetched with the generic fetcher (docs WIP) that always produces a pkg:generic purl, but it is possible it will be extended to other package managers supported by cachi2 as well.

The rule data for this policy are structured as a list of regex patterns for a given purl type. Examples in tests.

Finally, this is my first contribution to EC (and rego) so please excuse any antipatterns I might have invented, feedback very much welcomed.

Resolves ISV-5342

@kosciCZ
Copy link
Author

kosciCZ commented Nov 5, 2024

@ralphbean could I get your eyes on this as general concept as well as the effective_on date?

@kosciCZ kosciCZ mentioned this pull request Nov 5, 2024
@@ -266,3 +266,5 @@ rule_data_attributes_key := "disallowed_attributes"
rule_data_allowed_external_references_key := "allowed_external_references"

rule_data_disallowed_external_references_key := "disallowed_external_references"

rule_data_allowed_package_sources_key := "allowed_package_sources"
Copy link
Member

Choose a reason for hiding this comment

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

See the rule_data_errors rules above. Let's add a new one that verifies the consistency of this new rule data key. This helps us determine if the rule data provided has any problems.

Copy link
Author

Choose a reason for hiding this comment

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

Holding out on this until the the comment about rule data format is settled.

Copy link
Member

Choose a reason for hiding this comment

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

Since the rule is specific for cachi2, should it maybe be "cachi2_allowed_sources"?

policy/release/sbom_cyclonedx/sbom_cyclonedx.rego Outdated Show resolved Hide resolved
policy/release/sbom_cyclonedx/sbom_cyclonedx.rego Outdated Show resolved Hide resolved
policy/release/sbom_cyclonedx/sbom_cyclonedx.rego Outdated Show resolved Hide resolved
policy/release/sbom_cyclonedx/sbom_cyclonedx.rego Outdated Show resolved Hide resolved
policy/release/sbom_cyclonedx/sbom_cyclonedx.rego Outdated Show resolved Hide resolved
policy/release/sbom_cyclonedx/sbom_cyclonedx.rego Outdated Show resolved Hide resolved

parsed_purl := ec.purl.parse(component.purl)

# only look at components fetched by cachi2 that define an externalReferences
Copy link
Member

Choose a reason for hiding this comment

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

I do wonder if this rule can be a bit more data driven. For example, what if we allowed the rule data to be something like this:

- type: maven
  properties:
    - name: cachi2:found_by
      value: cachi2
  reference:
    type: distribution
  allowed_references:
    - ".*apache.org.*"
    - ".*example.com.*"

Or some variation of that. WDYT?

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if that brings any additional value, because cachi2 will always produce just the one type of externalReference. However, this might be useful in the case of cachi2 changing its name, which will likely occur.

I think that this also has a downside of making the rules less transparent with regards to what is actually allowed, because you get two additional parameters to control that. I think we should strike a balance between extensible and readable.

Copy link
Author

Choose a reason for hiding this comment

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

(Also I think we should settle this first before I get to some of the other comments)

@@ -148,6 +148,63 @@ deny contains result if {
result := lib.result_helper_with_term(rego.metadata.chain(), [id, reference.url, reference.type, msg], id)
}

# METADATA
Copy link
Member

Choose a reason for hiding this comment

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

We also need corresponding changes in sbom_spdx.rego 😭

Copy link
Author

Choose a reason for hiding this comment

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

Cachi2 currently does not produce an SPDX SBOM, so there's not yet anything to latch onto (as in to the externalReference of type distribution in CycloneDX). Holding out on this until the the comment about rule data format is settled.

Signed-off-by: Jan Koscielniak <[email protected]>
purl := component.purl
parsed_purl := ec.purl.parse(purl)

# patterns are either those defined by the rule for a give purl type, or empty by default
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# patterns are either those defined by the rule for a give purl type, or empty by default
# patterns are either those defined by the rule for a given purl type, or empty by default

@simonbaird
Copy link
Member

It would be nice to add some explanation to the commit message, and also include a Jira reference, e.g.:

Ref: https://issues.redhat.com/browse/ISV-5342

(I know it's explained in the PR, but having it in the git history is preferable IMO.)

Nice PR btw!

# description: >-
# For each of the components fetched by Cachi2 which define externalReferences of type
# distribution, verify they are allowed based on the allowed_package_sources rule data
# key. By default, allowed_package_sources is empty which means no components with such
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# key. By default, allowed_package_sources is empty which means no components with such
# key. By default, allowed_package_sources is empty, which means no components with such

🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants