Merge pull request #972 from cyli/fix-error-messages

Fix auth login error messages and messaging when witnessing a delegation with 0 valid keys

Author: cyli

CHANGELOG.md

@@ -1,6 +1,11 @@
 # Changelog
 
-## [v0.4.0](https://github.com/docker/notary/releases/tag/v0.4.0) 8/11/2016
+## [v0.4.1](https://github.com/docker/notary/releases/tag/v0.4.0) 9/27/2016
++ Preliminary Windows support for notary client [#970](https://github.com/docker/notary/pull/970)
++ Output message to CLI when repo changes have been successfully published [#974](https://github.com/docker/notary/pull/974)
++ Improved error messages for client authentication errors and for the witness command [#972](https://github.com/docker/notary/pull/972)
+
+## [v0.4.0](https://github.com/docker/notary/releases/tag/v0.4.0) 9/21/2016
 + Server-managed key rotations [#889](https://github.com/docker/notary/pull/889)
 + Remove `timestamp_keys` table, which stored redundant information [#889](https://github.com/docker/notary/pull/889)
 + Introduce `notary delete` command to delete local and/or remote repo data [#895](https://github.com/docker/notary/pull/895)

Godeps/Godeps.json

@@ -1 +1 @@
-0.4
+0.4.1

NOTARY_VERSION

@@ -1,10 +1,11 @@
 package client
 
 import (
+	"path/filepath"
+
 	"github.com/docker/notary/client/changelist"
 	"github.com/docker/notary/tuf"
 	"github.com/docker/notary/tuf/data"
-	"path/filepath"
 )
 
 // Witness creates change objects to witness (i.e. re-sign) the given
@@ -41,7 +42,18 @@ func witnessTargets(repo *tuf.Repo, invalid *tuf.Repo, role string) error {
 		r.Dirty = true
 		return nil
 	}
-	if invalid != nil {
+
+	if roleObj, err := repo.GetDelegationRole(role); err == nil && invalid != nil {
+		// A role with a threshold > len(keys) is technically invalid, but we let it build in the builder because
+		// we want to be able to download the role (which may still have targets on it), add more keys, and then
+		// witness the role, thus bringing it back to valid.  However, if no keys have been added before witnessing,
+		// then it is still an invalid role, and can't be witnessed because nothing can bring it back to valid.
+		if roleObj.Threshold > len(roleObj.Keys) {
+			return data.ErrInvalidRole{
+				Role:   role,
+				Reason: "role does not specify enough valid signing keys to meet its required threshold",
+			}
+		}
 		if r, ok := invalid.Targets[role]; ok {
 			// role is recognized but invalid, move to valid data and mark for re-signing
 			repo.Targets[role] = r

client/witness.go

@@ -1471,6 +1471,8 @@ func TestPurgeSingleKey(t *testing.T) {
 //  11. witness an invalid role and check for error on publish
 //  12. check non-targets base roles all fail
 //  13. test auto-publish functionality
+//  14. remove all keys from the delegation and publish
+//  15. witnessing the delegation should now fail
 func TestWitness(t *testing.T) {
 	setUp(t)
 
@@ -1637,6 +1639,12 @@ func TestWitness(t *testing.T) {
 	require.NoError(t, err)
 	require.Contains(t, output, targetName)
 	require.Contains(t, output, targetHash)
+
+	_, err = runCommand(t, tempDir, "-s", server.URL, "delegation", "remove", "-p", "gun", delgName, keyID, keyID2)
+	require.NoError(t, err)
+	_, err = runCommand(t, tempDir, "-s", server.URL, "witness", "-p", "gun", delgName)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "role does not specify enough valid signing keys to meet its required threshold")
 }
 
 func generateCertPrivKeyPair(t *testing.T, gun, keyAlgorithm string) (*x509.Certificate, data.PrivateKey, string) {

cmd/notary/integration_test.go

@@ -13,6 +13,8 @@ import (
 	"strings"
 	"time"
 
+	"golang.org/x/crypto/ssh/terminal"
+
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/distribution/registry/client/auth"
 	"github.com/docker/distribution/registry/client/transport"
@@ -755,7 +757,8 @@ type passwordStore struct {
 }
 
 func (ps passwordStore) Basic(u *url.URL) (string, string) {
-	if ps.anonymous {
+	// if it's not a terminal, don't wait on input
+	if ps.anonymous || !terminal.IsTerminal(int(os.Stdin.Fd())) {
 		return "", ""
 	}
 
@@ -795,6 +798,15 @@ func (ps passwordStore) Basic(u *url.URL) (string, string) {
 	return username, password
 }
 
+// to comply with the CredentialStore interface
+func (ps passwordStore) RefreshToken(u *url.URL, service string) string {
+	return ""
+}
+
+// to comply with the CredentialStore interface
+func (ps passwordStore) SetRefreshToken(u *url.URL, service string, token string) {
+}
+
 type httpAccess int
 
 const (

cmd/notary/tuf.go

@@ -3,10 +3,12 @@ package main
 import (
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"os"
 	"path/filepath"
 	"testing"
 
+	"github.com/docker/distribution/registry/client/auth"
 	"github.com/spf13/cobra"
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/require"
@@ -219,3 +221,18 @@ func TestGetTrustPinningErrors(t *testing.T) {
 	tc.sha256 = "88b76b34ab83a9e4d5abe3697950fb73f940aab1aa5b534f80cf9de9708942be"
 	require.Error(t, tc.tufAddByHash(&cobra.Command{}, []string{"gun", "test1", "100"}))
 }
+
+func TestPasswordStore(t *testing.T) {
+	myurl, err := url.Parse("https://docker.io")
+	require.NoError(t, err)
+
+	// whether or not we're anonymous, because this isn't a terminal,
+	for _, ps := range []auth.CredentialStore{passwordStore{}, passwordStore{anonymous: true}} {
+		username, passwd := ps.Basic(myurl)
+		require.Equal(t, "", username)
+		require.Equal(t, "", passwd)
+
+		ps.SetRefreshToken(myurl, "someService", "token") // doesn't return an error, just want to make sure no state changes
+		require.Equal(t, "", ps.RefreshToken(myurl, "someService"))
+	}
+}

cmd/notary/tuf_test.go

@@ -14,12 +14,17 @@ type ErrInsufficientSignatures struct {
 }
 
 func (e ErrInsufficientSignatures) Error() string {
-	candidates := strings.Join(e.MissingKeyIDs, ", ")
+	candidates := ""
+	if len(e.MissingKeyIDs) > 0 {
+		candidates = fmt.Sprintf(" (%s)", strings.Join(e.MissingKeyIDs, ", "))
+	}
+
 	if e.FoundKeys == 0 {
-		return fmt.Sprintf("signing keys not available, need %d keys out of: %s", e.NeededKeys, candidates)
+		return fmt.Sprintf("signing keys not available: need %d keys from %d possible keys%s",
+			e.NeededKeys, len(e.MissingKeyIDs), candidates)
 	}
-	return fmt.Sprintf("not enough signing keys: got %d of %d needed keys, other candidates: %s",
-		e.FoundKeys, e.NeededKeys, candidates)
+	return fmt.Sprintf("not enough signing keys: found %d of %d needed keys - %d other possible keys%s",
+		e.FoundKeys, e.NeededKeys, len(e.MissingKeyIDs), candidates)
 }
 
 // ErrExpired indicates a piece of metadata has expired

tuf/signed/errors.go

@@ -345,3 +345,16 @@ func TestSignFailingKeys(t *testing.T) {
 	require.Error(t, err)
 	require.IsType(t, FailingPrivateKeyErr{}, err)
 }
+
+// make sure we produce readable error messages
+func TestErrInsufficientSignaturesMessaging(t *testing.T) {
+	require.Contains(t,
+		ErrInsufficientSignatures{NeededKeys: 2, MissingKeyIDs: []string{"ID1", "ID2"}}.Error(),
+		"need 2 keys from 2 possible keys (ID1, ID2)")
+	require.Contains(t,
+		ErrInsufficientSignatures{FoundKeys: 1, NeededKeys: 2, MissingKeyIDs: []string{"ID1", "ID2"}}.Error(),
+		"found 1 of 2 needed keys - 2 other possible keys (ID1, ID2)")
+	require.Contains(t,
+		ErrInsufficientSignatures{FoundKeys: 1, NeededKeys: 2, MissingKeyIDs: []string{}}.Error(),
+		"found 1 of 2 needed keys - 0 other possible keys")
+}