Merge pull request #311 from mtrmac/invalid-passphrase

Fix error handling on invalid root passphrase

client/client.go

@@ -209,12 +209,8 @@ func (r *NotaryRepository) Initialize(rootKeyID string) error {
 	err = r.tufRepo.InitRoot(false)
 	if err != nil {
 		logrus.Debug("Error on InitRoot: ", err.Error())
-		switch err.(type) {
-		case signed.ErrInsufficientSignatures, trustmanager.ErrPasswordInvalid:
-		default:
 		return err
 	}
-	}
 	err = r.tufRepo.InitTargets()
 	if err != nil {
 		logrus.Debug("Error on InitTargets: ", err.Error())

client/client_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/docker/notary/certs"
 	"github.com/docker/notary/client/changelist"
 	"github.com/docker/notary/cryptoservice"
+	"github.com/docker/notary/passphrase"
 	"github.com/docker/notary/server"
 	"github.com/docker/notary/server/storage"
 	"github.com/docker/notary/trustmanager"
@@ -238,6 +239,77 @@ func testInitRepo(t *testing.T, rootType string) {
 	assertRepoHasExpectedMetadata(t, repo)
 }
 
+// TestInitRepoAttemptsExceeded tests error handling when passphrase.Retriever
+// (or rather the user) insists on an incorrect password.
+func TestInitRepoAttemptsExceeded(t *testing.T) {
+	testInitRepoAttemptsExceeded(t, data.ECDSAKey)
+	if !testing.Short() {
+		testInitRepoAttemptsExceeded(t, data.RSAKey)
+	}
+}
+
+func testInitRepoAttemptsExceeded(t *testing.T, rootType string) {
+	gun := "docker.com/notary"
+	// Temporary directory where test files will be created
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	ts, _, _ := simpleTestServer(t)
+	defer ts.Close()
+
+	retriever := passphrase.ConstantRetriever("password")
+	repo, err := NewNotaryRepository(tempBaseDir, gun, ts.URL, http.DefaultTransport, retriever)
+	assert.NoError(t, err, "error creating repo: %s", err)
+	rootPubKey, err := repo.CryptoService.Create("root", rootType)
+	assert.NoError(t, err, "error generating root key: %s", err)
+
+	retriever = passphrase.ConstantRetriever("incorrect password")
+	// repo.CryptoService’s FileKeyStore caches the unlocked private key, so to test
+	// private key unlocking we need a new repo instance.
+	repo, err = NewNotaryRepository(tempBaseDir, gun, ts.URL, http.DefaultTransport, retriever)
+	assert.NoError(t, err, "error creating repo: %s", err)
+	err = repo.Initialize(rootPubKey.ID())
+	assert.EqualError(t, err, trustmanager.ErrAttemptsExceeded{}.Error())
+}
+
+// TestInitRepoPasswordInvalid tests error handling when passphrase.Retriever
+// (or rather the user) fails to provide a correct password.
+func TestInitRepoPasswordInvalid(t *testing.T) {
+	testInitRepoPasswordInvalid(t, data.ECDSAKey)
+	if !testing.Short() {
+		testInitRepoPasswordInvalid(t, data.RSAKey)
+	}
+}
+
+func giveUpPassphraseRetriever(_, _ string, _ bool, _ int) (string, bool, error) {
+	return "", true, nil
+}
+
+func testInitRepoPasswordInvalid(t *testing.T, rootType string) {
+	gun := "docker.com/notary"
+	// Temporary directory where test files will be created
+	tempBaseDir, err := ioutil.TempDir("", "notary-test-")
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	ts, _, _ := simpleTestServer(t)
+	defer ts.Close()
+
+	retriever := passphrase.ConstantRetriever("password")
+	repo, err := NewNotaryRepository(tempBaseDir, gun, ts.URL, http.DefaultTransport, retriever)
+	assert.NoError(t, err, "error creating repo: %s", err)
+	rootPubKey, err := repo.CryptoService.Create("root", rootType)
+	assert.NoError(t, err, "error generating root key: %s", err)
+
+	// repo.CryptoService’s FileKeyStore caches the unlocked private key, so to test
+	// private key unlocking we need a new repo instance.
+	repo, err = NewNotaryRepository(tempBaseDir, gun, ts.URL, http.DefaultTransport, giveUpPassphraseRetriever)
+	assert.NoError(t, err, "error creating repo: %s", err)
+	err = repo.Initialize(rootPubKey.ID())
+	assert.EqualError(t, err, trustmanager.ErrPasswordInvalid{}.Error())
+}
+
 // TestAddTarget adds a target to the repo and confirms that the changelist
 // is updated correctly.
 // We test this with both an RSA and ECDSA root key

cryptoservice/crypto_service.go

@@ -82,10 +82,15 @@ func (cs *CryptoService) GetPrivateKey(keyID string) (k data.PrivateKey, role st
 	for _, ks := range cs.keyStores {
 		for _, keyPath := range keyPaths {
 			k, role, err = ks.GetKey(keyPath)
-			if err != nil {
+			if err == nil {
+				return
+			}
+			switch err.(type) {
+			case trustmanager.ErrPasswordInvalid, trustmanager.ErrAttemptsExceeded:
+				return
+			default:
 				continue
 			}
-			return
 		}
 	}
 	return // returns whatever the final values were

cryptoservice/crypto_service_test.go

@@ -3,12 +3,15 @@ package cryptoservice
 import (
 	"crypto/rand"
 	"fmt"
+	"io/ioutil"
+	"os"
 	"path/filepath"
 	"runtime"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 
+	"github.com/docker/notary/passphrase"
 	"github.com/docker/notary/trustmanager"
 	"github.com/docker/notary/tuf/data"
 	"github.com/docker/notary/tuf/signed"
@@ -23,9 +26,13 @@ var algoToSigType = map[string]data.SigAlgorithm{
 var passphraseRetriever = func(string, string, bool, int) (string, bool, error) { return "", false, nil }
 
 type CryptoServiceTester struct {
-	cryptoServiceFactory func() *CryptoService
 	role    string
 	keyAlgo string
+	gun     string
+}
+
+func (c CryptoServiceTester) cryptoServiceFactory() *CryptoService {
+	return NewCryptoService(c.gun, trustmanager.NewKeyMemoryStore(passphraseRetriever))
 }
 
 // asserts that created key exists
@@ -88,7 +95,10 @@ func (c CryptoServiceTester) TestGetNonexistentKey(t *testing.T) {
 		c.errorMsg("non-nil result for bogus keyid"))
 
 	_, _, err := cryptoService.GetPrivateKey("boguskeyid")
-	assert.NotNil(t, err)
+	assert.Error(t, err)
+	// The underlying error has been correctly propagated.
+	_, ok := err.(*trustmanager.ErrKeyNotFound)
+	assert.True(t, ok)
 }
 
 // asserts that signing with a created key creates a valid signature
@@ -147,6 +157,61 @@ func (c CryptoServiceTester) TestGetPrivateKeyMultipleKeystores(t *testing.T) {
 	assert.Equal(t, privKey.ID(), foundKey.ID())
 }
 
+func giveUpPassphraseRetriever(_, _ string, _ bool, _ int) (string, bool, error) {
+	return "", true, nil
+}
+
+// Test that ErrPasswordInvalid is correctly propagated
+func (c CryptoServiceTester) TestGetPrivateKeyPasswordInvalid(t *testing.T) {
+	tempBaseDir, err := ioutil.TempDir("", "cs-test-")
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	// Do not use c.cryptoServiceFactory(), we need a KeyFileStore.
+	retriever := passphrase.ConstantRetriever("password")
+	store, err := trustmanager.NewKeyFileStore(tempBaseDir, retriever)
+	assert.NoError(t, err)
+	cryptoService := NewCryptoService(c.gun, store)
+	pubKey, err := cryptoService.Create(c.role, c.keyAlgo)
+	assert.NoError(t, err, "error generating key: %s", err)
+
+	// cryptoService's FileKeyStore caches the unlocked private key, so to test
+	// private key unlocking we need a new instance.
+	store, err = trustmanager.NewKeyFileStore(tempBaseDir, giveUpPassphraseRetriever)
+	assert.NoError(t, err)
+	cryptoService = NewCryptoService(c.gun, store)
+
+	_, _, err = cryptoService.GetPrivateKey(pubKey.ID())
+	assert.EqualError(t, err, trustmanager.ErrPasswordInvalid{}.Error())
+}
+
+// Test that ErrAtttemptsExceeded is correctly propagated
+func (c CryptoServiceTester) TestGetPrivateKeyAttemptsExceeded(t *testing.T) {
+	tempBaseDir, err := ioutil.TempDir("", "cs-test-")
+	assert.NoError(t, err, "failed to create a temporary directory: %s", err)
+	defer os.RemoveAll(tempBaseDir)
+
+	// Do not use c.cryptoServiceFactory(), we need a KeyFileStore.
+	retriever := passphrase.ConstantRetriever("password")
+	store, err := trustmanager.NewKeyFileStore(tempBaseDir, retriever)
+	assert.NoError(t, err)
+	cryptoService := NewCryptoService(c.gun, store)
+	pubKey, err := cryptoService.Create(c.role, c.keyAlgo)
+	assert.NoError(t, err, "error generating key: %s", err)
+
+	// trustmanager.KeyFileStore and trustmanager.KeyMemoryStore both cache the unlocked
+	// private key, so to test private key unlocking we need a new instance using the
+	// same underlying storage; this also makes trustmanager.KeyMemoryStore (and
+	// c.cryptoServiceFactory()) unsuitable.
+	retriever = passphrase.ConstantRetriever("incorrect password")
+	store, err = trustmanager.NewKeyFileStore(tempBaseDir, retriever)
+	assert.NoError(t, err)
+	cryptoService = NewCryptoService(c.gun, store)
+
+	_, _, err = cryptoService.GetPrivateKey(pubKey.ID())
+	assert.EqualError(t, err, trustmanager.ErrAttemptsExceeded{}.Error())
+}
+
 // asserts that removing key that exists succeeds
 func (c CryptoServiceTester) TestRemoveCreatedKey(t *testing.T) {
 	cryptoService := c.cryptoServiceFactory()
@@ -251,10 +316,6 @@ func (c CryptoServiceTester) errorMsg(message string, args ...interface{}) strin
 }
 
 func testCryptoService(t *testing.T, gun string) {
-	getTestingCryptoService := func() *CryptoService {
-		return NewCryptoService(
-			gun, trustmanager.NewKeyMemoryStore(passphraseRetriever))
-	}
 	roles := []string{
 		data.CanonicalRootRole,
 		data.CanonicalTargetsRole,
@@ -265,9 +326,9 @@ func testCryptoService(t *testing.T, gun string) {
 	for _, role := range roles {
 		for algo := range algoToSigType {
 			cst := CryptoServiceTester{
-				cryptoServiceFactory: getTestingCryptoService,
 				role:    role,
 				keyAlgo: algo,
+				gun:     gun,
 			}
 			cst.TestCreateAndGetKey(t)
 			cst.TestCreateAndGetWhenMultipleKeystores(t)
@@ -278,6 +339,8 @@ func testCryptoService(t *testing.T, gun string) {
 			cst.TestRemoveCreatedKey(t)
 			cst.TestRemoveFromMultipleKeystores(t)
 			cst.TestListFromMultipleKeystores(t)
+			cst.TestGetPrivateKeyPasswordInvalid(t)
+			cst.TestGetPrivateKeyAttemptsExceeded(t)
 		}
 	}
 }