Clean up EntryImpl interface (#370)

* Refactor PKI factory and add type checking

This allows for more DRY addition of new PKI types, and stricter
type checking. This also allows for simpler enumeration of
supported PKI formats which will be used in further updates to
simplify the CLI codebase.

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

* revamp CLI flags; support different versions for upload

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

* Add Alpine Package type

This adds support for the alpine package format used by Alpine Linux,
which is the concatenation of three tgz files (signature, control data,
and then the actual package files).

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

* use shaFlag for --artifact-hash

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

* change arg type to PKIFormat

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

* defer type-specific validation logic to type code (instead of in CLI); also use CliLogger throughout CLI

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

* refactor factory code

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

* review comments

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

* Clean up EntryImpl interface

Make the interface clearer by removing ambiguity around who and when an
entry should have external objects fetched or validated.

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

* update pluggable type README

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

pkg/api/entries.go

@@ -311,15 +311,6 @@ func SearchLogQueryHandler(params entries.SearchLogQueryParams) middleware.Respo
 				if err != nil {
 					return err
 				}
-				if err := entry.Validate(); err != nil {
-					return err
-				}
-
-				if entry.HasExternalEntities() {
-					if err := entry.FetchExternalEntities(httpReqCtx); err != nil {
-						return err
-					}
-				}
 
 				leaf, err := entry.Canonicalize(httpReqCtx)
 				if err != nil {

pkg/api/error.go

@@ -111,6 +111,7 @@ func handleRekorAPIError(params interface{}, code int, err error, message string
 			}
 			return resp
 		default:
+			logMsg(params.HTTPRequest)
 			return entries.NewCreateLogEntryDefault(code).WithPayload(errorMsg(message, code))
 		}
 	case entries.SearchLogQueryParams:

pkg/types/README.md

@@ -6,7 +6,19 @@ Rekor supports pluggable types (aka different schemas) for entries stored in the
 
 ### Currently supported types
 
-- Rekord (default type) [schema](rekord/rekord_schema.json)
+- Alpine Packages [schema](alpine/alpine_schema.json)
+  - Versions: 0.0.1
+- Helm Provenance Files [schema](helm/helm_schema.json)
+  - Versions: 0.0.1
+- In-Toto Attestations [schema](intoto/intoto_schema.json)
+  - Versions: 0.0.1
+- Java Archives (JAR Files) [schema](jar/jar_schema.json)
+  - Versions: 0.0.1
+- Rekord *(default type)* [schema](rekord/rekord_schema.json)
+  - Versions: 0.0.1
+- RFC3161 Timestamps [schema](rfc3161/rfc3161_schema.json)
+  - Versions: 0.0.1
+- RPM Packages [schema](rpm/rpm_schema.json)
   - Versions: 0.0.1
 
 
@@ -61,14 +73,20 @@ To add a new type (called `newType` in this example):
 3. In this new Go package, define a struct that:
 ```go
 type TypeImpl interface {
+	CreateProposedEntry(context.Context, string, ArtifactProperties) (models.ProposedEntry, error)
+	DefaultVersion() string
+	SupportedVersions() []string
 	UnmarshalEntry(pe models.ProposedEntry) (EntryImpl, error)
 }
 ```
   - implements the `TypeImpl` interface as defined in `types.go`:
+    - `CreateProposedEntry` will be called with string specifying the requested version and an ArtifactProperties struct that calls the version's `CreateFromArtifactProperties` method
+    - `DefaultVersion` specifies the default version for the type to be used if one is not explicitly requested
+    - `SupportedVersions` specifies a list of all version strings that are supported by this Rekor instance
     - `UnmarshalEntry` will be called with a pointer to a struct that was automatically generated for the type defined in `openapi.yaml` by the [go-swagger](http://github.com/go-swagger/go-swagger) tool used by Rekor
       - This struct will be defined in the generated file at `pkg/generated/models/newType.go` (where `newType` is replaced with the name of the type you are adding)
       - This method should return a pointer to an instance of a struct that implements the `EntryImpl` interface as defined in `types.go`, or a `nil` pointer with an error specified
-  - embedds the `RekorType` type into the struct definition 
+  - embeds the `RekorType` type into the struct definition 
     - The purpose of this is to set the Kind variable to match the type name
     - `RekorType` also includes a `VersionMap` field, which provides the lookup for a version string from a proposed entry to find the correct implmentation code
 
@@ -78,21 +96,20 @@ type EntryImpl interface {
 	APIVersion() string
 	IndexKeys() []string
 	Canonicalize(ctx context.Context) ([]byte, error)
-	FetchExternalEntities(ctx context.Context) error
-	HasExternalEntities() bool
 	Unmarshal(pe models.ProposedEntry) error
-	Validate() error
+	Attestation() (string, []byte)
+	CreateFromArtifactProperties(context.Context, ArtifactProperties) (models.ProposedEntry, error)
 }
 ```
 
   - `APIVersion` should return a version string that identifies the version of the type supported by the Rekor server
   - `IndexKeys` should return a `[]string` that extracts the keys from an entry to be stored in the search index
   - `Canonicalize` should return a `[]byte` containing the canonicalized contents representing the entry. The canonicalization of contents is important as we should have one record per unique signed object in the transparency log.
-  - `FetchExternalEntities` should retrieve any entities that make up the entry which were not included in the object provided in the HTTP request to the Rekor server
-  - `HasExternalEntities` indicates whether the instance of the struct has any external entities it has yet to fetch and resolve
   - `Unmarshal` will be called with a pointer to a struct that was automatically generated for the type defined in `openapi.yaml` by the [go-swagger](http://github.com/go-swagger/go-swagger) tool used by Rekor
     - This method should validate the contents of the struct to ensure any string or cross-field dependencies are met to successfully insert an entry of this type into the transparency log
-  - `Validate` performs cross-field validation for fields in object that can not be expressed easily in the OpenAPI definition
+    - This method should also succesfully unmarshal entries that have been canonicalized and inserted into the log; however, it is worth noting that you may not be able to re-canonicalize an entry (since the original content required to verify the signature may not be present in the persisted log entry).
+  - `Attestation` provides types to return an in-toto attestation that should be persisted.
+  - `CreateFromArtifactProperties` creates a new entry given the specified artifact properties (typically passed in by `rekor-cli`).
 
 5. In the Go package you have created for the new type, be sure to add an entry in the `TypeMap` in `github.com/sigstore/rekor/pkg/types` for your new type in the `init` method for your package. The key for the map is the unique string used to define your type in `openapi.yaml` (e.g. `newType`), and the value for the map is the name of a factory function for an instance of `TypeImpl`.
 
@@ -104,7 +121,9 @@ func init() {
 
 6. Add an entry to `pluggableTypeMap` in `cmd/rekor-server/app/serve.go` that provides a reference to your package. This ensures that the `init` function of your type (and optionally, your version implementation) will be called before the server starts to process incoming requests and therefore will be added to the map that is used to route request processing for different types.
 
-7. After adding sufficient unit & integration tests, submit a pull request to `github.com/sigstore/rekor` for review and addition to the codebase.
+7. 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.
+
+8. After adding sufficient unit & integration tests, submit a pull request to `github.com/sigstore/rekor` for review and addition to the codebase.
 
 ## Adding a New Version of the `Rekord` type
 
@@ -120,4 +139,6 @@ To add new version of the default `Rekord` type:
 
 5. Add an entry to `pluggableTypeMap` in `cmd/rekor-server/app/serve.go` that provides a reference to the Go package implementing the new version. This ensures that the `init` function will be called before the server starts to process incoming requests and therefore will be added to the map that is used to route request processing for different types.
 
-6. After adding sufficient unit & integration tests, submit a pull request to `github.com/sigstore/rekor` for review and addition to the codebase.
+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.

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

@@ -69,8 +69,8 @@ func NewEntry() types.EntryImpl {
 func (v V001Entry) IndexKeys() []string {
 	var result []string
 
-	if v.HasExternalEntities() {
-		if err := v.FetchExternalEntities(context.Background()); err != nil {
+	if v.hasExternalEntities() {
+		if err := v.fetchExternalEntities(context.Background()); err != nil {
 			log.Logger.Error(err)
 			return result
 		}
@@ -108,11 +108,11 @@ func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
 	if err := v.AlpineModel.Validate(strfmt.Default); err != nil {
 		return err
 	}
-	return nil
 
+	return v.validate()
 }
 
-func (v V001Entry) HasExternalEntities() bool {
+func (v V001Entry) hasExternalEntities() bool {
 	if v.fetchedExternalEntities {
 		return false
 	}
@@ -126,12 +126,12 @@ func (v V001Entry) HasExternalEntities() bool {
 	return false
 }
 
-func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
+func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 	if v.fetchedExternalEntities {
 		return nil
 	}
 
-	if err := v.Validate(); err != nil {
+	if err := v.validate(); err != nil {
 		return err
 	}
 
@@ -269,7 +269,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
 }
 
 func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
-	if err := v.FetchExternalEntities(ctx); err != nil {
+	if err := v.fetchExternalEntities(ctx); err != nil {
 		return nil, err
 	}
 	if v.keyObj == nil {
@@ -308,8 +308,8 @@ func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
 	return json.Marshal(&apk)
 }
 
-// Validate performs cross-field validation for fields in object
-func (v V001Entry) Validate() error {
+// validate performs cross-field validation for fields in object
+func (v V001Entry) validate() error {
 	key := v.AlpineModel.PublicKey
 	if key == nil {
 		return errors.New("missing public key")
@@ -323,15 +323,13 @@ func (v V001Entry) Validate() error {
 		return errors.New("missing package")
 	}
 
-	if len(pkg.Content) == 0 && pkg.URL.String() == "" {
-		return errors.New("one of 'content' or 'url' must be specified for package")
-	}
-
 	hash := pkg.Hash
 	if hash != nil {
 		if !govalidator.IsHash(swag.StringValue(hash.Value), swag.StringValue(hash.Algorithm)) {
 			return errors.New("invalid value for hash")
 		}
+	} else if len(pkg.Content) == 0 && pkg.URL.String() == "" {
+		return errors.New("one of 'content' or 'url' must be specified for package")
 	}
 
 	return nil
@@ -387,12 +385,12 @@ func (v V001Entry) CreateFromArtifactProperties(ctx context.Context, props types
 		re.AlpineModel.PublicKey.Content = strfmt.Base64(publicKeyBytes)
 	}
 
-	if err := re.Validate(); err != nil {
+	if err := re.validate(); err != nil {
 		return nil, err
 	}
 
-	if re.HasExternalEntities() {
-		if err := re.FetchExternalEntities(ctx); err != nil {
+	if re.hasExternalEntities() {
+		if err := re.fetchExternalEntities(ctx); err != nil {
 			return nil, fmt.Errorf("error retrieving external entities: %v", err)
 		}
 	}

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

@@ -16,6 +16,7 @@
 package alpine
 
 import (
+	"bytes"
 	"context"
 	"crypto/sha256"
 	"encoding/hex"
@@ -26,11 +27,13 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/go-openapi/runtime"
 	"github.com/go-openapi/strfmt"
 	"github.com/go-openapi/swag"
 	"go.uber.org/goleak"
 
 	"github.com/sigstore/rekor/pkg/generated/models"
+	"github.com/sigstore/rekor/pkg/types"
 )
 
 func TestMain(m *testing.M) {
@@ -363,7 +366,7 @@ func TestCrossFieldValidation(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		if err := tc.entry.Validate(); (err == nil) != tc.expectUnmarshalSuccess {
+		if err := tc.entry.validate(); (err == nil) != tc.expectUnmarshalSuccess {
 			t.Errorf("unexpected result in '%v': %v", tc.caseDesc, err)
 		}
 
@@ -377,18 +380,28 @@ func TestCrossFieldValidation(t *testing.T) {
 			if err := v.Unmarshal(&r); err != nil {
 				return err
 			}
-			return v.Validate()
+			return v.validate()
 		}
 		if err := unmarshalAndValidate(); (err == nil) != tc.expectUnmarshalSuccess {
 			t.Errorf("unexpected result in '%v': %v", tc.caseDesc, err)
 		}
 
-		if tc.entry.HasExternalEntities() != tc.hasExtEntities {
+		if tc.entry.hasExternalEntities() != tc.hasExtEntities {
 			t.Errorf("unexpected result from HasExternalEntities for '%v'", tc.caseDesc)
 		}
 
-		if _, err := tc.entry.Canonicalize(context.TODO()); (err == nil) != tc.expectCanonicalizeSuccess {
+		b, err := v.Canonicalize(context.TODO())
+		if (err == nil) != tc.expectCanonicalizeSuccess {
 			t.Errorf("unexpected result from Canonicalize for '%v': %v", tc.caseDesc, err)
 		}
+		if b != nil {
+			pe, err := models.UnmarshalProposedEntry(bytes.NewReader(b), runtime.JSONConsumer())
+			if err != nil {
+				t.Errorf("unexpected err from Unmarshalling canonicalized entry for '%v': %v", tc.caseDesc, err)
+			}
+			if _, err := types.NewEntry(pe); err != nil {
+				t.Errorf("unexpected err from type-specific unmarshalling for '%v': %v", tc.caseDesc, err)
+			}
+		}
 	}
 }

pkg/types/entries.go

@@ -31,11 +31,8 @@ import (
 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
-	Canonicalize(ctx context.Context) ([]byte, error) // generate the canonical entry to be put into the tlog
-	FetchExternalEntities(ctx context.Context) error  // gather all external content required to process the entry
-	HasExternalEntities() bool                        // indicates whether there is a need fetch any additional external content required to process the 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
-	Validate() error                                  // performs any cross-field validation that is not expressed in the OpenAPI spec
 	Attestation() (string, []byte)
 	CreateFromArtifactProperties(context.Context, ArtifactProperties) (models.ProposedEntry, error)
 }

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

@@ -70,8 +70,8 @@ func NewEntry() types.EntryImpl {
 func (v V001Entry) IndexKeys() []string {
 	var result []string
 
-	if v.HasExternalEntities() {
-		if err := v.FetchExternalEntities(context.Background()); err != nil {
+	if v.hasExternalEntities() {
+		if err := v.fetchExternalEntities(context.Background()); err != nil {
 			log.Logger.Error(err)
 			return result
 		}
@@ -115,13 +115,12 @@ func (v *V001Entry) Unmarshal(pe models.ProposedEntry) error {
 	if err := v.HelmObj.Validate(strfmt.Default); err != nil {
 		return err
 	}
-	// cross field validation
-	return nil
 
+	// cross field validation
+	return v.validate()
 }
 
-func (v V001Entry) HasExternalEntities() bool {
-
+func (v V001Entry) hasExternalEntities() bool {
 	if v.fetchedExternalEntities {
 		return false
 	}
@@ -136,13 +135,12 @@ func (v V001Entry) HasExternalEntities() bool {
 	return false
 }
 
-func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
-
+func (v *V001Entry) fetchExternalEntities(ctx context.Context) error {
 	if v.fetchedExternalEntities {
 		return nil
 	}
 
-	if err := v.Validate(); err != nil {
+	if err := v.validate(); err != nil {
 		return err
 	}
 
@@ -257,8 +255,7 @@ func (v *V001Entry) FetchExternalEntities(ctx context.Context) error {
 }
 
 func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
-
-	if err := v.FetchExternalEntities(ctx); err != nil {
+	if err := v.fetchExternalEntities(ctx); err != nil {
 		return nil, err
 	}
 
@@ -317,8 +314,8 @@ func (v *V001Entry) Canonicalize(ctx context.Context) ([]byte, error) {
 
 }
 
-// Validate performs cross-field validation for fields in object
-func (v V001Entry) Validate() error {
+// validate performs cross-field validation for fields in object
+func (v V001Entry) validate() error {
 
 	key := v.HelmObj.PublicKey
 
@@ -342,8 +339,10 @@ func (v V001Entry) Validate() error {
 		return errors.New("missing provenance")
 	}
 
-	if len(provenance.Content) == 0 && provenance.URL.String() == "" {
-		return errors.New("one of 'content' or 'url' must be specified for provenance")
+	if provenance.Signature == nil || provenance.Signature.Content == nil {
+		if len(provenance.Content) == 0 && provenance.URL.String() == "" {
+			return errors.New("one of 'content' or 'url' must be specified for provenance")
+		}
 	}
 
 	return nil
@@ -395,12 +394,12 @@ func (v V001Entry) CreateFromArtifactProperties(ctx context.Context, props types
 		re.HelmObj.PublicKey.Content = strfmt.Base64(publicKeyBytes)
 	}
 
-	if err := re.Validate(); err != nil {
+	if err := re.validate(); err != nil {
 		return nil, err
 	}
 
-	if re.HasExternalEntities() {
-		if err := re.FetchExternalEntities(ctx); err != nil {
+	if re.hasExternalEntities() {
+		if err := re.fetchExternalEntities(ctx); err != nil {
 			return nil, fmt.Errorf("error retrieving external entities: %v", err)
 		}
 	}