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

feat: Add externalParameters.source for v1 specs for BYOB #2193

Closed
wants to merge 2 commits into from

Conversation

laurentsimon
Copy link
Collaborator

closes #2186

Signed-off-by: laurentsimon <laurentsimon@google.com>
Signed-off-by: laurentsimon <laurentsimon@google.com>
@@ -40,5 +40,6 @@ e2e_verify_predicate_v1_runDetails_builder_id "$PREDICATE_CONTENT" "https://gith
e2e_verify_predicate_v1_buildDefinition_externalParameters_workflow "$PREDICATE_CONTENT" "$(e2e_this_file_full_path)" "$GITHUB_REF" "git+https://github.com/$GITHUB_REPOSITORY"

# Verify external parameters source
e2e_verify_predicate_v1_buildDefinition_externalParameters_source "$PREDICATE_CONTENT" "{\"uri\":\"git+https://github.com/$GITHUB_REPOSITORY@$GITHUB_REF\",\"digest\":{\"sha1\":\"$GITHUB_SHA\"}}"
digest=$(e2e_get_source_sha1)
e2e_verify_predicate_v1_buildDefinition_externalParameters_source "$PREDICATE_CONTENT" "{\"uri\":\"git+https://github.com/$GITHUB_REPOSITORY@$GITHUB_REF\",\"digest\":{\"sha1\":\"$digest\"}}"
Copy link
Collaborator Author

@laurentsimon laurentsimon May 30, 2023

Choose a reason for hiding this comment

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

this check should have been failing. The daily runs were not working due to permission errors https://github.com/slsa-framework/slsa-github-generator/actions/runs/5125842921
I've tried addressing this in this PR by changing the default perms to permissions: {} in the workflows

@ianlewis
Copy link
Member

ianlewis commented May 31, 2023

We had a discussion about adding an identifier for the source repo to the annotations field in resolvedDependencies? What do you think about that idea?

I think @asraa had it on her PR slsa-framework/slsa-verifier#621 but removed it.

@asraa
Copy link
Collaborator

asraa commented May 31, 2023

We had a discussion about adding an identifier for the source repo to the annotations field in resolvedDependencies?

I think if we do, it'd be great to have a universal annotation that's agreed upon between multiple builders, so that parsing the arbitrary object can be shared logic.

Since annotations are maps of strings to objects, maybe it can be usage: <some-source-type-uri>?

@ianlewis
Copy link
Member

ianlewis commented May 31, 2023

I think if we do, it'd be great to have a universal annotation that's agreed upon between multiple builders, so that parsing the arbitrary object can be shared logic.

Yeah, I agree. It should probably get into the https://github.com/slsa-framework/github-actions-buildtypes repo at the very least.

Since annotations are maps of strings to objects, maybe it can be usage: <some-source-type-uri>?

For reference, Kubernetes has a similar type of annotations (though string: string) and they use URIs in the keys with "official" annotations using a kubernetes.io domain.
https://kubernetes.io/docs/reference/labels-annotations-taints/

Maybe SLSA needs a few of these well known annotations.

@laurentsimon
Copy link
Collaborator Author

laurentsimon commented May 31, 2023

Maybe SLSA needs a few of these well known annotations.

I'm curious what's the point of these annotation in practice. iiuc, the verification is only required to look at externalParameters.source, and anything in material is to do recursive verification of dependencies... so do we get anything by adding annotations today? I think working with SLSA specs to figure what those annotations are is a good idea since there may be many corner cases... ingestion of a package dep (e.g., npm), etc.. but I wonder if you may be able to verify recursively just using the uri / purl.

@laurentsimon
Copy link
Collaborator Author

There is WIP for specs to resolve this, so closing for now

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.

[feature] Add source:URI under externalParameters
3 participants