diff --git a/pkg/api/entries.go b/pkg/api/entries.go index 265bc7280..17769e278 100644 --- a/pkg/api/entries.go +++ b/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) } diff --git a/pkg/api/error.go b/pkg/api/error.go index 06cd86227..aace35f90 100644 --- a/pkg/api/error.go +++ b/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)" diff --git a/pkg/pki/x509/x509.go b/pkg/pki/x509/x509.go index 1534d147d..d2500e80f 100644 --- a/pkg/pki/x509/x509.go +++ b/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 diff --git a/pkg/types/README.md b/pkg/types/README.md index 3a9db59ac..5e6cbf21a 100644 --- a/pkg/types/README.md +++ b/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. \ No newline at end of file diff --git a/pkg/types/alpine/v0.0.1/entry.go b/pkg/types/alpine/v0.0.1/entry.go index 08dfd985c..a674416f1 100644 --- a/pkg/types/alpine/v0.0.1/entry.go +++ b/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 diff --git a/pkg/types/alpine/v0.0.1/entry_test.go b/pkg/types/alpine/v0.0.1/entry_test.go index ebb4f7671..2bbbfb5d1 100644 --- a/pkg/types/alpine/v0.0.1/entry_test.go +++ b/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()) diff --git a/pkg/types/error.go b/pkg/types/error.go new file mode 100644 index 000000000..01b8c14db --- /dev/null +++ b/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 diff --git a/pkg/types/helm/v0.0.1/entry.go b/pkg/types/helm/v0.0.1/entry.go index 30bc7f20f..f6a3c76b2 100644 --- a/pkg/types/helm/v0.0.1/entry.go +++ b/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 diff --git a/pkg/types/helm/v0.0.1/entry_test.go b/pkg/types/helm/v0.0.1/entry_test.go index 02eebe3f3..fbbc6801b 100644 --- a/pkg/types/helm/v0.0.1/entry_test.go +++ b/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()) diff --git a/pkg/types/intoto/v0.0.1/entry.go b/pkg/types/intoto/v0.0.1/entry.go index 5fdf05234..d62a2dc40 100644 --- a/pkg/types/intoto/v0.0.1/entry.go +++ b/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 diff --git a/pkg/types/jar/v0.0.1/entry.go b/pkg/types/jar/v0.0.1/entry.go index ee0ac6947..29598f634 100644 --- a/pkg/types/jar/v0.0.1/entry.go +++ b/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 diff --git a/pkg/types/jar/v0.0.1/entry_test.go b/pkg/types/jar/v0.0.1/entry_test.go index 44240b5b7..5eb924b30 100644 --- a/pkg/types/jar/v0.0.1/entry_test.go +++ b/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()) diff --git a/pkg/types/rekord/v0.0.1/entry.go b/pkg/types/rekord/v0.0.1/entry.go index de5a24108..99c77346d 100644 --- a/pkg/types/rekord/v0.0.1/entry.go +++ b/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 { diff --git a/pkg/types/rekord/v0.0.1/entry_test.go b/pkg/types/rekord/v0.0.1/entry_test.go index 2dc847ae3..5c4016484 100644 --- a/pkg/types/rekord/v0.0.1/entry_test.go +++ b/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()) diff --git a/pkg/types/rfc3161/v0.0.1/entry_test.go b/pkg/types/rfc3161/v0.0.1/entry_test.go index 972cbcb69..663270a4f 100644 --- a/pkg/types/rfc3161/v0.0.1/entry_test.go +++ b/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()) diff --git a/pkg/types/rpm/v0.0.1/entry.go b/pkg/types/rpm/v0.0.1/entry.go index 8677fe52d..b0ebaf52a 100644 --- a/pkg/types/rpm/v0.0.1/entry.go +++ b/pkg/types/rpm/v0.0.1/entry.go @@ -134,7 +134,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) @@ -199,7 +199,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 { @@ -220,16 +220,16 @@ 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)) } keyring, err := v.keyObj.(*pgp.PublicKey).KeyRing() if err != nil { - return closePipesOnError(err) + return closePipesOnError(types.ValidationError(err)) } if _, err := rpmutils.GPGCheck(sigR, keyring); err != nil { - return closePipesOnError(err) + return closePipesOnError(types.ValidationError(err)) } select { @@ -245,7 +245,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error { var err error v.rpmObj, err = rpmutils.ReadPackageFile(rpmR) if err != nil { - return closePipesOnError(err) + return closePipesOnError(types.ValidationError(err)) } // ReadPackageFile does not drain the entire reader so we need to discard the rest if _, err = io.Copy(ioutil.Discard, rpmR); err != nil { diff --git a/pkg/types/rpm/v0.0.1/entry_test.go b/pkg/types/rpm/v0.0.1/entry_test.go index 6df90a043..cb49b12ee 100644 --- a/pkg/types/rpm/v0.0.1/entry_test.go +++ b/pkg/types/rpm/v0.0.1/entry_test.go @@ -393,7 +393,12 @@ 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()) if err != nil { diff --git a/tests/e2e_test.go b/tests/e2e_test.go index 511e66328..9a8840b5b 100644 --- a/tests/e2e_test.go +++ b/tests/e2e_test.go @@ -319,6 +319,10 @@ func TestAPK(t *testing.T) { outputContains(t, out, "Created entry at") out = runCli(t, "upload", "--artifact", artifactPath, "--type", "alpine", "--public-key", pubPath) outputContains(t, out, "Entry already exists") + // pass invalid public key, ensure we see a 400 error with helpful message + out = runCliErr(t, "upload", "--artifact", artifactPath, "--type", "alpine", "--public-key", artifactPath) + outputContains(t, out, "400") + outputContains(t, out, "invalid public key") } func TestIntoto(t *testing.T) {