Fix rekor-cli backwards incompatibility & run harness tests against HEAD (#1030)

* Run harness tests against 2 previous versions and HEAD

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

* Only collect stdout

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

* Update e2e tests

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

* iterate through versions of intoto type if default is specified

Signed-off-by: Bob Callaway <bcallaway@google.com>

* Skip harness tests if rekor-cli is incompatible with rekor-server version

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

* Code review comments

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>

Signed-off-by: Priya Wadhwa <priya@chainguard.dev>
Signed-off-by: Bob Callaway <bcallaway@google.com>
Co-authored-by: Bob Callaway <bcallaway@google.com>

Author: priyawadhwa

cmd/rekor-cli/app/pflags_test.go

@@ -762,7 +762,7 @@ func TestParseTypeFlag(t *testing.T) {
 		{
 			caseDesc:      "explicit intoto v0.0.1",
 			typeStr:       "intoto:0.0.1",
-			expectSuccess: false,
+			expectSuccess: true,
 		},
 		{
 			caseDesc:      "explicit intoto v0.0.2",

cmd/rekor-cli/app/upload.go

@@ -31,6 +31,7 @@ import (
 
 	"github.com/sigstore/rekor/cmd/rekor-cli/app/format"
 	"github.com/sigstore/rekor/pkg/client"
+	gen_client "github.com/sigstore/rekor/pkg/generated/client"
 	"github.com/sigstore/rekor/pkg/generated/client/entries"
 	"github.com/sigstore/rekor/pkg/generated/models"
 	"github.com/sigstore/rekor/pkg/log"
@@ -73,8 +74,6 @@ var uploadCmd = &cobra.Command{
 			return nil, err
 		}
 		var entry models.ProposedEntry
-		params := entries.NewCreateLogEntryParams()
-		params.SetTimeout(viper.GetDuration("timeout"))
 
 		entryStr := viper.GetString("entry")
 		if entryStr != "" {
@@ -111,9 +110,7 @@ var uploadCmd = &cobra.Command{
 				return nil, fmt.Errorf("error: %w", err)
 			}
 		}
-		params.SetProposedEntry(entry)
-
-		resp, err := rekorClient.Entries.CreateLogEntry(params)
+		resp, err := tryUpload(rekorClient, entry)
 		if err != nil {
 			switch e := err.(type) {
 			case *entries.CreateLogEntryConflict:
@@ -160,6 +157,29 @@ var uploadCmd = &cobra.Command{
 	}),
 }
 
+func tryUpload(rekorClient *gen_client.Rekor, entry models.ProposedEntry) (*entries.CreateLogEntryCreated, error) {
+	params := entries.NewCreateLogEntryParams()
+	params.SetTimeout(viper.GetDuration("timeout"))
+	if pei, ok := entry.(types.ProposedEntryIterator); ok {
+		params.SetProposedEntry(pei.Get())
+	} else {
+		params.SetProposedEntry(entry)
+	}
+	resp, err := rekorClient.Entries.CreateLogEntry(params)
+	if err != nil {
+		if e, ok := err.(*entries.CreateLogEntryBadRequest); ok {
+			if pei, ok := entry.(types.ProposedEntryIterator); ok {
+				if pei.HasNext() {
+					log.CliLogger.Errorf("failed to upload entry: %v", e)
+					return tryUpload(rekorClient, pei.GetNext())
+				}
+			}
+		}
+		return nil, err
+	}
+	return resp, nil
+}
+
 func init() {
 	initializePFlagMap()
 	if err := addArtifactPFlags(uploadCmd); err != nil {

pkg/types/entries.go

@@ -45,6 +45,14 @@ type EntryWithAttestationImpl interface {
 	AttestationKeyValue() (string, []byte) // returns the key to be used when storing the attestation as well as the attestation itself
 }
 
+// ProposedEntryIterator is an iterator over a list of proposed entries
+type ProposedEntryIterator interface {
+	models.ProposedEntry
+	HasNext() bool
+	Get() models.ProposedEntry
+	GetNext() models.ProposedEntry
+}
+
 // EntryFactory describes a factory function that can generate structs for a specific versioned type
 type EntryFactory func() EntryImpl
 

pkg/types/intoto/intoto.go

@@ -21,6 +21,7 @@ import (
 	"fmt"
 
 	"github.com/sigstore/rekor/pkg/generated/models"
+	"github.com/sigstore/rekor/pkg/log"
 	"github.com/sigstore/rekor/pkg/types"
 	"golang.org/x/exp/slices"
 )
@@ -60,9 +61,41 @@ func (it BaseIntotoType) UnmarshalEntry(pe models.ProposedEntry) (types.EntryImp
 }
 
 func (it *BaseIntotoType) CreateProposedEntry(ctx context.Context, version string, props types.ArtifactProperties) (models.ProposedEntry, error) {
+	var head ProposedIntotoEntryIterator
+	var next *ProposedIntotoEntryIterator
 	if version == "" {
+		// get default version as head of list
 		version = it.DefaultVersion()
+		ei, err := it.VersionedUnmarshal(nil, version)
+		if err != nil {
+			return nil, fmt.Errorf("fetching default Intoto version implementation: %w", err)
+		}
+		pe, err := ei.CreateFromArtifactProperties(ctx, props)
+		if err != nil {
+			return nil, fmt.Errorf("creating default Intoto entry: %w", err)
+		}
+		head.ProposedEntry = pe
+		next = &head
+		for _, v := range it.SupportedVersions() {
+			if v == it.DefaultVersion() {
+				continue
+			}
+			ei, err := it.VersionedUnmarshal(nil, v)
+			if err != nil {
+				log.ContextLogger(ctx).Errorf("fetching Intoto version (%v) implementation: %w", v, err)
+				continue
+			}
+			versionedPE, err := ei.CreateFromArtifactProperties(ctx, props)
+			if err != nil {
+				log.ContextLogger(ctx).Errorf("error creating Intoto entry of version (%v): %w", v, err)
+				continue
+			}
+			next.next = &ProposedIntotoEntryIterator{versionedPE, nil}
+			next = next.next.(*ProposedIntotoEntryIterator)
+		}
+		return head, nil
 	}
+
 	ei, err := it.VersionedUnmarshal(nil, version)
 	if err != nil {
 		return nil, fmt.Errorf("fetching Intoto version implementation: %w", err)
@@ -74,14 +107,29 @@ func (it BaseIntotoType) DefaultVersion() string {
 	return "0.0.2"
 }
 
-// SupportedVersions returns the supported versions for this type;
-// it deliberately omits 0.0.1 from the list of supported versions as that
-// version did not persist signatures inside the log entry
+// SupportedVersions returns the supported versions for this type in the order of preference
 func (it BaseIntotoType) SupportedVersions() []string {
-	return []string{"0.0.2"}
+	return []string{"0.0.2", "0.0.1"}
 }
 
 // IsSupportedVersion returns true if the version can be inserted into the log, and false if not
 func (it *BaseIntotoType) IsSupportedVersion(proposedVersion string) bool {
 	return slices.Contains(it.SupportedVersions(), proposedVersion)
 }
+
+type ProposedIntotoEntryIterator struct {
+	models.ProposedEntry
+	next models.ProposedEntry
+}
+
+func (p ProposedIntotoEntryIterator) HasNext() bool {
+	return p.next != nil
+}
+
+func (p ProposedIntotoEntryIterator) GetNext() models.ProposedEntry {
+	return p.next
+}
+
+func (p ProposedIntotoEntryIterator) Get() models.ProposedEntry {
+	return p.ProposedEntry
+}

tests/e2e_test.go

@@ -33,19 +33,16 @@ import (
 	"fmt"
 	"io/ioutil"
 	"net/http"
-	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"reflect"
-	"runtime"
 	"strconv"
 	"strings"
 	"testing"
 	"time"
 
 	"golang.org/x/sync/errgroup"
-	"sigs.k8s.io/release-utils/version"
 
 	"github.com/cyberphone/json-canonicalization/go/src/webpki.org/jsoncanonicalizer"
 	"github.com/go-openapi/strfmt"
@@ -524,11 +521,6 @@ func TestIntoto(t *testing.T) {
 	write(t, string(eb), attestationPath)
 	write(t, ecdsaPub, pubKeyPath)
 
-	// ensure that we can't upload a intoto v0.0.1 entry
-	v001out := runCliErr(t, "upload", "--artifact", attestationPath, "--type", "intoto:0.0.1", "--public-key", pubKeyPath)
-	outputContains(t, v001out, "type intoto does not support version 0.0.1")
-
-	// If we do it twice, it should already exist
 	out := runCli(t, "upload", "--artifact", attestationPath, "--type", "intoto", "--public-key", pubKeyPath)
 	outputContains(t, out, "Created entry at")
 	uuid := getUUIDFromUploadOutput(t, out)
@@ -658,11 +650,6 @@ func TestIntotoMultiSig(t *testing.T) {
 	write(t, ecdsaPub, ecdsapubKeyPath)
 	write(t, pubKey, rsapubKeyPath)
 
-	// ensure that we can't upload a intoto v0.0.1 entry
-	v001out := runCliErr(t, "upload", "--artifact", attestationPath, "--type", "intoto:0.0.1", "--public-key", ecdsapubKeyPath, "--public-key", rsapubKeyPath)
-	outputContains(t, v001out, "type intoto does not support version 0.0.1")
-
-	// If we do it twice, it should already exist
 	out := runCli(t, "upload", "--artifact", attestationPath, "--type", "intoto", "--public-key", ecdsapubKeyPath, "--public-key", rsapubKeyPath)
 	outputContains(t, out, "Created entry at")
 	uuid := getUUIDFromUploadOutput(t, out)
@@ -706,6 +693,7 @@ func TestIntotoMultiSig(t *testing.T) {
 
 }
 
+/*
 func TestIntotoBlockV001(t *testing.T) {
 	td := t.TempDir()
 	attestationPath := filepath.Join(td, "attestation.json")
@@ -795,13 +783,11 @@ func TestIntotoBlockV001(t *testing.T) {
 	params.SetProposedEntry(entry)
 
 	_, err = rekorClient.Entries.CreateLogEntry(params)
-	if err == nil {
-		t.Fatal("insertion of v0.0.1 entry should fail")
-	}
-	if !strings.Contains(err.Error(), "entry kind 'intoto' does not support inserting entries of version '0.0.1'") {
-		t.Errorf("Expected error as intoto v0.0.1 should not be allowed to be entered into rekor")
+	if err != nil {
+		t.Fatalf("failed inserting v0.0.1 entry: %v", err)
 	}
 }
+*/
 
 func TestTimestampArtifact(t *testing.T) {
 	var out string

tests/harness_test.go

@@ -77,9 +77,12 @@ func TestHarnessAddEntry(t *testing.T) {
 	uuid := getUUIDFromUploadOutput(t, out)
 	logIndex := getLogIndexFromUploadOutput(t, out)
 
-	// Now we should be able to verify it.
-	out = runCli(t, "verify", "--type=hashedrekord", "--pki-format=x509", "--artifact-hash", dataSHA, "--signature", sigPath, "--public-key", pubPath)
-	outputContains(t, out, "Inclusion Proof:")
+	if !rekorCLIIncompatible() {
+		// Now we should be able to verify it.
+		out = runCli(t, "verify", "--type=hashedrekord", "--pki-format=x509", "--artifact-hash", dataSHA, "--signature", sigPath, "--public-key", pubPath)
+		outputContains(t, out, "Inclusion Proof:")
+	}
+
 	saveEntry(t, logIndex, StoredEntry{UUID: uuid})
 }
 
@@ -150,12 +153,12 @@ func TestHarnessAddIntoto(t *testing.T) {
 	write(t, ecdsaPub, pubKeyPath)
 
 	// If we do it twice, it should already exist
-	out := runCli(t, "upload", "--artifact", attestationPath, "--type", "intoto", "--public-key", pubKeyPath)
+	out := runCliStdout(t, "upload", "--artifact", attestationPath, "--type", "intoto", "--public-key", pubKeyPath)
 	outputContains(t, out, "Created entry at")
 	uuid := getUUIDFromUploadOutput(t, out)
 	logIndex := getLogIndexFromUploadOutput(t, out)
 
-	out = runCli(t, "get", "--uuid", uuid, "--format=json")
+	out = runCli(t, "get", "--log-index", fmt.Sprintf("%d", logIndex), "--format=json")
 	g := getOut{}
 	if err := json.Unmarshal([]byte(out), &g); err != nil {
 		t.Fatal(err)
@@ -265,11 +268,16 @@ func TestHarnessGetAllEntriesLogIndex(t *testing.T) {
 }
 
 func TestHarnessGetAllEntriesUUID(t *testing.T) {
+	if rekorCLIIncompatible() {
+		t.Skipf("Skipping getting entries by UUID, old rekor-cli version %s is incompatible with server version %s", os.Getenv("CLI_VERSION"), os.Getenv("SERVER_VERSION"))
+	}
+
 	treeSize := activeTreeSize(t)
 	if treeSize == 0 {
 		t.Fatal("There are 0 entries in the log, there should be at least 2")
 	}
 	_, entries := getEntries(t)
+
 	for _, e := range entries {
 		outUUID := runCli(t, "get", "--uuid", e.UUID, "--format", "json")
 		outEntryID := runCli(t, "get", "--uuid", entryID(t, e.UUID), "--format", "json")
@@ -294,6 +302,9 @@ func TestHarnessGetAllEntriesUUID(t *testing.T) {
 }
 
 func entryID(t *testing.T, uuid string) string {
+	if sharding.ValidateEntryID(uuid) == nil {
+		return uuid
+	}
 	treeID, err := strconv.Atoi(os.Getenv("TREE_ID"))
 	if err != nil {
 		t.Fatal(err)
@@ -317,3 +328,14 @@ func activeTreeSize(t *testing.T) int {
 	}
 	return s.ActiveTreeSize
 }
+
+// Check if we have a new server version and an old CLI version
+// since the new server returns an EntryID but the old CLI version expects a UUID
+func rekorCLIIncompatible() bool {
+	if sv := os.Getenv("SERVER_VERSION"); sv != "v0.10.0" && sv != "v0.11.0" {
+		if cv := os.Getenv("CLI_VERSION"); cv == "v0.10.0" || cv == "v0.11.0" {
+			return true
+		}
+	}
+	return false
+}

tests/rekor-harness.sh

@@ -75,7 +75,7 @@ function run_tests () {
     trap "rm -rf $REKORTMPDIR" EXIT
 
     go clean -testcache
-    if ! REKORTMPDIR=$REKORTMPDIR go test -run TestHarness -v -tags=e2e ./tests/ ; then 
+    if ! REKORTMPDIR=$REKORTMPDIR SERVER_VERSION=$1 CLI_VERSION=$2 go test -run TestHarness -v -tags=e2e ./tests/ ; then 
         docker-compose logs --no-color > /tmp/docker-compose.log
         exit 1
     fi
@@ -87,10 +87,17 @@ function run_tests () {
     fi
 }
 
-# Get last 3 server versions
+# Get last 2 server versions
 git fetch origin
-NUM_VERSIONS_TO_TEST=3
+NUM_VERSIONS_TO_TEST=2
 VERSIONS=$(git tag --sort=-version:refname | head -n $NUM_VERSIONS_TO_TEST | tac)
+
+# Also add the commit @ HEAD
+HEAD=$(git log --pretty="%H" -n 1 )
+echo "Also testing at HEAD at commit $HEAD"
+
+VERSIONS="$VERSIONS $HEAD"
+
 echo $VERSIONS
 
 export REKOR_HARNESS_TMPDIR="$(mktemp -d -t rekor_test_harness.XXXXXX)"
@@ -105,17 +112,17 @@ do
         echo "Running tests with server version $server_version and CLI version $cli_version"
 
         build_cli $cli_version
-        run_tests
+        run_tests $server_version $cli_version
 
         echo "Tests passed successfully."
         echo "======================================================="
     done
 done
 
-# Since we add two entries to the log for every test, once all tests are run we should have 2*($NUM_VERSIONS_TO_TEST^2) entries
+# Since we add two entries to the log for every test, once all tests are run we should have 2*(($NUM_VERSIONS_TO_TEST+1)^2) entries
 make rekor-cli
 actual=$(./rekor-cli loginfo --rekor_server http://localhost:3000 --format json --store_tree_state=false | jq -r .ActiveTreeSize)
-expected=$((2*$NUM_VERSIONS_TO_TEST*$NUM_VERSIONS_TO_TEST))
+expected=$((2*(1+$NUM_VERSIONS_TO_TEST)*(1+$NUM_VERSIONS_TO_TEST)))
 if [[ ! "$expected" == "$actual" ]]; then
     echo "ERROR: Log had $actual entries instead of expected $expected"
     exit 1