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.

Sign up for GitHub

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

Jump to bottom

Github SLSA Provenance #105

Closed
wants to merge 5 commits into from
Closed

Github SLSA Provenance #105

wants to merge 5 commits into from

Conversation

puerco
Copy link
Contributor

@puerco puerco commented May 2, 2024

Work in progress:

Attestation rules

puerco and others added 2 commits May 2, 2024 01:24
@jhrozek
Copy link
Contributor

jhrozek commented May 2, 2024

@puerco I pushed a new patch into your branch (hopefully that's OK since you can just delete it if you don't like the patch). I will point out the changes in inline comments.

jhrozek
jhrozek reviewed May 2, 2024
View reviewed changes
rule-types/github/attestation_slsa_github.yaml Outdated
@@ -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

jhrozek
jhrozek reviewed May 2, 2024
View reviewed changes
rule-types/github/attestation_slsa_github.yaml Outdated
- 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.

jhrozek
jhrozek reviewed May 2, 2024
View reviewed changes
rule-types/github/attestation_slsa_github.yaml Outdated
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...

jhrozek
jhrozek reviewed May 2, 2024
View reviewed changes
rule-types/github/attestation_slsa_github.yaml Outdated

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 🙏

jhrozek
jhrozek reviewed May 2, 2024
View reviewed changes
rule-types/github/attestation_slsa_github.yaml Outdated
# #}
# }
#}
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.

minder: https://github.com/stacklok/minder/blob/ee66f6c0763212503c898cfefb65ce1450c7f5ac/internal/engine/eval/rego/result.go#L94

jhrozek
jhrozek reviewed May 2, 2024
View reviewed changes
rule-types/github/attestation_slsa_github.yaml Outdated
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?

@jhrozek
Copy link
Contributor

jhrozek commented May 2, 2024 •
edited
Loading

here's the profile I was testing with:

# sample policy for validating artifact signatures
version: v1
type: profile
name: artifact-signature-extended
context:
  provider: github
artifact:
  - type: artifact_signature_slsa
    params:
      tags: ["latest"]
      #tags: []
      name: good-repo-go
    def:
      is_signed: true
      is_verified: true
      workflow_repository: https://github.com/jakubtestorg/good-repo-go
      workflow_ref: refs/heads/main
      event: ["workflow_dispatch"]
      runner_environment:  https://github.com/actions/runner/github-hosted
      signer_identity: .github/workflows/build-image-signed-ghat-malicious.yml
      cert_issuer: https://token.actions.githubusercontent.com # or for example - https://accounts.google.com

@jhrozek
more changes
37059e2 
- Get rid of the is_verified, allowed_workflow and cert_issuer
  attributes
- fix descriptions
- check that if is_verified and is_signed is present, it must be true
- check runner_environment
@jhrozek
Copy link
Contributor

jhrozek commented May 2, 2024

note: I just updated the runner_environment from simple string to a URL. I caught that when I was testing the profile for our blog post and fixed it there, but not here in the example in this PR.

@puerco
Rename slsa ruletype to artifact_attestation_slsa
3018f7c 
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
@puerco puerco changed the title WIP Github SLSA Provenance Github SLSA Provenance May 2, 2024
@puerco
Update attestaion ruletype name
d64ebcc 
Signed-off-by: Adolfo García Veytia (Puerco) <[email protected]>
@puerco
Copy link
Contributor Author

puerco commented May 2, 2024

Closing this one in favor of #106 (from a branch, not a fork)

@puerco puerco closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jhrozek jhrozek jhrozek left review comments

@JAORMX JAORMX JAORMX left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@puerco @jhrozek @JAORMX