Fix error handling on invalid root passphrase

When the user insists on an invalid passphrase (or aborts the
operation), CryptoService.GetPrivateKey will try the correct root
location first, correctly failing, and then try to look for the root key
in the $gun subdirectory, and so will return the last error, a confusing
”open $path: no such file or directory”.

So, recognize the passphrase-related errors and fail with them directly.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

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"
@@ -92,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
@@ -151,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()
@@ -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)
 		}
 	}
 }