From a924ca172f6baf0da2703bf9c7dd11feada7f1c5 Mon Sep 17 00:00:00 2001
From: Ying Li <ying.li@docker.com>
Date: Wed, 9 Dec 2015 10:40:50 -0800
Subject: [PATCH] When initializing a repo, create local keys before getting
 remote keys.

Signed-off-by: Ying Li <ying.li@docker.com>
---
 client/client.go      | 41 +++++++++++++++++++++---------------
 client/client_test.go | 48 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 64 insertions(+), 25 deletions(-)

diff --git a/client/client.go b/client/client.go
index 8fddd233a5..08a8211dc1 100644
--- a/client/client.go
+++ b/client/client.go
@@ -192,26 +192,33 @@ func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...st
 		return err
 	}
 
-	for role, isManaged := range rolesAreManaged {
-		var key data.PublicKey
-		if isManaged {
-			// This key is generated by the remote server.
-			key, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
+	// we want to create all the local keys first so we don't have to
+	// make unnecessary network calls
+	for _, isManaged := range []bool{false, true} {
+		for role, shouldBeManaged := range rolesAreManaged {
+			if isManaged != shouldBeManaged {
+				continue
+			}
+			var key data.PublicKey
+			if isManaged {
+				// This key is generated by the remote server.
+				key, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
+				if err != nil {
+					return err
+				}
+				logrus.Debugf("got remote %s %s key with keyID: %s",
+					role, key.Algorithm(), key.ID())
+			} else {
+				// This is currently hardcoding the keys to ECDSA.
+				key, err = r.CryptoService.Create(role, data.ECDSAKey)
+				if err != nil {
+					return err
+				}
+			}
+			err = addKeyForRole(kdb, role, key)
 			if err != nil {
 				return err
 			}
-			logrus.Debugf("got remote %s %s key with keyID: %s",
-				role, key.Algorithm(), key.ID())
-		} else {
-			// This is currently hardcoding the keys to ECDSA.
-			key, err = r.CryptoService.Create(role, data.ECDSAKey)
-			if err != nil {
-				return err
-			}
-		}
-		err = addKeyForRole(kdb, role, key)
-		if err != nil {
-			return err
 		}
 	}
 
diff --git a/client/client_test.go b/client/client_test.go
index 20743b1399..9dfd34da6e 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -784,7 +784,6 @@ func testPublish(t *testing.T, rootType string, serverManagesSnapshot bool) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
-
 	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
 
 	gun := "docker.com/notary"
@@ -855,7 +854,6 @@ func testPublishAfterPullServerHasSnapshotKey(t *testing.T, rootType string) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
-
 	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
 
 	gun := "docker.com/notary"
@@ -904,7 +902,6 @@ func testPublishNoOneHasSnapshotKey(t *testing.T, rootType string) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
-
 	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
 
 	gun := "docker.com/notary"
@@ -932,17 +929,12 @@ func testPublishNoOneHasSnapshotKey(t *testing.T, rootType string) {
 func TestPublishSnapshotCorrupt(t *testing.T) {
 	testPublishSnapshotCorrupt(t, data.ECDSAKey, true)
 	testPublishSnapshotCorrupt(t, data.ECDSAKey, false)
-	if !testing.Short() {
-		testPublishSnapshotCorrupt(t, data.RSAKey, true)
-		testPublishSnapshotCorrupt(t, data.RSAKey, false)
-	}
 }
 
 func testPublishSnapshotCorrupt(t *testing.T, rootType string, serverManagesSnapshot bool) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
 	defer os.RemoveAll(tempBaseDir)
-
 	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
 
 	gun := "docker.com/notary"
@@ -958,6 +950,46 @@ func testPublishSnapshotCorrupt(t *testing.T, rootType string, serverManagesSnap
 	assert.Error(t, err)
 }
 
+type cannotCreateKeys struct {
+	signed.CryptoService
+}
+
+func (cs cannotCreateKeys) Create(_, _ string) (data.PublicKey, error) {
+	return nil, fmt.Errorf("Oh no I cannot create keys")
+}
+
+// If there is an error creating the local keys, no call is made to get a
+// remote key.
+func TestPublishSnapshotLocalKeysCreatedFirst(t *testing.T) {
+	// Temporary directory where test files will be created
+	tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
+	defer os.RemoveAll(tempBaseDir)
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	gun := "docker.com/notary"
+
+	requestMade := false
+	ts := httptest.NewServer(http.HandlerFunc(
+		func(http.ResponseWriter, *http.Request) { requestMade = true }))
+	defer ts.Close()
+
+	repo, err := NewNotaryRepository(
+		tempBaseDir, gun, ts.URL, http.DefaultTransport, passphraseRetriever)
+	assert.NoError(t, err, "error creating repo: %s", err)
+
+	cs := cryptoservice.NewCryptoService(gun,
+		trustmanager.NewKeyMemoryStore(passphraseRetriever))
+
+	rootPubKey, err := cs.Create(data.CanonicalRootRole, data.ECDSAKey)
+	assert.NoError(t, err, "error generating root key: %s", err)
+
+	repo.CryptoService = cannotCreateKeys{CryptoService: cs}
+
+	err = repo.Initialize(rootPubKey.ID(), data.CanonicalSnapshotRole)
+	assert.Error(t, err)
+	assert.Contains(t, err.Error(), "Oh no I cannot create keys")
+	assert.False(t, requestMade)
+}
+
 func TestRotate(t *testing.T) {
 	// Temporary directory where test files will be created
 	tempBaseDir, err := ioutil.TempDir("", "notary-test-")