From 9df041b2fa32dfa0ad53af6fd8888040ed3e7f82 Mon Sep 17 00:00:00 2001
From: dlorenc <dlorenc@google.com>
Date: Mon, 20 Dec 2021 06:57:41 -0600
Subject: [PATCH] This refactors IndexKeys to return an error. (#553)

We were catching these inside the IndexKeys function calls and logging, this
change moves that up to the caller. This is much more standard and simplifies the
implementations.

Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>
---
 pkg/api/entries.go                          |  7 ++++++-
 pkg/types/alpine/alpine_test.go             |  4 ++--
 pkg/types/alpine/v0.0.1/entry.go            | 18 +++++-------------
 pkg/types/entries.go                        |  2 +-
 pkg/types/hashedrekord/hashedrekord_test.go |  4 ++--
 pkg/types/hashedrekord/v0.0.1/entry.go      | 12 +++++-------
 pkg/types/helm/helm_test.go                 |  4 ++--
 pkg/types/helm/v0.0.1/entry.go              | 18 +++++-------------
 pkg/types/intoto/intoto_test.go             |  4 ++--
 pkg/types/intoto/v0.0.1/entry.go            |  7 +++----
 pkg/types/intoto/v0.0.1/entry_test.go       |  4 ++--
 pkg/types/jar/jar_test.go                   |  4 ++--
 pkg/types/jar/v0.0.1/entry.go               | 18 +++++-------------
 pkg/types/rekord/rekord_test.go             |  4 ++--
 pkg/types/rekord/v0.0.1/entry.go            |  4 ++--
 pkg/types/rfc3161/rfc3161_test.go           |  4 ++--
 pkg/types/rfc3161/v0.0.1/entry.go           | 17 ++++++++---------
 pkg/types/rpm/rpm_test.go                   |  4 ++--
 pkg/types/rpm/v0.0.1/entry.go               | 18 +++++-------------
 pkg/types/tuf/tuf_test.go                   |  4 ++--
 pkg/types/tuf/v0.0.1/entry.go               | 14 +++-----------
 21 files changed, 68 insertions(+), 107 deletions(-)

diff --git a/pkg/api/entries.go b/pkg/api/entries.go
index da2be72..fd6d2bb 100644
--- a/pkg/api/entries.go
+++ b/pkg/api/entries.go
@@ -194,7 +194,12 @@ func createLogEntry(params entries.CreateLogEntryParams) (models.LogEntry, middl
 
 	if viper.GetBool("enable_retrieve_api") {
 		go func() {
-			for _, key := range entry.IndexKeys() {
+			keys, err := entry.IndexKeys()
+			if err != nil {
+				log.RequestIDLogger(params.HTTPRequest).Error(err)
+				return
+			}
+			for _, key := range keys {
 				if err := addToIndex(context.Background(), key, uuid); err != nil {
 					log.RequestIDLogger(params.HTTPRequest).Error(err)
 				}
diff --git a/pkg/types/alpine/alpine_test.go b/pkg/types/alpine/alpine_test.go
index 331306b..a1e5741 100644
--- a/pkg/types/alpine/alpine_test.go
+++ b/pkg/types/alpine/alpine_test.go
@@ -38,8 +38,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/alpine/v0.0.1/entry.go b/pkg/types/alpine/v0.0.1/entry.go
index 18c1c7f..4d18773 100644
--- a/pkg/types/alpine/v0.0.1/entry.go
+++ b/pkg/types/alpine/v0.0.1/entry.go
@@ -65,23 +65,15 @@ func NewEntry() types.EntryImpl {
 	return &V001Entry{}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
-	if v.hasExternalEntities() {
-		if err := v.fetchExternalEntities(context.Background()); err != nil {
-			log.Logger.Error(err)
-			return result
-		}
-	}
-
 	key, err := v.keyObj.CanonicalValue()
 	if err != nil {
-		log.Logger.Error(err)
-	} else {
-		keyHash := sha256.Sum256(key)
-		result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
+		return nil, err
 	}
+	keyHash := sha256.Sum256(key)
+	result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
 
 	result = append(result, v.keyObj.EmailAddresses()...)
 
@@ -90,7 +82,7 @@ func (v V001Entry) IndexKeys() []string {
 		result = append(result, hashKey)
 	}
 
-	return result
+	return result, nil
 }
 
 func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
diff --git a/pkg/types/entries.go b/pkg/types/entries.go
index e046482..0d5c3eb 100644
--- a/pkg/types/entries.go
+++ b/pkg/types/entries.go
@@ -32,7 +32,7 @@ import (
 // EntryImpl specifies the behavior of a versioned type
 type EntryImpl interface {
 	APIVersion() string                               // the supported versions for this implementation
-	IndexKeys() []string                              // the keys that should be added to the external index for this entry
+	IndexKeys() ([]string, error)                     // the keys that should be added to the external index for this entry
 	Canonicalize(ctx context.Context) ([]byte, error) // marshal the canonical entry to be put into the tlog
 	Unmarshal(e models.ProposedEntry) error           // unmarshal the abstract entry into the specific struct for this versioned type
 	Attestation() (string, []byte)
diff --git a/pkg/types/hashedrekord/hashedrekord_test.go b/pkg/types/hashedrekord/hashedrekord_test.go
index d3ed2e2..0abde63 100644
--- a/pkg/types/hashedrekord/hashedrekord_test.go
+++ b/pkg/types/hashedrekord/hashedrekord_test.go
@@ -42,8 +42,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/hashedrekord/v0.0.1/entry.go b/pkg/types/hashedrekord/v0.0.1/entry.go
index c4eab28..80a49a1 100644
--- a/pkg/types/hashedrekord/v0.0.1/entry.go
+++ b/pkg/types/hashedrekord/v0.0.1/entry.go
@@ -63,17 +63,15 @@ func NewEntry() types.EntryImpl {
 	return &V001Entry{}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
 	key, err := v.keyObj.CanonicalValue()
 	if err != nil {
-		log.Logger.Error(err)
-	} else {
-		keyHash := sha256.Sum256(key)
-		result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
+		return nil, err
 	}
-
+	keyHash := sha256.Sum256(key)
+	result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
 	result = append(result, v.keyObj.EmailAddresses()...)
 
 	if v.HashedRekordObj.Data.Hash != nil {
@@ -81,7 +79,7 @@ func (v V001Entry) IndexKeys() []string {
 		result = append(result, hashKey)
 	}
 
-	return result
+	return result, nil
 }
 
 func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
diff --git a/pkg/types/helm/helm_test.go b/pkg/types/helm/helm_test.go
index b4402b9..e628a57 100644
--- a/pkg/types/helm/helm_test.go
+++ b/pkg/types/helm/helm_test.go
@@ -42,8 +42,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/helm/v0.0.1/entry.go b/pkg/types/helm/v0.0.1/entry.go
index 530764b..7f94a14 100644
--- a/pkg/types/helm/v0.0.1/entry.go
+++ b/pkg/types/helm/v0.0.1/entry.go
@@ -66,23 +66,15 @@ func NewEntry() types.EntryImpl {
 	return &V001Entry{}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
-	if v.hasExternalEntities() {
-		if err := v.fetchExternalEntities(context.Background()); err != nil {
-			log.Logger.Error(err)
-			return result
-		}
-	}
-
 	key, err := v.keyObj.CanonicalValue()
 	if err != nil {
-		log.Logger.Error(err)
-	} else {
-		keyHash := sha256.Sum256(key)
-		result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
+		return nil, err
 	}
+	keyHash := sha256.Sum256(key)
+	result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
 
 	result = append(result, v.keyObj.EmailAddresses()...)
 
@@ -95,7 +87,7 @@ func (v V001Entry) IndexKeys() []string {
 		result = append(result, hashKey)
 	}
 
-	return result
+	return result, nil
 }
 
 func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
diff --git a/pkg/types/intoto/intoto_test.go b/pkg/types/intoto/intoto_test.go
index 35b1d04..7ff7cfd 100644
--- a/pkg/types/intoto/intoto_test.go
+++ b/pkg/types/intoto/intoto_test.go
@@ -42,8 +42,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/intoto/v0.0.1/entry.go b/pkg/types/intoto/v0.0.1/entry.go
index 9fb0e78..262e8b5 100644
--- a/pkg/types/intoto/v0.0.1/entry.go
+++ b/pkg/types/intoto/v0.0.1/entry.go
@@ -69,7 +69,7 @@ func NewEntry() types.EntryImpl {
 	return &V001Entry{}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
 	h := sha256.Sum256([]byte(v.env.Payload))
@@ -80,8 +80,7 @@ func (v V001Entry) IndexKeys() []string {
 	case in_toto.PayloadType:
 		statement, err := parseStatement(v.env.Payload)
 		if err != nil {
-			log.Logger.Info("invalid id in_toto Statement")
-			return result
+			return result, err
 		}
 		for _, s := range statement.Subject {
 			for alg, ds := range s.Digest {
@@ -91,7 +90,7 @@ func (v V001Entry) IndexKeys() []string {
 	default:
 		log.Logger.Infof("Unknown in_toto Statement Type: %s", v.env.PayloadType)
 	}
-	return result
+	return result, nil
 }
 
 func parseStatement(p string) (*in_toto.Statement, error) {
diff --git a/pkg/types/intoto/v0.0.1/entry_test.go b/pkg/types/intoto/v0.0.1/entry_test.go
index f035544..b0b65f1 100644
--- a/pkg/types/intoto/v0.0.1/entry_test.go
+++ b/pkg/types/intoto/v0.0.1/entry_test.go
@@ -204,7 +204,7 @@ func TestV001Entry_Unmarshal(t *testing.T) {
 				if err := v.validate(); err != nil {
 					return err
 				}
-				keys := v.IndexKeys()
+				keys, _ := v.IndexKeys()
 				h := sha256.Sum256([]byte(v.env.Payload))
 				sha := "sha256:" + hex.EncodeToString(h[:])
 				if keys[0] != sha {
@@ -268,7 +268,7 @@ func TestV001Entry_IndexKeys(t *testing.T) {
 			// Always start with the hash
 			want := []string{"sha256:" + hex.EncodeToString(sha[:])}
 			want = append(want, tt.want...)
-			if got := v.IndexKeys(); !reflect.DeepEqual(got, want) {
+			if got, _ := v.IndexKeys(); !reflect.DeepEqual(got, want) {
 				t.Errorf("V001Entry.IndexKeys() = %v, want %v", got, tt.want)
 			}
 		})
diff --git a/pkg/types/jar/jar_test.go b/pkg/types/jar/jar_test.go
index 683a31c..92ee515 100644
--- a/pkg/types/jar/jar_test.go
+++ b/pkg/types/jar/jar_test.go
@@ -37,8 +37,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/jar/v0.0.1/entry.go b/pkg/types/jar/v0.0.1/entry.go
index e033d07..8e395eb 100644
--- a/pkg/types/jar/v0.0.1/entry.go
+++ b/pkg/types/jar/v0.0.1/entry.go
@@ -70,30 +70,22 @@ func NewEntry() types.EntryImpl {
 	return &V001Entry{}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
-	if v.hasExternalEntities() {
-		if err := v.fetchExternalEntities(context.Background()); err != nil {
-			log.Logger.Error(err)
-			return result
-		}
-	}
-
 	key, err := v.keyObj.CanonicalValue()
 	if err != nil {
-		log.Logger.Error(err)
-	} else {
-		keyHash := sha256.Sum256(key)
-		result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
+		return nil, err
 	}
+	keyHash := sha256.Sum256(key)
+	result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
 
 	if v.JARModel.Archive.Hash != nil {
 		hashKey := strings.ToLower(fmt.Sprintf("%s:%s", *v.JARModel.Archive.Hash.Algorithm, *v.JARModel.Archive.Hash.Value))
 		result = append(result, hashKey)
 	}
 
-	return result
+	return result, nil
 }
 
 func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
diff --git a/pkg/types/rekord/rekord_test.go b/pkg/types/rekord/rekord_test.go
index 20b2923..1f781eb 100644
--- a/pkg/types/rekord/rekord_test.go
+++ b/pkg/types/rekord/rekord_test.go
@@ -42,8 +42,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/rekord/v0.0.1/entry.go b/pkg/types/rekord/v0.0.1/entry.go
index d0423f9..5412d70 100644
--- a/pkg/types/rekord/v0.0.1/entry.go
+++ b/pkg/types/rekord/v0.0.1/entry.go
@@ -64,7 +64,7 @@ func NewEntry() types.EntryImpl {
 	return &V001Entry{}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
 	key, err := v.keyObj.CanonicalValue()
@@ -82,7 +82,7 @@ func (v V001Entry) IndexKeys() []string {
 		result = append(result, hashKey)
 	}
 
-	return result
+	return result, nil
 }
 
 func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
diff --git a/pkg/types/rfc3161/rfc3161_test.go b/pkg/types/rfc3161/rfc3161_test.go
index 17b447d..dd8c6e6 100644
--- a/pkg/types/rfc3161/rfc3161_test.go
+++ b/pkg/types/rfc3161/rfc3161_test.go
@@ -42,8 +42,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/rfc3161/v0.0.1/entry.go b/pkg/types/rfc3161/v0.0.1/entry.go
index 118c8f6..f67e1e5 100644
--- a/pkg/types/rfc3161/v0.0.1/entry.go
+++ b/pkg/types/rfc3161/v0.0.1/entry.go
@@ -76,22 +76,21 @@ func NewEntryFromBytes(timestamp []byte) models.ProposedEntry {
 	}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
 	str := v.Rfc3161Obj.Tsr.Content.String()
 	tb, err := base64.StdEncoding.DecodeString(str)
 	if err != nil {
-		log.Logger.Warn(err)
-	} else {
-		h := sha256.Sum256(tb)
-		hx := hex.EncodeToString(h[:])
-
-		payloadKey := "sha256:" + hx
-		result = append(result, payloadKey)
+		return nil, err
 	}
+	h := sha256.Sum256(tb)
+	hx := hex.EncodeToString(h[:])
+
+	payloadKey := "sha256:" + hx
+	result = append(result, payloadKey)
 
-	return result
+	return result, nil
 }
 
 func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
diff --git a/pkg/types/rpm/rpm_test.go b/pkg/types/rpm/rpm_test.go
index 8fe48d1..df276d5 100644
--- a/pkg/types/rpm/rpm_test.go
+++ b/pkg/types/rpm/rpm_test.go
@@ -38,8 +38,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/rpm/v0.0.1/entry.go b/pkg/types/rpm/v0.0.1/entry.go
index ae6bbf4..c9d71f0 100644
--- a/pkg/types/rpm/v0.0.1/entry.go
+++ b/pkg/types/rpm/v0.0.1/entry.go
@@ -67,23 +67,15 @@ func NewEntry() types.EntryImpl {
 	return &V001Entry{}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
-	if v.HasExternalEntities() {
-		if err := v.FetchExternalEntities(context.Background()); err != nil {
-			log.Logger.Error(err)
-			return result
-		}
-	}
-
 	key, err := v.keyObj.CanonicalValue()
 	if err != nil {
-		log.Logger.Error(err)
-	} else {
-		keyHash := sha256.Sum256(key)
-		result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
+		return nil, err
 	}
+	keyHash := sha256.Sum256(key)
+	result = append(result, strings.ToLower(hex.EncodeToString(keyHash[:])))
 
 	result = append(result, v.keyObj.EmailAddresses()...)
 
@@ -92,7 +84,7 @@ func (v V001Entry) IndexKeys() []string {
 		result = append(result, hashKey)
 	}
 
-	return result
+	return result, nil
 }
 
 func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
diff --git a/pkg/types/tuf/tuf_test.go b/pkg/types/tuf/tuf_test.go
index 3001663..172d0cb 100644
--- a/pkg/types/tuf/tuf_test.go
+++ b/pkg/types/tuf/tuf_test.go
@@ -38,8 +38,8 @@ func (u UnmarshalTester) APIVersion() string {
 	return "2.0.1"
 }
 
-func (u UnmarshalTester) IndexKeys() []string {
-	return []string{}
+func (u UnmarshalTester) IndexKeys() ([]string, error) {
+	return []string{}, nil
 }
 
 func (u UnmarshalTester) Canonicalize(ctx context.Context) ([]byte, error) {
diff --git a/pkg/types/tuf/v0.0.1/entry.go b/pkg/types/tuf/v0.0.1/entry.go
index f92dc2c..afe78fe 100644
--- a/pkg/types/tuf/v0.0.1/entry.go
+++ b/pkg/types/tuf/v0.0.1/entry.go
@@ -76,16 +76,9 @@ func NewEntry() types.EntryImpl {
 	return &V001Entry{}
 }
 
-func (v V001Entry) IndexKeys() []string {
+func (v V001Entry) IndexKeys() ([]string, error) {
 	var result []string
 
-	if v.hasExternalEntities() {
-		if err := v.fetchExternalEntities(context.Background()); err != nil {
-			log.Logger.Error(err)
-			return result
-		}
-	}
-
 	// Index metadata hash, type, and version.
 	metadata, err := v.sigObj.CanonicalValue()
 	if err != nil {
@@ -97,8 +90,7 @@ func (v V001Entry) IndexKeys() []string {
 
 	signed, ok := v.sigObj.(*ptuf.Signature)
 	if !ok {
-		log.Logger.Error(errors.New("invalid metadata format"))
-		return result
+		return nil, errors.New("invalid metadata format")
 	}
 
 	result = append(result, signed.Role)
@@ -114,7 +106,7 @@ func (v V001Entry) IndexKeys() []string {
 	}
 
 	// TODO: Index individual key IDs
-	return result
+	return result, nil
 }
 
 func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
-- 
GitLab