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

Github SLSA Provenance #105

Closed
wants to merge 5 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 25 additions & 36 deletions rule-types/github/attestation_slsa_github.yaml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
---
version: v1
type: rule-type
name: artifact_signature
name: artifact_signature_slsa
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should not change the existing type but add a new one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with this, if we want to change the signature, behavior or even deprecate the previous behavior, we should just document the deprecation and create a new rule type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes totally agree i had not gotten to that part

context:
provider: github
description: |
Expand Down Expand Up @@ -50,13 +50,15 @@ def:
event:
type: array
description: "Events allowed to trigger a build"
enum:
- worflow_dispatch
- push
default: ["worflow_dispatch", "push"]
items:
type: string
Copy link
Contributor

Choose a reason for hiding this comment

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

I was getting errors unless I made the array type a string and then enumerated the allowed values.

enum:
- workflow_dispatch
- push
default: ["workflow_dispatch", "push"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that the default has no effect on minder right now...

workflow_repository:
type: string
description: "Tepository expected to produce the artifact, i.e. https://github.com/stacklok/minder"
description: "Repository expected to produce the artifact, i.e. https://github.com/stacklok/minder"
workflow_ref:
type: string
description: "The git reference of the executed workflow"
Expand Down Expand Up @@ -91,43 +93,30 @@ def:
import rego.v1

default allow := false
default skip := false

allow if {
artifacts_github_provenance = {artifact |
Copy link
Contributor

Choose a reason for hiding this comment

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

this filter is the most important change and one I am not sure is correct. But based on my reading of your code what you wanted was to:

  • evaluate only artifacts whose predicate_type is "https://slsa.dev/provenance/v1" and the buildType is "https://slsa-framework.github.io/github-actions-buildtypes/workflow/v1" and skip the rest right?
  • those that match the two conditions above must match the profile

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes correct! Thank you 🙏

some artifact in input.ingested
artifact.Verification.attestation.predicate_type == "https://slsa.dev/provenance/v1"

# These parameters only apply when the slsa attestation has a
# buildDefinition.buildType of https://slsa-framework.github.io/github-actions-buildtypes/workflow/v1

# Check the external parameters
artifact.Verification.attestation.predicate.buildDefinition.externalParameters.workflow.path == ".github/workflows/build-image-signed-ghat.yml"
artifact.Verification.attestation.predicate.buildDefinition.externalParameters.workflow.ref == "refs/heads/main"
artifact.Verification.attestation.predicate.buildDefinition.externalParameters.workflow.repository == "https://github.com/jakubtestorg/good-repo-go"

# Internal parameters
# Event is anm arrau
artifact.Verification.attestation.predicate.buildDefinition.internalParameters.github.event_name == "workflow_dispatch"
artifact.Verification.attestation.predicate.buildDefinition.buildType == "https://slsa-framework.github.io/github-actions-buildtypes/workflow/v1"
}

allow if {
count(artifacts_github_provenance) > 0

every artifact in artifacts_github_provenance {
artifact.Verification.attestation.predicate.buildDefinition.externalParameters.workflow.path == input.profile.signer_identity
artifact.Verification.attestation.predicate.buildDefinition.externalParameters.workflow.ref == input.profile.workflow_ref
artifact.Verification.attestation.predicate.buildDefinition.externalParameters.workflow.repository == input.profile.workflow_repository
Copy link
Contributor

Choose a reason for hiding this comment

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

note that these conditions don't handle all the parameters in the rule_schema but I wasn't sure if you only want to match those under the attestation or also those in the top-level attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to match all parameters? At some point it would be good just to have minimal rules that check different aspects of the attestation. That way we'd get more relevant error messages. Similar to what we did for branch protection rules.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that's why I think we should create a new rule.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should match all that are defined if possible. But I expect these to evolve, possible to a more advanced and general SLSA rule type.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, can you elaborate? Now we do match all defined as params in the ruletype did you mean all that exist in the ingestion result that we send?


#every artifactVersion in input.ingested {
# some x in artifactVersion.Verification.attestation.predicateType
# x == "https://slsa.dev/provenance/v1"
##artifactVersion.Verification.attestation.predicateType == "https://slsa.dev/provenance/v1" {
# artifactVersion.Verification["is_signed"]
#}
#}
some event in input.profile.event
artifact.Verification.attestation.predicate.buildDefinition.internalParameters.github.event_name == event
}
}

#allow if {
# every artifactVersion in input.ingested {
# if artifactVersion.Verification.attestation.predicateType == "https://slsa.dev/provenance/v1" {
# artifactVersion.Verification["is_signed"]
# }
## #every key, value in input.profile {
# # # artifactVersion.Verification[key] == value
# #}
# }
#}
skip if {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should mark the rule as skipped if there are no matching artifacts (with "https://slsa-framework.github.io/github-actions-buildtypes/workflow/v1" == buildType and ""https://slsa.dev/provenance/v1" == predicate_type)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh interesting. Is skip a custom value that minder recognizes or is it just vanilla rego?

Copy link
Contributor

Choose a reason for hiding this comment

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

count(artifacts_github_provenance) == 0
}
# Defines the configuration for alerting on the rule
alert:
type: security_advisory
Expand Down
Loading