Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

Parse SLSA v1 provenances to the internal representation #227

Merged
merged 2 commits into from
May 3, 2023

Conversation

rbehjati
Copy link
Contributor

@rbehjati rbehjati commented May 2, 2023

Ref #145

Adds structs for SLSA v1 provenance format, and functionality for parsing a SLSA v1 to ProvenanceIR.

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR
    • Not needed.

@rbehjati rbehjati force-pushed the slsav1 branch 2 times, most recently from c41f6d4 to e71086b Compare May 2, 2023 17:06
Copy link
Contributor

@mariaschett mariaschett left a comment

Choose a reason for hiding this comment

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

LGTM! Nice!

// ProvenanceData contains metadata about a provenance statement, identified by a URI and the
// SHA256 digest of the content of the provenance.
// ProvenanceData contains metadata about a provenance statement. The statement may be wrapped in a
// DSSE envelope, or a Sigstore Bundle. The metadata identifies the provenance via a URI and a
Copy link
Contributor

Choose a reason for hiding this comment

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

Understanding question: Does "the metadata identifie" means "The metadata consists of" or "contains"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... Don't "consists of" and "contains" mean the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think "consist of" and "contains" mean the same roughly.

I wasn't sure how the metadata relate to the the URI and SHA256 digest.

What do you mean by "metadata identifies the provenance via a URI and SHA256 digest"? Are URI and SHA256 the metadata?

But this is nit :) We can follow up offline!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the text and removed "metadata".

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks, I get it now :)


// ParseContainerBasedSLSAv1Provenance parses the given object as a
// ProvenancePredicate, with its BuildDefinition.ExternalParameters parsed into
// an instance of DockerBasedExternalParameters. Returns an error if any of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: You could drop "Returns an error if any of the conversions is unsuccessful". Or is there anything you want to emphasize which I am missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We generally document the error case like this.

package v1

// For more details about the SLSA v1 provenance format see
// https://github.com/slsa-framework/slsa/blob/8df69c20b6f5a08fc71e8591ee2035a780557182/docs/provenance/schema/v1/provenance.proto
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice :)

DockerBasedBuildType = "https://slsa.dev/container-based-build/v0.1?draft"
)

// ProvenancePredicate is the provenance predicate definition.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: description does not give a lot of info beyond the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment, with a link to SLSA v1 provenance spec.

Copy link
Contributor Author

@rbehjati rbehjati left a comment

Choose a reason for hiding this comment

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

Thanks for the review :)

// ProvenanceData contains metadata about a provenance statement, identified by a URI and the
// SHA256 digest of the content of the provenance.
// ProvenanceData contains metadata about a provenance statement. The statement may be wrapped in a
// DSSE envelope, or a Sigstore Bundle. The metadata identifies the provenance via a URI and a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm... Don't "consists of" and "contains" mean the same thing?

DockerBasedBuildType = "https://slsa.dev/container-based-build/v0.1?draft"
)

// ProvenancePredicate is the provenance predicate definition.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the comment, with a link to SLSA v1 provenance spec.


// ParseContainerBasedSLSAv1Provenance parses the given object as a
// ProvenancePredicate, with its BuildDefinition.ExternalParameters parsed into
// an instance of DockerBasedExternalParameters. Returns an error if any of the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We generally document the error case like this.

@rbehjati rbehjati merged commit 18358bd into project-oak:main May 3, 2023
@rbehjati rbehjati deleted the slsav1 branch May 3, 2023 11:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants