Merge pull request #811 from endophage/reportcard_fixes

addressing vet and ineffassign items in goreport

client/client_test.go

@@ -2422,6 +2422,7 @@ func TestPublishRemoveDelegationKeyFromDelegationRole(t *testing.T) {
 	require.NoError(t, err)
 
 	cl, err := changelist.NewFileChangelist(filepath.Join(ownerRepo.tufRepoPath, "changelist"))
+	require.NoError(t, err)
 	require.NoError(t, cl.Add(changelist.NewTUFChange(
 		changelist.ActionUpdate,
 		"targets/a",
@@ -3055,6 +3056,7 @@ func TestFullAddDelegationChangefileApplicable(t *testing.T) {
 	})
 	change := newCreateDelegationChange(delegationName, tdJSON)
 	cl, err := changelist.NewFileChangelist(filepath.Join(repo.tufRepoPath, "changelist"))
+	require.NoError(t, err)
 	addChange(cl, change, delegationName)
 
 	changes := getChanges(t, repo)
@@ -3104,6 +3106,7 @@ func TestFullRemoveDelegationChangefileApplicable(t *testing.T) {
 	})
 	change := newUpdateDelegationChange(delegationName, tdJSON)
 	cl, err := changelist.NewFileChangelist(filepath.Join(repo.tufRepoPath, "changelist"))
+	require.NoError(t, err)
 	addChange(cl, change, delegationName)
 
 	changes = getChanges(t, repo)

passphrase/passphrase.go

@@ -49,141 +49,157 @@ func PromptRetriever() notary.PassRetriever {
 	return PromptRetrieverWithInOut(os.Stdin, os.Stdout, nil)
 }
 
-// PromptRetrieverWithInOut returns a new Retriever which will provide a
-// prompt using the given in and out readers. The passphrase will be cached
-// such that subsequent prompts will produce the same passphrase.
-// aliasMap can be used to specify display names for TUF key aliases. If aliasMap
-// is nil, a sensible default will be used.
-func PromptRetrieverWithInOut(in io.Reader, out io.Writer, aliasMap map[string]string) notary.PassRetriever {
-	userEnteredTargetsSnapshotsPass := false
-	targetsSnapshotsPass := ""
-	userEnteredRootsPass := false
-	rootsPass := ""
-
-	return func(keyName string, alias string, createNew bool, numAttempts int) (string, bool, error) {
-		if alias == tufRootAlias && createNew && numAttempts == 0 {
-			fmt.Fprintln(out, tufRootKeyGenerationWarning)
-		}
-		if numAttempts > 0 {
-			if !createNew {
-				fmt.Fprintln(out, "Passphrase incorrect. Please retry.")
-			}
-		}
-
-		// Figure out if we should display a different string for this alias
-		displayAlias := alias
-		if aliasMap != nil {
-			if val, ok := aliasMap[alias]; ok {
-				displayAlias = val
-			}
+type boundRetriever struct {
+	in                             io.Reader
+	out                            io.Writer
+	aliasMap                       map[string]string
+	userEnteredTargetsSnapshotPass bool
+	targetsSnapshotPass            string
+	userEnteredRootPass            bool
+	rootPass                       string
+}
 
+func (br *boundRetriever) getPassphrase(keyName, alias string, createNew bool, numAttempts int) (string, bool, error) {
+	if numAttempts == 0 {
+		if alias == tufRootAlias && createNew {
+			fmt.Fprintln(br.out, tufRootKeyGenerationWarning)
 		}
 
-		// First, check if we have a password cached for this alias.
-		if numAttempts == 0 {
-			if userEnteredTargetsSnapshotsPass && (alias == tufSnapshotAlias || alias == tufTargetsAlias) {
-				return targetsSnapshotsPass, false, nil
-			}
-			if userEnteredRootsPass && (alias == "root") {
-				return rootsPass, false, nil
-			}
+		if br.userEnteredTargetsSnapshotPass && (alias == tufSnapshotAlias || alias == tufTargetsAlias) {
+			return br.targetsSnapshotPass, false, nil
 		}
-
-		if numAttempts > 3 && !createNew {
-			return "", true, ErrTooManyAttempts
+		if br.userEnteredRootPass && (alias == "root") {
+			return br.rootPass, false, nil
 		}
-
-		// If typing on the terminal, we do not want the terminal to echo the
-		// password that is typed (so it doesn't display)
-		if term.IsTerminal(0) {
-			state, err := term.SaveState(0)
-			if err != nil {
-				return "", false, err
-			}
-			term.DisableEcho(0, state)
-			defer term.RestoreTerminal(0, state)
+	} else if !createNew { // per `if`, numAttempts > 0 if we're at this `else`
+		if numAttempts > 3 {
+			return "", true, ErrTooManyAttempts
 		}
+		fmt.Fprintln(br.out, "Passphrase incorrect. Please retry.")
+	}
 
-		stdin := bufio.NewReader(in)
+	// passphrase not cached and we're not aborting, get passphrase from user!
+	return br.requestPassphrase(keyName, alias, createNew, numAttempts)
+}
 
-		indexOfLastSeparator := strings.LastIndex(keyName, string(filepath.Separator))
-		if indexOfLastSeparator == -1 {
-			indexOfLastSeparator = 0
-		}
+func (br *boundRetriever) requestPassphrase(keyName, alias string, createNew bool, numAttempts int) (string, bool, error) {
+	// Figure out if we should display a different string for this alias
+	displayAlias := alias
+	if val, ok := br.aliasMap[alias]; ok {
+		displayAlias = val
+	}
 
-		var shortName string
-		if len(keyName) > indexOfLastSeparator+idBytesToDisplay {
-			if indexOfLastSeparator > 0 {
-				keyNamePrefix := keyName[:indexOfLastSeparator]
-				keyNameID := keyName[indexOfLastSeparator+1 : indexOfLastSeparator+idBytesToDisplay+1]
-				shortName = keyNameID + " (" + keyNamePrefix + ")"
-			} else {
-				shortName = keyName[indexOfLastSeparator : indexOfLastSeparator+idBytesToDisplay]
-			}
+	// If typing on the terminal, we do not want the terminal to echo the
+	// password that is typed (so it doesn't display)
+	if term.IsTerminal(0) {
+		state, err := term.SaveState(0)
+		if err != nil {
+			return "", false, err
 		}
+		term.DisableEcho(0, state)
+		defer term.RestoreTerminal(0, state)
+	}
 
-		withID := fmt.Sprintf(" with ID %s", shortName)
-		if shortName == "" {
-			withID = ""
-		}
+	indexOfLastSeparator := strings.LastIndex(keyName, string(filepath.Separator))
+	if indexOfLastSeparator == -1 {
+		indexOfLastSeparator = 0
+	}
 
-		if createNew {
-			fmt.Fprintf(out, "Enter passphrase for new %s key%s: ", displayAlias, withID)
-		} else if displayAlias == "yubikey" {
-			fmt.Fprintf(out, "Enter the %s for the attached Yubikey: ", keyName)
+	var shortName string
+	if len(keyName) > indexOfLastSeparator+idBytesToDisplay {
+		if indexOfLastSeparator > 0 {
+			keyNamePrefix := keyName[:indexOfLastSeparator]
+			keyNameID := keyName[indexOfLastSeparator+1 : indexOfLastSeparator+idBytesToDisplay+1]
+			shortName = keyNameID + " (" + keyNamePrefix + ")"
 		} else {
-			fmt.Fprintf(out, "Enter passphrase for %s key%s: ", displayAlias, withID)
+			shortName = keyName[indexOfLastSeparator : indexOfLastSeparator+idBytesToDisplay]
 		}
+	}
 
-		passphrase, err := stdin.ReadBytes('\n')
-		fmt.Fprintln(out)
-		if err != nil {
-			return "", false, err
-		}
+	withID := fmt.Sprintf(" with ID %s", shortName)
+	if shortName == "" {
+		withID = ""
+	}
 
-		retPass := strings.TrimSpace(string(passphrase))
-
-		if !createNew {
-			if alias == tufSnapshotAlias || alias == tufTargetsAlias {
-				userEnteredTargetsSnapshotsPass = true
-				targetsSnapshotsPass = retPass
-			}
-			if alias == tufRootAlias {
-				userEnteredRootsPass = true
-				rootsPass = retPass
-			}
-			return retPass, false, nil
-		}
+	switch {
+	case createNew:
+		fmt.Fprintf(br.out, "Enter passphrase for new %s key%s: ", displayAlias, withID)
+	case displayAlias == "yubikey":
+		fmt.Fprintf(br.out, "Enter the %s for the attached Yubikey: ", keyName)
+	default:
+		fmt.Fprintf(br.out, "Enter passphrase for %s key%s: ", displayAlias, withID)
+	}
 
-		if len(retPass) < 8 {
-			fmt.Fprintln(out, "Passphrase is too short. Please use a password manager to generate and store a good random passphrase.")
-			return "", false, ErrTooShort
-		}
+	stdin := bufio.NewReader(br.in)
+	passphrase, err := stdin.ReadBytes('\n')
+	fmt.Fprintln(br.out)
+	if err != nil {
+		return "", false, err
+	}
 
-		fmt.Fprintf(out, "Repeat passphrase for new %s key%s: ", displayAlias, withID)
-		confirmation, err := stdin.ReadBytes('\n')
-		fmt.Fprintln(out)
+	retPass := strings.TrimSpace(string(passphrase))
+
+	if createNew {
+		err = br.verifyAndConfirmPassword(stdin, retPass, displayAlias, withID)
 		if err != nil {
 			return "", false, err
 		}
-		confirmationStr := strings.TrimSpace(string(confirmation))
+	}
 
-		if retPass != confirmationStr {
-			fmt.Fprintln(out, "Passphrases do not match. Please retry.")
-			return "", false, ErrDontMatch
-		}
+	br.cachePassword(alias, retPass)
 
-		if alias == tufSnapshotAlias || alias == tufTargetsAlias {
-			userEnteredTargetsSnapshotsPass = true
-			targetsSnapshotsPass = retPass
-		}
-		if alias == tufRootAlias {
-			userEnteredRootsPass = true
-			rootsPass = retPass
-		}
+	return retPass, false, nil
+}
+
+func (br *boundRetriever) verifyAndConfirmPassword(stdin *bufio.Reader, retPass, displayAlias, withID string) error {
+	if len(retPass) < 8 {
+		fmt.Fprintln(br.out, "Passphrase is too short. Please use a password manager to generate and store a good random passphrase.")
+		return ErrTooShort
+	}
+
+	fmt.Fprintf(br.out, "Repeat passphrase for new %s key%s: ", displayAlias, withID)
+	confirmation, err := stdin.ReadBytes('\n')
+	fmt.Fprintln(br.out)
+	if err != nil {
+		return err
+	}
+	confirmationStr := strings.TrimSpace(string(confirmation))
 
-		return retPass, false, nil
+	if retPass != confirmationStr {
+		fmt.Fprintln(br.out, "Passphrases do not match. Please retry.")
+		return ErrDontMatch
 	}
+	return nil
+}
+
+func (br *boundRetriever) cachePassword(alias, retPass string) {
+	if alias == tufSnapshotAlias || alias == tufTargetsAlias {
+		br.userEnteredTargetsSnapshotPass = true
+		br.targetsSnapshotPass = retPass
+	}
+	if alias == tufRootAlias {
+		br.userEnteredRootPass = true
+		br.rootPass = retPass
+	}
+}
+
+// PromptRetrieverWithInOut returns a new Retriever which will provide a
+// prompt using the given in and out readers. The passphrase will be cached
+// such that subsequent prompts will produce the same passphrase.
+// aliasMap can be used to specify display names for TUF key aliases. If aliasMap
+// is nil, a sensible default will be used.
+func PromptRetrieverWithInOut(in io.Reader, out io.Writer, aliasMap map[string]string) notary.PassRetriever {
+	bound := &boundRetriever{
+		in:                             in,
+		out:                            out,
+		aliasMap:                       aliasMap,
+		userEnteredTargetsSnapshotPass: false,
+		targetsSnapshotPass:            "",
+		userEnteredRootPass:            false,
+		rootPass:                       "",
+	}
+
+	return bound.getPassphrase
 }
 
 // ConstantRetriever returns a new Retriever which will return a constant string

server/handlers/validation_test.go

@@ -226,7 +226,7 @@ func TestValidateGetCurrentTimestampBroken(t *testing.T) {
 	}
 
 	serverCrypto := testutils.CopyKeys(t, cs, data.CanonicalTimestampRole)
-	updates, err = validateUpdate(serverCrypto, gun, updates, store)
+	_, err = validateUpdate(serverCrypto, gun, updates, store)
 	require.Error(t, err)
 	require.IsType(t, data.ErrNoSuchRole{}, err)
 }
@@ -624,7 +624,7 @@ func TestRootRotationNotSignedWithOldKeysForOldRole(t *testing.T) {
 
 	sn, err = repo.SignSnapshot(data.DefaultExpires(data.CanonicalSnapshotRole))
 	require.NoError(t, err)
-	root, targets, snapshot, timestamp, err = getUpdates(r, tg, sn, ts)
+	root, _, snapshot, _, err = getUpdates(r, tg, sn, ts)
 	require.NoError(t, err)
 
 	_, err = validateUpdate(serverCrypto, gun, []storage.MetaUpdate{root, snapshot}, store)
@@ -730,6 +730,7 @@ func TestValidateSnapshotGenerateWithPrev(t *testing.T) {
 		if u.Role == data.CanonicalSnapshotRole {
 			curr := &data.SignedSnapshot{}
 			err = json.Unmarshal(u.Data, curr)
+			require.NoError(t, err)
 			require.Equal(t, prev.Signed.Version+1, curr.Signed.Version)
 			require.Equal(t, u.Version, curr.Signed.Version)
 		}
@@ -763,7 +764,7 @@ func TestValidateSnapshotGeneratePrevCorrupt(t *testing.T) {
 	store.UpdateCurrent(gun, snapshot)
 
 	serverCrypto := testutils.CopyKeys(t, cs, data.CanonicalTimestampRole, data.CanonicalSnapshotRole)
-	updates, err = validateUpdate(serverCrypto, gun, updates, store)
+	_, err = validateUpdate(serverCrypto, gun, updates, store)
 	require.Error(t, err)
 	require.IsType(t, &json.SyntaxError{}, err)
 }
@@ -819,7 +820,7 @@ func TestValidateSnapshotGenerateNoTargets(t *testing.T) {
 	updates := []storage.MetaUpdate{root}
 
 	serverCrypto := testutils.CopyKeys(t, cs, data.CanonicalTimestampRole, data.CanonicalSnapshotRole)
-	updates, err = validateUpdate(serverCrypto, gun, updates, store)
+	_, err = validateUpdate(serverCrypto, gun, updates, store)
 	require.Error(t, err)
 }
 
@@ -846,7 +847,7 @@ func TestValidateSnapshotGenerate(t *testing.T) {
 	store.UpdateCurrent(gun, root)
 
 	serverCrypto := testutils.CopyKeys(t, cs, data.CanonicalTimestampRole, data.CanonicalSnapshotRole)
-	updates, err = validateUpdate(serverCrypto, gun, updates, store)
+	_, err = validateUpdate(serverCrypto, gun, updates, store)
 	require.NoError(t, err)
 }
 

server/storage/database_test.go

@@ -298,6 +298,7 @@ func TestSQLGetKeyNoKey(t *testing.T) {
 		t, query.Error, "Inserting timestamp into empty DB should succeed")
 
 	cipher, public, err = dbStore.GetKey("testGUN", data.CanonicalTimestampRole)
+	require.NoError(t, err)
 	require.Equal(t, "testCipher", cipher,
 		"Returned cipher was incorrect")
 	require.Equal(t, []byte("1"), public, "Returned pubkey was incorrect")

signer/api/api_test.go

@@ -199,7 +199,7 @@ func TestSoftwareSignWithInvalidRequestHandler(t *testing.T) {
 
 	var sig *pb.Signature
 	err = json.Unmarshal(jsonBlob, &sig)
-
+	require.Error(t, err)
 	require.Equal(t, 400, res.StatusCode)
 }
 

trustmanager/x509utils_test.go

@@ -187,7 +187,7 @@ func TestRSAX509PublickeyID(t *testing.T) {
 
 	actualTUFKey := CertToKey(cert)
 	actualTUFID, err := X509PublicKeyID(actualTUFKey)
-
+	require.NoError(t, err)
 	require.Equal(t, sameWayTUFID, actualTUFID)
 	require.Equal(t, expectedTUFID, actualTUFID)
 }

trustmanager/yubikey/yubikeystore.go

@@ -902,13 +902,12 @@ func login(ctx IPKCS11Ctx, session pkcs11.SessionHandle, passRetriever notary.Pa
 			return trustmanager.ErrAttemptsExceeded{}
 		}
 
-		// Try to convert PEM encoded bytes back to a PrivateKey using the passphrase
+		// attempt to login. Loop if failed
 		err = ctx.Login(session, userFlag, passwd)
 		if err == nil {
 			return nil
 		}
 	}
-	return nil
 }
 
 func buildKeyMap(keys map[string]yubiSlot) map[string]trustmanager.KeyInfo {