From eb82fe87a5d557062a649564494ac62f93946350 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 12:38:27 +0100 Subject: [PATCH 01/12] cli/trust: make NotaryServer a const This var used to be vendored from github.com/docker/docker/registry, but was removed there, and made a local var in a1cbaa827b5cfc2516b8dde407e4dabe0f9378f6. It is (and should never be) modified, so let's change it into a const. Signed-off-by: Sebastiaan van Stijn --- cli/trust/trust.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cli/trust/trust.go b/cli/trust/trust.go index bb7e597aa56d..65beb3aa4eab 100644 --- a/cli/trust/trust.go +++ b/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") From eae4c38023c8bf337a11d3294d835e01a473dfd2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 14:15:33 +0100 Subject: [PATCH 02/12] internal/test/notary: add testPassRetriever helper Add a basic helper to provide the equivalent of passphrase.ConstantRetriever with a fixed passphrase for testing. Signed-off-by: Sebastiaan van Stijn --- internal/test/notary/client.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/test/notary/client.go b/internal/test/notary/client.go index 3ee625fd3be3..a948ad62cf54 100644 --- a/internal/test/notary/client.go +++ b/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) } From dd617b1464b7b7858ed7e85d491cb82e2ffeee3d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 12:56:47 +0100 Subject: [PATCH 03/12] cli/command/trust: remove unused passphrase-retriever from test The test only validates that an error is produced because the notary server is offline, and does not sent a passphrase. Signed-off-by: Sebastiaan van Stijn --- cli/trust/trust_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/cli/trust/trust_test.go b/cli/trust/trust_test.go index 771b4291df5c..836ed3892f58 100644 --- a/cli/trust/trust_test.go +++ b/cli/trust/trust_test.go @@ -6,7 +6,6 @@ import ( "github.com/distribution/reference" "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 +46,9 @@ 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, &target) - assert.Error(t, err, "client is offline") + _, err = GetSignableRoles(notaryRepo, &client.Target{}) + const expected = "client is offline" + assert.Error(t, err, expected) } From d4217eb205e1014be5a13c5519f1177630a5f629 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 13:47:32 +0100 Subject: [PATCH 04/12] cli/command/trust: remove TestGetOrGenerateNotaryKeyAndInitRepo This test was only testing trust.GetSignableRoles to return an error if it's offline, which was duplicating the [TestGetSignableRolesError] test. [TestGetSignableRolesError]: https://github.com/docker/cli/blob/fe0a8d27912dc6fddc60cedcd35bbef27b776355/cli/trust/trust_test.go#L49-L55 Signed-off-by: Sebastiaan van Stijn --- cli/command/trust/helpers_test.go | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 cli/command/trust/helpers_test.go diff --git a/cli/command/trust/helpers_test.go b/cli/command/trust/helpers_test.go deleted file mode 100644 index 9ce19259d11e..000000000000 --- a/cli/command/trust/helpers_test.go +++ /dev/null @@ -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") -} From 1d8f87a2fb2dbe5ca9ea8e956acde7de4cfa5a1b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 14:07:03 +0100 Subject: [PATCH 05/12] cli/command/trust: remove TestGetSignableRolesForTargetAndRemoveError This test was only testing trust.GetSignableRoles to return an error if it's offline, which was duplicating the [TestGetSignableRolesError] test. [TestGetSignableRolesError]: https://github.com/docker/cli/blob/fe0a8d27912dc6fddc60cedcd35bbef27b776355/cli/trust/trust_test.go#L49-L55 Signed-off-by: Sebastiaan van Stijn --- cli/command/trust/revoke_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/cli/command/trust/revoke_test.go b/cli/command/trust/revoke_test.go index 6e2ebd66bfb6..da1e48ecf876 100644 --- a/cli/command/trust/revoke_test.go +++ b/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) From 791bdf7b3c1a7f36853dc3715b4c5229f967412c Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 14:10:35 +0100 Subject: [PATCH 06/12] cli/command/trust: add testPassRetriever helper Add a basic helper to provide the equivalent of passphrase.ConstantRetriever with a fixed passphrase for testing. Signed-off-by: Sebastiaan van Stijn --- cli/command/trust/helpers.go | 6 ++++++ cli/command/trust/key_generate_test.go | 5 +---- cli/command/trust/key_load_test.go | 11 +++-------- cli/command/trust/sign_test.go | 15 ++++++--------- 4 files changed, 16 insertions(+), 21 deletions(-) diff --git a/cli/command/trust/helpers.go b/cli/command/trust/helpers.go index 3d2ba4acc3e5..0a9ef6671f41 100644 --- a/cli/command/trust/helpers.go +++ b/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 +} diff --git a/cli/command/trust/key_generate_test.go b/cli/command/trust/key_generate_test.go index 74fc3fed6891..1efd1d31214c 100644 --- a/cli/command/trust/key_generate_test.go +++ b/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) diff --git a/cli/command/trust/key_load_test.go b/cli/command/trust/key_load_test.go index b2233f282a69..2c004d38849d 100644 --- a/cli/command/trust/key_load_test.go +++ b/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 ~//private/.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) } diff --git a/cli/command/trust/sign_test.go b/cli/command/trust/sign_test.go index 7e8d0a08e70f..e24ffbba73cb 100644 --- a/cli/command/trust/sign_test.go +++ b/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) From 049f84c94d4a354596a838fb7cf871726ce52ef5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 13:01:05 +0100 Subject: [PATCH 07/12] cli/command/image: remove TestAddTargetToAllSignableRolesError This test was only testing trust.GetSignableRoles to return an error if it's offline, which was duplicating the [TestGetSignableRolesError] test in the cli/trust package. [TestGetSignableRolesError]: https://github.com/docker/cli/blob/fe0a8d27912dc6fddc60cedcd35bbef27b776355/cli/trust/trust_test.go#L49-L55 Signed-off-by: Sebastiaan van Stijn --- cli/command/image/trust_test.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/cli/command/image/trust_test.go b/cli/command/image/trust_test.go index eba38cbb97df..59e58e193e1f 100644 --- a/cli/command/image/trust_test.go +++ b/cli/command/image/trust_test.go @@ -5,10 +5,6 @@ import ( "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" ) @@ -47,11 +43,3 @@ func TestNonOfficialTrustServer(t *testing.T) { 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") -} From 55bc30a78446afc665e880e23d85fd996f3de394 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 13:16:24 +0100 Subject: [PATCH 08/12] cli/command/image: use t.SetEnv in trust tests Signed-off-by: Sebastiaan van Stijn --- cli/command/image/trust_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cli/command/image/trust_test.go b/cli/command/image/trust_test.go index 59e58e193e1f..1846ceba37c5 100644 --- a/cli/command/image/trust_test.go +++ b/cli/command/image/trust_test.go @@ -5,11 +5,10 @@ import ( "github.com/docker/cli/cli/trust" registrytypes "github.com/docker/docker/api/types/registry" - "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"}) + t.Setenv("DOCKER_CONTENT_TRUST_SERVER", "https://notary-test.example.com:5000") indexInfo := ®istrytypes.IndexInfo{Name: "testserver"} output, err := trust.Server(indexInfo) expectedStr := "https://notary-test.example.com:5000" @@ -19,7 +18,7 @@ func TestENVTrustServer(t *testing.T) { } func TestHTTPENVTrustServer(t *testing.T) { - env.PatchAll(t, map[string]string{"DOCKER_CONTENT_TRUST_SERVER": "http://notary-test.example.com:5000"}) + t.Setenv("DOCKER_CONTENT_TRUST_SERVER", "http://notary-test.example.com:5000") indexInfo := ®istrytypes.IndexInfo{Name: "testserver"} _, err := trust.Server(indexInfo) if err == nil { From e6382db10e01e16d830dd8d999647635f9f92627 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 11:58:18 +0100 Subject: [PATCH 09/12] cli/command/image: move trust unit-tests to trust package These tests were not testing functionality that was implemented in the image package. Move them to the trust package, where they belong. Signed-off-by: Sebastiaan van Stijn --- cli/command/image/trust_test.go | 44 --------------------------------- cli/trust/trust_test.go | 37 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 44 deletions(-) delete mode 100644 cli/command/image/trust_test.go diff --git a/cli/command/image/trust_test.go b/cli/command/image/trust_test.go deleted file mode 100644 index 1846ceba37c5..000000000000 --- a/cli/command/image/trust_test.go +++ /dev/null @@ -1,44 +0,0 @@ -package image - -import ( - "testing" - - "github.com/docker/cli/cli/trust" - registrytypes "github.com/docker/docker/api/types/registry" -) - -func TestENVTrustServer(t *testing.T) { - t.Setenv("DOCKER_CONTENT_TRUST_SERVER", "https://notary-test.example.com:5000") - indexInfo := ®istrytypes.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) { - t.Setenv("DOCKER_CONTENT_TRUST_SERVER", "http://notary-test.example.com:5000") - indexInfo := ®istrytypes.IndexInfo{Name: "testserver"} - _, err := trust.Server(indexInfo) - if err == nil { - t.Fatal("Expected error with invalid scheme") - } -} - -func TestOfficialTrustServer(t *testing.T) { - indexInfo := ®istrytypes.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 := ®istrytypes.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) - } -} diff --git a/cli/trust/trust_test.go b/cli/trust/trust_test.go index 836ed3892f58..336b66e0b886 100644 --- a/cli/trust/trust_test.go +++ b/cli/trust/trust_test.go @@ -4,6 +4,7 @@ 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/trustpinning" @@ -52,3 +53,39 @@ func TestGetSignableRolesError(t *testing.T) { const expected = "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 := ®istrytypes.IndexInfo{Name: "testserver"} + output, err := 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) { + t.Setenv("DOCKER_CONTENT_TRUST_SERVER", "http://notary-test.example.com:5000") + indexInfo := ®istrytypes.IndexInfo{Name: "testserver"} + _, err := Server(indexInfo) + if err == nil { + t.Fatal("Expected error with invalid scheme") + } +} + +func TestOfficialTrustServer(t *testing.T) { + indexInfo := ®istrytypes.IndexInfo{Name: "testserver", Official: true} + output, err := Server(indexInfo) + if err != nil || output != NotaryServer { + t.Fatalf("Expected server to be %s, got %s", NotaryServer, output) + } +} + +func TestNonOfficialTrustServer(t *testing.T) { + indexInfo := ®istrytypes.IndexInfo{Name: "testserver", Official: false} + output, err := Server(indexInfo) + expectedStr := "https://" + indexInfo.Name + if err != nil || output != expectedStr { + t.Fatalf("Expected server to be %s, got %s", expectedStr, output) + } +} From d95385057fc577bbc46215aeef4e76b639014b99 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 13:22:00 +0100 Subject: [PATCH 10/12] cli/command/trust: use gotest.tools in tests Signed-off-by: Sebastiaan van Stijn --- cli/trust/trust_test.go | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/cli/trust/trust_test.go b/cli/trust/trust_test.go index 336b66e0b886..18531bf4c941 100644 --- a/cli/trust/trust_test.go +++ b/cli/trust/trust_test.go @@ -58,34 +58,31 @@ func TestENVTrustServer(t *testing.T) { t.Setenv("DOCKER_CONTENT_TRUST_SERVER", "https://notary-test.example.com:5000") indexInfo := ®istrytypes.IndexInfo{Name: "testserver"} output, err := 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) - } + 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 := ®istrytypes.IndexInfo{Name: "testserver"} _, err := Server(indexInfo) - if err == nil { - t.Fatal("Expected error with invalid scheme") - } + 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 := ®istrytypes.IndexInfo{Name: "testserver", Official: true} output, err := Server(indexInfo) - if err != nil || output != NotaryServer { - t.Fatalf("Expected server to be %s, got %s", NotaryServer, output) - } + const expected = NotaryServer + assert.NilError(t, err) + assert.Equal(t, output, expected) } func TestNonOfficialTrustServer(t *testing.T) { indexInfo := ®istrytypes.IndexInfo{Name: "testserver", Official: false} output, err := Server(indexInfo) - expectedStr := "https://" + indexInfo.Name - if err != nil || output != expectedStr { - t.Fatalf("Expected server to be %s, got %s", expectedStr, output) - } + const expected = "https://testserver" + assert.NilError(t, err) + assert.Equal(t, output, expected) } From 7a6270d1904bc5b3d14b12fc2867f17abf94a093 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 13:34:34 +0100 Subject: [PATCH 11/12] cli/command/image: move AddTargetToAllSignableRoles to cli/trust This utility was shared between the "image" and "trust" packages, and a shallow wrapper around features in the cli/trust package. Move it there instead and rename it to `trust.AddToAllSignableRoles`. There are no known external consumers of this utility, so skipping a deprecation. Signed-off-by: Sebastiaan van Stijn --- cli/command/image/trust.go | 15 +-------------- cli/command/trust/sign.go | 2 +- cli/trust/trust.go | 14 ++++++++++++++ 3 files changed, 16 insertions(+), 15 deletions(-) diff --git a/cli/command/image/trust.go b/cli/command/image/trust.go index 274344452f39..2b4f92794c6d 100644 --- a/cli/command/image/trust.go +++ b/cli/command/image/trust.go @@ -137,7 +137,7 @@ func PushTrustedReference(ctx context.Context, ioStreams command.Streams, repoIn err = repo.AddTarget(target, data.CanonicalTargetsRole) case nil: // already initialized and we have successfully downloaded the latest metadata - err = AddTargetToAllSignableRoles(repo, target) + err = trust.AddToAllSignableRoles(repo, target) 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) diff --git a/cli/command/trust/sign.go b/cli/command/trust/sign.go index fa2192bef941..bf974d0dc0d2 100644 --- a/cli/command/trust/sign.go +++ b/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() diff --git a/cli/trust/trust.go b/cli/trust/trust.go index 65beb3aa4eab..62bbd8f8336d 100644 --- a/cli/trust/trust.go +++ b/cli/trust/trust.go @@ -239,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) { From c7072a885ddb6dc3d94bece38b8c1609506a8f47 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 2 Mar 2025 11:09:11 +0100 Subject: [PATCH 12/12] cli/command/image: rename vars that shadowed type Signed-off-by: Sebastiaan van Stijn --- cli/command/image/trust.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/cli/command/image/trust.go b/cli/command/image/trust.go index 2b4f92794c6d..c47e1747373c 100644 --- a/cli/command/image/trust.go +++ b/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 - target.Hashes = data.Hashes{string(dgst.Algorithm()): h} - target.Length = int64(pushResult.Size) + notaryTarget.Name = pushResult.Tag + notaryTarget.Hashes = data.Hashes{string(dgst.Algorithm()): h} + 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 = trust.AddToAllSignableRoles(repo, target) + err = trust.AddToAllSignableRoles(repo, notaryTarget) default: return trust.NotaryError(repoInfo.Name.Name(), err) }