Add build cmd not empty #164

mariaschett · 2022-12-12T11:17:58Z

Add a check that build_cmd is not empty.
Refactor to use the ProvenanceMetadataVerifier.
Extract parseBuildConfig and add GetBuildCmd.

rbehjati · 2022-12-12T12:52:33Z

internal/endorser/endorser.go

@@ -58,14 +57,14 @@ func loadAndVerifyProvenances(provenanceURIs []string, referenceValues verifier.
 	}

 	// load provenances from URIs
-	provenances := make([]slsa.ValidatedProvenance, 0, len(provenanceURIs))
+	provenances := make([]amber.ValidatedProvenance, 0, len(provenanceURIs))


I intentionally changed this from amber to slsa because we are deprecating amber. Please keep slsa.

In fact, slsa.ValidatedProvenance and amber.ValidatedProvenance are the same struct, but are located in two different packages. As discussed, it is better to have it (and the ProvenanceIR) in package internal/common.

The reason I did not already put ValidatedProvenance in package internal/common was to avoid a circular dependency, and I thought we'd remove amber anyway. I can think of two ways to avoid the circular dependency:

add a package common (or a similar name) in pkg, in addition to the one we currently have in internal/commom.

Move more code from amber and slsa to internal/common to avoid the circular dependency.

Please let me know if you want to discuss more or prefer me to do this refactoring :)

Perhaps some of this generic provenance-related structure and functionality could go to pkg/provenance, and be made public as well.

Yes! :) I'll do this this in the next PR.

rbehjati · 2022-12-12T12:53:18Z

internal/verifier/verifier.go

@@ -127,7 +127,8 @@ func chdir(dir string) {
 // AmberProvenanceMetadataVerifier verifies Amber provenances by comparing the
 // content of the provenance predicate against a given set of expected values.
 type AmberProvenanceMetadataVerifier struct {
-	provenanceFilePath string
+	Got  *amber.ValidatedProvenance


Please use slsa.ValidatedProvenance here.

SG, but AmberProvenanceMetadataVerifier might a bit of a confusing name then. Should I try to add a comment/issue explaining?

I agree. I think ProvenanceMetadataVerifier is a better name. We will no longer have an Amber provenance.

rbehjati · 2022-12-12T16:20:18Z

internal/endorser/endorser.go

 	provenancesData := make([]amber.ProvenanceData, 0, len(provenanceURIs))
 	for _, uri := range provenanceURIs {
 		provenanceBytes, err := getProvenanceBytes(uri)
 		if err != nil {
 			return nil, fmt.Errorf("couldn't load the provenance file from %s: %v", uri, err)
 		}
-		provenance, err := slsa.ParseProvenanceData(provenanceBytes)
+		provenance, err := amber.ParseProvenanceData(provenanceBytes)


I think it would be nicer to also replace this with a generic function that returns a ProvenanceIR. So we don't have an explicit dependency to slsa, amber or any other package. Would that work?

I think that would be a good idea. I would propose this in a different PR.

rbehjati

Nice. Thanks.

rbehjati · 2022-12-12T18:08:41Z

internal/verifier/verifier.go

 // TODO(#69): Check metadata against the expected values.
-// TODO(#126): Refactor and separate verification logic from the logic for reading the file.


Can #126 be closed now? Or are there more references to it?

Yes, there are: The ReproducibleProvenanceVerifier.

rbehjati · 2022-12-12T18:14:08Z

internal/verifier/verifier.go

-	if err != nil {
-		return result, fmt.Errorf("provenance must have exactly one binary SHA256 digest value, got (%v)", verifier.Got.BinarySHA256Digests)
+	// Verify HasBuildCmd.
+	if verifier.Want.BuildCmds != nil {


When we load a toml file of reference values, which does not have a build command, is BuildCmds set to nil or empty array?

This is a good question! I will need to re-work the logic for reference values and would postpone the answer until than (see #166).

rbehjati · 2022-12-12T18:16:33Z

internal/verifier/verifier.go

@@ -232,3 +245,18 @@ func (got *ProvenanceIR) verifyBinarySHA256Digest(want ProvenanceIR) (Verificati

 	return result, nil
 }
+
+// verifyHasBuildCmd verifies that the build cmd is not empty.
+func (got *ProvenanceIR) verifyHasBuildCmd() (VerificationResult, error) {


Should this be renamed to verifyHasOneBuildCmd to be clear?

I would stick with HasBuildCmd, as this is what we want to check from the provenance. The additonal check (got.BuildCmds) is an internal one, which is an artifact of using lists in ProvenanceIR. I'd like to remove this check when I rework the logic (see #166)

rbehjati · 2022-12-12T18:18:58Z

internal/verifier/verifier_test.go

@@ -93,6 +93,48 @@ func TestReproducibleProvenanceVerifier_badCommand(t *testing.T) {
 	}
 }

+func TestLoadAndVerifyProvenances_HasBuildCmd(t *testing.T) {


Perhaps add a test for ProvenanceIRVerifier.Verify() where Want contains an empty array for BuildCmds. I am not sure what we'd expect in that case.

My vision with #166 would be. If we have an empty array in Want, the ProvenanceIRVerifier does not verify that value.

Will there be a way to verify that a command does not exist in the provenance?

What exactly would we want to check?

I would have thought, whether a command exists depends on the format of the provenance, e.g., for SLSA v.02, buildCmd does not exist. So I am not sure what we can check.

The way I imagined we can check on whether the value should exists for a provenance format is through reference values (#166 ).

Actually I think a valid provenance (amber or slsav1) should always have a command. If the only thing we want to check about the build command is its presence, and not its exact value, then we probably don't even need a reference value for it.

The only thing we should consider in checking hasBuildCommand is the provenance version. In particular, in slsa02 we don't set the build command.

I would have thought, whether a command exists depends on the format of the provenance, e.g., for SLSA v.02, buildCmd does not exist. So I am not sure what we can check.

True. So I don't think a reference value is very helpful in this case. Unless we want to check the exact value of the build command.

The way I imagined we can check on whether the value should exists for a provenance format is through reference values (#166 ).

I think reference values are one source of information, but the provenance format/version should be taken into account as well. Is there going to be a reference value for the accepted provenance format/version (e.g., slsa02, slsa1)?

I added a draft PR to show how I thought the check on provenanceFormat/version (version is a better work) could work #167.

NB: Sadly the changes are a bit hidden, because I need to base on this PR. I can submit this PR and we discuss on the next PR.

I think it is better to add a test for ProvenanceIRVerifier.Verify() where Want contains an empty array for BuildCmds, and merge this PR, and continue the discussion on #167.

mariaschett

Thank you!

mariaschett · 2022-12-12T19:17:06Z

internal/verifier/verifier.go

-	if err != nil {
-		return result, fmt.Errorf("provenance must have exactly one binary SHA256 digest value, got (%v)", verifier.Got.BinarySHA256Digests)
+	// Verify HasBuildCmd.
+	if verifier.Want.BuildCmds != nil {


This is a good question! I will need to re-work the logic for reference values and would postpone the answer until than (see #166).

mariaschett · 2022-12-12T19:19:34Z

internal/verifier/verifier.go

@@ -232,3 +245,18 @@ func (got *ProvenanceIR) verifyBinarySHA256Digest(want ProvenanceIR) (Verificati

 	return result, nil
 }
+
+// verifyHasBuildCmd verifies that the build cmd is not empty.
+func (got *ProvenanceIR) verifyHasBuildCmd() (VerificationResult, error) {


I would stick with HasBuildCmd, as this is what we want to check from the provenance. The additonal check (got.BuildCmds) is an internal one, which is an artifact of using lists in ProvenanceIR. I'd like to remove this check when I rework the logic (see #166)

mariaschett · 2022-12-12T20:38:18Z

internal/verifier/verifier_test.go

@@ -93,6 +93,48 @@ func TestReproducibleProvenanceVerifier_badCommand(t *testing.T) {
 	}
 }

+func TestLoadAndVerifyProvenances_HasBuildCmd(t *testing.T) {


My vision with #166 would be. If we have an empty array in Want, the ProvenanceIRVerifier does not verify that value.

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

mariaschett requested a review from rbehjati December 12, 2022 12:48

rbehjati reviewed Dec 12, 2022

View reviewed changes

mariaschett requested a review from rbehjati December 12, 2022 17:28

rbehjati approved these changes Dec 12, 2022

View reviewed changes

mariaschett commented Dec 12, 2022

View reviewed changes

mariaschett added 9 commits December 14, 2022 13:42

Add GetBuildCmd and extract parseBuildConfig

5ad9128

Add verifyHasBuildCmd

9fb4987

Implement and use AmberProvenanceMetadataVerifier

eb8f03d

Add check for non-empty build cmd

7c98ba3

Fix linting, add checks on non-empty fields

1a9bef1

Revert to slsa provenance, add unit tests for verifier of build cmds

21b6171

Remove check for build type

1073d9f

Fix naming

b42a47e

Add test with empty build cmds

af5595e

mariaschett force-pushed the add-build-cmd-not-empty branch from 4cbc54f to af5595e Compare December 14, 2022 14:05

Fix typo

daaaa75

mariaschett merged commit a7f89ea into project-oak:main Dec 14, 2022

mariaschett deleted the add-build-cmd-not-empty branch December 14, 2022 14:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add build cmd not empty #164

Add build cmd not empty #164

mariaschett commented Dec 12, 2022 •

edited

Loading

rbehjati Dec 12, 2022

mariaschett Dec 12, 2022

rbehjati Dec 12, 2022

rbehjati Dec 12, 2022

mariaschett Dec 12, 2022

rbehjati Dec 12, 2022

mariaschett Dec 12, 2022

rbehjati Dec 12, 2022

rbehjati Dec 12, 2022

mariaschett Dec 12, 2022

rbehjati left a comment

rbehjati Dec 12, 2022

mariaschett Dec 12, 2022

rbehjati Dec 12, 2022

mariaschett Dec 12, 2022

rbehjati Dec 12, 2022

mariaschett Dec 12, 2022

rbehjati Dec 12, 2022

mariaschett Dec 12, 2022 •

edited

Loading

rbehjati Dec 13, 2022

mariaschett Dec 13, 2022

rbehjati Dec 13, 2022

rbehjati Dec 13, 2022

mariaschett Dec 13, 2022

rbehjati Dec 13, 2022

mariaschett Dec 14, 2022

mariaschett left a comment

mariaschett Dec 12, 2022

mariaschett Dec 12, 2022

mariaschett Dec 12, 2022 •

edited

Loading

		// TODO(#69): Check metadata against the expected values.
		// TODO(#126): Refactor and separate verification logic from the logic for reading the file.

Add build cmd not empty #164

Add build cmd not empty #164

Conversation

mariaschett commented Dec 12, 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

Choose a reason for hiding this comment

rbehjati 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

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 Dec 12, 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

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

mariaschett Dec 12, 2022 • edited Loading

Choose a reason for hiding this comment

mariaschett commented Dec 12, 2022 •

edited

Loading

mariaschett Dec 12, 2022 •

edited

Loading

mariaschett Dec 12, 2022 •

edited

Loading