Refactor Internal Endorser #189

mariaschett · 2022-12-21T18:16:18Z

Fixes #165 .

This PR merges the types.ValidatedProvenance and common.ProvenanceIR into types.ProvenanceIR.

Benefits:

only one internal, central representation of provenances
reduces code duplication, in particular binary digest & binary name

Downside:
To avoid cyclic dependencies, we have a bit of a round-about.

To determine whether we have a SLSAv02 or Amber provenance, we need to first parse the intoto.statement with types.ParseStatementData. This only sets the field provenanceStatement in a ProvenanceIR p.
To determine the other fields in ProvenanceIR we need to call common.SetProvenanceValues on p (this has to live in common to avoid cyclic dependencies).

rbehjati · 2022-12-22T11:56:45Z

internal/common/common.go

@@ -92,6 +93,13 @@ func NewProvenanceIR(binarySHA256Digest string, options ...func(p *ProvenanceIR)
 	return provenance
 }

+// WithBinaryName adds a binary name when creating a new ProvenanceIR.
+func WithBinaryName(binaryName string) func(p *ProvenanceIR) {


I am not sure if we want to verify this, but I could be wrong. Was there anything about this when you did the requirements analysis?

No, not at all, I removed it!

rbehjati · 2023-01-03T10:28:41Z

internal/common/common_test.go


-	if diff := cmp.Diff(got, want, cmp.AllowUnexported(ProvenanceIR{})); diff != "" {


I had intentionally used cmp.Diff. This is the standard pattern that we should use everywhere (see #188). Please revert this change, and rewrite the test using cmp.

Hmmm... I didn't know where to get a good in-toto test statement for generating a NewProvenanceIR(provenanceStatement *intoto.Statement---so this was the quickest fix I could think of.

I see two option:

I think I could use ParseStatementData to get the *intoto.Statement from the provenanceExample path, but that seemed a bit round-about. Or am I missing a simpler solution to get a test intoto.Statement?

2.make sure the cmp.Diff does not compare the provenanceStatement, but I am not sure if that is possible.

Do you think that any of those two much better?

Actually, what exactly is this test case testing? The name still says TestFromProvenance_Amber, but FromProvenance is replaced with SetProvenanceData, which is not even called in this test.

I think fixing this test will be easier once SetProvenanceData is fixed (ref my other comments on SetXYZData).

rbehjati · 2023-01-03T10:29:04Z

internal/common/common.go

-	predType := prov.PredicateType()
+// SetProvenanceData sets, depending on the predicate of the underlying provenance intoto.Statement,
+// the data for verifying the provenance.
+func SetProvenanceData(provenanceIR *types.ProvenanceIR) error {


I prefer the immutable style.

Sure, do you think it needs to be changed?

Yes, and after a closer look at the code I think it is doable.

rbehjati · 2023-01-03T10:34:46Z

pkg/types/types.go

+// ProvenanceIR wraps an intoto.Statement representing a valid SLSA provenance statement. A provenance statement is valid if it contains a single subject, with a SHA256 hash.
+// ProvenanceIR also holds internal intermediate representations of data from provenances. We want to map different provenances of different build types to ProvenanceIR, so
+// all fields except for `provenanceStatement` are optional.
+type ProvenanceIR struct {


I don't see why this is moved from common to here. In particular, ProvenanceIR is an internal representation, and belongs to internal not pkg, which by convention provides the public interface.

Please move the content of this file to common, and leave only ParseStatementData in this file.

internal/verifier/verifier.go

rbehjati · 2023-01-03T10:46:51Z

pkg/amber/provenance.go

+		return nil, fmt.Errorf("could not parse the provenance bytes: %v", err)
+	}
+
+	if err := SetAmberProvenanceData(provenanceIR); err != nil {


In general we want ParseProvenanceFile and ParseStatementData (or perhaps ParseProvenanceData considering the new change) to be only different in that one uses a file as input and the other uses a byte array as input.

I suggest changing SetAmberProvenanceData to take a byte array as input, first calling types.ParseStatementData(statementBytes) and then the rest. I also suggest renaming it to ParseProvenanceData. Then in ParseProvenanceFile as before, you only need to call one function (ParseProvenanceData) and no other logic.

I am not sure SetAmberProvenanceData to take bytes as input will work with the dependencies.

I was quite struggling to get the dependencies right, but I might be missing something.

I suggest I'll first move the definition of ProvenanceIR from types to common as in the comment below and then revisit this comment.

Well yes, you don't want to introduce a dependency to internal/common in the files and packages in pkg. But if all these functions are tightly related to ProvenanceIR they should go into common too.

But having a function ParseProvenanceFile in this file returning a data structure in pkg (e.g., intoto.Statement) gives a more convenient API.

Currently types.ValidatedProvenance is the glue between internal and pkg. The only thing that types.ValidatedProvenance adds to intoto.Statement is a validity check. If we only care about this validity in contexts where ProvenanceIR is used, then we can move the validation logic to a function that creates an instance of ProvenanceIR. Then we can remove types.ValidatedProvenance, and replace all its occurrences with intoto.Statement.

Currently types.ValidatedProvenance is the glue between internal and pkg.

Yes, you are right, and I think types.ValidatedProvenance being the glue is the key.

I just tried to move ProvenanceIR to internal.common, which brings me in a full circle of where I started: needing both: ProvenanceIR and ValidatedProvenance.

Now after all this, I think we should keep both: ValidatedProvenance and ProvenanceIR.

The main reason is:

We have to go from bytes to ValidatedProvenance to ProvenanceIR. The last step, from ValidatedProvenace to ProvenanceIR depends on ValidatedProvenance.

So I do not think it a good idea to merge the two types, if we want to keep the flexibility, especially as

replace all its occurrences with intoto.Statement

Yes, that is what we would need to do, but I feel we are loosing information and control over the type in this case.

To summarize: I think we should keep ValidatedProvenance and ProvenanceIR.

However, I am not sure about the guarantees the two types give (and related their names).

Should ProvenanceIR hold the binary digest/name? More generally: do we, internally, only want to work on ProvenanceIR?

To summarize: I think we should keep ValidatedProvenance and ProvenanceIR.

Sounds good.

However, I am not sure about the guarantees the two types give (and related their names).

Should ProvenanceIR hold the binary digest/name? More generally: do we, internally, only want to work on ProvenanceIR?

I think it would be nice to do that. That would require converting ValidatedProvenance to ProvenanceIR as early as possible. Having separate fields in ProvenanceIR for digest and name makes sense to me, although IIRC, we won't have a reference value for name.

rbehjati · 2023-01-03T10:49:58Z

pkg/types/types.go

 	// The field is private so that invalid instances cannot be created.
-	provenance intoto.Statement
+	provenanceStatement      *intoto.Statement


Why do we still need to keep the provenance statement? It feels redundant.

For GetBinarySHA256Digest and GetBinaryName.

Update after discussion: We need the intoto.Statement ih the ReproducibleProvenanceVerifier for LoadBuildConfig

rbehjati · 2023-01-03T10:57:32Z

pkg/intoto/slsa_provenance/v0.2/provenance.go

@@ -200,3 +201,20 @@ func GetMaterialsGitURI(pred ProvenancePredicate) []string {
 	}
 	return gitURIs
 }
+
+// SetSLSAv02ProvenanceData sets data to verify a SLSA v02 provenance in the given ProvenanceIR.
+func SetSLSAv02ProvenanceData(provenanceIR *types.ProvenanceIR) error {


Same as above, I suggest renaming this to ParseProvenanceData, and changing it to take a byte array as input.

rbehjati · 2023-01-03T10:59:03Z

pkg/amber/provenance.go

+func SetAmberProvenanceData(provenanceIR *types.ProvenanceIR) error {
+	buildType := AmberBuildTypeV1
+
+	predicate, err := slsav02.ParseSLSAv02Predicate(provenanceIR.GetProvenance().Predicate)


If you make the other changes I suggested, then here you could call slsav02.ParseProvenanceData and reuse more of the common functionality between slsav02 and amber.

…rovenanceMetadataVerifier

mariaschett

Thanks for the review, I answered most comments.

Meta question: Are you happy with the change?

It is mainly a refactoring to get rid of the two definitions ProvenanceIR and ValidatedProvenance, but it is not strictly necessary.

I can drop it, if you prefer!

mariaschett · 2023-01-03T15:23:23Z

internal/common/common.go

-	predType := prov.PredicateType()
+// SetProvenanceData sets, depending on the predicate of the underlying provenance intoto.Statement,
+// the data for verifying the provenance.
+func SetProvenanceData(provenanceIR *types.ProvenanceIR) error {


Sure, do you think it needs to be changed?

mariaschett · 2023-01-03T15:27:18Z

internal/common/common_test.go


-	if diff := cmp.Diff(got, want, cmp.AllowUnexported(ProvenanceIR{})); diff != "" {


Hmmm... I didn't know where to get a good in-toto test statement for generating a NewProvenanceIR(provenanceStatement *intoto.Statement---so this was the quickest fix I could think of.

I see two option:

I think I could use ParseStatementData to get the *intoto.Statement from the provenanceExample path, but that seemed a bit round-about. Or am I missing a simpler solution to get a test intoto.Statement?

2.make sure the cmp.Diff does not compare the provenanceStatement, but I am not sure if that is possible.

Do you think that any of those two much better?

internal/verifier/verifier.go

mariaschett · 2023-01-03T15:59:04Z

pkg/types/types.go

 	// The field is private so that invalid instances cannot be created.
-	provenance intoto.Statement
+	provenanceStatement      *intoto.Statement


For GetBinarySHA256Digest and GetBinaryName.

mariaschett · 2023-01-03T16:10:34Z

pkg/amber/provenance.go

+		return nil, fmt.Errorf("could not parse the provenance bytes: %v", err)
+	}
+
+	if err := SetAmberProvenanceData(provenanceIR); err != nil {


I am not sure SetAmberProvenanceData to take bytes as input will work with the dependencies.

I was quite struggling to get the dependencies right, but I might be missing something.

I suggest I'll first move the definition of ProvenanceIR from types to common as in the comment below and then revisit this comment.

rbehjati · 2023-01-04T11:15:49Z

internal/common/common.go

-	predType := prov.PredicateType()
+// SetProvenanceData sets, depending on the predicate of the underlying provenance intoto.Statement,
+// the data for verifying the provenance.
+func SetProvenanceData(provenanceIR *types.ProvenanceIR) error {


Yes, and after a closer look at the code I think it is doable.

rbehjati · 2023-01-04T11:21:23Z

internal/common/common_test.go


-	if diff := cmp.Diff(got, want, cmp.AllowUnexported(ProvenanceIR{})); diff != "" {


Actually, what exactly is this test case testing? The name still says TestFromProvenance_Amber, but FromProvenance is replaced with SetProvenanceData, which is not even called in this test.

I think fixing this test will be easier once SetProvenanceData is fixed (ref my other comments on SetXYZData).

internal/verifier/verifier.go

rbehjati · 2023-01-04T11:32:41Z

pkg/amber/provenance.go

+		return nil, fmt.Errorf("could not parse the provenance bytes: %v", err)
+	}
+
+	if err := SetAmberProvenanceData(provenanceIR); err != nil {


Well yes, you don't want to introduce a dependency to internal/common in the files and packages in pkg. But if all these functions are tightly related to ProvenanceIR they should go into common too.

But having a function ParseProvenanceFile in this file returning a data structure in pkg (e.g., intoto.Statement) gives a more convenient API.

Currently types.ValidatedProvenance is the glue between internal and pkg. The only thing that types.ValidatedProvenance adds to intoto.Statement is a validity check. If we only care about this validity in contexts where ProvenanceIR is used, then we can move the validation logic to a function that creates an instance of ProvenanceIR. Then we can remove types.ValidatedProvenance, and replace all its occurrences with intoto.Statement.

rbehjati · 2023-01-04T11:33:58Z

internal/common/common_test.go

+	testutil.AssertEq(t, "builder image digest", gotBuilderImageDigest, "9e2ba52487d945504d250de186cb4fe2e3ba023ed2921dd6ac8b97ed43e76af9")
+
+	gotRepoURIs := got.GetRepoURIs()
+	testutil.AssertEq(t, "repo URI", gotRepoURIs[0], "https://github.com/project-oak/transparent-release")
 }

 func TestFromProvenance_Slsav1(t *testing.T) {


Update the name of the test.

mariaschett · 2023-01-10T10:20:09Z

Close in favour of the fresh PR #210 adding discussed changes.

mariaschett mentioned this pull request Dec 21, 2022

Generalize endorser to work on ProvenanceIR #165

Closed

rbehjati reviewed Dec 22, 2022

View reviewed changes

mariaschett force-pushed the 165-generalize-endorser branch 2 times, most recently from b579e0e to 4251b13 Compare December 30, 2022 11:03

mariaschett requested a review from rbehjati December 30, 2022 11:17

mariaschett marked this pull request as ready for review December 30, 2022 12:34

rbehjati reviewed Jan 3, 2023

View reviewed changes

mariaschett added 9 commits January 3, 2023 14:55

Refactor internal endorser

359cce7

Make ProvenanceIRVerifier depend on ProvenanceIR, remove superflous P…

e1527fb

…rovenanceMetadataVerifier

Change input of verifyConsistency to ProvenanceIR

ae96f14

Fix linter

04e0dfd

Move ProvenanceIR to types to merge with ValidatedProvenance

540c8f5

Merge ValidatedProvenance and ProvenanceIR

1cc7fc1

Fix linter

abc4a4a

Fix linter

189a90d

Fix linter

9a257c7

mariaschett commented Jan 3, 2023

View reviewed changes

rbehjati reviewed Jan 4, 2023

View reviewed changes

Move ProvenanceIR back to common

8e8012c

mariaschett force-pushed the 165-generalize-endorser branch from a765fb1 to 8e8012c Compare January 4, 2023 14:42

mariaschett closed this Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Internal Endorser #189

Refactor Internal Endorser #189

mariaschett commented Dec 21, 2022 •

edited

Loading

rbehjati Dec 22, 2022

mariaschett Dec 22, 2022 •

edited

Loading

rbehjati Jan 3, 2023

mariaschett Jan 3, 2023

rbehjati Jan 4, 2023

rbehjati Jan 3, 2023

mariaschett Jan 3, 2023

rbehjati Jan 4, 2023

rbehjati Jan 3, 2023

rbehjati Jan 3, 2023

mariaschett Jan 3, 2023 •

edited

Loading

rbehjati Jan 4, 2023

mariaschett Jan 4, 2023

rbehjati Jan 4, 2023

rbehjati Jan 3, 2023

mariaschett Jan 3, 2023

mariaschett Jan 3, 2023

rbehjati Jan 3, 2023

rbehjati Jan 3, 2023

mariaschett left a comment

mariaschett Jan 3, 2023

mariaschett Jan 3, 2023

mariaschett Jan 3, 2023

mariaschett Jan 3, 2023 •

edited

Loading

rbehjati Jan 4, 2023

rbehjati Jan 4, 2023

rbehjati Jan 4, 2023

rbehjati Jan 4, 2023

mariaschett commented Jan 10, 2023


		if diff := cmp.Diff(got, want, cmp.AllowUnexported(ProvenanceIR{})); diff != "" {

Refactor Internal Endorser #189

Refactor Internal Endorser #189

Conversation

mariaschett commented Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

mariaschett Dec 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariaschett Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariaschett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariaschett Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariaschett commented Jan 10, 2023

mariaschett commented Dec 21, 2022 •

edited

Loading

mariaschett Dec 22, 2022 •

edited

Loading

mariaschett Jan 3, 2023 •

edited

Loading

mariaschett Jan 3, 2023 •

edited

Loading