trust: address review feedback, refactor to align with existing cli/c…

…ommand semantics

Signed-off-by: Riyaz Faizullabhoy <[email protected]>

cli/command/image/trust.go

@@ -5,7 +5,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"path"
 	"sort"
 
 	"github.com/Sirupsen/logrus"
@@ -154,60 +153,12 @@ func PushTrustedReference(streams command.Streams, repoInfo *registry.Repository
 	return nil
 }
 
-// GetSignableRoles returns a list of roles for which we have valid signing
-// keys, given a notary repository and a target
-func GetSignableRoles(repo *client.NotaryRepository, target *client.Target) ([]data.RoleName, error) {
-	var signableRoles []data.RoleName
-
-	// translate the full key names, which includes the GUN, into just the key IDs
-	allCanonicalKeyIDs := make(map[string]struct{})
-	for fullKeyID := range repo.CryptoService.ListAllKeys() {
-		allCanonicalKeyIDs[path.Base(fullKeyID)] = struct{}{}
-	}
-
-	allDelegationRoles, err := repo.GetDelegationRoles()
-	if err != nil {
-		return signableRoles, err
-	}
-
-	// if there are no delegation roles, then just try to sign it into the targets role
-	if len(allDelegationRoles) == 0 {
-		signableRoles = append(signableRoles, data.CanonicalTargetsRole)
-		return signableRoles, nil
-	}
-
-	// there are delegation roles, find every delegation role we have a key for, and
-	// attempt to sign into into all those roles.
-	for _, delegationRole := range allDelegationRoles {
-		// We do not support signing any delegation role that isn't a direct child of the targets role.
-		// Also don't bother checking the keys if we can't add the target
-		// to this role due to path restrictions
-		if path.Dir(delegationRole.Name.String()) != data.CanonicalTargetsRole.String() || !delegationRole.CheckPaths(target.Name) {
-			continue
-		}
-
-		for _, canonicalKeyID := range delegationRole.KeyIDs {
-			if _, ok := allCanonicalKeyIDs[canonicalKeyID]; ok {
-				signableRoles = append(signableRoles, delegationRole.Name)
-				break
-			}
-		}
-	}
-
-	if len(signableRoles) == 0 {
-		return signableRoles, errors.Errorf("no valid signing keys for delegation roles")
-	}
-
-	return signableRoles, nil
-
-}
-
 // AddTargetToAllSignableRoles attempts to add the image target to all the top level delegation roles we can
 // (based on whether we have the signing key and whether the role's path allows
 // us to).
 // If there are no delegation roles, we add to the targets role.
 func AddTargetToAllSignableRoles(repo *client.NotaryRepository, target *client.Target) error {
-	signableRoles, err := GetSignableRoles(repo, target)
+	signableRoles, err := trust.GetSignableRoles(repo, target)
 	if err != nil {
 		return err
 	}

cli/command/image/trust_test.go

@@ -79,6 +79,6 @@ func TestGetSignableRolesError(t *testing.T) {
 
 	notaryRepo, err := client.NewFileCachedNotaryRepository(tmpDir, "gun", "https://localhost", nil, passphrase.ConstantRetriever("password"), trustpinning.TrustPinConfig{})
 	target := client.Target{}
-	_, err = GetSignableRoles(notaryRepo, &target)
+	_, err = trust.GetSignableRoles(notaryRepo, &target)
 	assert.EqualError(t, err, "client is offline")
 }

cli/command/trust/cmd.go

@@ -10,7 +10,7 @@ import (
 func NewTrustCommand(dockerCli command.Cli) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "trust",
-		Short: "Sign images to establish trust",
+		Short: "Manage trust on Docker images",
 		Args:  cli.NoArgs,
 		RunE:  command.ShowHelp(dockerCli.Err()),
 	}

cli/command/trust/helpers.go

@@ -3,7 +3,6 @@ package trust
 import (
 	"context"
 	"fmt"
-	"io"
 	"strings"
 
 	"github.com/docker/cli/cli/command"
@@ -17,26 +16,53 @@ import (
 
 const releasedRoleName = "Repo Admin"
 
-func checkLocalImageExistence(ctx context.Context, cli command.Cli, imageName string) error {
-	_, _, err := cli.Client().ImageInspectWithRaw(ctx, imageName)
-	return err
+// ImageRefAndAuth contains all reference information and the auth config for an image request
+type ImageRefAndAuth struct {
+	authConfig *types.AuthConfig
+	reference  reference.Named
+	repoInfo   *registry.RepositoryInfo
+	tag        string
 }
 
-func getImageReferencesAndAuth(cli command.Cli, imgName string) (context.Context, reference.Named, *registry.RepositoryInfo, *types.AuthConfig, error) {
+// AuthConfig returns the auth information (username, etc) for a given ImageRefAndAuth
+func (imgRefAuth *ImageRefAndAuth) AuthConfig() *types.AuthConfig {
+	return imgRefAuth.authConfig
+}
+
+// Reference returns the Image reference for a given ImageRefAndAuth
+func (imgRefAuth *ImageRefAndAuth) Reference() reference.Named {
+	return imgRefAuth.reference
+}
+
+// RepoInfo returns the repository information for a given ImageRefAndAuth
+func (imgRefAuth *ImageRefAndAuth) RepoInfo() *registry.RepositoryInfo {
+	return imgRefAuth.repoInfo
+}
+
+// Tag returns the Image tag for a given ImageRefAndAuth
+func (imgRefAuth *ImageRefAndAuth) Tag() string {
+	return imgRefAuth.tag
+}
+
+func getImageReferencesAndAuth(ctx context.Context, cli command.Cli, imgName string) (*ImageRefAndAuth, error) {
 	ref, err := reference.ParseNormalizedNamed(imgName)
 	if err != nil {
-		return nil, nil, nil, nil, err
+		return nil, err
+	}
+
+	tag, err := getTag(ref)
+	if err != nil {
+		return nil, err
 	}
 
 	// Resolve the Repository name from fqn to RepositoryInfo
 	repoInfo, err := registry.ParseRepositoryInfo(ref)
 	if err != nil {
-		return nil, nil, nil, nil, err
+		return nil, err
 	}
 
-	ctx := context.Background()
 	authConfig := command.ResolveAuthConfig(ctx, cli, repoInfo.Index)
-	return ctx, ref, repoInfo, &authConfig, err
+	return &ImageRefAndAuth{&authConfig, ref, repoInfo, tag}, err
 }
 
 func getTag(ref reference.Named) (string, error) {
@@ -66,25 +92,10 @@ func notaryRoleToSigner(tufRole data.RoleName) string {
 	return strings.TrimPrefix(tufRole.String(), "targets/")
 }
 
-func askConfirm(input io.Reader) bool {
-	var res string
-	if _, err := fmt.Fscanln(input, &res); err != nil {
-		return false
-	}
-	if strings.EqualFold(res, "y") || strings.EqualFold(res, "yes") {
-		return true
-	}
-	return false
-}
-
 func clearChangeList(notaryRepo *client.NotaryRepository) error {
-
 	cl, err := notaryRepo.GetChangelist()
 	if err != nil {
 		return err
 	}
-	if err = cl.Clear(""); err != nil {
-		return err
-	}
-	return nil
+	return cl.Clear("")
 }

cli/command/trust/helpers_test.go

@@ -11,15 +11,18 @@ func TestGetTag(t *testing.T) {
 	ref, err := reference.ParseNormalizedNamed("ubuntu@sha256:45b23dee08af5e43a7fea6c4cf9c25ccf269ee113168c19722f87876677c5cb2")
 	assert.NoError(t, err)
 	tag, err := getTag(ref)
+	assert.Error(t, err)
 	assert.EqualError(t, err, "cannot use a digest reference for IMAGE:TAG")
 
 	ref, err = reference.ParseNormalizedNamed("alpine:latest")
 	assert.NoError(t, err)
 	tag, err = getTag(ref)
+	assert.NoError(t, err)
 	assert.Equal(t, tag, "latest")
 
 	ref, err = reference.ParseNormalizedNamed("alpine")
 	assert.NoError(t, err)
 	tag, err = getTag(ref)
+	assert.NoError(t, err)
 	assert.Equal(t, tag, "")
 }

cli/command/trust/inspect.go

@@ -1,8 +1,10 @@
 package trust
 
 import (
+	"context"
 	"encoding/hex"
 	"fmt"
+	"io"
 	"sort"
 	"strings"
 
@@ -56,35 +58,34 @@ func newInspectCommand(dockerCli command.Cli) *cobra.Command {
 }
 
 func lookupTrustInfo(cli command.Cli, remote string) error {
-	_, ref, repoInfo, authConfig, err := getImageReferencesAndAuth(cli, remote)
+	ctx := context.Background()
+	imgRefAndAuth, err := getImageReferencesAndAuth(ctx, cli, remote)
 	if err != nil {
 		return err
 	}
-	notaryRepo, err := trust.GetNotaryRepository(cli, repoInfo, *authConfig, "pull")
+	tag := imgRefAndAuth.Tag()
+	notaryRepo, err := trust.GetNotaryRepository(cli, imgRefAndAuth.RepoInfo(), *imgRefAndAuth.AuthConfig(), "pull")
 	if err != nil {
-		return trust.NotaryError(ref.Name(), err)
+		return trust.NotaryError(imgRefAndAuth.Reference().Name(), err)
 	}
 
 	if err = clearChangeList(notaryRepo); err != nil {
 		return err
 	}
 	defer clearChangeList(notaryRepo)
-	tag, err := getTag(ref)
-	if err != nil {
-		return err
-	}
+
 	// Retrieve all released signatures, match them, and pretty print them
 	allSignedTargets, err := notaryRepo.GetAllTargetMetadataByName(tag)
 	if err != nil {
-		logrus.Debug(trust.NotaryError(ref.Name(), err))
+		logrus.Debug(trust.NotaryError(imgRefAndAuth.Reference().Name(), err))
 		// print an empty table if we don't have signed targets, but have an initialized notary repo
 		if _, ok := err.(client.ErrNoSuchTarget); !ok {
 			return fmt.Errorf("No signatures or cannot access %s", remote)
 		}
 	}
 	signatureRows := matchReleasedSignatures(allSignedTargets)
 	if len(signatureRows) > 0 {
-		if err := printSignatures(cli, signatureRows); err != nil {
+		if err := printSignatures(cli.Out(), signatureRows); err != nil {
 			return err
 		}
 	} else {
@@ -108,17 +109,17 @@ func lookupTrustInfo(cli command.Cli, remote string) error {
 	// If we do not have additional signers, do not display
 	if len(signerRoleToKeyIDs) > 0 {
 		fmt.Fprintf(cli.Out(), "\nList of signers and their KeyIDs:\n\n")
-		printSignerInfo(cli, signerRoleToKeyIDs)
+		printSignerInfo(cli.Out(), signerRoleToKeyIDs)
 	}
 
 	// This will always have the root and targets information
 	fmt.Fprintf(cli.Out(), "\nAdministrative keys for %s:\n", strings.Split(remote, ":")[0])
-	printSortedAdminKeys(adminRoleToKeyIDs, cli)
+	printSortedAdminKeys(cli.Out(), adminRoleToKeyIDs)
 
 	return nil
 }
 
-func printSortedAdminKeys(adminRoleToKeyIDs map[string]string, cli command.Cli) {
+func printSortedAdminKeys(out io.Writer, adminRoleToKeyIDs map[string]string) {
 	keyNames := []string{}
 	for name := range adminRoleToKeyIDs {
 		keyNames = append(keyNames, name)
@@ -127,7 +128,7 @@ func printSortedAdminKeys(adminRoleToKeyIDs map[string]string, cli command.Cli)
 	sort.Strings(keyNames)
 
 	for _, keyName := range keyNames {
-		fmt.Fprintf(cli.Out(), "%s:\t%s\n", keyName, adminRoleToKeyIDs[keyName])
+		fmt.Fprintf(out, "%s:\t%s\n", keyName, adminRoleToKeyIDs[keyName])
 	}
 }
 
@@ -191,9 +192,9 @@ func matchReleasedSignatures(allTargets []client.TargetSignedStruct) trustTagRow
 }
 
 // pretty print with ordered rows
-func printSignatures(dockerCli command.Cli, signatureRows trustTagRowList) error {
+func printSignatures(out io.Writer, signatureRows trustTagRowList) error {
 	trustTagCtx := formatter.Context{
-		Output: dockerCli.Out(),
+		Output: out,
 		Format: formatter.NewTrustTagFormat(),
 	}
 	// convert the formatted type before printing
@@ -212,9 +213,9 @@ func printSignatures(dockerCli command.Cli, signatureRows trustTagRowList) error
 	return formatter.TrustTagWrite(trustTagCtx, formattedTags)
 }
 
-func printSignerInfo(dockerCli command.Cli, roleToKeyIDs map[string][]string) error {
+func printSignerInfo(out io.Writer, roleToKeyIDs map[string][]string) error {
 	signerInfoCtx := formatter.Context{
-		Output: dockerCli.Out(),
+		Output: out,
 		Format: formatter.NewSignerInfoFormat(),
 		Trunc:  true,
 	}

cli/command/trust/inspect_test.go

@@ -19,7 +19,7 @@ type fakeClient struct {
 	dockerClient.Client
 }
 
-func TestTrustInfoErrors(t *testing.T) {
+func TestTrustInspectCommandErrors(t *testing.T) {
 	testCases := []struct {
 		name          string
 		args          []string
@@ -69,7 +69,7 @@ func TestTrustInfoErrors(t *testing.T) {
 	}
 }
 
-func TestTrustInfo(t *testing.T) {
+func TestTrustInspectCommandFullRepoWithoutSigners(t *testing.T) {
 	cli := test.NewFakeCli(&fakeClient{})
 	cmd := newInspectCommand(cli)
 	cmd.SetArgs([]string{"alpine"})
@@ -84,9 +84,11 @@ func TestTrustInfo(t *testing.T) {
 	assert.Contains(t, cli.OutBuffer().String(), "(Repo Admin)")
 	// no delegations on this repo
 	assert.NotContains(t, cli.OutBuffer().String(), "List of signers and their KeyIDs:")
+}
 
-	cli = test.NewFakeCli(&fakeClient{})
-	cmd = newInspectCommand(cli)
+func TestTrustInspectCommandOneTagWithoutSigners(t *testing.T) {
+	cli := test.NewFakeCli(&fakeClient{})
+	cmd := newInspectCommand(cli)
 	cmd.SetArgs([]string{"alpine:3.5"})
 	assert.NoError(t, cmd.Execute())
 	assert.Contains(t, cli.OutBuffer().String(), "SIGNED TAG")
@@ -101,9 +103,11 @@ func TestTrustInfo(t *testing.T) {
 	// no delegations on this repo
 	assert.NotContains(t, cli.OutBuffer().String(), "3.6")
 	assert.NotContains(t, cli.OutBuffer().String(), "List of signers and their KeyIDs:")
+}
 
-	cli = test.NewFakeCli(&fakeClient{})
-	cmd = newInspectCommand(cli)
+func TestTrustInspectCommandFullRepoWithSigners(t *testing.T) {
+	cli := test.NewFakeCli(&fakeClient{})
+	cmd := newInspectCommand(cli)
 	cmd.SetArgs([]string{"dockerorcadev/trust-fixture"})
 	assert.NoError(t, cmd.Execute())
 
@@ -120,9 +124,11 @@ func TestTrustInfo(t *testing.T) {
 	assert.Contains(t, cli.OutBuffer().String(), "Root Key")
 	// all signers have names
 	assert.NotContains(t, cli.OutBuffer().String(), "(Repo Admin)")
+}
 
-	cli = test.NewFakeCli(&fakeClient{})
-	cmd = newInspectCommand(cli)
+func TestTrustInspectCommandUnsignedTagInSignedRepo(t *testing.T) {
+	cli := test.NewFakeCli(&fakeClient{})
+	cmd := newInspectCommand(cli)
 	cmd.SetArgs([]string{"dockerorcadev/trust-fixture:unsigned"})
 	assert.NoError(t, cmd.Execute())
 
@@ -144,7 +150,7 @@ func TestTrustInfo(t *testing.T) {
 	assert.NotContains(t, cli.OutBuffer().String(), "(Repo Admin)")
 }
 
-func TestTUFToSigner(t *testing.T) {
+func TestNotaryRoleToSigner(t *testing.T) {
 	assert.Equal(t, releasedRoleName, notaryRoleToSigner(data.CanonicalTargetsRole))
 	assert.Equal(t, releasedRoleName, notaryRoleToSigner(trust.ReleasesRole))
 	assert.Equal(t, "signer", notaryRoleToSigner("targets/signer"))