Improve error messages for invalid content

pkg/api/entries.go

@@ -146,10 +146,13 @@ func createLogEntry(params entries.CreateLogEntryParams) (models.LogEntry, middl
 	ctx := params.HTTPRequest.Context()
 	entry, err := types.NewEntry(params.ProposedEntry)
 	if err != nil {
-		return nil, handleRekorAPIError(params, http.StatusBadRequest, err, err.Error())
+		return nil, handleRekorAPIError(params, http.StatusBadRequest, err, fmt.Sprintf(validationError, err))
 	}
 	leaf, err := entry.Canonicalize(ctx)
 	if err != nil {
+		if _, ok := (err).(types.ValidationError); ok {
+			return nil, handleRekorAPIError(params, http.StatusBadRequest, err, fmt.Sprintf(validationError, err))
+		}
 		return nil, handleRekorAPIError(params, http.StatusInternalServerError, err, failedToGenerateCanonicalEntry)
 	}
 

pkg/api/error.go

@@ -35,6 +35,7 @@ import (
 const (
 	trillianCommunicationError        = "Unexpected error communicating with transparency log"
 	trillianUnexpectedResult          = "Unexpected result from transparency log"
+	validationError                   = "Error processing entry: %v"
 	failedToGenerateCanonicalEntry    = "Error generating canonicalized entry"
 	entryAlreadyExists                = "An equivalent entry already exists in the transparency log with UUID %v"
 	firstSizeLessThanLastSize         = "firstSize(%d) must be less than lastSize(%d)"

pkg/pki/x509/x509.go

@@ -99,7 +99,7 @@ func NewPublicKey(r io.Reader) (*PublicKey, error) {
 
 	block, _ := pem.Decode(rawPub)
 	if block == nil {
-		return nil, fmt.Errorf("invalid public key: %s", string(rawPub))
+		return nil, errors.New("invalid public key: failure decoding PEM")
 	}
 
 	switch block.Type {
@@ -120,7 +120,7 @@ func NewPublicKey(r io.Reader) (*PublicKey, error) {
 				b: block.Bytes,
 			}}, nil
 	}
-	return nil, fmt.Errorf("invalid public key: %s", string(rawPub))
+	return nil, fmt.Errorf("invalid public key: cannot handle type %v", block.Type)
 }
 
 // CanonicalValue implements the pki.PublicKey interface

pkg/types/README.md

@@ -142,3 +142,7 @@ To add new version of the default `Rekord` type:
 6. Add an import statement to `cmd/rekor-cli/app/root.go` that provides a reference to your package/new version. This ensures that the `init` function of your type (and optionally, your version implementation) will be called before the CLI runs; this populates the required type maps and allows the CLI to interact with the type implementations in a loosely coupled manner.
 
 7. After adding sufficient unit & integration tests, submit a pull request to `github.com/sigstore/rekor` for review and addition to the codebase.
+
+## Error Handling
+
+The `Canonicalize` method in the `EntryImpl` interface generates the canonicalized content for insertion into the transparency log. While errors returned from the `Unmarshal` method should always be considered as an error with what the client has provided (thus generating a HTTP 400 Bad Request response), errors from the `Canonicalize` method can be either `types.ValidationError` which means the content provided inside of (or referred to by) the incoming request could not be successfully processed or verified, or a generic `error` which should be interpreted as a HTTP 500 Internal Server Error to which the client can not resolve.

pkg/types/alpine/v0.0.1/entry.go

@@ -132,6 +132,11 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 	}
 
 	if err := v.validate(); err != nil {
+		return types.ValidationError(err)
+	}
+
+	artifactFactory, err := pki.NewArtifactFactory(pki.X509)
+	if err != nil {
 		return err
 	}
 
@@ -160,7 +165,6 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 	if v.AlpineModel.Package.Hash != nil && v.AlpineModel.Package.Hash.Value != nil {
 		oldSHA = swag.StringValue(v.AlpineModel.Package.Hash.Value)
 	}
-	artifactFactory, _ := pki.NewArtifactFactory(pki.X509)
 
 	g.Go(func() error {
 		defer hashW.Close()
@@ -191,7 +195,7 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 
 		computedSHA := hex.EncodeToString(hasher.Sum(nil))
 		if oldSHA != "" && computedSHA != oldSHA {
-			return closePipesOnError(fmt.Errorf("SHA mismatch: %s != %s", computedSHA, oldSHA))
+			return closePipesOnError(types.ValidationError(fmt.Errorf("SHA mismatch: %s != %s", computedSHA, oldSHA)))
 		}
 
 		select {
@@ -215,7 +219,7 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 
 		v.keyObj, err = artifactFactory.NewPublicKey(keyReadCloser)
 		if err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		select {
@@ -229,7 +233,7 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 	g.Go(func() error {
 		apk := alpine.Package{}
 		if err := apk.Unmarshal(apkR); err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		key := <-keyResult
@@ -238,7 +242,7 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 		}
 
 		if err := apk.VerifySignature(key.CryptoPubKey()); err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		v.apkObj = &apk

pkg/types/alpine/v0.0.1/entry_test.go

@@ -393,6 +393,10 @@ func TestCrossFieldValidation(t *testing.T) {
 		b, err := v.Canonicalize(context.TODO())
 		if (err == nil) != tc.expectCanonicalizeSuccess {
 			t.Errorf("unexpected result from Canonicalize for '%v': %v", tc.caseDesc, err)
+		} else if err != nil {
+			if _, ok := err.(types.ValidationError); !ok {
+				t.Errorf("canonicalize returned an unexpected error that isn't of type types.ValidationError: %v", err)
+			}
 		}
 		if b != nil {
 			pe, err := models.UnmarshalProposedEntry(bytes.NewReader(b), runtime.JSONConsumer())

pkg/types/error.go

@@ -0,0 +1,20 @@
+//
+// Copyright 2021 The Sigstore Authors.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package types
+
+// ValidationError indicates that there is an issue with the content in the HTTP Request that
+// should result in an HTTP 400 Bad Request error being returned to the client
+type ValidationError error

pkg/types/helm/v0.0.1/entry.go

@@ -141,7 +141,7 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 	}
 
 	if err := v.validate(); err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 
 	artifactFactory, err := pki.NewArtifactFactory(pki.PGP)
@@ -197,9 +197,8 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 		defer keyReadCloser.Close()
 
 		v.keyObj, err = artifactFactory.NewPublicKey(keyReadCloser)
-
 		if err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		select {
@@ -214,7 +213,7 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 
 		provenance := helm.Provenance{}
 		if err := provenance.Unmarshal(provenanceR); err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		key := <-keyResult
@@ -224,14 +223,13 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 
 		// Set signature
 		sig, err := artifactFactory.NewSignature(provenance.Block.ArmoredSignature.Body)
-
 		if err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		// Verify signature
 		if err := sig.Verify(bytes.NewReader(provenance.Block.Bytes), key); err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		v.sigObj = sig
@@ -251,7 +249,6 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 
 	v.fetchedExternalEntities = true
 	return nil
-
 }
 
 func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
@@ -305,13 +302,7 @@ func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
 	helmObj.APIVersion = swag.String(APIVERSION)
 	helmObj.Spec = &canonicalEntry
 
-	bytes, err := json.Marshal(&helmObj)
-	if err != nil {
-		return nil, err
-	}
-
-	return bytes, nil
-
+	return json.Marshal(&helmObj)
 }
 
 // validate performs cross-field validation for fields in object

pkg/types/helm/v0.0.1/entry_test.go

@@ -249,6 +249,10 @@ func TestCrossFieldValidation(t *testing.T) {
 		b, err := v.Canonicalize(context.TODO())
 		if (err == nil) != tc.expectCanonicalizeSuccess {
 			t.Errorf("unexpected result from Canonicalize for '%v': %v", tc.caseDesc, err)
+		} else if err != nil {
+			if _, ok := err.(types.ValidationError); !ok {
+				t.Errorf("canonicalize returned an unexpected error that isn't of type types.ValidationError: %v", err)
+			}
 		}
 		if b != nil {
 			pe, err := models.UnmarshalProposedEntry(bytes.NewReader(b), runtime.JSONConsumer())

pkg/types/intoto/v0.0.1/entry.go

@@ -163,12 +163,7 @@ func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
 	itObj.APIVersion = swag.String(APIVERSION)
 	itObj.Spec = &canonicalEntry
 
-	bytes, err := json.Marshal(&itObj)
-	if err != nil {
-		return nil, err
-	}
-
-	return bytes, nil
+	return json.Marshal(&itObj)
 }
 
 // validate performs cross-field validation for fields in object

pkg/types/jar/v0.0.1/entry.go

@@ -132,7 +132,7 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 	}
 
 	if err := v.validate(); err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 
 	oldSHA := ""
@@ -156,26 +156,26 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 
 	computedSHA := hex.EncodeToString(hasher.Sum(nil))
 	if oldSHA != "" && computedSHA != oldSHA {
-		return fmt.Errorf("SHA mismatch: %s != %s", computedSHA, oldSHA)
+		return types.ValidationError(fmt.Errorf("SHA mismatch: %s != %s", computedSHA, oldSHA))
 	}
 
 	zipReader, err := zip.NewReader(bytes.NewReader(b.Bytes()), n)
 	if err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 
 	// this ensures that the JAR is signed and the signature verifies, as
 	// well as checks that the hashes in the signed manifest are all valid
 	jarObj, err := jarutils.Verify(zipReader, false)
 	if err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 	switch len(jarObj) {
 	case 0:
-		return errors.New("no signatures detected in JAR archive")
+		return types.ValidationError(errors.New("no signatures detected in JAR archive"))
 	case 1:
 	default:
-		return errors.New("multiple signatures detected in JAR; unable to process")
+		return types.ValidationError(errors.New("multiple signatures detected in JAR; unable to process"))
 	}
 	v.jarObj = jarObj[0]
 
@@ -186,17 +186,17 @@ func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 	// we need to find and extract the PKCS7 bundle from the JAR file manually
 	sigPKCS7, err := extractPKCS7SignatureFromJAR(zipReader)
 	if err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 
 	v.keyObj, err = af.NewPublicKey(bytes.NewReader(sigPKCS7))
 	if err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 
 	v.sigObj, err = af.NewSignature(bytes.NewReader(sigPKCS7))
 	if err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 
 	// if we get here, all goroutines succeeded without error
@@ -256,12 +256,7 @@ func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
 	jar.APIVersion = swag.String(APIVERSION)
 	jar.Spec = &canonicalEntry
 
-	bytes, err := json.Marshal(&jar)
-	if err != nil {
-		return nil, err
-	}
-
-	return bytes, nil
+	return json.Marshal(&jar)
 }
 
 // validate performs cross-field validation for fields in object

pkg/types/jar/v0.0.1/entry_test.go

@@ -222,6 +222,10 @@ func TestCrossFieldValidation(t *testing.T) {
 		b, err := v.Canonicalize(context.TODO())
 		if (err == nil) != tc.expectCanonicalizeSuccess {
 			t.Errorf("unexpected result from Canonicalize for '%v': %v", tc.caseDesc, err)
+		} else if err != nil {
+			if _, ok := err.(types.ValidationError); !ok {
+				t.Errorf("canonicalize returned an unexpected error that isn't of type types.ValidationError: %v", err)
+			}
 		}
 		if b != nil {
 			pe, err := models.UnmarshalProposedEntry(bytes.NewReader(b), runtime.JSONConsumer())

pkg/types/rekord/v0.0.1/entry.go

@@ -135,7 +135,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
 	}
 
 	if err := v.validate(); err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 
 	g, ctx := errgroup.WithContext(ctx)
@@ -165,7 +165,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
 	}
 	artifactFactory, err := pki.NewArtifactFactory(pki.Format(v.RekordObj.Signature.Format))
 	if err != nil {
-		return err
+		return types.ValidationError(err)
 	}
 
 	g.Go(func() error {
@@ -197,7 +197,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
 
 		computedSHA := hex.EncodeToString(hasher.Sum(nil))
 		if oldSHA != "" && computedSHA != oldSHA {
-			return closePipesOnError(fmt.Errorf("SHA mismatch: %s != %s", computedSHA, oldSHA))
+			return closePipesOnError(types.ValidationError(fmt.Errorf("SHA mismatch: %s != %s", computedSHA, oldSHA)))
 		}
 
 		select {
@@ -222,7 +222,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
 
 		signature, err := artifactFactory.NewSignature(sigReadCloser)
 		if err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		select {
@@ -247,7 +247,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
 
 		key, err := artifactFactory.NewPublicKey(keyReadCloser)
 		if err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		select {
@@ -267,7 +267,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
 
 		var err error
 		if err = v.sigObj.Verify(sigR, v.keyObj); err != nil {
-			return closePipesOnError(err)
+			return closePipesOnError(types.ValidationError(err))
 		}
 
 		select {

pkg/types/rekord/v0.0.1/entry_test.go

@@ -553,6 +553,10 @@ func TestCrossFieldValidation(t *testing.T) {
 		b, err := v.Canonicalize(context.TODO())
 		if (err == nil) != tc.expectCanonicalizeSuccess {
 			t.Errorf("unexpected result from Canonicalize for '%v': %v", tc.caseDesc, err)
+		} else if err != nil {
+			if _, ok := err.(types.ValidationError); !ok {
+				t.Errorf("canonicalize returned an unexpected error that isn't of type types.ValidationError: %v", err)
+			}
 		}
 		if b != nil {
 			pe, err := models.UnmarshalProposedEntry(bytes.NewReader(b), runtime.JSONConsumer())

pkg/types/rfc3161/v0.0.1/entry_test.go

@@ -185,6 +185,10 @@ func TestCrossFieldValidation(t *testing.T) {
 		b, err := v.Canonicalize(context.TODO())
 		if (err == nil) != tc.expectCanonicalizeSuccess {
 			t.Errorf("unexpected result from Canonicalize for '%v': %v", tc.caseDesc, err)
+		} else if err != nil {
+			if _, ok := err.(types.ValidationError); !ok {
+				t.Errorf("canonicalize returned an unexpected error that isn't of type types.ValidationError: %v", err)
+			}
 		}
 		if b != nil {
 			pe, err := models.UnmarshalProposedEntry(bytes.NewReader(b), runtime.JSONConsumer())