Check the hash of the binary in the verifier #124
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,14 +16,17 @@ package verify | |
|
|
||
| import ( | ||
| "os" | ||
| "strings" | ||
| "testing" | ||
|
|
||
| "github.com/project-oak/transparent-release/internal/testutil" | ||
| ) | ||
|
|
||
| const validProvenancePath = "testdata/provenance.json" | ||
| const invalidHashProvenancePath = "testdata/invalid_hash_provenance.json" | ||
| const badCommandProvenancePath = "testdata/bad_command_provenance.json" | ||
|
|
||
| func TestReproducibleProvenanceVerifier_validProvenance(t *testing.T) { | ||
| // The path to provenance is specified relative to the root of the repo, so we need to go one level up. | ||
| // Get the current directory before that to restore the path at the end of the test. | ||
| currentDir, err := os.Getwd() | ||
|
|
@@ -34,11 +37,49 @@ func TestReproducibleProvenanceVerifier(t *testing.T) { | |
| testutil.Chdir(t, "../../") | ||
| verifier := ReproducibleProvenanceVerifier{} | ||
|
|
||
| if err := verifier.Verify(validProvenancePath); err != nil { | ||
| t.Fatalf("couldn't verify the provenance file: %v", err) | ||
| } | ||
| } | ||
|
|
||
| // TODO(#126): Update the test once Verify is refactored. | ||
| func TestReproducibleProvenanceVerifier_invalidHash(t *testing.T) { | ||
| // The path to provenance is specified relative to the root of the repo, so we need to go one level up. | ||
| // Get the current directory before that to restore the path at the end of the test. | ||
|
There was a problem hiding this comment. Why does the path need to be restored? Does it mean that it affects the execution of other test cases? That would seem problematic, since the execution of the tests would differ whether they are running concurrently or not. Is it possible to make the entire process not depend on the PWD and instead use absolute paths for everything? It would still be possible here to concat PWD with a relative path to obtain an absolute path and use that, but at least that would not mutate global state. There was a problem hiding this comment. This is basically because when provenances are loaded, they are also validated against the schema. The path of the schema is hardcoded inside some other file, and getting the path right requires changing the directory. I have not been able to come up with a reliable and neat solution yet, but it certainly needs to be fixed in a separate PR. We have this bit of code in all tests, and it is causing a lot of clutter. We plan to switch to the SLSA format, and retire the amber provenance format. So it probably is better to wait for that change. I believe this problem will automatically go away when that change is completed. Created #127. There was a problem hiding this comment. +1 I think the basic functions (which are tested here, e.g. |
||
| currentDir, err := os.Getwd() | ||
| if err != nil { | ||
| t.Fatalf("couldn't get current directory: %v", err) | ||
| } | ||
| defer testutil.Chdir(t, currentDir) | ||
| testutil.Chdir(t, "../../") | ||
| verifier := ReproducibleProvenanceVerifier{} | ||
|
|
||
| want := "failed to verify the hash of the built binary" | ||
|
|
||
| if got := verifier.Verify(invalidHashProvenancePath); !strings.Contains(got.Error(), want) { | ||
| t.Fatalf("got %v, want error message containing %q,", got, want) | ||
| } | ||
| } | ||
|
|
||
| // TODO(#126): Update the test once Verify is refactored. | ||
| func TestReproducibleProvenanceVerifier_badCommand(t *testing.T) { | ||
|
There was a problem hiding this comment. Really cool test! As idea: If we need the Similar with the There was a problem hiding this comment. That is a very good idea, and I agree. Let me do this in a separate PR since doing it nicely requires some refactoring in |
||
| // The path to provenance is specified relative to the root of the repo, so we need to go one level up. | ||
| // Get the current directory before that to restore the path at the end of the test. | ||
| currentDir, err := os.Getwd() | ||
| if err != nil { | ||
| t.Fatalf("couldn't get current directory: %v", err) | ||
| } | ||
| defer testutil.Chdir(t, currentDir) | ||
| testutil.Chdir(t, "../../") | ||
| verifier := ReproducibleProvenanceVerifier{} | ||
|
|
||
| want := "couldn't build the binary" | ||
|
|
||
| if got := verifier.Verify(badCommandProvenancePath); !strings.Contains(got.Error(), want) { | ||
| t.Fatalf("got %v, want error message containing %q,", got, want) | ||
| } | ||
| } | ||
|
|
||
| func TestAmberProvenanceMetadataVerifier(t *testing.T) { | ||
| // The path to provenance is specified relative to the root of the repo, so we need to go one level up. | ||
| // Get the current directory before that to restore the path at the end of the test. | ||
|
|
@@ -50,7 +91,7 @@ func TestAmberProvenanceMetadataVerifier(t *testing.T) { | |
| testutil.Chdir(t, "../../") | ||
| verifier := AmberProvenanceMetadataVerifier{} | ||
|
|
||
| if err := verifier.Verify(validProvenancePath); err != nil { | ||
| t.Fatalf("couldn't verify the provenance file: %v", err) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would still add explicit checks here, so that if the provenance happens to be malformed (e.g. the digest empty), we will get a nice error message. But if it's already guaranteed to be covered elsewhere, feel free to ignore (though I would still be a bit worried that over time it may get out of sync).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified
ParseProvenanceFileto return aValidatedProvenance, to guarantee that the condition is already checked. Although, Golang is not very good at these things, since it is not easy to clone structs, or declare immutable variables.