Merge pull request #5878 from thaJeztah/trust_cleans

Assorted cleanups to reduce trust / notary imports

cli/command/image/trust.go

@@ -48,7 +48,7 @@ func TrustedPush(ctx context.Context, cli command.Cli, repoInfo *registry.Reposi
 func PushTrustedReference(ctx context.Context, ioStreams command.Streams, repoInfo *registry.RepositoryInfo, ref reference.Named, authConfig registrytypes.AuthConfig, in io.Reader) error {
 	// If it is a trusted push we would like to find the target entry which match the
 	// tag provided in the function and then do an AddTarget later.
-	target := &client.Target{}
+	notaryTarget := &client.Target{}
 	// Count the times of calling for handleTarget,
 	// if it is called more that once, that should be considered an error in a trusted push.
 	cnt := 0
@@ -65,12 +65,12 @@ func PushTrustedReference(ctx context.Context, ioStreams command.Streams, repoIn
 			if dgst, err := digest.Parse(pushResult.Digest); err == nil {
 				h, err := hex.DecodeString(dgst.Hex())
 				if err != nil {
-					target = nil
+					notaryTarget = nil
 					return
 				}
-				target.Name = pushResult.Tag
+				notaryTarget.Name = pushResult.Tag
-				target.Hashes = data.Hashes{string(dgst.Algorithm()): h}
+				notaryTarget.Hashes = data.Hashes{string(dgst.Algorithm()): h}
-				target.Length = int64(pushResult.Size)
+				notaryTarget.Length = int64(pushResult.Size)
 			}
 		}
 	}
@@ -99,7 +99,7 @@ func PushTrustedReference(ctx context.Context, ioStreams command.Streams, repoIn
 		return errors.Errorf("internal error: only one call to handleTarget expected")
 	}
 
-	if target == nil {
+	if notaryTarget == nil {
 		return errors.Errorf("no targets found, provide a specific tag in order to sign it")
 	}
 
@@ -134,10 +134,10 @@ func PushTrustedReference(ctx context.Context, ioStreams command.Streams, repoIn
 			return trust.NotaryError(repoInfo.Name.Name(), err)
 		}
 		_, _ = fmt.Fprintf(ioStreams.Out(), "Finished initializing %q\n", repoInfo.Name.Name())
-		err = repo.AddTarget(target, data.CanonicalTargetsRole)
+		err = repo.AddTarget(notaryTarget, data.CanonicalTargetsRole)
 	case nil:
 		// already initialized and we have successfully downloaded the latest metadata
-		err = AddTargetToAllSignableRoles(repo, target)
+		err = trust.AddToAllSignableRoles(repo, notaryTarget)
 	default:
 		return trust.NotaryError(repoInfo.Name.Name(), err)
 	}
@@ -155,19 +155,6 @@ func PushTrustedReference(ctx context.Context, ioStreams command.Streams, repoIn
 	return 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.Repository, target *client.Target) error {
-	signableRoles, err := trust.GetSignableRoles(repo, target)
-	if err != nil {
-		return err
-	}
-
-	return repo.AddTarget(target, signableRoles...)
-}
-
 // trustedPull handles content trust pulling of an image
 func trustedPull(ctx context.Context, cli command.Cli, imgRefAndAuth trust.ImageRefAndAuth, opts PullOptions) error {
 	refs, err := getTrustedPullTargets(cli, imgRefAndAuth)

cli/command/image/trust_test.go

@@ -1,57 +0,0 @@
-package image
-
-import (
-	"testing"
-
-	"github.com/docker/cli/cli/trust"
-	registrytypes "github.com/docker/docker/api/types/registry"
-	"github.com/theupdateframework/notary/client"
-	"github.com/theupdateframework/notary/passphrase"
-	"github.com/theupdateframework/notary/trustpinning"
-	"gotest.tools/v3/assert"
-	"gotest.tools/v3/env"
-)
-
-func TestENVTrustServer(t *testing.T) {
-	env.PatchAll(t, map[string]string{"DOCKER_CONTENT_TRUST_SERVER": "https://notary-test.example.com:5000"})
-	indexInfo := &registrytypes.IndexInfo{Name: "testserver"}
-	output, err := trust.Server(indexInfo)
-	expectedStr := "https://notary-test.example.com:5000"
-	if err != nil || output != expectedStr {
-		t.Fatalf("Expected server to be %s, got %s", expectedStr, output)
-	}
-}
-
-func TestHTTPENVTrustServer(t *testing.T) {
-	env.PatchAll(t, map[string]string{"DOCKER_CONTENT_TRUST_SERVER": "http://notary-test.example.com:5000"})
-	indexInfo := &registrytypes.IndexInfo{Name: "testserver"}
-	_, err := trust.Server(indexInfo)
-	if err == nil {
-		t.Fatal("Expected error with invalid scheme")
-	}
-}
-
-func TestOfficialTrustServer(t *testing.T) {
-	indexInfo := &registrytypes.IndexInfo{Name: "testserver", Official: true}
-	output, err := trust.Server(indexInfo)
-	if err != nil || output != trust.NotaryServer {
-		t.Fatalf("Expected server to be %s, got %s", trust.NotaryServer, output)
-	}
-}
-
-func TestNonOfficialTrustServer(t *testing.T) {
-	indexInfo := &registrytypes.IndexInfo{Name: "testserver", Official: false}
-	output, err := trust.Server(indexInfo)
-	expectedStr := "https://" + indexInfo.Name
-	if err != nil || output != expectedStr {
-		t.Fatalf("Expected server to be %s, got %s", expectedStr, output)
-	}
-}
-
-func TestAddTargetToAllSignableRolesError(t *testing.T) {
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever("password"), trustpinning.TrustPinConfig{})
-	assert.NilError(t, err)
-	target := client.Target{}
-	err = AddTargetToAllSignableRoles(notaryRepo, &target)
-	assert.Error(t, err, "client is offline")
-}

cli/command/trust/helpers.go

@@ -47,3 +47,9 @@ func getOrGenerateRootKeyAndInitRepo(notaryRepo client.Repository) error {
 	}
 	return notaryRepo.Initialize([]string{rootKey.ID()}, data.CanonicalSnapshotRole)
 }
+
+const testPass = "password"
+
+func testPassRetriever(string, string, bool, int) (string, bool, error) {
+	return testPass, false, nil
+}

cli/command/trust/helpers_test.go

@@ -1,18 +0,0 @@
-package trust
-
-import (
-	"testing"
-
-	"github.com/theupdateframework/notary/client"
-	"github.com/theupdateframework/notary/passphrase"
-	"github.com/theupdateframework/notary/trustpinning"
-	"gotest.tools/v3/assert"
-)
-
-func TestGetOrGenerateNotaryKeyAndInitRepo(t *testing.T) {
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever(passwd), trustpinning.TrustPinConfig{})
-	assert.NilError(t, err)
-
-	err = getOrGenerateRootKeyAndInitRepo(notaryRepo)
-	assert.Error(t, err, "client is offline")
-}

cli/command/trust/key_generate_test.go

@@ -11,7 +11,6 @@ import (
 	"github.com/docker/cli/cli/config"
 	"github.com/docker/cli/internal/test"
 	"github.com/theupdateframework/notary"
-	"github.com/theupdateframework/notary/passphrase"
 	"github.com/theupdateframework/notary/trustmanager"
 	tufutils "github.com/theupdateframework/notary/tuf/utils"
 	"gotest.tools/v3/assert"
@@ -51,11 +50,9 @@ func TestGenerateKeySuccess(t *testing.T) {
 	pubKeyCWD := t.TempDir()
 	privKeyStorageDir := t.TempDir()
 
-	const testPass = "password"
-	cannedPasswordRetriever := passphrase.ConstantRetriever(testPass)
 	// generate a single key
 	keyName := "alice"
-	privKeyFileStore, err := trustmanager.NewKeyFileStore(privKeyStorageDir, cannedPasswordRetriever)
+	privKeyFileStore, err := trustmanager.NewKeyFileStore(privKeyStorageDir, testPassRetriever)
 	assert.NilError(t, err)
 
 	pubKeyPEM, err := generateKeyAndOutputPubPEM(keyName, privKeyFileStore)

cli/command/trust/key_load_test.go

@@ -12,7 +12,6 @@ import (
 	"github.com/docker/cli/cli/config"
 	"github.com/docker/cli/internal/test"
 	"github.com/theupdateframework/notary"
-	"github.com/theupdateframework/notary/passphrase"
 	"github.com/theupdateframework/notary/storage"
 	"github.com/theupdateframework/notary/trustmanager"
 	tufutils "github.com/theupdateframework/notary/tuf/utils"
@@ -122,8 +121,6 @@ func TestLoadKeyFromPath(t *testing.T) {
 
 			keyStorageDir := t.TempDir()
 
-			const passwd = "password"
-			cannedPasswordRetriever := passphrase.ConstantRetriever(passwd)
 			keyFileStore, err := storage.NewPrivateKeyFileStorage(keyStorageDir, notary.KeyExtension)
 			assert.NilError(t, err)
 			privKeyImporters := []trustmanager.Importer{keyFileStore}
@@ -133,7 +130,7 @@ func TestLoadKeyFromPath(t *testing.T) {
 			assert.NilError(t, err)
 
 			// import the key to our keyStorageDir
-			assert.Check(t, loadPrivKeyBytesToStore(privKeyBytes, privKeyImporters, privKeyFilepath, "signer-name", cannedPasswordRetriever))
+			assert.Check(t, loadPrivKeyBytesToStore(privKeyBytes, privKeyImporters, privKeyFilepath, "signer-name", testPassRetriever))
 
 			// check that the appropriate ~/<trust_dir>/private/<key_id>.key file exists
 			expectedImportKeyPath := filepath.Join(keyStorageDir, notary.PrivDir, keyID+"."+notary.KeyExtension)
@@ -151,7 +148,7 @@ func TestLoadKeyFromPath(t *testing.T) {
 			// assert encrypted header
 			assert.Check(t, is.Equal("ENCRYPTED PRIVATE KEY", keyPEM.Type))
 
-			decryptedKey, err := tufutils.ParsePKCS8ToTufKey(keyPEM.Bytes, []byte(passwd))
+			decryptedKey, err := tufutils.ParsePKCS8ToTufKey(keyPEM.Bytes, []byte(testPass))
 			assert.NilError(t, err)
 			fixturePEM, _ := pem.Decode(keyBytes)
 			assert.Check(t, is.DeepEqual(fixturePEM.Bytes, decryptedKey.Private()))
@@ -213,8 +210,6 @@ func TestLoadPubKeyFailure(t *testing.T) {
 	assert.NilError(t, os.WriteFile(pubKeyFilepath, pubKeyFixture, notary.PrivNoExecPerms))
 	keyStorageDir := t.TempDir()
 
-	const passwd = "password"
-	cannedPasswordRetriever := passphrase.ConstantRetriever(passwd)
 	keyFileStore, err := storage.NewPrivateKeyFileStorage(keyStorageDir, notary.KeyExtension)
 	assert.NilError(t, err)
 	privKeyImporters := []trustmanager.Importer{keyFileStore}
@@ -223,7 +218,7 @@ func TestLoadPubKeyFailure(t *testing.T) {
 	assert.NilError(t, err)
 
 	// import the key to our keyStorageDir - it should fail
-	err = loadPrivKeyBytesToStore(pubKeyBytes, privKeyImporters, pubKeyFilepath, "signer-name", cannedPasswordRetriever)
+	err = loadPrivKeyBytesToStore(pubKeyBytes, privKeyImporters, pubKeyFilepath, "signer-name", testPassRetriever)
 	expected := fmt.Sprintf("provided file %s is not a supported private key - to add a signer's public key use docker trust signer add", pubKeyFilepath)
 	assert.Error(t, err, expected)
 }

cli/command/trust/revoke_test.go

@@ -9,8 +9,6 @@ import (
 	"github.com/docker/cli/internal/test"
 	"github.com/docker/cli/internal/test/notary"
 	"github.com/theupdateframework/notary/client"
-	"github.com/theupdateframework/notary/passphrase"
-	"github.com/theupdateframework/notary/trustpinning"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
 	"gotest.tools/v3/golden"
@@ -151,14 +149,6 @@ func TestTrustRevokeCommand(t *testing.T) {
 	}
 }
 
-func TestGetSignableRolesForTargetAndRemoveError(t *testing.T) {
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever("password"), trustpinning.TrustPinConfig{})
-	assert.NilError(t, err)
-	target := client.Target{}
-	err = getSignableRolesForTargetAndRemove(target, notaryRepo)
-	assert.Error(t, err, "client is offline")
-}
-
 func TestRevokeTrustPromptTermination(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	t.Cleanup(cancel)

cli/command/trust/sign.go

@@ -116,7 +116,7 @@ func signAndPublishToTarget(out io.Writer, imgRefAndAuth trust.ImageRefAndAuth,
 	if err != nil {
 		return err
 	}
-	err = image.AddTargetToAllSignableRoles(notaryRepo, &target)
+	err = trust.AddToAllSignableRoles(notaryRepo, &target)
 	if err == nil {
 		prettyPrintExistingSignatureInfo(out, existingSigInfo)
 		err = notaryRepo.Publish()

cli/command/trust/sign_test.go

@@ -14,7 +14,6 @@ import (
 	"github.com/theupdateframework/notary"
 	"github.com/theupdateframework/notary/client"
 	"github.com/theupdateframework/notary/client/changelist"
-	"github.com/theupdateframework/notary/passphrase"
 	"github.com/theupdateframework/notary/trustpinning"
 	"github.com/theupdateframework/notary/tuf/data"
 	"gotest.tools/v3/assert"
@@ -22,8 +21,6 @@ import (
 	"gotest.tools/v3/skip"
 )
 
-const passwd = "password"
-
 func TestTrustSignCommandErrors(t *testing.T) {
 	testCases := []struct {
 		name          string
@@ -83,7 +80,7 @@ func TestTrustSignCommandOfflineErrors(t *testing.T) {
 }
 
 func TestGetOrGenerateNotaryKey(t *testing.T) {
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever(passwd), trustpinning.TrustPinConfig{})
+	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, testPassRetriever, trustpinning.TrustPinConfig{})
 	assert.NilError(t, err)
 
 	// repo is empty, try making a root key
@@ -126,7 +123,7 @@ func TestGetOrGenerateNotaryKey(t *testing.T) {
 func TestAddStageSigners(t *testing.T) {
 	skip.If(t, runtime.GOOS == "windows", "FIXME: not supported currently")
 
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever(passwd), trustpinning.TrustPinConfig{})
+	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, testPassRetriever, trustpinning.TrustPinConfig{})
 	assert.NilError(t, err)
 
 	// stage targets/user
@@ -207,7 +204,7 @@ func TestAddStageSigners(t *testing.T) {
 }
 
 func TestGetSignedManifestHashAndSize(t *testing.T) {
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever(passwd), trustpinning.TrustPinConfig{})
+	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, testPassRetriever, trustpinning.TrustPinConfig{})
 	assert.NilError(t, err)
 	_, _, err = getSignedManifestHashAndSize(notaryRepo, "test")
 	assert.Error(t, err, "client is offline")
@@ -229,7 +226,7 @@ func TestGetReleasedTargetHashAndSize(t *testing.T) {
 }
 
 func TestCreateTarget(t *testing.T) {
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever(passwd), trustpinning.TrustPinConfig{})
+	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, testPassRetriever, trustpinning.TrustPinConfig{})
 	assert.NilError(t, err)
 	_, err = createTarget(notaryRepo, "")
 	assert.Error(t, err, "no tag specified")
@@ -238,7 +235,7 @@ func TestCreateTarget(t *testing.T) {
 }
 
 func TestGetExistingSignatureInfoForReleasedTag(t *testing.T) {
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever(passwd), trustpinning.TrustPinConfig{})
+	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, testPassRetriever, trustpinning.TrustPinConfig{})
 	assert.NilError(t, err)
 	_, err = getExistingSignatureInfoForReleasedTag(notaryRepo, "test")
 	assert.Error(t, err, "client is offline")
@@ -267,7 +264,7 @@ func TestSignCommandChangeListIsCleanedOnError(t *testing.T) {
 	err := cmd.Execute()
 	assert.Assert(t, err != nil)
 
-	notaryRepo, err := client.NewFileCachedRepository(tmpDir, "docker.io/library/ubuntu", "https://localhost", nil, passphrase.ConstantRetriever(passwd), trustpinning.TrustPinConfig{})
+	notaryRepo, err := client.NewFileCachedRepository(tmpDir, "docker.io/library/ubuntu", "https://localhost", nil, testPassRetriever, trustpinning.TrustPinConfig{})
 	assert.NilError(t, err)
 	cl, err := notaryRepo.GetChangelist()
 	assert.NilError(t, err)

cli/trust/trust.go

@@ -40,10 +40,11 @@ var (
 	ActionsPullOnly = []string{"pull"}
 	// ActionsPushAndPull defines the actions for read-write interactions with a Notary Repository
 	ActionsPushAndPull = []string{"pull", "push"}
-	// NotaryServer is the endpoint serving the Notary trust server
-	NotaryServer = "https://notary.docker.io"
 )
 
+// NotaryServer is the endpoint serving the Notary trust server
+const NotaryServer = "https://notary.docker.io"
+
 // GetTrustDirectory returns the base trust directory name
 func GetTrustDirectory() string {
 	return filepath.Join(config.Dir(), "trust")
@@ -238,6 +239,20 @@ func NotaryError(repoName string, err error) error {
 	return err
 }
 
+// AddToAllSignableRoles 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 AddToAllSignableRoles(repo client.Repository, target *client.Target) error {
+	signableRoles, err := GetSignableRoles(repo, target)
+	if err != nil {
+		return err
+	}
+
+	return repo.AddTarget(target, signableRoles...)
+}
+
 // GetSignableRoles returns a list of roles for which we have valid signing
 // keys, given a notary repository and a target
 func GetSignableRoles(repo client.Repository, target *client.Target) ([]data.RoleName, error) {

cli/trust/trust_test.go

@@ -4,9 +4,9 @@ import (
 	"testing"
 
 	"github.com/distribution/reference"
+	registrytypes "github.com/docker/docker/api/types/registry"
 	"github.com/opencontainers/go-digest"
 	"github.com/theupdateframework/notary/client"
-	"github.com/theupdateframework/notary/passphrase"
 	"github.com/theupdateframework/notary/trustpinning"
 	"gotest.tools/v3/assert"
 	is "gotest.tools/v3/assert/cmp"
@@ -47,9 +47,42 @@ func TestGetDigest(t *testing.T) {
 }
 
 func TestGetSignableRolesError(t *testing.T) {
-	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, passphrase.ConstantRetriever("password"), trustpinning.TrustPinConfig{})
+	notaryRepo, err := client.NewFileCachedRepository(t.TempDir(), "gun", "https://localhost", nil, nil, trustpinning.TrustPinConfig{})
 	assert.NilError(t, err)
-	target := client.Target{}
+	_, err = GetSignableRoles(notaryRepo, &client.Target{})
-	_, err = GetSignableRoles(notaryRepo, &target)
+	const expected = "client is offline"
-	assert.Error(t, err, "client is offline")
+	assert.Error(t, err, expected)
+}
+
+func TestENVTrustServer(t *testing.T) {
+	t.Setenv("DOCKER_CONTENT_TRUST_SERVER", "https://notary-test.example.com:5000")
+	indexInfo := &registrytypes.IndexInfo{Name: "testserver"}
+	output, err := Server(indexInfo)
+	const expected = "https://notary-test.example.com:5000"
+	assert.NilError(t, err)
+	assert.Equal(t, output, expected)
+}
+
+func TestHTTPENVTrustServer(t *testing.T) {
+	t.Setenv("DOCKER_CONTENT_TRUST_SERVER", "http://notary-test.example.com:5000")
+	indexInfo := &registrytypes.IndexInfo{Name: "testserver"}
+	_, err := Server(indexInfo)
+	const expected = "valid https URL required for trust server"
+	assert.ErrorContains(t, err, expected, "Expected error with invalid scheme")
+}
+
+func TestOfficialTrustServer(t *testing.T) {
+	indexInfo := &registrytypes.IndexInfo{Name: "testserver", Official: true}
+	output, err := Server(indexInfo)
+	const expected = NotaryServer
+	assert.NilError(t, err)
+	assert.Equal(t, output, expected)
+}
+
+func TestNonOfficialTrustServer(t *testing.T) {
+	indexInfo := &registrytypes.IndexInfo{Name: "testserver", Official: false}
+	output, err := Server(indexInfo)
+	const expected = "https://testserver"
+	assert.NilError(t, err)
+	assert.Equal(t, output, expected)
 }

internal/test/notary/client.go

@@ -5,7 +5,6 @@ import (
 	"github.com/theupdateframework/notary/client"
 	"github.com/theupdateframework/notary/client/changelist"
 	"github.com/theupdateframework/notary/cryptoservice"
-	"github.com/theupdateframework/notary/passphrase"
 	"github.com/theupdateframework/notary/storage"
 	"github.com/theupdateframework/notary/trustmanager"
 	"github.com/theupdateframework/notary/tuf/data"
@@ -494,11 +493,17 @@ func (LoadedNotaryRepository) GetDelegationRoles() ([]data.Role, error) {
 	return loadedDelegationRoles, nil
 }
 
+const testPass = "password"
+
+func testPassRetriever(string, string, bool, int) (string, bool, error) {
+	return testPass, false, nil
+}
+
 // GetCryptoService is the getter for the repository's CryptoService
 func (l LoadedNotaryRepository) GetCryptoService() signed.CryptoService {
 	if l.statefulCryptoService == nil {
 		// give it an in-memory cryptoservice with a root key and targets key
-		l.statefulCryptoService = cryptoservice.NewCryptoService(trustmanager.NewKeyMemoryStore(passphrase.ConstantRetriever("password")))
+		l.statefulCryptoService = cryptoservice.NewCryptoService(trustmanager.NewKeyMemoryStore(testPassRetriever))
 		l.statefulCryptoService.AddKey(data.CanonicalRootRole, l.GetGUN(), nil)
 		l.statefulCryptoService.AddKey(data.CanonicalTargetsRole, l.GetGUN(), nil)
 	}