Allow provenance-less endorsement generation #237

rbehjati · 2023-07-07T21:53:01Z

Fixes #236 and #234

Replaces ReferenceValues with VerificationOptions, which may or may not contain a ProvenanceReferenceValue object. The new VerificationOptions type allows invoking endorsement generation in the absence of any provenances.
To get a good balance between expressiveness and simplicity, VerificationOptions is a protobuf.
Consequently, toml files for reference value specification are replaced with textproto files.
Removes binary digest from reference values, and explicitly passes it, alongside binary name, to GenerateEndorsement. This is required to enable provenance-less endorsement generation.

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

Tests pass
Appropriate changes to README are included in PR

…rateEndorsement

mariaschett

Intermediate Comments :)

mariaschett · 2023-07-13T13:18:02Z

internal/endorser/endorser.go

-	verifiedProvenances, err := verifyAndSummarizeProvenances(referenceValues, provenances)
+// GenerateEndorsement generates an endorsement statement for the given binary
+// and the given validity duration, using the given provenances as evidence and
+// reference values to verify them. If more than one provenance statements are


What are reference values here? Are they inside the VerificationOptions?

Thanks. Updated the comment. I meant VerificationOptions.

mariaschett · 2023-07-13T13:19:30Z

internal/endorser/endorser.go

+// about a set of verified provenances, or an error. An error is returned if
+// any of the following conditions is met:
+// (1) The list of provenances is empty, but verification options contains a
+// nonempty provenance reference.


nit: what is a "provenance reference" and is it the same as a "reference provenance" in line 53?

Yes. I meant "reference provenance".

mariaschett · 2023-07-13T13:29:15Z

proto/provenance_verification.proto

+}
+
+message ProvenanceReferenceValues {
+    bool must_have_build_command = 1;


Not sure, but I think generally all fields to be optional is preferred. Happy to discuss offline!

I think you are right. The guidelines say:

Make all fields optional or repeated. You never know how long a message type is going to last and whether someone will be forced to fill in your required field with an empty string or zero in four years when it’s no longer logically required but the proto still says it is.

But I think for primitive fields it should be fine. Even though I have not made them explicitly optional, I can leave them unset.

Making them optional causes additional an unnecessary indirection, which does not look right.

In proto3, everything is optional by default.

I removed optional from other fields too. Here is the change in the generated Go code. Some oneof annotation and a oneof wrapper are removed. I am not sure if it has any serious impact on serialization.

rbehjati

Thanks for the review.

rbehjati · 2023-07-13T15:52:57Z

proto/provenance_verification.proto

+}
+
+message ProvenanceReferenceValues {
+    bool must_have_build_command = 1;


I think you are right. The guidelines say:

Make all fields optional or repeated. You never know how long a message type is going to last and whether someone will be forced to fill in your required field with an empty string or zero in four years when it’s no longer logically required but the proto still says it is.

But I think for primitive fields it should be fine. Even though I have not made them explicitly optional, I can leave them unset.

Making them optional causes additional an unnecessary indirection, which does not look right.

rbehjati · 2023-07-13T15:59:59Z

internal/endorser/endorser.go

-	verifiedProvenances, err := verifyAndSummarizeProvenances(referenceValues, provenances)
+// GenerateEndorsement generates an endorsement statement for the given binary
+// and the given validity duration, using the given provenances as evidence and
+// reference values to verify them. If more than one provenance statements are


Thanks. Updated the comment. I meant VerificationOptions.

rbehjati · 2023-07-13T16:02:12Z

internal/endorser/endorser.go

+// about a set of verified provenances, or an error. An error is returned if
+// any of the following conditions is met:
+// (1) The list of provenances is empty, but verification options contains a
+// nonempty provenance reference.


Yes. I meant "reference provenance".

mariaschett

I might be missing something on how SkipProvenanceVerification and the list of provenances interact. Could you have a look and let me know?

mariaschett · 2023-07-17T08:36:36Z

internal/endorser/endorser.go

+// VerificationOptions to verify them. If more than one provenance statements
+// are provided the endorsement statement is generated only if the provenance
+// statements are consistent and valid according to the input
+// VerificationOptions. If no provenances are provided, a provenance-less


A design question: might it happen, that I accidentially endorse something if I forget to add a provenance? Do we want to make this more explict? E.g., make GenerateEndorsement private but expose two methods GenerateEndorsementWithProvenance and GenerateEndorsementWithoutProvenance?

That is being handled by the VerificationOptions. It requires explicitly setting skip provenance verification. If that is not set, at least one provenance is required.

So I'd say those accidents are unlikely.

mariaschett · 2023-07-17T08:43:04Z

internal/endorser/endorser.go

+// endorsement is generated, if the input VerificationOptions does not contain
+// a reference provenance.
+func GenerateEndorsement(binaryName, binaryDigest string, verOpt *prover.VerificationOptions, validityDuration claims.ClaimValidity, provenances []ParsedProvenance) (*intoto.Statement, error) {
+	if (verOpt.GetSkipProvenanceVerification() == nil) && (verOpt.GetReferenceProvenance() == nil) {


Just checking: Don't you get from oneof in the proto definition that exactly one of verOpt is not nil?

No. I think this is a shortcoming of Go. I was able to create an instance where both options are null, so I had to add this check.

Ah, thanks! I'd have expected...

mariaschett · 2023-07-17T08:51:36Z

internal/endorser/endorser.go

@@ -73,22 +83,35 @@ func verifyAndSummarizeProvenances(referenceValues *verification.ReferenceValues
 		provenancesData = append(provenancesData, p.SourceMetadata)
 	}

-	errs := multierr.Append(verifyConsistency(provenanceIRs), verifyProvenances(referenceValues, provenanceIRs))
+	var errs error
+	if len(provenanceIRs) > 0 {


I am trying to understand what happens if we have multiple provenances but SkipProvenanceVerification.

IIUC the control flow lands here, but then GetReferenceProvenance would be nil? Is this okay with verifyProvenances?

On a higher level: What do we expect to happen then?

We expect the provenances to be consistent, but they are not verified against a set of reference values. Added tests to cover these scenarios.

Thanks, sounds good!

Just to double check: In the GetReferenceProvenance case, verifyProvenances will get nil for GetReferenceProvenance.

Good catch. Yes. verifyProvenances is called with a nil. Added a check to immediately return if the input ProvenanceReferenceValues is nil.

mariaschett · 2023-07-17T08:57:34Z

internal/endorser/endorser_test.go

@@ -69,7 +71,67 @@ func TestGenerateEndorsement_SingleValidEndorsement(t *testing.T) {
 	testutil.AssertEq(t, "notAfter date", predicate.Validity.NotAfter, &nextWeek)
 }

-func TestLoadAndVerifyProvenances_MultipleValidEndorsement(t *testing.T) {
+func TestGenerateEndorsement_NoProvenance(t *testing.T) {


What happens if you give provenances and also set SkipProvenanceVerification?

Added a few tests and updated the documentation on GenerateEndorsement. It generates an endorsement with the given provenances as evidence, if the provenances are consistent and match the given subject.

Let me know if you expect some other behavior.

Hmmm... I am not sure I would expect "unverified provenances" as evidence, but actually: why not?

However, is it clear afterwards if the provenances were verified? Or does that not really matter?

They are not totally unverified. But they are not verified against a set of reference values. Actually this is similar to providing verification options with an empty ReferenceProvenance. Except that the latter enforces having some provenances. Clarified this, and renamed SkipVerification to EndorseProvenanceLess to be clear that we are not skipping the entire verification logic.

However, is it clear afterwards if the provenances were verified? Or does that not really matter?

Currently we don't include the verification options in the endorsement, but I think it is a good idea to include it. As an extension. Created #238.

tiziano88 · 2023-07-13T17:59:56Z

docs/development-guidelines.md

@@ -23,3 +23,27 @@ You need to have:
 - Format files: `./scripts/formatting.sh`
 - Check linting: `./scripts/linting.sh`
 - Additional checks: `go vet ./...`
+
+## Using protocol buffers


FYI this is mostly solved by bazel and / or nix. Especially since the instructions here refer to latest, which will stop working at some point.

Thanks. We used to use bazel in this repo, and it caused a lot of problems. I think this is fine for now.

tiziano88 · 2023-07-13T18:01:12Z

internal/endorser/endorser.go

@@ -32,6 +32,7 @@ import (
 	"github.com/project-oak/transparent-release/internal/verification"
 	"github.com/project-oak/transparent-release/pkg/claims"
 	"github.com/project-oak/transparent-release/pkg/intoto"
+	prover "github.com/project-oak/transparent-release/pkg/proto/verification"


Having two different packages with the same name but in different locations in the same repo seems problematic. Maybe one of them should be renamed at some point

I agree, but I cannot think of a better name. I don't like long names with underscore in them.
Do you have any suggestions?

Renamed the other verification into verifier. The only files in it now are verifier.go and verifier_test.go so I think that makes sense.

tiziano88 · 2023-07-13T18:02:54Z

internal/endorser/endorser_test.go

-	binaryHash        = "d059c38cea82047ad316a1c6c6fbd13ecf7a0abdcc375463920bd25bf5c142cc"
-	binaryName        = "oak_functions_freestanding_bin"
-	errorBinaryDigest = "do not contain the actual binary SHA256 digest"
+	binaryHash                   = "d059c38cea82047ad316a1c6c6fbd13ecf7a0abdcc375463920bd25bf5c142cc"


Suggested change

binaryHash = "d059c38cea82047ad316a1c6c6fbd13ecf7a0abdcc375463920bd25bf5c142cc"

binaryDigestSha256 = "d059c38cea82047ad316a1c6c6fbd13ecf7a0abdcc375463920bd25bf5c142cc"

for consistency

tiziano88 · 2023-07-17T10:34:20Z

golangci-linters.yaml

@@ -65,7 +65,7 @@ linters:
    - noctx
    - nolintlint
    - nonamedreturns
-    - nosnakecase
+    # - nosnakecase


The template kept these as commented out. I think this is for convenience to be able to enable or disable them easily.

tiziano88 · 2023-07-17T10:51:53Z

pkg/claims/endorsement_test.go

@@ -68,6 +68,25 @@ func TestNotAfterBeforeNotBeforeEndorsement(t *testing.T) {
 	}
 }

+func TestGenerateProvenanceLessEndorsement(t *testing.T) {
+	newNotBefore := time.Now().AddDate(0, 0, 1)


Ideally the tests (and the functions under tests) would not rely on the current time, but would accept a reference time from the outside. This will make them easier to test (without having to depend on relative times), and also avoid actual bugs (TOCTOU and related issues, if multiple functions get their own time.Now() independently at slightly different times).

For the endorsement to be valid the notBefore time should be after the issued time, which uses the current time. For the test to pass, we have to use the current time here.

tiziano88 · 2023-07-17T10:54:26Z

proto/provenance_verification.proto

+// An allow list of digests, represented as a mapping from cryptographic hash
+// function names, to their allow listed values. 
+message Digests {
+    // Keys are cryptographic hash algorithms (e.g., sha256).


I thought we were using the numeric multihash id somewhere else (from https://github.com/multiformats/multicodec/blob/master/table.csv ).

in particular sha256 is ambiguous; it should be sha2-256 or something like that, to be unambiguos -- but the numeric id solves that problem (without having to actual implement full multihash -- I don't think we should do that for now).

Not in this repo. This repo follows the in-toto and SLSA style of using digests. We can change that in the future, but that is out of the scope of this PR.

rbehjati

Thanks for the reviews.

rbehjati · 2023-07-17T10:38:01Z

internal/endorser/endorser.go

+// VerificationOptions to verify them. If more than one provenance statements
+// are provided the endorsement statement is generated only if the provenance
+// statements are consistent and valid according to the input
+// VerificationOptions. If no provenances are provided, a provenance-less


That is being handled by the VerificationOptions. It requires explicitly setting skip provenance verification. If that is not set, at least one provenance is required.

So I'd say those accidents are unlikely.

rbehjati · 2023-07-17T10:39:27Z

internal/endorser/endorser.go

+// endorsement is generated, if the input VerificationOptions does not contain
+// a reference provenance.
+func GenerateEndorsement(binaryName, binaryDigest string, verOpt *prover.VerificationOptions, validityDuration claims.ClaimValidity, provenances []ParsedProvenance) (*intoto.Statement, error) {
+	if (verOpt.GetSkipProvenanceVerification() == nil) && (verOpt.GetReferenceProvenance() == nil) {


No. I think this is a shortcoming of Go. I was able to create an instance where both options are null, so I had to add this check.

rbehjati · 2023-07-17T10:40:40Z

internal/endorser/endorser.go

@@ -73,22 +83,35 @@ func verifyAndSummarizeProvenances(referenceValues *verification.ReferenceValues
 		provenancesData = append(provenancesData, p.SourceMetadata)
 	}

-	errs := multierr.Append(verifyConsistency(provenanceIRs), verifyProvenances(referenceValues, provenanceIRs))
+	var errs error
+	if len(provenanceIRs) > 0 {


We expect the provenances to be consistent, but they are not verified against a set of reference values. Added tests to cover these scenarios.

rbehjati · 2023-07-17T10:56:06Z

internal/endorser/endorser_test.go

@@ -69,7 +71,67 @@ func TestGenerateEndorsement_SingleValidEndorsement(t *testing.T) {
 	testutil.AssertEq(t, "notAfter date", predicate.Validity.NotAfter, &nextWeek)
 }

-func TestLoadAndVerifyProvenances_MultipleValidEndorsement(t *testing.T) {
+func TestGenerateEndorsement_NoProvenance(t *testing.T) {


Added a few tests and updated the documentation on GenerateEndorsement. It generates an endorsement with the given provenances as evidence, if the provenances are consistent and match the given subject.

Let me know if you expect some other behavior.

rbehjati · 2023-07-17T11:19:10Z

docs/development-guidelines.md

@@ -23,3 +23,27 @@ You need to have:
 - Format files: `./scripts/formatting.sh`
 - Check linting: `./scripts/linting.sh`
 - Additional checks: `go vet ./...`
+
+## Using protocol buffers


Thanks. We used to use bazel in this repo, and it caused a lot of problems. I think this is fine for now.

rbehjati · 2023-07-17T11:20:28Z

golangci-linters.yaml

@@ -65,7 +65,7 @@ linters:
    - noctx
    - nolintlint
    - nonamedreturns
-    - nosnakecase
+    # - nosnakecase


The template kept these as commented out. I think this is for convenience to be able to enable or disable them easily.

rbehjati · 2023-07-17T11:23:16Z

internal/endorser/endorser_test.go

-	binaryHash        = "d059c38cea82047ad316a1c6c6fbd13ecf7a0abdcc375463920bd25bf5c142cc"
-	binaryName        = "oak_functions_freestanding_bin"
-	errorBinaryDigest = "do not contain the actual binary SHA256 digest"
+	binaryHash                   = "d059c38cea82047ad316a1c6c6fbd13ecf7a0abdcc375463920bd25bf5c142cc"


rbehjati · 2023-07-17T11:24:02Z

pkg/claims/endorsement_test.go

@@ -68,6 +68,25 @@ func TestNotAfterBeforeNotBeforeEndorsement(t *testing.T) {
 	}
 }

+func TestGenerateProvenanceLessEndorsement(t *testing.T) {
+	newNotBefore := time.Now().AddDate(0, 0, 1)


For the endorsement to be valid the notBefore time should be after the issued time, which uses the current time. For the test to pass, we have to use the current time here.

rbehjati · 2023-07-17T11:25:04Z

proto/provenance_verification.proto

+// An allow list of digests, represented as a mapping from cryptographic hash
+// function names, to their allow listed values. 
+message Digests {
+    // Keys are cryptographic hash algorithms (e.g., sha256).


Not in this repo. This repo follows the in-toto and SLSA style of using digests. We can change that in the future, but that is out of the scope of this PR.

rbehjati · 2023-07-17T11:26:30Z

internal/endorser/endorser.go

@@ -32,6 +32,7 @@ import (
 	"github.com/project-oak/transparent-release/internal/verification"
 	"github.com/project-oak/transparent-release/pkg/claims"
 	"github.com/project-oak/transparent-release/pkg/intoto"
+	prover "github.com/project-oak/transparent-release/pkg/proto/verification"


I agree, but I cannot think of a better name. I don't like long names with underscore in them.
Do you have any suggestions?

mariaschett

Looks good, added a couple more comments, where I wasn't 100% sure.

mariaschett · 2023-07-17T12:23:28Z

internal/endorser/endorser.go

+// endorsement is generated, if the input VerificationOptions does not contain
+// a reference provenance.
+func GenerateEndorsement(binaryName, binaryDigest string, verOpt *prover.VerificationOptions, validityDuration claims.ClaimValidity, provenances []ParsedProvenance) (*intoto.Statement, error) {
+	if (verOpt.GetSkipProvenanceVerification() == nil) && (verOpt.GetReferenceProvenance() == nil) {


Ah, thanks! I'd have expected...

mariaschett · 2023-07-17T12:33:32Z

internal/endorser/endorser_test.go

@@ -69,7 +71,67 @@ func TestGenerateEndorsement_SingleValidEndorsement(t *testing.T) {
 	testutil.AssertEq(t, "notAfter date", predicate.Validity.NotAfter, &nextWeek)
 }

-func TestLoadAndVerifyProvenances_MultipleValidEndorsement(t *testing.T) {
+func TestGenerateEndorsement_NoProvenance(t *testing.T) {


Hmmm... I am not sure I would expect "unverified provenances" as evidence, but actually: why not?

However, is it clear afterwards if the provenances were verified? Or does that not really matter?

mariaschett · 2023-07-17T12:37:32Z

internal/endorser/endorser.go

@@ -73,22 +83,35 @@ func verifyAndSummarizeProvenances(referenceValues *verification.ReferenceValues
 		provenancesData = append(provenancesData, p.SourceMetadata)
 	}

-	errs := multierr.Append(verifyConsistency(provenanceIRs), verifyProvenances(referenceValues, provenanceIRs))
+	var errs error
+	if len(provenanceIRs) > 0 {


Thanks, sounds good!

Just to double check: In the GetReferenceProvenance case, verifyProvenances will get nil for GetReferenceProvenance.

mariaschett · 2023-07-17T12:38:37Z

internal/endorser/endorser.go

 		Provenances:  provenancesData,
 	}

 	return &verifiedProvenances, nil
 }

 // verifyProvenances verifies the given list of provenances. An error is returned if verification fails for one of them.
-func verifyProvenances(referenceValues *verification.ReferenceValues, provenances []model.ProvenanceIR) error {
+func verifyProvenances(referenceValues *prover.ProvenanceReferenceValues, provenances []model.ProvenanceIR) error {


Maybe add a comment what happens if prover.ProvenanceReferenceValues == nil?

rbehjati added 4 commits July 7, 2023 22:51

Replace ReferenceValues with proto and allow no provenance verification

446f030

Fix linting

8865be5

Add test for provenance-less endorsement

303fa54

Simplify verifier, and explicitly pass binary name and digest to Gene…

5e0eb9b

…rateEndorsement

rbehjati force-pushed the pro-ver branch from 91f9f37 to 5e0eb9b Compare July 12, 2023 16:21

Add test for provenance-less endorsement generation

62c6d0e

rbehjati requested review from tiziano88 and mariaschett July 12, 2023 16:50

rbehjati changed the title ~~Replace ReferenceValues with proto and allow no provenance verification~~ Allow provenance-less endorsement generation Jul 12, 2023

mariaschett reviewed Jul 13, 2023

View reviewed changes

Make skip verification an explicit field in the proto

a1fcaa7

rbehjati commented Jul 13, 2023

View reviewed changes

rbehjati added 2 commits July 14, 2023 09:16

Add missing test file, and disable nosnakecase lint errors

726084f

Remove superficial optionals from proto definition

d3f2a6e

rbehjati requested a review from mariaschett July 14, 2023 12:07

mariaschett reviewed Jul 17, 2023

View reviewed changes

tiziano88 approved these changes Jul 17, 2023

View reviewed changes

rbehjati added 2 commits July 17, 2023 12:09

Add more tests

fc1288f

Fixes after review

95f0cc3

rbehjati commented Jul 17, 2023

View reviewed changes

Rename package internal/verification to internal/verifier

31a9063

mariaschett approved these changes Jul 17, 2023

View reviewed changes

Rename SkipVerification to EndorseProvenanceLess

5102936

rbehjati force-pushed the pro-ver branch from 3b45e17 to 5102936 Compare July 17, 2023 16:37

cleanup

38ce654

rbehjati merged commit efb5eb8 into project-oak:main Jul 17, 2023

rbehjati deleted the pro-ver branch July 17, 2023 18:46

	binaryHash = "d059c38cea82047ad316a1c6c6fbd13ecf7a0abdcc375463920bd25bf5c142cc"
	binaryDigestSha256 = "d059c38cea82047ad316a1c6c6fbd13ecf7a0abdcc375463920bd25bf5c142cc"

Allow provenance-less endorsement generation #237

Allow provenance-less endorsement generation #237

Conversation

rbehjati commented Jul 7, 2023 • edited Loading

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

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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rbehjati commented Jul 7, 2023 •

edited

Loading