Merge pull request #349 from endophage/server_snapshot_bugfixes

fixing bugs raised by @mtrmac

Makefile

@@ -43,8 +43,8 @@ GO_VERSION = $(shell go version | awk '{print $$3}')
 .DEFAULT: default
 
 go_version:
-ifneq ("$(GO_VERSION)", "go1.5.1")
+ifeq (,$(findstring go1.5.,$(GO_VERSION)))
-	$(error Requires go version 1.5.1 - found $(GO_VERSION))
+	$(error Requires go version 1.5.x - found $(GO_VERSION))
 else
 	@echo
 endif

server/handlers/validation.go

@@ -81,9 +81,21 @@ func validateUpdate(cs signed.CryptoService, gun string, updates []storage.MetaU
 			return nil, validation.ErrBadTargets{Msg: err.Error()}
 		}
 		repo.SetTargets(targetsRole, t)
+	} else {
+		targetsJSON, err := store.GetCurrent(gun, data.CanonicalTargetsRole)
+		if err != nil {
+			return nil, validation.ErrValidation{Msg: err.Error()}
+		}
+		targets := &data.SignedTargets{}
+		err = json.Unmarshal(targetsJSON, targets)
+		if err != nil {
+			return nil, validation.ErrValidation{Msg: err.Error()}
+		}
+		repo.SetTargets(data.CanonicalTargetsRole, targets)
 	}
 	logrus.Debug("Successfully validated targets")
 
+	// At this point, root and targets must have been loaded into the repo
 	if _, ok := roles[snapshotRole]; ok {
 		var oldSnap *data.SignedSnapshot
 		oldSnapJSON, err := store.GetCurrent(gun, snapshotRole)
@@ -111,10 +123,6 @@ func validateUpdate(cs signed.CryptoService, gun string, updates []storage.MetaU
 		// Then:
 		//   - generate a new snapshot
 		//   - add it to the updates
-		err := prepRepo(gun, repo, store)
-		if err != nil {
-			return nil, err
-		}
 		update, err := generateSnapshot(gun, kdb, repo, store)
 		if err != nil {
 			return nil, err
@@ -124,34 +132,6 @@ func validateUpdate(cs signed.CryptoService, gun string, updates []storage.MetaU
 	return updates, nil
 }
 
-func prepRepo(gun string, repo *tuf.Repo, store storage.MetaStore) error {
-	if repo.Root == nil {
-		rootJSON, err := store.GetCurrent(gun, data.CanonicalRootRole)
-		if err != nil {
-			return fmt.Errorf("could not load repo for snapshot generation: %v", err)
-		}
-		root := &data.SignedRoot{}
-		err = json.Unmarshal(rootJSON, root)
-		if err != nil {
-			return fmt.Errorf("could not load repo for snapshot generation: %v", err)
-		}
-		repo.SetRoot(root)
-	}
-	if repo.Targets[data.CanonicalTargetsRole] == nil {
-		targetsJSON, err := store.GetCurrent(gun, data.CanonicalTargetsRole)
-		if err != nil {
-			return fmt.Errorf("could not load repo for snapshot generation: %v", err)
-		}
-		targets := &data.SignedTargets{}
-		err = json.Unmarshal(targetsJSON, targets)
-		if err != nil {
-			return fmt.Errorf("could not load repo for snapshot generation: %v", err)
-		}
-		repo.SetTargets(data.CanonicalTargetsRole, targets)
-	}
-	return nil
-}
-
 func generateSnapshot(gun string, kdb *keys.KeyDB, repo *tuf.Repo, store storage.MetaStore) (*storage.MetaUpdate, error) {
 	role := kdb.GetRole(data.RoleName(data.CanonicalSnapshotRole))
 	if role == nil {
@@ -159,8 +139,8 @@ func generateSnapshot(gun string, kdb *keys.KeyDB, repo *tuf.Repo, store storage
 	}
 
 	algo, keyBytes, err := store.GetKey(gun, data.CanonicalSnapshotRole)
-	if role == nil {
+	if err != nil {
-		return nil, validation.ErrBadRoot{Msg: "root did not include snapshot key"}
+		return nil, validation.ErrBadHierarchy{Msg: "could not retrieve snapshot key. client must provide snapshot"}
 	}
 	foundK := data.NewPublicKey(algo, keyBytes)
 
@@ -180,7 +160,7 @@ func generateSnapshot(gun string, kdb *keys.KeyDB, repo *tuf.Repo, store storage
 	currentJSON, err := store.GetCurrent(gun, data.CanonicalSnapshotRole)
 	if err != nil {
 		if _, ok := err.(*storage.ErrNotFound); !ok {
-			return nil, fmt.Errorf("could not retrieve previous snapshot: %v", err)
+			return nil, validation.ErrValidation{Msg: err.Error()}
 		}
 	}
 	var sn *data.SignedSnapshot
@@ -188,25 +168,25 @@ func generateSnapshot(gun string, kdb *keys.KeyDB, repo *tuf.Repo, store storage
 		sn = new(data.SignedSnapshot)
 		err := json.Unmarshal(currentJSON, sn)
 		if err != nil {
-			return nil, fmt.Errorf("could not retrieve previous snapshot: %v", err)
+			return nil, validation.ErrValidation{Msg: err.Error()}
 		}
 		err = repo.SetSnapshot(sn)
 		if err != nil {
-			return nil, fmt.Errorf("could not load previous snapshot: %v", err)
+			return nil, validation.ErrValidation{Msg: err.Error()}
 		}
 	} else {
 		err := repo.InitSnapshot()
 		if err != nil {
-			return nil, fmt.Errorf("could not initialize snapshot: %v", err)
+			return nil, validation.ErrBadSnapshot{Msg: err.Error()}
 		}
 	}
 	sgnd, err := repo.SignSnapshot(data.DefaultExpires(data.CanonicalSnapshotRole))
 	if err != nil {
-		return nil, fmt.Errorf("could not sign snapshot: %v", err)
+		return nil, validation.ErrBadSnapshot{Msg: err.Error()}
 	}
 	sgndJSON, err := json.Marshal(sgnd)
 	if err != nil {
-		return nil, fmt.Errorf("could not save snapshot: %v", err)
+		return nil, validation.ErrBadSnapshot{Msg: err.Error()}
 	}
 	return &storage.MetaUpdate{
 		Role:    data.CanonicalSnapshotRole,

server/handlers/validation_test.go

@@ -7,7 +7,6 @@ import (
 	"testing"
 
 	"github.com/docker/notary/trustmanager"
-	"github.com/docker/notary/tuf"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/keys"
 	"github.com/docker/notary/tuf/signed"
@@ -377,71 +376,6 @@ func TestValidateSnapshotGenerateLoadRootTargets(t *testing.T) {
 	assert.NoError(t, err)
 }
 
-func TestPrepRepoLoadRootTargets(t *testing.T) {
-	_, repo, _ := testutils.EmptyRepo()
-	store := storage.NewMemStorage()
-
-	r, tg, sn, ts, err := testutils.Sign(repo)
-	assert.NoError(t, err)
-	root, targets, _, _, err := getUpdates(r, tg, sn, ts)
-	assert.NoError(t, err)
-
-	store.UpdateCurrent("testGUN", root)
-	store.UpdateCurrent("testGUN", targets)
-
-	toPrep := tuf.NewRepo(keys.NewDB(), nil)
-	assert.Nil(t, toPrep.Root)
-	assert.Nil(t, toPrep.Targets[data.CanonicalTargetsRole])
-	err = prepRepo("testGUN", toPrep, store)
-	assert.NoError(t, err)
-	assert.NotNil(t, toPrep.Root)
-	assert.NotNil(t, toPrep.Targets[data.CanonicalTargetsRole])
-}
-
-func TestPrepRepoLoadRootCorrupt(t *testing.T) {
-	_, repo, _ := testutils.EmptyRepo()
-	store := storage.NewMemStorage()
-
-	r, tg, sn, ts, err := testutils.Sign(repo)
-	assert.NoError(t, err)
-	root, targets, _, _, err := getUpdates(r, tg, sn, ts)
-	assert.NoError(t, err)
-
-	root.Data = root.Data[:1]
-	store.UpdateCurrent("testGUN", root)
-	store.UpdateCurrent("testGUN", targets)
-
-	toPrep := tuf.NewRepo(keys.NewDB(), nil)
-	err = prepRepo("testGUN", toPrep, store)
-	assert.Error(t, err)
-}
-
-func TestPrepRepoLoadTargetsCorrupt(t *testing.T) {
-	_, repo, _ := testutils.EmptyRepo()
-	store := storage.NewMemStorage()
-
-	r, tg, sn, ts, err := testutils.Sign(repo)
-	assert.NoError(t, err)
-	root, targets, _, _, err := getUpdates(r, tg, sn, ts)
-	assert.NoError(t, err)
-
-	targets.Data = targets.Data[:1]
-	store.UpdateCurrent("testGUN", root)
-	store.UpdateCurrent("testGUN", targets)
-
-	toPrep := tuf.NewRepo(keys.NewDB(), nil)
-	err = prepRepo("testGUN", toPrep, store)
-	assert.Error(t, err)
-}
-
-func TestPrepRepoLoadRootMissing(t *testing.T) {
-	store := storage.NewMemStorage()
-
-	toPrep := tuf.NewRepo(nil, nil)
-	err := prepRepo("testGUN", toPrep, store)
-	assert.Error(t, err)
-}
-
 // If there is no timestamp key in the store, validation fails.  This could
 // happen if pushing an existing repository from one server to another that
 // does not have the repo.
@@ -831,3 +765,22 @@ func TestValidateTargetsModifiedHash(t *testing.T) {
 }
 
 // ### End snapshot hash mismatch negative tests ###
+
+// ### generateSnapshot tests ###
+func TestGenerateSnapshotNoRole(t *testing.T) {
+	kdb := keys.NewDB()
+	_, err := generateSnapshot("gun", kdb, nil, nil)
+	assert.Error(t, err)
+	assert.IsType(t, validation.ErrBadRoot{}, err)
+}
+
+func TestGenerateSnapshotNoKey(t *testing.T) {
+	kdb, _, _ := testutils.EmptyRepo()
+	store := storage.NewMemStorage()
+
+	_, err := generateSnapshot("gun", kdb, nil, store)
+	assert.Error(t, err)
+	assert.IsType(t, validation.ErrBadHierarchy{}, err)
+}
+
+// ### End generateSnapshot tests ###

server/snapshot/snapshot.go

@@ -1,8 +1,8 @@
 package snapshot
 
 import (
-	"bytes"
 	"encoding/json"
+
 	"github.com/Sirupsen/logrus"
 
 	"github.com/docker/notary/server/storage"
@@ -43,7 +43,8 @@ func GetOrCreateSnapshotKey(gun string, store storage.KeyStore, crypto signed.Cr
 }
 
 // GetOrCreateSnapshot either returns the exisiting latest snapshot, or uses
-// whatever the most recent snapshot is to generate a new one.
+// whatever the most recent snapshot is to create the next one, only updating
+// the expiry time and version.
 func GetOrCreateSnapshot(gun string, store storage.MetaStore, cryptoService signed.CryptoService) ([]byte, error) {
 
 	d, err := store.GetCurrent(gun, "snapshot")
@@ -58,7 +59,8 @@ func GetOrCreateSnapshot(gun string, store storage.MetaStore, cryptoService sign
 			logrus.Error("Failed to unmarshal existing snapshot")
 			return nil, err
 		}
-		if !snapshotExpired(sn) && !contentExpired(gun, sn, store) {
+
+		if !snapshotExpired(sn) {
 			return d, nil
 		}
 	}
@@ -85,57 +87,13 @@ func snapshotExpired(sn *data.SignedSnapshot) bool {
 	return signed.IsExpired(sn.Signed.Expires)
 }
 
-// contentExpired checks to see if any of the roles already in the snapshot
-// have been updated. It will update any roles that have changed as it goes
-// so that we don't have to run through all this again a second time.
-func contentExpired(gun string, sn *data.SignedSnapshot, store storage.MetaStore) bool {
-	expired := false
-	updatedMeta := make(data.Files)
-	for role, meta := range sn.Signed.Meta {
-		curr, err := store.GetCurrent(gun, role)
-		if err != nil {
-			return false
-		}
-		roleExp, newHash := roleExpired(curr, meta)
-		if roleExp {
-			updatedMeta[role] = data.FileMeta{
-				Length: int64(len(curr)),
-				Hashes: data.Hashes{
-					"sha256": newHash,
-				},
-			}
-		}
-		expired = expired || roleExp
-	}
-	if expired {
-		sn.Signed.Meta = updatedMeta
-	}
-	return expired
-}
-
-// roleExpired checks if the content for a specific role differs from
-// the snapshot
-func roleExpired(roleData []byte, meta data.FileMeta) (bool, []byte) {
-	currMeta, err := data.NewFileMeta(bytes.NewReader(roleData), "sha256")
-	if err != nil {
-		// if we can't generate FileMeta from the current roleData, we should
-		// continue to serve the old role if it isn't time expired
-		// because we won't be able to generate a new one.
-		return false, nil
-	}
-	hash := currMeta.Hashes["sha256"]
-	return !bytes.Equal(hash, meta.Hashes["sha256"]), hash
-}
-
 // createSnapshot uses an existing snapshot to create a new one.
 // Important things to be aware of:
 //   - It requires that a snapshot already exists. We create snapshots
 //     on upload so there should always be an existing snapshot if this
 //     gets called.
 //   - It doesn't update what roles are present in the snapshot, as those
-//     were validated during upload. We also updated the hashes of the
+//     were validated during upload.
-//     already present roles as part of our checks on whether we could
-//     serve the previous version of the snapshot
 func createSnapshot(gun string, sn *data.SignedSnapshot, store storage.MetaStore, cryptoService signed.CryptoService) (*data.Signed, int, error) {
 	algorithm, public, err := store.GetKey(gun, data.CanonicalSnapshotRole)
 	if err != nil {

server/snapshot/snapshot_test.go

@@ -114,32 +114,6 @@ func TestGetSnapshotKeyExistsOnSet(t *testing.T) {
 	assert.NotNil(t, k2, "Key should not be nil")
 }
 
-func TestRoleExpired(t *testing.T) {
-	meta := data.FileMeta{
-		Hashes: data.Hashes{
-			"sha256": []byte{1},
-		},
-	}
-	newData := []byte{2}
-	res, _ := roleExpired(newData, meta)
-	assert.True(t, res)
-}
-
-func TestRoleNotExpired(t *testing.T) {
-	newData := []byte{2}
-	currMeta, err := data.NewFileMeta(bytes.NewReader(newData), "sha256")
-	assert.NoError(t, err)
-
-	meta := data.FileMeta{
-		Hashes: data.Hashes{
-			"sha256": currMeta.Hashes["sha256"],
-		},
-	}
-
-	res, _ := roleExpired(newData, meta)
-	assert.False(t, res)
-}
-
 func TestGetSnapshotNotExists(t *testing.T) {
 	store := storage.NewMemStorage()
 	crypto := signed.NewEd25519()
@@ -211,3 +185,24 @@ func TestGetSnapshotCurrCorrupt(t *testing.T) {
 	_, err = GetOrCreateSnapshot("gun", store, crypto)
 	assert.Error(t, err)
 }
+
+func TestCreateSnapshotNoKeyInStorage(t *testing.T) {
+	store := storage.NewMemStorage()
+	crypto := signed.NewEd25519()
+
+	_, _, err := createSnapshot("gun", nil, store, crypto)
+	assert.Error(t, err)
+}
+
+func TestCreateSnapshotNoKeyInCrypto(t *testing.T) {
+	store := storage.NewMemStorage()
+	crypto := signed.NewEd25519()
+
+	_, err := GetOrCreateSnapshotKey("gun", store, crypto, data.ED25519Key)
+
+	// reset crypto so the store has the key but crypto doesn't
+	crypto = signed.NewEd25519()
+
+	_, _, err = createSnapshot("gun", &data.SignedSnapshot{}, store, crypto)
+	assert.Error(t, err)
+}