Repository: sigstore/rekor
Pull request #889 (2022-06-28 17:02:05)

Fix intoto index keys
Ensure we include all appropriate index keys including:
- entire DSSE envelope SHA256 digest
- envelope (base64 decoded) SHA256 digest
- entire X509 signing certificate
- any relevant keys extracted from X509 signing certificate

Fixes: #890 
Fixes: #876

Signed-off-by: Bob Callaway <bcallaway@google.com>


codecov-commenter: # [Codecov](https://codecov.io/gh/sigstore/rekor/pull/889?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) Report
> Merging [#889](https://codecov.io/gh/sigstore/rekor/pull/889?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) (ceb43c5) into [main](https://codecov.io/gh/sigstore/rekor/commit/89363a575d38ae99525737849dcf0783999b1c88?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) (89363a5) will **increase** coverage by `0.66%`.
> The diff coverage is `58.82%`.

```diff
@@            Coverage Diff             @@
##             main     #889      +/-   ##
==========================================
+ Coverage   46.99%   47.65%   +0.66%     
==========================================
  Files          62       62              
  Lines        5413     5426      +13     
==========================================
+ Hits         2544     2586      +42     
+ Misses       2587     2554      -33     
- Partials      282      286       +4     
```


| [Impacted Files](https://codecov.io/gh/sigstore/rekor/pull/889?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore) | Coverage Δ | |
|---|---|---|
| [pkg/types/intoto/v0.0.1/entry.go](https://codecov.io/gh/sigstore/rekor/pull/889/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3R5cGVzL2ludG90by92MC4wLjEvZW50cnkuZ28=) | `51.75% <58.82%> (+15.01%)` | :arrow_up: |
| [pkg/types/alpine/v0.0.1/entry.go](https://codecov.io/gh/sigstore/rekor/pull/889/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore#diff-cGtnL3R5cGVzL2FscGluZS92MC4wLjEvZW50cnkuZ28=) | `55.78% <0.00%> (+1.23%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/sigstore/rekor/pull/889?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/sigstore/rekor/pull/889?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore). Last update [89363a5...ceb43c5](https://codecov.io/gh/sigstore/rekor/pull/889?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=sigstore).

bobcallaway: /hold

Patch:
diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml
index 00fe33a..17f6bb1 100644
--- a/.github/workflows/codeql-analysis.yml
+++ b/.github/workflows/codeql-analysis.yml
@@ -43,12 +43,12 @@ jobs:
 
     # Initializes the CodeQL tools for scanning.
     - name: Initialize CodeQL
-      uses: github/codeql-action/init@41a4ada31ba866a7f1196b9602703a89edd69e22 # v2.1.14
+      uses: github/codeql-action/init@3f62b754e23e0dd60f91b744033e1dc1654c0ec6 # v2.1.14
       with:
         languages: ${{ matrix.language }}
 
     - name: Autobuild
-      uses: github/codeql-action/autobuild@41a4ada31ba866a7f1196b9602703a89edd69e22 # v2.1.14
+      uses: github/codeql-action/autobuild@3f62b754e23e0dd60f91b744033e1dc1654c0ec6 # v2.1.14
 
     - name: Perform CodeQL Analysis
-      uses: github/codeql-action/analyze@41a4ada31ba866a7f1196b9602703a89edd69e22 # v2.1.14
+      uses: github/codeql-action/analyze@3f62b754e23e0dd60f91b744033e1dc1654c0ec6 # v2.1.14
diff --git a/.github/workflows/scorecard_action.yml b/.github/workflows/scorecard_action.yml
index 9bb3e10..b1960c8 100644
--- a/.github/workflows/scorecard_action.yml
+++ b/.github/workflows/scorecard_action.yml
@@ -52,6 +52,6 @@ jobs:
 
       # Upload the results to GitHub's code scanning dashboard.
       - name: "Upload to code-scanning"
-        uses: github/codeql-action/upload-sarif@41a4ada31ba866a7f1196b9602703a89edd69e22 # v2.1.14
+        uses: github/codeql-action/upload-sarif@3f62b754e23e0dd60f91b744033e1dc1654c0ec6 # v2.1.14
         with:
           sarif_file: results.sarif
diff --git a/pkg/types/intoto/v0.0.1/entry.go b/pkg/types/intoto/v0.0.1/entry.go
index 6327255..279cef7 100644
--- a/pkg/types/intoto/v0.0.1/entry.go
+++ b/pkg/types/intoto/v0.0.1/entry.go
@@ -73,20 +73,45 @@ func NewEntry() types.EntryImpl {
 func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
-	h := sha256.Sum256([]byte(v.env.Payload))
-	payloadKey := "sha256:" + hex.EncodeToString(h[:])
-	result = append(result, payloadKey)
+	// add digest over entire DSSE envelope
+	if v.IntotoObj.Content != nil && v.IntotoObj.Content.Hash != nil {
+		hashkey := strings.ToLower(fmt.Sprintf("%s:%s", swag.StringValue(v.IntotoObj.Content.Hash.Algorithm), swag.StringValue(v.IntotoObj.Content.Hash.Value)))
+		result = append(result, hashkey)
+	} else {
+		log.Logger.Error("could not find content digest to include in index keys")
+	}
 
-	switch v.env.PayloadType {
-	case in_toto.PayloadType:
-		if v.IntotoObj.Content != nil && v.IntotoObj.Content.Hash != nil {
-			hashkey := strings.ToLower(fmt.Sprintf("%s:%s", swag.StringValue(v.IntotoObj.Content.Hash.Algorithm), swag.StringValue(v.IntotoObj.Content.Hash.Value)))
-			result = append(result, hashkey)
+	// add digest over public key
+	if v.keyObj != nil {
+		key, err := v.keyObj.CanonicalValue()
+		if err == nil {
+			keyHash := sha256.Sum256(key)
+			result = append(result, fmt.Sprintf("sha256:%s", strings.ToLower(hex.EncodeToString(keyHash[:]))))
+
+			// add digest over any email addresses within signing certificate
+			result = append(result, v.keyObj.EmailAddresses()...)
+		} else {
+			log.Logger.Errorf("could not canonicalize public key to include in index keys: %w", err)
 		}
+	} else {
+		log.Logger.Error("could not find public key to include in index keys")
+	}
 
+	// 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[:]))))
+	} else {
+		log.Logger.Errorf("error decoding intoto payload to compute digest: %w", err)
+	}
+
+	switch v.env.PayloadType {
+	case in_toto.PayloadType:
 		statement, err := parseStatement(v.env.Payload)
 		if err != nil {
-			return result, err
+			log.Logger.Errorf("error parsing payload as intoto statement: %w", err)
+			break
 		}
 		for _, s := range statement.Subject {
 			for alg, ds := range s.Digest {
@@ -106,7 +131,7 @@ func (v V001Entry) IndexKeys() ([]string, error) {
 			}
 		}
 	default:
-		log.Logger.Infof("Unknown in_toto Statement Type: %s", v.env.PayloadType)
+		log.Logger.Infof("unknown in_toto statement type (%s), cannot extract additional index keys", v.env.PayloadType)
 	}
 	return result, nil
 }
@@ -180,8 +205,7 @@ func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
 			},
 		},
 	}
-	attKey, attValue := v.AttestationKeyValue()
-	if attValue != nil {
+	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)),


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 bae76e5..512d0e8 100644
--- a/pkg/types/intoto/v0.0.1/entry_test.go
+++ b/pkg/types/intoto/v0.0.1/entry_test.go
@@ -16,6 +16,8 @@
 package intoto
 
 import (
+	"bytes"
+	"context"
 	"crypto"
 	"crypto/ecdsa"
 	"crypto/elliptic"
@@ -33,13 +35,16 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/go-openapi/runtime"
 	"github.com/go-openapi/strfmt"
 	"github.com/go-openapi/swag"
 	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
 	"github.com/in-toto/in-toto-golang/in_toto"
 	slsa "github.com/in-toto/in-toto-golang/in_toto/slsa_provenance/v0.2"
 	"github.com/secure-systems-lab/go-securesystemslib/dsse"
 	"github.com/sigstore/rekor/pkg/generated/models"
+	"github.com/sigstore/rekor/pkg/types"
 	"github.com/sigstore/sigstore/pkg/signature"
 	"go.uber.org/goleak"
 )
@@ -105,7 +110,8 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 	}
 
 	ca := &x509.Certificate{
-		SerialNumber: big.NewInt(1),
+		SerialNumber:   big.NewInt(1),
+		EmailAddresses: []string{"joe@schmoe.com"},
 	}
 	caBytes, err := x509.CreateCertificate(rand.Reader, ca, ca, &priv.PublicKey, priv)
 	if err != nil {
@@ -131,10 +137,11 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 	validPayload := "hellothispayloadisvalid"
 
 	tests := []struct {
-		name    string
-		want    models.IntotoV001Schema
-		it      *models.IntotoV001Schema
-		wantErr bool
+		name                string
+		want                models.IntotoV001Schema
+		it                  *models.IntotoV001Schema
+		wantErr             bool
+		additionalIndexKeys []string
 	}{
 		{
 			name:    "empty",
@@ -156,7 +163,20 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "valid",
+			name: "valid intoto",
+			it: &models.IntotoV001Schema{
+				PublicKey: p(pub),
+				Content: &models.IntotoV001SchemaContent{
+					Envelope: envelope(t, key, validPayload, "application/vnd.in-toto+json"),
+					Hash: &models.IntotoV001SchemaContentHash{
+						Algorithm: swag.String(models.IntotoV001SchemaContentHashAlgorithmSha256),
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "valid dsse but invalid intoto",
 			it: &models.IntotoV001Schema{
 				PublicKey: p(pub),
 				Content: &models.IntotoV001SchemaContent{
@@ -179,7 +199,8 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 					},
 				},
 			},
-			wantErr: false,
+			additionalIndexKeys: []string{"joe@schmoe.com"},
+			wantErr:             false,
 		},
 		{
 			name: "invalid",
@@ -213,6 +234,7 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 			v := &V001Entry{}
 			if tt.it.Content != nil {
 				h := sha256.Sum256([]byte(tt.it.Content.Envelope))
+				tt.it.Content.Hash.Algorithm = swag.String(models.IntotoV001SchemaContentHashAlgorithmSha256)
 				tt.it.Content.Hash.Value = swag.String(hex.EncodeToString(h[:]))
 			}
 
@@ -227,13 +249,41 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 				if err := v.validate(); err != nil {
 					return err
 				}
-				sha := sha256.Sum256([]byte(v.env.Payload))
+				keysWanted := tt.additionalIndexKeys
+				if tt.it.PublicKey != nil {
+					h := sha256.Sum256(*tt.it.PublicKey)
+					keysWanted = append(keysWanted, fmt.Sprintf("sha256:%s", hex.EncodeToString(h[:])))
+				}
+				payloadBytes, _ := v.env.DecodeB64Payload()
+				payloadSha := sha256.Sum256(payloadBytes)
+				payloadHash := hex.EncodeToString(payloadSha[:])
 				// Always start with the hash
-				want := []string{"sha256:" + hex.EncodeToString(sha[:])}
+				keysWanted = append(keysWanted, "sha256:"+payloadHash)
 				hashkey := strings.ToLower(fmt.Sprintf("%s:%s", *tt.it.Content.Hash.Algorithm, *tt.it.Content.Hash.Value))
-				want = append(want, hashkey)
-				if got, _ := v.IndexKeys(); !reflect.DeepEqual(got, want) {
-					t.Errorf("V001Entry.IndexKeys() = %v, want %v", got, tt.want)
+				keysWanted = append(keysWanted, hashkey)
+				if got, _ := v.IndexKeys(); !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())
+				if err != nil {
+					t.Errorf("error canonicalizing entry: %v", err)
+				}
+
+				pe, err := models.UnmarshalProposedEntry(bytes.NewReader(canonicalBytes), runtime.JSONConsumer())
+				if err != nil {
+					t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tt.name, err)
+				}
+				canonicalEntry, err := types.NewEntry(pe)
+				if err != nil {
+					t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tt.name, err)
+				}
+				canonicalV001 := canonicalEntry.(*V001Entry)
+				fmt.Printf("%v", canonicalV001.IntotoObj.Content)
+				if *canonicalV001.IntotoObj.Content.Hash.Value != *tt.it.Content.Hash.Value {
+					t.Errorf("envelope hashes do not match post canonicalization: %v %v", *canonicalV001.IntotoObj.Content.Hash.Value, *tt.it.Content.Hash.Value)
+				}
+				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)
 				}
 
 				return nil
@@ -316,7 +366,7 @@ func TestV001Entry_IndexKeys(t *testing.T) {
 					PayloadType: in_toto.PayloadType,
 				},
 			}
-			sha := sha256.Sum256([]byte(payload))
+			sha := sha256.Sum256(b)
 			// Always start with the hash
 			want := []string{"sha256:" + hex.EncodeToString(sha[:])}
 			want = append(want, tt.want...)
@@ -355,7 +405,7 @@ func TestIndexKeysNoContentHash(t *testing.T) {
 			PayloadType: in_toto.PayloadType,
 		},
 	}
-	sha := sha256.Sum256([]byte(payload))
+	sha := sha256.Sum256(b)
 	// Always start with the hash
 	want := []string{"sha256:" + hex.EncodeToString(sha[:])}
 	want = append(want, "sha256:mysha256digest")