Improve error messages for invalid content (#377)

Previously we returned an HTTP 500 "error canonicalizing entry" error if
Rekor was unable to parse or verify the proposed content of a new log
entry. This adds a new error type ValidationError that allows
implementers of the Canonicalize method to delineate between internal,
transient errors and errors that clients can rectify.

With this patch, errors parsing or validating (provided or referenced)
artifacts will return an HTTP 400 message to the client with a message
about the issue.

Fixes: #362

Signed-off-by: Bob Callaway <bob.callaway@gmail.com>

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())