Repository: sigstore/rekor (go)

Issue:
IndexKeys is run on pre-canonicalized entries
**Description**

<!--Tell us about the bug. What went wrong? How can we reproduce it? What should the correct behavior be?
-->

**Version**

<!--What version are you using? e.g. `cosign version`...
-->

The flow of creating a log entry is curious to me:
https://github.com/sigstore/rekor/blob/4b934999dc28386d22dfa3004930767d203e7580/pkg/api/entries.go#L173

1. A new entry is created based on the parameter proposed entry.
2. A leaf node is created by canonicalizing the entry
3. The original entry calls index keys.

In many entry types, logic is performed in `Canonicalize` to set up the entry, and it's not clear that `IndexKeys`  does not have access to the canonicalized entry fields. This is also a problem if Canonicalize does some operation that may modify IndexKeys.

I realized this was the root cause to the bug that @priyawadhwa fixed from https://github.com/sigstore/rekor/pull/800 where assumed that intoto entries populated the Content Hash (but only canonicalized ones did).

Shouldn't `IndexKeys` operate on the canonicalized entry?


Pull request:
compute payload and envelope hashes upon validating intoto proposed entries
This ensures that the server-side values used by `IndexKeys()` are computed during validation time rather than deferring it to when the indices are requested. This also ensures that client-provided values for these digests are not used when computing the list of indices. 

Fixes #854 

Patch:
diff --git a/pkg/api/metrics.go b/pkg/api/metrics.go
index 2e33e32..02a89d7 100644
--- a/pkg/api/metrics.go
+++ b/pkg/api/metrics.go
@@ -30,4 +30,9 @@ var (
 		Name: "rekor_api_latency",
 		Help: "Api Latency on calls",
 	}, []string{"path", "code"})
+
+	MetricLatencySummary = promauto.NewSummaryVec(prometheus.SummaryOpts{
+		Name: "rekor_api_latency_summary",
+		Help: "Api Latency on calls",
+	}, []string{"path", "code"})
 )
diff --git a/pkg/generated/restapi/configure_rekor_server.go b/pkg/generated/restapi/configure_rekor_server.go
index 18ef50e..61be285 100644
--- a/pkg/generated/restapi/configure_rekor_server.go
+++ b/pkg/generated/restapi/configure_rekor_server.go
@@ -167,11 +167,13 @@ func wrapMetrics(handler http.Handler) http.Handler {
 		start := time.Now()
 		ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor)
 		defer func() {
-			// This logs latency broken down by URL path and response code
-			pkgapi.MetricLatency.With(map[string]string{
+			labels := map[string]string{
 				"path": r.URL.Path,
 				"code": strconv.Itoa(ww.Status()),
-			}).Observe(float64(time.Since(start)))
+			}
+			// This logs latency broken down by URL path and response code
+			pkgapi.MetricLatency.With(labels).Observe(float64(time.Since(start)))
+			pkgapi.MetricLatencySummary.With(labels).Observe(float64(time.Since(start)))
 		}()
 
 		handler.ServeHTTP(ww, r)
diff --git a/pkg/types/intoto/v0.0.1/entry.go b/pkg/types/intoto/v0.0.1/entry.go
index 4c93ddb..49e42cc 100644
--- a/pkg/types/intoto/v0.0.1/entry.go
+++ b/pkg/types/intoto/v0.0.1/entry.go
@@ -98,12 +98,11 @@ func (v V001Entry) IndexKeys() ([]string, error) {
 	}
 
 	// add digest base64-decoded payload inside of DSSE envelope
-	payloadBytes, err := base64.StdEncoding.DecodeString(v.env.Payload)
-	if err == nil {
-		payloadHash := sha256.Sum256(payloadBytes)
-		result = append(result, fmt.Sprintf("sha256:%s", strings.ToLower(hex.EncodeToString(payloadHash[:]))))
+	if v.IntotoObj.Content != nil && v.IntotoObj.Content.PayloadHash != nil {
+		payloadHash := strings.ToLower(fmt.Sprintf("%s:%s", swag.StringValue(v.IntotoObj.Content.PayloadHash.Algorithm), swag.StringValue(v.IntotoObj.Content.PayloadHash.Value)))
+		result = append(result, payloadHash)
 	} else {
-		log.Logger.Errorf("error decoding intoto payload to compute digest: %w", err)
+		log.Logger.Error("could not find payload digest to include in index keys")
 	}
 
 	switch v.env.PayloadType {
@@ -194,23 +193,19 @@ func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
 	}
 	pkb := strfmt.Base64(pk)
 
-	h := sha256.Sum256([]byte(v.IntotoObj.Content.Envelope))
-
 	canonicalEntry := models.IntotoV001Schema{
 		PublicKey: &pkb,
 		Content: &models.IntotoV001SchemaContent{
 			Hash: &models.IntotoV001SchemaContentHash{
-				Algorithm: swag.String(models.IntotoV001SchemaContentHashAlgorithmSha256),
-				Value:     swag.String(hex.EncodeToString(h[:])),
+				Algorithm: v.IntotoObj.Content.Hash.Algorithm,
+				Value:     v.IntotoObj.Content.Hash.Value,
+			},
+			PayloadHash: &models.IntotoV001SchemaContentPayloadHash{
+				Algorithm: v.IntotoObj.Content.PayloadHash.Algorithm,
+				Value:     v.IntotoObj.Content.PayloadHash.Value,
 			},
 		},
 	}
-	if attKey, attValue := v.AttestationKeyValue(); attValue != nil {
-		canonicalEntry.Content.PayloadHash = &models.IntotoV001SchemaContentPayloadHash{
-			Algorithm: swag.String(models.IntotoV001SchemaContentHashAlgorithmSha256),
-			Value:     swag.String(strings.Replace(attKey, fmt.Sprintf("%s:", models.IntotoV001SchemaContentHashAlgorithmSha256), "", 1)),
-		}
-	}
 
 	itObj := models.Intoto{}
 	itObj.APIVersion = swag.String(APIVERSION)
@@ -237,7 +232,27 @@ func (v *V001Entry) validate() error {
 	if err := dsseVerifier.VerifySignature(strings.NewReader(v.IntotoObj.Content.Envelope), nil); err != nil {
 		return err
 	}
-	return json.Unmarshal([]byte(v.IntotoObj.Content.Envelope), &v.env)
+	if err := json.Unmarshal([]byte(v.IntotoObj.Content.Envelope), &v.env); err != nil {
+		return err
+	}
+
+	attBytes, err := base64.StdEncoding.DecodeString(v.env.Payload)
+	if err != nil {
+		return err
+	}
+	// validation logic complete without errors, hydrate local object
+	attHash := sha256.Sum256(attBytes)
+	v.IntotoObj.Content.PayloadHash = &models.IntotoV001SchemaContentPayloadHash{
+		Algorithm: swag.String(models.IntotoV001SchemaContentPayloadHashAlgorithmSha256),
+		Value:     swag.String(hex.EncodeToString(attHash[:])),
+	}
+
+	h := sha256.Sum256([]byte(v.IntotoObj.Content.Envelope))
+	v.IntotoObj.Content.Hash = &models.IntotoV001SchemaContentHash{
+		Algorithm: swag.String(models.IntotoV001SchemaContentHashAlgorithmSha256),
+		Value:     swag.String(hex.EncodeToString(h[:])),
+	}
+	return nil
 }
 
 // AttestationKey returns the digest of the attestation that was uploaded, to be used to lookup the attestation from storage
@@ -256,9 +271,7 @@ func (v *V001Entry) AttestationKeyValue() (string, []byte) {
 		return "", nil
 	}
 	attBytes, _ := base64.StdEncoding.DecodeString(v.env.Payload)
-	attHash := sha256.Sum256(attBytes)
-	attKey := fmt.Sprintf("%s:%s", models.IntotoV001SchemaContentHashAlgorithmSha256, hex.EncodeToString(attHash[:]))
-	return attKey, attBytes
+	return v.AttestationKey(), attBytes
 }
 
 func (v V001Entry) CreateFromArtifactProperties(_ context.Context, props types.ArtifactProperties) (models.ProposedEntry, error) {


Test patch:
diff --git a/pkg/types/intoto/v0.0.1/entry_test.go b/pkg/types/intoto/v0.0.1/entry_test.go
index b876680..5d4151e 100644
--- a/pkg/types/intoto/v0.0.1/entry_test.go
+++ b/pkg/types/intoto/v0.0.1/entry_test.go
@@ -28,6 +28,7 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"math/big"
 	"reflect"
@@ -240,6 +241,10 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 				if err := v.validate(); err != nil {
 					return err
 				}
+				if v.IntotoObj.Content.Hash == nil || v.IntotoObj.Content.Hash.Algorithm != tt.it.Content.Hash.Algorithm || v.IntotoObj.Content.Hash.Value != tt.it.Content.Hash.Value {
+					return errors.New("missing envelope hash in validated object")
+				}
+
 				keysWanted := tt.additionalIndexKeys
 				if tt.it.PublicKey != nil {
 					h := sha256.Sum256(*tt.it.PublicKey)
@@ -252,7 +257,8 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 				keysWanted = append(keysWanted, "sha256:"+payloadHash)
 				hashkey := strings.ToLower(fmt.Sprintf("%s:%s", *tt.it.Content.Hash.Algorithm, *tt.it.Content.Hash.Value))
 				keysWanted = append(keysWanted, hashkey)
-				if got, _ := v.IndexKeys(); !cmp.Equal(got, keysWanted, cmpopts.SortSlices(func(x, y string) bool { return x < y })) {
+				got, _ := v.IndexKeys()
+				if !cmp.Equal(got, keysWanted, cmpopts.SortSlices(func(x, y string) bool { return x < y })) {
 					t.Errorf("V001Entry.IndexKeys() = %v, want %v", got, keysWanted)
 				}
 				canonicalBytes, err := v.Canonicalize(context.Background())
@@ -276,6 +282,10 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 				if canonicalV001.AttestationKey() != "" && *canonicalV001.IntotoObj.Content.PayloadHash.Value != payloadHash {
 					t.Errorf("payload hashes do not match post canonicalization: %v %v", canonicalV001.IntotoObj.Content.PayloadHash.Value, payloadHash)
 				}
+				canonicalIndexKeys, _ := canonicalV001.IndexKeys()
+				if !cmp.Equal(got, canonicalIndexKeys, cmpopts.SortSlices(func(x, y string) bool { return x < y })) {
+					t.Errorf("index keys from hydrated object do not match those generated from canonicalized (and re-hydrated) object: %v %v", got, canonicalIndexKeys)
+				}
 
 				return nil
 			}
@@ -343,6 +353,7 @@ func TestV001Entry_IndexKeys(t *testing.T) {
 				t.Fatal(err)
 			}
 			payload := base64.StdEncoding.EncodeToString(b)
+			payloadHash := sha256.Sum256(b)
 			v := V001Entry{
 				IntotoObj: models.IntotoV001Schema{
 					Content: &models.IntotoV001SchemaContent{
@@ -350,6 +361,10 @@ func TestV001Entry_IndexKeys(t *testing.T) {
 							Algorithm: swag.String(models.IntotoV001SchemaContentHashAlgorithmSha256),
 							Value:     swag.String(dataSHA),
 						},
+						PayloadHash: &models.IntotoV001SchemaContentPayloadHash{
+							Algorithm: swag.String(models.IntotoV001SchemaContentPayloadHashAlgorithmSha256),
+							Value:     swag.String(hex.EncodeToString(payloadHash[:])),
+						},
 					},
 				},
 				env: dsse.Envelope{
@@ -370,43 +385,3 @@ func TestV001Entry_IndexKeys(t *testing.T) {
 		})
 	}
 }
-
-func TestIndexKeysNoContentHash(t *testing.T) {
-	statement := in_toto.Statement{
-		Predicate: "hello",
-		StatementHeader: in_toto.StatementHeader{
-			Subject: []in_toto.Subject{
-				{
-					Name: "myimage",
-					Digest: slsa.DigestSet{
-						"sha256": "mysha256digest",
-					},
-				},
-			},
-		},
-	}
-	b, err := json.Marshal(statement)
-	if err != nil {
-		t.Fatal(err)
-	}
-	payload := base64.StdEncoding.EncodeToString(b)
-	v := V001Entry{
-		env: dsse.Envelope{
-			Payload:     payload,
-			PayloadType: in_toto.PayloadType,
-		},
-	}
-	sha := sha256.Sum256(b)
-	// Always start with the hash
-	want := []string{"sha256:" + hex.EncodeToString(sha[:])}
-	want = append(want, "sha256:mysha256digest")
-	got, err := v.IndexKeys()
-	if err != nil {
-		t.Fatal(err)
-	}
-	sort.Strings(got)
-	sort.Strings(want)
-	if !cmp.Equal(got, want) {
-		t.Errorf("V001Entry.IndexKeys() = %v, want %v", got, want)
-	}
-}