From ddfe7271bb364a2c14e817bea79c4f8c7bf65f8d Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Thu, 9 Feb 2017 15:29:44 -0800 Subject: [PATCH 01/13] Add dependency injection of remote store for NotaryRepository initialization Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 78 +++++++++++++++++++++++-------------------- client/client_test.go | 37 ++++++++------------ 2 files changed, 56 insertions(+), 59 deletions(-) diff --git a/client/client.go b/client/client.go index b74f62496..0257cbeb3 100644 --- a/client/client.go +++ b/client/client.go @@ -17,7 +17,6 @@ import ( "github.com/docker/notary/client/changelist" "github.com/docker/notary/cryptoservice" store "github.com/docker/notary/storage" - "github.com/docker/notary/trustmanager" "github.com/docker/notary/trustpinning" "github.com/docker/notary/tuf" "github.com/docker/notary/tuf/data" @@ -44,6 +43,7 @@ type NotaryRepository struct { baseURL string tufRepoPath string cache store.MetadataStore + remoteStore store.RemoteStore CryptoService signed.CryptoService tufRepo *tuf.Repo invalid *tuf.Repo // known data that was parsable but deemed invalid @@ -53,7 +53,9 @@ type NotaryRepository struct { } // NewFileCachedNotaryRepository is a wrapper for NewNotaryRepository that initializes -// a file cache from the provided repository and local config information +// a file cache from the provided repository, local config information and a crypto service. +// It also retrieves the remote store associated to the base directory under where all the +// trust files will be stored and the specified GUN. func NewFileCachedNotaryRepository(baseDir string, gun data.GUN, baseURL string, rt http.RoundTripper, retriever notary.PassRetriever, trustPinning trustpinning.TrustPinConfig) ( *NotaryRepository, error) { @@ -65,47 +67,44 @@ func NewFileCachedNotaryRepository(baseDir string, gun data.GUN, baseURL string, if err != nil { return nil, err } - return NewNotaryRepository(baseDir, gun, baseURL, rt, cache, retriever, trustPinning) -} - -// NewNotaryRepository is a helper method that returns a new notary repository. -// It takes the base directory under where all the trust files will be stored -// (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling -// docker content trust). -func NewNotaryRepository(baseDir string, gun data.GUN, baseURL string, rt http.RoundTripper, cache store.MetadataStore, - retriever notary.PassRetriever, trustPinning trustpinning.TrustPinConfig) ( - *NotaryRepository, error) { keyStores, err := getKeyStores(baseDir, retriever) if err != nil { return nil, err } - return repositoryFromKeystores(baseDir, gun, baseURL, rt, cache, - keyStores, trustPinning) -} + cryptoService := cryptoservice.NewCryptoService(keyStores...) -// repositoryFromKeystores is a helper function for NewNotaryRepository that -// takes some basic NotaryRepository parameters as well as keystores (in order -// of usage preference), and returns a NotaryRepository. -func repositoryFromKeystores(baseDir string, gun data.GUN, baseURL string, rt http.RoundTripper, cache store.MetadataStore, - keyStores []trustmanager.KeyStore, trustPin trustpinning.TrustPinConfig) (*NotaryRepository, error) { + remoteStore, err := getRemoteStore(baseURL, gun, rt) + if err != nil { + return nil, err + } - cryptoService := cryptoservice.NewCryptoService(keyStores...) + return NewNotaryRepository(baseDir, gun, baseURL, remoteStore, rt, cache, trustPinning, cryptoService) +} + +// NewNotaryRepository is the base method that returns a new notary repository. +// It takes the base directory under where all the trust files will be stored +// (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling +// docker content trust). +// It expects initiated remote store and cache. +func NewNotaryRepository(baseDir string, gun data.GUN, baseURL string, remoteStore store.RemoteStore, rt http.RoundTripper, cache store.MetadataStore, + trustPinning trustpinning.TrustPinConfig, cryptoService signed.CryptoService) ( + *NotaryRepository, error) { nRepo := &NotaryRepository{ gun: gun, baseDir: baseDir, baseURL: baseURL, tufRepoPath: filepath.Join(baseDir, tufDir, filepath.FromSlash(gun.String())), + cache: cache, + remoteStore: remoteStore, CryptoService: cryptoService, roundTrip: rt, - trustPinning: trustPin, + trustPinning: trustPinning, LegacyVersions: 0, // By default, don't sign with legacy roles } - nRepo.cache = cache - return nRepo, nil } @@ -686,10 +685,7 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error { return err } - remote, err := getRemoteStore(r.baseURL, r.gun, r.roundTrip) - if err != nil { - return err - } + remote := r.remoteStore return remote.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles)) } @@ -970,10 +966,8 @@ func (r *NotaryRepository) bootstrapClient(checkInitialized bool) (*TUFClient, e } } - remote, remoteErr := getRemoteStore(r.baseURL, r.gun, r.roundTrip) - if remoteErr != nil { - logrus.Error(remoteErr) - } else if !newBuilder.IsLoaded(data.CanonicalRootRole) || checkInitialized { + remote := r.remoteStore + if !newBuilder.IsLoaded(data.CanonicalRootRole) || checkInitialized { // remoteErr was nil and we were not able to load a root from cache or // are specifically checking for initialization of the repo. @@ -1146,13 +1140,25 @@ func (r *NotaryRepository) DeleteTrustData(deleteRemote bool) error { } // Note that this will require admin permission in this NotaryRepository's roundtripper if deleteRemote { - remote, err := getRemoteStore(r.baseURL, r.gun, r.roundTrip) - if err != nil { - return err - } + remote := r.remoteStore if err := remote.RemoveAll(); err != nil { return err } } return nil } + +// SetBaseURL updates the repository's base URL and its associated remote store +func (r *NotaryRepository) setBaseURL(baseURL string) error { + // We try to retrieve the remote store before updating the repository + remoteStore, err := getRemoteStore(baseURL, r.gun, r.roundTrip) + if err != nil { + return err + } + + // Update the repository + r.baseURL = baseURL + r.remoteStore = remoteStore + + return nil +} diff --git a/client/client_test.go b/client/client_test.go index 081ec3b0d..de2299edb 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -3507,10 +3507,10 @@ func TestBootstrapClientBadURL(t *testing.T) { require.EqualError(t, err, err2.Error()) } -// TestBootstrapClientInvalidURL checks that bootstrapClient correctly -// returns an error when the URL is valid but does not point to +// TestClientInvalidURL checks that instantiating a new NotaryRepository +// correctly returns an error when the URL is valid but does not point to // a TUF server -func TestBootstrapClientInvalidURL(t *testing.T) { +func TestClientInvalidURL(t *testing.T) { tempBaseDir, err := ioutil.TempDir("", "notary-test-") require.NoError(t, err, "failed to create a temporary directory: %s", err) repo, err := NewFileCachedNotaryRepository( @@ -3521,20 +3521,11 @@ func TestBootstrapClientInvalidURL(t *testing.T) { passphraseRetriever, trustpinning.TrustPinConfig{}, ) - require.NoError(t, err, "error creating repo: %s", err) - - c, err := repo.bootstrapClient(false) - require.Nil(t, c) + // NewFileCachedNotaryRepository should fail and return an error + // since it initializes the cache but also the remote repository + // from the baseURL and the GUN + require.Nil(t, repo) require.Error(t, err) - - c, err2 := repo.bootstrapClient(true) - require.Nil(t, c) - require.Error(t, err2) - - // same error should be returned because we don't have local data - // and are requesting remote root regardless of checkInitialized - // value - require.EqualError(t, err, err2.Error()) } func TestPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t *testing.T) { @@ -3702,8 +3693,7 @@ func TestDeleteRemoteRepo(t *testing.T) { requireRepoHasExpectedKeys(t, repo, rootKeyID, true) // Try connecting to the remote store directly and make sure that no metadata exists for this gun - remoteStore, err := getRemoteStore(repo.baseURL, repo.gun, repo.roundTrip) - require.NoError(t, err) + remoteStore := repo.remoteStore meta, err := remoteStore.GetSized(data.CanonicalRootRole.String(), store.NoSizeLimit) require.Error(t, err) require.IsType(t, store.ErrMetaNotFound{}, err) @@ -3728,8 +3718,7 @@ func TestDeleteRemoteRepo(t *testing.T) { require.Len(t, longLivingCl.List(), 1) // Check that the other repo's remote data is unaffected - remoteStore, err = getRemoteStore(longLivingRepo.baseURL, longLivingRepo.gun, longLivingRepo.roundTrip) - require.NoError(t, err) + remoteStore = longLivingRepo.remoteStore meta, err = remoteStore.GetSized(data.CanonicalRootRole.String(), store.NoSizeLimit) require.NoError(t, err) require.NotNil(t, meta) @@ -3743,9 +3732,11 @@ func TestDeleteRemoteRepo(t *testing.T) { require.NoError(t, err) require.NotNil(t, meta) - // Try deleting again with an invalid server URL - repo.baseURL = "invalid" - require.Error(t, repo.DeleteTrustData(true)) + // Try setting an invalid server URL before deleting + require.Error(t, repo.setBaseURL("invalid")) + + // Try deleting again the remote + require.NoError(t, repo.DeleteTrustData(true)) } // Test that we get a correct list of roles with keys and signatures From e4cacd02e933e0559b85d9f679633dc07588a3cf Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Fri, 10 Feb 2017 10:26:47 -0800 Subject: [PATCH 02/13] Remove helper and test for changing remote store base URL once instantiated Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 15 --------------- client/client_test.go | 6 ------ 2 files changed, 21 deletions(-) diff --git a/client/client.go b/client/client.go index 0257cbeb3..555d024dd 100644 --- a/client/client.go +++ b/client/client.go @@ -1147,18 +1147,3 @@ func (r *NotaryRepository) DeleteTrustData(deleteRemote bool) error { } return nil } - -// SetBaseURL updates the repository's base URL and its associated remote store -func (r *NotaryRepository) setBaseURL(baseURL string) error { - // We try to retrieve the remote store before updating the repository - remoteStore, err := getRemoteStore(baseURL, r.gun, r.roundTrip) - if err != nil { - return err - } - - // Update the repository - r.baseURL = baseURL - r.remoteStore = remoteStore - - return nil -} diff --git a/client/client_test.go b/client/client_test.go index de2299edb..69c83d375 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -3731,12 +3731,6 @@ func TestDeleteRemoteRepo(t *testing.T) { meta, err = remoteStore.GetSized(data.CanonicalTimestampRole.String(), store.NoSizeLimit) require.NoError(t, err) require.NotNil(t, meta) - - // Try setting an invalid server URL before deleting - require.Error(t, repo.setBaseURL("invalid")) - - // Try deleting again the remote - require.NoError(t, repo.DeleteTrustData(true)) } // Test that we get a correct list of roles with keys and signatures From ae436101d08ec246887319f535eaf1efcce02c17 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Mon, 13 Feb 2017 10:44:55 -0800 Subject: [PATCH 03/13] Remove the RoundTripper object out of NotaryRepository Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 11 +++++------ client/helpers.go | 12 ++---------- client/helpers_test.go | 6 ++++-- 3 files changed, 11 insertions(+), 18 deletions(-) diff --git a/client/client.go b/client/client.go index 555d024dd..e92818c4d 100644 --- a/client/client.go +++ b/client/client.go @@ -80,7 +80,7 @@ func NewFileCachedNotaryRepository(baseDir string, gun data.GUN, baseURL string, return nil, err } - return NewNotaryRepository(baseDir, gun, baseURL, remoteStore, rt, cache, trustPinning, cryptoService) + return NewNotaryRepository(baseDir, gun, baseURL, remoteStore, cache, trustPinning, cryptoService) } // NewNotaryRepository is the base method that returns a new notary repository. @@ -88,7 +88,7 @@ func NewFileCachedNotaryRepository(baseDir string, gun data.GUN, baseURL string, // (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling // docker content trust). // It expects initiated remote store and cache. -func NewNotaryRepository(baseDir string, gun data.GUN, baseURL string, remoteStore store.RemoteStore, rt http.RoundTripper, cache store.MetadataStore, +func NewNotaryRepository(baseDir string, gun data.GUN, baseURL string, remoteStore store.RemoteStore, cache store.MetadataStore, trustPinning trustpinning.TrustPinConfig, cryptoService signed.CryptoService) ( *NotaryRepository, error) { @@ -100,7 +100,6 @@ func NewNotaryRepository(baseDir string, gun data.GUN, baseURL string, remoteSto cache: cache, remoteStore: remoteStore, CryptoService: cryptoService, - roundTrip: rt, trustPinning: trustPinning, LegacyVersions: 0, // By default, don't sign with legacy roles } @@ -276,7 +275,7 @@ func (r *NotaryRepository) initializeRoles(rootKeys []data.PublicKey, localRoles for _, role := range remoteRoles { // This key is generated by the remote server. var key data.PublicKey - key, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip) + key, err = getRemoteKey(role, r.remoteStore) if err != nil { return } @@ -1031,7 +1030,7 @@ func (r *NotaryRepository) pubKeyListForRotation(role data.RoleName, serverManag // If server manages the key being rotated, request a rotation and return the new key if serverManaged { - pubKey, err = rotateRemoteKey(r.baseURL, r.gun, role, r.roundTrip) + pubKey, err = rotateRemoteKey(role, r.remoteStore) pubKeyList = make(data.KeyList, 0, 1) pubKeyList = append(pubKeyList, pubKey) if err != nil { @@ -1056,7 +1055,7 @@ func (r *NotaryRepository) pubKeyListForRotation(role data.RoleName, serverManag for _, keyID := range newKeys { pubKey = r.CryptoService.GetKey(keyID) if pubKey == nil { - return nil, fmt.Errorf("unable to find key: %s") + return nil, fmt.Errorf("unable to find key: %s", keyID) } pubKeyList = append(pubKeyList, pubKey) } diff --git a/client/helpers.go b/client/helpers.go index a473cd7f6..de55533d6 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -218,11 +218,7 @@ func warnRolesNearExpiry(r *tuf.Repo) { } // Fetches a public key from a remote store, given a gun and role -func getRemoteKey(url string, gun data.GUN, role data.RoleName, rt http.RoundTripper) (data.PublicKey, error) { - remote, err := getRemoteStore(url, gun, rt) - if err != nil { - return nil, err - } +func getRemoteKey(role data.RoleName, remote store.RemoteStore) (data.PublicKey, error) { rawPubKey, err := remote.GetKey(role) if err != nil { return nil, err @@ -237,11 +233,7 @@ func getRemoteKey(url string, gun data.GUN, role data.RoleName, rt http.RoundTri } // Rotates a private key in a remote store and returns the public key component -func rotateRemoteKey(url string, gun data.GUN, role data.RoleName, rt http.RoundTripper) (data.PublicKey, error) { - remote, err := getRemoteStore(url, gun, rt) - if err != nil { - return nil, err - } +func rotateRemoteKey(role data.RoleName, remote store.RemoteStore) (data.PublicKey, error) { rawPubKey, err := remote.RotateKey(role) if err != nil { return nil, err diff --git a/client/helpers_test.go b/client/helpers_test.go index c0e12151b..f8730f2e7 100644 --- a/client/helpers_test.go +++ b/client/helpers_test.go @@ -1024,12 +1024,14 @@ func TestAllNotNearExpiry(t *testing.T) { func TestRotateRemoteKeyOffline(t *testing.T) { // without a valid roundtripper, rotation should fail since we cannot initialize a HTTPStore - key, err := rotateRemoteKey("invalidURL", "gun", data.CanonicalSnapshotRole, nil) + remote, err := getRemoteStore("invalidURL", "gun", nil) + key, err := rotateRemoteKey(data.CanonicalSnapshotRole, remote) require.Error(t, err) require.Nil(t, key) // if the underlying remote store is faulty and cannot rotate keys, we should get back the error - key, err = rotateRemoteKey("https://notary-server", "gun", data.CanonicalSnapshotRole, http.DefaultTransport) + remote, err = getRemoteStore("https://notary-server", "gun", http.DefaultTransport) + key, err = rotateRemoteKey(data.CanonicalSnapshotRole, remote) require.Error(t, err) require.Nil(t, key) } From 7d8ec9f4023265b13e66019135a10ae50d03541f Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sun, 19 Feb 2017 10:35:02 -0800 Subject: [PATCH 04/13] making changelist injectable Signed-off-by: David Lawrence (github: endophage) --- client/changelist/changelist.go | 5 +++ client/changelist/file_changelist.go | 5 +++ client/changelist/interface.go | 3 ++ client/client.go | 67 ++++++++++++---------------- client/client_test.go | 39 ++++++++++------ client/delegations.go | 49 +++----------------- client/witness.go | 11 +---- cmd/notary/tuf.go | 20 +++------ cmd/notary/tuf_test.go | 1 - 9 files changed, 82 insertions(+), 118 deletions(-) diff --git a/client/changelist/changelist.go b/client/changelist/changelist.go index 9b52981ad..30cf69459 100644 --- a/client/changelist/changelist.go +++ b/client/changelist/changelist.go @@ -21,6 +21,11 @@ func (cl *memChangelist) Add(c Change) error { return nil } +// Location returns the string "memory" +func (cl memChangelist) Location() string { + return "memory" +} + // Remove deletes the changes found at the given indices func (cl *memChangelist) Remove(idxs []int) error { remove := make(map[int]struct{}) diff --git a/client/changelist/file_changelist.go b/client/changelist/file_changelist.go index a25215482..7e128a194 100644 --- a/client/changelist/file_changelist.go +++ b/client/changelist/file_changelist.go @@ -137,6 +137,11 @@ func (cl FileChangelist) Close() error { return nil } +// Location returns the file path to the changelist +func (cl FileChangelist) Location() string { + return cl.dir +} + // NewIterator creates an iterator from FileChangelist func (cl FileChangelist) NewIterator() (ChangeIterator, error) { fileInfos, err := getFileNames(cl.dir) diff --git a/client/changelist/interface.go b/client/changelist/interface.go index d1af57445..70dc0a2d0 100644 --- a/client/changelist/interface.go +++ b/client/changelist/interface.go @@ -27,6 +27,9 @@ type Changelist interface { // NewIterator returns an iterator for walking through the list // of changes currently stored NewIterator() (ChangeIterator, error) + + // Location returns the place the changelist is stores + Location() string } const ( diff --git a/client/client.go b/client/client.go index e92818c4d..51e440473 100644 --- a/client/client.go +++ b/client/client.go @@ -7,7 +7,6 @@ import ( "io/ioutil" "net/http" "net/url" - "os" "path/filepath" "regexp" "time" @@ -22,6 +21,7 @@ import ( "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/utils" + "os" ) const ( @@ -41,7 +41,7 @@ type NotaryRepository struct { baseDir string gun data.GUN baseURL string - tufRepoPath string + changelist changelist.Changelist cache store.MetadataStore remoteStore store.RemoteStore CryptoService signed.CryptoService @@ -80,23 +80,30 @@ func NewFileCachedNotaryRepository(baseDir string, gun data.GUN, baseURL string, return nil, err } - return NewNotaryRepository(baseDir, gun, baseURL, remoteStore, cache, trustPinning, cryptoService) + cl, err := changelist.NewFileChangelist(filepath.Join( + filepath.Join(baseDir, tufDir, filepath.FromSlash(gun.String()), "changelist"), + )) + if err != nil { + return nil, err + } + + return NewNotaryRepository(baseDir, gun, baseURL, remoteStore, cache, trustPinning, cryptoService, cl) } // NewNotaryRepository is the base method that returns a new notary repository. // It takes the base directory under where all the trust files will be stored // (This is normally defaults to "~/.notary" or "~/.docker/trust" when enabling // docker content trust). -// It expects initiated remote store and cache. +// It expects an initialized remote store and cache. func NewNotaryRepository(baseDir string, gun data.GUN, baseURL string, remoteStore store.RemoteStore, cache store.MetadataStore, - trustPinning trustpinning.TrustPinConfig, cryptoService signed.CryptoService) ( + trustPinning trustpinning.TrustPinConfig, cryptoService signed.CryptoService, cl changelist.Changelist) ( *NotaryRepository, error) { nRepo := &NotaryRepository{ gun: gun, - baseDir: baseDir, baseURL: baseURL, - tufRepoPath: filepath.Join(baseDir, tufDir, filepath.FromSlash(gun.String())), + baseDir: baseDir, + changelist: cl, cache: cache, remoteStore: remoteStore, CryptoService: cryptoService, @@ -300,8 +307,7 @@ func (r *NotaryRepository) initializeRoles(rootKeys []data.PublicKey, localRoles } // adds a TUF Change template to the given roles -func addChange(cl *changelist.FileChangelist, c changelist.Change, roles ...data.RoleName) error { - +func addChange(cl changelist.Changelist, c changelist.Change, roles ...data.RoleName) error { if len(roles) == 0 { roles = []data.RoleName{data.CanonicalTargetsRole} } @@ -342,11 +348,6 @@ func (r *NotaryRepository) AddTarget(target *Target, roles ...data.RoleName) err if len(target.Hashes) == 0 { return fmt.Errorf("no hashes specified for target \"%s\"", target.Name) } - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return err - } - defer cl.Close() logrus.Debugf("Adding target \"%s\" with sha256 \"%x\" and size %d bytes.\n", target.Name, target.Hashes["sha256"], target.Length) meta := data.FileMeta{Length: target.Length, Hashes: target.Hashes} @@ -358,22 +359,17 @@ func (r *NotaryRepository) AddTarget(target *Target, roles ...data.RoleName) err template := changelist.NewTUFChange( changelist.ActionCreate, "", changelist.TypeTargetsTarget, target.Name, metaJSON) - return addChange(cl, template, roles...) + return addChange(r.changelist, template, roles...) } // RemoveTarget creates new changelist entries to remove a target from the given // roles in the repository when the changelist gets applied at publish time. // If roles are unspecified, the default role is "target". func (r *NotaryRepository) RemoveTarget(targetName string, roles ...data.RoleName) error { - - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return err - } logrus.Debugf("Removing target \"%s\"", targetName) template := changelist.NewTUFChange(changelist.ActionDelete, "", changelist.TypeTargetsTarget, targetName, nil) - return addChange(cl, template, roles...) + return addChange(r.changelist, template, roles...) } // ListTargets lists all targets for the current repository. The list of @@ -531,13 +527,7 @@ func (r *NotaryRepository) GetAllTargetMetadataByName(name string) ([]TargetSign // GetChangelist returns the list of the repository's unpublished changes func (r *NotaryRepository) GetChangelist() (changelist.Changelist, error) { - changelistDir := filepath.Join(r.tufRepoPath, "changelist") - cl, err := changelist.NewFileChangelist(changelistDir) - if err != nil { - logrus.Debug("Error initializing changelist") - return nil, err - } - return cl, nil + return r.changelist, nil } // RoleWithSignatures is a Role with its associated signatures @@ -588,18 +578,14 @@ func (r *NotaryRepository) ListRoles() ([]RoleWithSignatures, error) { // Publish pushes the local changes in signed material to the remote notary-server // Conceptually it performs an operation similar to a `git rebase` func (r *NotaryRepository) Publish() error { - cl, err := r.GetChangelist() - if err != nil { - return err - } - if err = r.publish(cl); err != nil { + if err := r.publish(r.changelist); err != nil { return err } - if err = cl.Clear(""); err != nil { + if err := r.changelist.Clear(""); err != nil { // This is not a critical problem when only a single host is pushing // but will cause weird behaviour if changelist cleanup is failing // and there are multiple hosts writing to the repo. - logrus.Warn("Unable to clear changelist. You may want to manually delete the folder ", filepath.Join(r.tufRepoPath, "changelist")) + logrus.Warn("Unable to clear changelist. You may want to manually delete the folder ", r.changelist.Location()) } return nil } @@ -1132,14 +1118,19 @@ func (r *NotaryRepository) rootFileKeyChange(cl changelist.Changelist, role data // DeleteTrustData removes the trust data stored for this repo in the TUF cache on the client side // Note that we will not delete any private key material from local storage -func (r *NotaryRepository) DeleteTrustData(deleteRemote bool) error { +func DeleteTrustData(baseDir string, gun data.GUN, URL string, rt http.RoundTripper, deleteRemote bool) error { + localRepo := filepath.Join(baseDir, tufDir, filepath.FromSlash(gun.String())) // Remove the tufRepoPath directory, which includes local TUF metadata files and changelist information - if err := os.RemoveAll(r.tufRepoPath); err != nil { + if err := os.RemoveAll(localRepo); err != nil { return fmt.Errorf("error clearing TUF repo data: %v", err) } // Note that this will require admin permission in this NotaryRepository's roundtripper if deleteRemote { - remote := r.remoteStore + remote, err := getRemoteStore(URL, gun, rt) + if err != nil { + logrus.Error("unable to instantiate remote store") + return err + } if err := remote.RemoveAll(); err != nil { return err } diff --git a/client/client_test.go b/client/client_test.go index 69c83d375..125b41f34 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -837,13 +837,15 @@ func testAddTargetToSpecifiedInvalidRoles(t *testing.T, clearCache bool) { func testErrorWritingChangefiles(t *testing.T, writeChangeFile func(*NotaryRepository) error) { ts, _, _ := simpleTestServer(t) defer ts.Close() - - repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, false) + gun := "docker.com/notary" + repo, _ := initializeRepo(t, data.ECDSAKey, gun, ts.URL, false) defer os.RemoveAll(repo.baseDir) // first, make the actual changefile unwritable by making the changelist // directory unwritable - changelistPath := filepath.Join(repo.tufRepoPath, "changelist") + changelistPath := filepath.Join( + filepath.Join(repo.baseDir, tufDir, filepath.FromSlash(gun)), "changelist", + ) err := os.MkdirAll(changelistPath, 0744) require.NoError(t, err, "could not create changelist dir") err = os.Chmod(changelistPath, 0600) @@ -2463,7 +2465,12 @@ func TestPublishRemoveDelegationKeyFromDelegationRole(t *testing.T) { }) require.NoError(t, err) - cl, err := changelist.NewFileChangelist(filepath.Join(ownerRepo.tufRepoPath, "changelist")) + cl, err := changelist.NewFileChangelist( + filepath.Join( + filepath.Join(ownerRepo.baseDir, tufDir, filepath.FromSlash(gun)), + "changelist", + ), + ) require.NoError(t, err) require.NoError(t, cl.Add(changelist.NewTUFChange( changelist.ActionUpdate, @@ -3404,7 +3411,9 @@ func TestFullAddDelegationChangefileApplicable(t *testing.T) { }) require.NoError(t, err) change := newCreateDelegationChange(delegationName, tdJSON) - cl, err := changelist.NewFileChangelist(filepath.Join(repo.tufRepoPath, "changelist")) + cl, err := changelist.NewFileChangelist( + filepath.Join(repo.baseDir, tufDir, filepath.FromSlash(gun), "changelist"), + ) require.NoError(t, err) addChange(cl, change, delegationName) @@ -3455,7 +3464,9 @@ func TestFullRemoveDelegationChangefileApplicable(t *testing.T) { }) require.NoError(t, err) change := newUpdateDelegationChange(delegationName, tdJSON) - cl, err := changelist.NewFileChangelist(filepath.Join(repo.tufRepoPath, "changelist")) + cl, err := changelist.NewFileChangelist( + filepath.Join(repo.baseDir, tufDir, filepath.FromSlash(gun), "changelist"), + ) require.NoError(t, err) addChange(cl, change, delegationName) @@ -3586,12 +3597,12 @@ func testPublishTargetsDelegationCanUseUserKeyWithArbitraryRole(t *testing.T, x5 // TestDeleteRepo tests that local repo data is deleted from the client library call func TestDeleteRepo(t *testing.T) { - gun := "docker.com/notary" + var gun data.GUN = "docker.com/notary" ts, _, _ := simpleTestServer(t) defer ts.Close() - repo, rootKeyID := initializeRepo(t, data.ECDSAKey, gun, ts.URL, false) + repo, rootKeyID := initializeRepo(t, data.ECDSAKey, gun.String(), ts.URL, false) defer os.RemoveAll(repo.baseDir) // Assert initialization was successful before we delete @@ -3609,7 +3620,7 @@ func TestDeleteRepo(t *testing.T) { require.Len(t, cl.List(), 1) // Delete all local trust data for repo - err = repo.DeleteTrustData(false) + err = DeleteTrustData(repo.baseDir, gun, "", nil, false) require.NoError(t, err) // Assert no metadata for this repo exists locally @@ -3622,7 +3633,7 @@ func TestDeleteRepo(t *testing.T) { require.Len(t, cl.List(), 0) // Check that the tuf/ directory itself is gone - _, err = os.Stat(repo.tufRepoPath) + _, err = os.Stat(filepath.Join(repo.baseDir, tufDir, filepath.FromSlash(gun.String()))) require.Error(t, err) // Assert keys for this repo exist locally @@ -3631,13 +3642,13 @@ func TestDeleteRepo(t *testing.T) { // TestDeleteRemoteRepo tests that local and remote repo data is deleted from the client library call func TestDeleteRemoteRepo(t *testing.T) { - gun := "docker.com/notary" + var gun data.GUN = "docker.com/notary" ts := fullTestServer(t) defer ts.Close() // Create and publish a repo to delete - repo, rootKeyID := initializeRepo(t, data.ECDSAKey, gun, ts.URL, false) + repo, rootKeyID := initializeRepo(t, data.ECDSAKey, gun.String(), ts.URL, false) defer os.RemoveAll(repo.baseDir) require.NoError(t, repo.Publish()) @@ -3673,7 +3684,7 @@ func TestDeleteRemoteRepo(t *testing.T) { require.Len(t, repoCl.List(), 1) // Delete all local and remote trust data for one repo - err = repo.DeleteTrustData(true) + err = DeleteTrustData(repo.baseDir, gun, ts.URL, http.DefaultTransport, true) require.NoError(t, err) // Assert no metadata for that repo exists locally @@ -3686,7 +3697,7 @@ func TestDeleteRemoteRepo(t *testing.T) { require.Len(t, repoCl.List(), 0) // Check that the tuf/ directory itself is gone - _, err = os.Stat(repo.tufRepoPath) + _, err = os.Stat(filepath.Join(repo.baseDir, tufDir, filepath.FromSlash(gun.String()))) require.Error(t, err) // Assert keys for this repo still exist locally diff --git a/client/delegations.go b/client/delegations.go index 605af93e3..d32c558fc 100644 --- a/client/delegations.go +++ b/client/delegations.go @@ -3,7 +3,6 @@ package client import ( "encoding/json" "fmt" - "path/filepath" "github.com/Sirupsen/logrus" "github.com/docker/notary" @@ -40,12 +39,6 @@ func (r *NotaryRepository) AddDelegationRoleAndKeys(name data.RoleName, delegati return data.ErrInvalidRole{Role: name, Reason: "invalid delegation role name"} } - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return err - } - defer cl.Close() - logrus.Debugf(`Adding delegation "%s" with threshold %d, and %d keys\n`, name, notary.MinThreshold, len(delegationKeys)) @@ -59,7 +52,7 @@ func (r *NotaryRepository) AddDelegationRoleAndKeys(name data.RoleName, delegati } template := newCreateDelegationChange(name, tdJSON) - return addChange(cl, template, name) + return addChange(r.changelist, template, name) } // AddDelegationPaths creates a changelist entry to add provided paths to an existing delegation. @@ -70,12 +63,6 @@ func (r *NotaryRepository) AddDelegationPaths(name data.RoleName, paths []string return data.ErrInvalidRole{Role: name, Reason: "invalid delegation role name"} } - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return err - } - defer cl.Close() - logrus.Debugf(`Adding %s paths to delegation %s\n`, paths, name) tdJSON, err := json.Marshal(&changelist.TUFDelegation{ @@ -86,7 +73,7 @@ func (r *NotaryRepository) AddDelegationPaths(name data.RoleName, paths []string } template := newCreateDelegationChange(name, tdJSON) - return addChange(cl, template, name) + return addChange(r.changelist, template, name) } // RemoveDelegationKeysAndPaths creates changelist entries to remove provided delegation key IDs and paths. @@ -114,16 +101,10 @@ func (r *NotaryRepository) RemoveDelegationRole(name data.RoleName) error { return data.ErrInvalidRole{Role: name, Reason: "invalid delegation role name"} } - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return err - } - defer cl.Close() - logrus.Debugf(`Removing delegation "%s"\n`, name) template := newDeleteDelegationChange(name, nil) - return addChange(cl, template, name) + return addChange(r.changelist, template, name) } // RemoveDelegationPaths creates a changelist entry to remove provided paths from an existing delegation. @@ -133,12 +114,6 @@ func (r *NotaryRepository) RemoveDelegationPaths(name data.RoleName, paths []str return data.ErrInvalidRole{Role: name, Reason: "invalid delegation role name"} } - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return err - } - defer cl.Close() - logrus.Debugf(`Removing %s paths from delegation "%s"\n`, paths, name) tdJSON, err := json.Marshal(&changelist.TUFDelegation{ @@ -149,7 +124,7 @@ func (r *NotaryRepository) RemoveDelegationPaths(name data.RoleName, paths []str } template := newUpdateDelegationChange(name, tdJSON) - return addChange(cl, template, name) + return addChange(r.changelist, template, name) } // RemoveDelegationKeys creates a changelist entry to remove provided keys from an existing delegation. @@ -163,12 +138,6 @@ func (r *NotaryRepository) RemoveDelegationKeys(name data.RoleName, keyIDs []str return data.ErrInvalidRole{Role: name, Reason: "invalid delegation role name"} } - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return err - } - defer cl.Close() - logrus.Debugf(`Removing %s keys from delegation "%s"\n`, keyIDs, name) tdJSON, err := json.Marshal(&changelist.TUFDelegation{ @@ -179,7 +148,7 @@ func (r *NotaryRepository) RemoveDelegationKeys(name data.RoleName, keyIDs []str } template := newUpdateDelegationChange(name, tdJSON) - return addChange(cl, template, name) + return addChange(r.changelist, template, name) } // ClearDelegationPaths creates a changelist entry to remove all paths from an existing delegation. @@ -189,12 +158,6 @@ func (r *NotaryRepository) ClearDelegationPaths(name data.RoleName) error { return data.ErrInvalidRole{Role: name, Reason: "invalid delegation role name"} } - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return err - } - defer cl.Close() - logrus.Debugf(`Removing all paths from delegation "%s"\n`, name) tdJSON, err := json.Marshal(&changelist.TUFDelegation{ @@ -205,7 +168,7 @@ func (r *NotaryRepository) ClearDelegationPaths(name data.RoleName) error { } template := newUpdateDelegationChange(name, tdJSON) - return addChange(cl, template, name) + return addChange(r.changelist, template, name) } func newUpdateDelegationChange(name data.RoleName, content []byte) *changelist.TUFChange { diff --git a/client/witness.go b/client/witness.go index 636d4f01f..72aed031c 100644 --- a/client/witness.go +++ b/client/witness.go @@ -1,8 +1,6 @@ package client import ( - "path/filepath" - "github.com/docker/notary/client/changelist" "github.com/docker/notary/tuf" "github.com/docker/notary/tuf/data" @@ -11,12 +9,7 @@ import ( // Witness creates change objects to witness (i.e. re-sign) the given // roles on the next publish. One change is created per role func (r *NotaryRepository) Witness(roles ...data.RoleName) ([]data.RoleName, error) { - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { - return nil, err - } - defer cl.Close() - + var err error successful := make([]data.RoleName, 0, len(roles)) for _, role := range roles { // scope is role @@ -27,7 +20,7 @@ func (r *NotaryRepository) Witness(roles ...data.RoleName) ([]data.RoleName, err "", nil, ) - err = cl.Add(c) + err = r.changelist.Add(c) if err != nil { break } diff --git a/cmd/notary/tuf.go b/cmd/notary/tuf.go index 54bb06cab..c80509a71 100644 --- a/cmd/notary/tuf.go +++ b/cmd/notary/tuf.go @@ -359,11 +359,6 @@ func (t *tufCommander) tufDeleteGUN(cmd *cobra.Command, args []string) error { gun := data.GUN(args[0]) - trustPin, err := getTrustPinning(config) - if err != nil { - return err - } - // Only initialize a roundtripper if we get the remote flag var rt http.RoundTripper var remoteDeleteInfo string @@ -375,16 +370,15 @@ func (t *tufCommander) tufDeleteGUN(cmd *cobra.Command, args []string) error { remoteDeleteInfo = " and remote" } - nRepo, err := notaryclient.NewFileCachedNotaryRepository( - config.GetString("trust_dir"), gun, getRemoteTrustServer(config), rt, t.retriever, trustPin) - - if err != nil { - return err - } - cmd.Printf("Deleting trust data for repository %s\n", gun) - if err := nRepo.DeleteTrustData(t.deleteRemote); err != nil { + if err := notaryclient.DeleteTrustData( + config.GetString("trust_dir"), + gun, + getRemoteTrustServer(config), + rt, + t.deleteRemote, + ); err != nil { return err } cmd.Printf("Successfully deleted local%s trust data for repository %s\n", remoteDeleteInfo, gun) diff --git a/cmd/notary/tuf_test.go b/cmd/notary/tuf_test.go index 6e32051c3..90c7c9a00 100644 --- a/cmd/notary/tuf_test.go +++ b/cmd/notary/tuf_test.go @@ -206,7 +206,6 @@ func TestGetTrustPinningErrors(t *testing.T) { require.Error(t, tc.tufStatus(&cobra.Command{}, []string{"gun"})) tc.resetAll = true require.Error(t, tc.tufReset(&cobra.Command{}, []string{"gun"})) - require.Error(t, tc.tufDeleteGUN(&cobra.Command{}, []string{"gun"})) require.Error(t, tc.tufInit(&cobra.Command{}, []string{"gun"})) require.Error(t, tc.tufPublish(&cobra.Command{}, []string{"gun"})) require.Error(t, tc.tufVerify(&cobra.Command{}, []string{"gun", "target", "file"})) From 1070d54a0a0855b9d7545e3c8bd32167c98ac8e2 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Sun, 19 Feb 2017 10:49:43 -0800 Subject: [PATCH 05/13] fixing an ineffassign error Signed-off-by: David Lawrence (github: endophage) --- client/helpers_test.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/client/helpers_test.go b/client/helpers_test.go index f8730f2e7..42d30d856 100644 --- a/client/helpers_test.go +++ b/client/helpers_test.go @@ -10,6 +10,7 @@ import ( log "github.com/Sirupsen/logrus" "github.com/docker/notary/client/changelist" + "github.com/docker/notary/storage" "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/testutils" "github.com/stretchr/testify/require" @@ -1023,14 +1024,19 @@ func TestAllNotNearExpiry(t *testing.T) { } func TestRotateRemoteKeyOffline(t *testing.T) { + // http store requires an absolute baseURL + _, err := getRemoteStore("invalidURL", "gun", nil) + require.Error(t, err) + // without a valid roundtripper, rotation should fail since we cannot initialize a HTTPStore - remote, err := getRemoteStore("invalidURL", "gun", nil) + var remote storage.RemoteStore = storage.OfflineStore{} key, err := rotateRemoteKey(data.CanonicalSnapshotRole, remote) require.Error(t, err) require.Nil(t, key) // if the underlying remote store is faulty and cannot rotate keys, we should get back the error remote, err = getRemoteStore("https://notary-server", "gun", http.DefaultTransport) + require.NoError(t, err) key, err = rotateRemoteKey(data.CanonicalSnapshotRole, remote) require.Error(t, err) require.Nil(t, key) From e8d8eaea99480d9faa6018838b250809dc91ce09 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Fri, 24 Feb 2017 16:31:37 +0100 Subject: [PATCH 06/13] Minor fixes from reviews Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/client/client.go b/client/client.go index 51e440473..5fdd8a182 100644 --- a/client/client.go +++ b/client/client.go @@ -10,6 +10,7 @@ import ( "path/filepath" "regexp" "time" + "os" "github.com/Sirupsen/logrus" "github.com/docker/notary" @@ -21,7 +22,6 @@ import ( "github.com/docker/notary/tuf/data" "github.com/docker/notary/tuf/signed" "github.com/docker/notary/tuf/utils" - "os" ) const ( @@ -670,9 +670,7 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error { return err } - remote := r.remoteStore - - return remote.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles)) + return r.remoteStore.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles)) } func signRootIfNecessary(updates map[data.RoleName][]byte, repo *tuf.Repo, extraSigningKeys data.KeyList, initialPublish bool) error { From 144a9ad695257e9cc3cd257738348aad97be0038 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Sat, 25 Feb 2017 03:01:25 +0100 Subject: [PATCH 07/13] Add a getter for RemoteStore of a repo returning a valid remote store or an OfflineStore Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 43 +++++++++++++++++++++++++++++++++++++++---- client/client_test.go | 6 ++++-- client/helpers.go | 1 + 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/client/client.go b/client/client.go index 5fdd8a182..774dab994 100644 --- a/client/client.go +++ b/client/client.go @@ -99,6 +99,11 @@ func NewNotaryRepository(baseDir string, gun data.GUN, baseURL string, remoteSto trustPinning trustpinning.TrustPinConfig, cryptoService signed.CryptoService, cl changelist.Changelist) ( *NotaryRepository, error) { + // Repo's remote store is either a valid remote store or an OfflineStore + if remoteStore == nil { + remoteStore = store.OfflineStore{} + } + nRepo := &NotaryRepository{ gun: gun, baseURL: baseURL, @@ -279,10 +284,15 @@ func (r *NotaryRepository) initializeRoles(rootKeys []data.PublicKey, localRoles ) } } + + remote, err := r.GetRemoteStore() + if err != nil { + logrus.Errorf("could not retrieve the remote store: %v", err) + } for _, role := range remoteRoles { // This key is generated by the remote server. var key data.PublicKey - key, err = getRemoteKey(role, r.remoteStore) + key, err = getRemoteKey(role, remote) if err != nil { return } @@ -530,6 +540,18 @@ func (r *NotaryRepository) GetChangelist() (changelist.Changelist, error) { return r.changelist, nil } +// GetRemoteStore returns the remoteStore of a repository if valid or +// or an OfflineStore otherwise +func (r *NotaryRepository) GetRemoteStore() (store.RemoteStore, error) { + if r.remoteStore != nil { + return r.remoteStore, nil + } + + r.remoteStore = &store.OfflineStore{} + + return r.remoteStore, nil +} + // RoleWithSignatures is a Role with its associated signatures type RoleWithSignatures struct { Signatures []data.Signature @@ -670,7 +692,12 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error { return err } - return r.remoteStore.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles)) + remote, err := r.GetRemoteStore() + if err != nil { + return err + } + + return remote.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles)) } func signRootIfNecessary(updates map[data.RoleName][]byte, repo *tuf.Repo, extraSigningKeys data.KeyList, initialPublish bool) error { @@ -949,7 +976,11 @@ func (r *NotaryRepository) bootstrapClient(checkInitialized bool) (*TUFClient, e } } - remote := r.remoteStore + remote, err := r.GetRemoteStore() + if err != nil { + logrus.Error(err) + } + if !newBuilder.IsLoaded(data.CanonicalRootRole) || checkInitialized { // remoteErr was nil and we were not able to load a root from cache or // are specifically checking for initialization of the repo. @@ -1014,7 +1045,11 @@ func (r *NotaryRepository) pubKeyListForRotation(role data.RoleName, serverManag // If server manages the key being rotated, request a rotation and return the new key if serverManaged { - pubKey, err = rotateRemoteKey(role, r.remoteStore) + remote, err := r.GetRemoteStore() + if err != nil { + return nil, fmt.Errorf("unable to get remote store: %s", err) + } + pubKey, err = rotateRemoteKey(role, remote) pubKeyList = make(data.KeyList, 0, 1) pubKeyList = append(pubKeyList, pubKey) if err != nil { diff --git a/client/client_test.go b/client/client_test.go index 125b41f34..7538d2955 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -3704,7 +3704,8 @@ func TestDeleteRemoteRepo(t *testing.T) { requireRepoHasExpectedKeys(t, repo, rootKeyID, true) // Try connecting to the remote store directly and make sure that no metadata exists for this gun - remoteStore := repo.remoteStore + remoteStore, err := repo.GetRemoteStore() + require.NoError(t, err) meta, err := remoteStore.GetSized(data.CanonicalRootRole.String(), store.NoSizeLimit) require.Error(t, err) require.IsType(t, store.ErrMetaNotFound{}, err) @@ -3729,7 +3730,8 @@ func TestDeleteRemoteRepo(t *testing.T) { require.Len(t, longLivingCl.List(), 1) // Check that the other repo's remote data is unaffected - remoteStore = longLivingRepo.remoteStore + remoteStore, err = longLivingRepo.GetRemoteStore() + require.NoError(t, err) meta, err = remoteStore.GetSized(data.CanonicalRootRole.String(), store.NoSizeLimit) require.NoError(t, err) require.NotNil(t, meta) diff --git a/client/helpers.go b/client/helpers.go index de55533d6..e7e9c1141 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -24,6 +24,7 @@ func getRemoteStore(baseURL string, gun data.GUN, rt http.RoundTripper) (store.R rt, ) if err != nil { + logrus.Error(err) return store.OfflineStore{}, err } return s, err From 1d03a508613b7ce99834f3b74d0006c21fcbe72c Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Sun, 26 Feb 2017 00:14:01 +0100 Subject: [PATCH 08/13] Minor gofmt fix Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/client.go b/client/client.go index 774dab994..43929e308 100644 --- a/client/client.go +++ b/client/client.go @@ -7,10 +7,10 @@ import ( "io/ioutil" "net/http" "net/url" + "os" "path/filepath" "regexp" "time" - "os" "github.com/Sirupsen/logrus" "github.com/docker/notary" From 9b5aeb765ae3420c881b7d8fb6c428836001b3f9 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Tue, 28 Feb 2017 01:58:16 +0100 Subject: [PATCH 09/13] Remove unncesserary error return value from repo's remoteStore getter Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 27 ++++++++------------------- client/client_test.go | 8 ++++---- 2 files changed, 12 insertions(+), 23 deletions(-) diff --git a/client/client.go b/client/client.go index 43929e308..28779f5f1 100644 --- a/client/client.go +++ b/client/client.go @@ -285,10 +285,8 @@ func (r *NotaryRepository) initializeRoles(rootKeys []data.PublicKey, localRoles } } - remote, err := r.GetRemoteStore() - if err != nil { - logrus.Errorf("could not retrieve the remote store: %v", err) - } + remote := r.GetRemoteStore() + for _, role := range remoteRoles { // This key is generated by the remote server. var key data.PublicKey @@ -542,14 +540,14 @@ func (r *NotaryRepository) GetChangelist() (changelist.Changelist, error) { // GetRemoteStore returns the remoteStore of a repository if valid or // or an OfflineStore otherwise -func (r *NotaryRepository) GetRemoteStore() (store.RemoteStore, error) { +func (r *NotaryRepository) GetRemoteStore() store.RemoteStore { if r.remoteStore != nil { - return r.remoteStore, nil + return r.remoteStore } r.remoteStore = &store.OfflineStore{} - return r.remoteStore, nil + return r.remoteStore } // RoleWithSignatures is a Role with its associated signatures @@ -692,10 +690,7 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error { return err } - remote, err := r.GetRemoteStore() - if err != nil { - return err - } + remote := r.GetRemoteStore() return remote.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles)) } @@ -976,10 +971,7 @@ func (r *NotaryRepository) bootstrapClient(checkInitialized bool) (*TUFClient, e } } - remote, err := r.GetRemoteStore() - if err != nil { - logrus.Error(err) - } + remote := r.GetRemoteStore() if !newBuilder.IsLoaded(data.CanonicalRootRole) || checkInitialized { // remoteErr was nil and we were not able to load a root from cache or @@ -1045,10 +1037,7 @@ func (r *NotaryRepository) pubKeyListForRotation(role data.RoleName, serverManag // If server manages the key being rotated, request a rotation and return the new key if serverManaged { - remote, err := r.GetRemoteStore() - if err != nil { - return nil, fmt.Errorf("unable to get remote store: %s", err) - } + remote := r.GetRemoteStore() pubKey, err = rotateRemoteKey(role, remote) pubKeyList = make(data.KeyList, 0, 1) pubKeyList = append(pubKeyList, pubKey) diff --git a/client/client_test.go b/client/client_test.go index 7538d2955..68faf1da4 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -3704,8 +3704,8 @@ func TestDeleteRemoteRepo(t *testing.T) { requireRepoHasExpectedKeys(t, repo, rootKeyID, true) // Try connecting to the remote store directly and make sure that no metadata exists for this gun - remoteStore, err := repo.GetRemoteStore() - require.NoError(t, err) + remoteStore := repo.GetRemoteStore() + require.NotNil(t, remoteStore) meta, err := remoteStore.GetSized(data.CanonicalRootRole.String(), store.NoSizeLimit) require.Error(t, err) require.IsType(t, store.ErrMetaNotFound{}, err) @@ -3730,8 +3730,8 @@ func TestDeleteRemoteRepo(t *testing.T) { require.Len(t, longLivingCl.List(), 1) // Check that the other repo's remote data is unaffected - remoteStore, err = longLivingRepo.GetRemoteStore() - require.NoError(t, err) + remoteStore = longLivingRepo.GetRemoteStore() + require.NotNil(t, remoteStore) meta, err = remoteStore.GetSized(data.CanonicalRootRole.String(), store.NoSizeLimit) require.NoError(t, err) require.NotNil(t, meta) From 10577ed32a61790db076280ba3407c10c460181e Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Tue, 28 Feb 2017 02:03:34 +0100 Subject: [PATCH 10/13] Let getRemoteStore helper function caller decide wether to log and exit on error or not Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 2 +- client/helpers.go | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/client/client.go b/client/client.go index 28779f5f1..661b078f2 100644 --- a/client/client.go +++ b/client/client.go @@ -77,7 +77,7 @@ func NewFileCachedNotaryRepository(baseDir string, gun data.GUN, baseURL string, remoteStore, err := getRemoteStore(baseURL, gun, rt) if err != nil { - return nil, err + logrus.Error(err) } cl, err := changelist.NewFileChangelist(filepath.Join( diff --git a/client/helpers.go b/client/helpers.go index e7e9c1141..de55533d6 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -24,7 +24,6 @@ func getRemoteStore(baseURL string, gun data.GUN, rt http.RoundTripper) (store.R rt, ) if err != nil { - logrus.Error(err) return store.OfflineStore{}, err } return s, err From 1afae72e401a647e5a3fc2633b24f1de07a543a9 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Tue, 28 Feb 2017 02:17:09 +0100 Subject: [PATCH 11/13] Remove export of repo's remoteStore getter Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 12 ++++++------ client/client_test.go | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/client/client.go b/client/client.go index 661b078f2..8be53412e 100644 --- a/client/client.go +++ b/client/client.go @@ -285,7 +285,7 @@ func (r *NotaryRepository) initializeRoles(rootKeys []data.PublicKey, localRoles } } - remote := r.GetRemoteStore() + remote := r.getRemoteStore() for _, role := range remoteRoles { // This key is generated by the remote server. @@ -538,9 +538,9 @@ func (r *NotaryRepository) GetChangelist() (changelist.Changelist, error) { return r.changelist, nil } -// GetRemoteStore returns the remoteStore of a repository if valid or +// getRemoteStore returns the remoteStore of a repository if valid or // or an OfflineStore otherwise -func (r *NotaryRepository) GetRemoteStore() store.RemoteStore { +func (r *NotaryRepository) getRemoteStore() store.RemoteStore { if r.remoteStore != nil { return r.remoteStore } @@ -690,7 +690,7 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error { return err } - remote := r.GetRemoteStore() + remote := r.getRemoteStore() return remote.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles)) } @@ -971,7 +971,7 @@ func (r *NotaryRepository) bootstrapClient(checkInitialized bool) (*TUFClient, e } } - remote := r.GetRemoteStore() + remote := r.getRemoteStore() if !newBuilder.IsLoaded(data.CanonicalRootRole) || checkInitialized { // remoteErr was nil and we were not able to load a root from cache or @@ -1037,7 +1037,7 @@ func (r *NotaryRepository) pubKeyListForRotation(role data.RoleName, serverManag // If server manages the key being rotated, request a rotation and return the new key if serverManaged { - remote := r.GetRemoteStore() + remote := r.getRemoteStore() pubKey, err = rotateRemoteKey(role, remote) pubKeyList = make(data.KeyList, 0, 1) pubKeyList = append(pubKeyList, pubKey) diff --git a/client/client_test.go b/client/client_test.go index 68faf1da4..557639070 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -3704,7 +3704,7 @@ func TestDeleteRemoteRepo(t *testing.T) { requireRepoHasExpectedKeys(t, repo, rootKeyID, true) // Try connecting to the remote store directly and make sure that no metadata exists for this gun - remoteStore := repo.GetRemoteStore() + remoteStore := repo.getRemoteStore() require.NotNil(t, remoteStore) meta, err := remoteStore.GetSized(data.CanonicalRootRole.String(), store.NoSizeLimit) require.Error(t, err) @@ -3730,7 +3730,7 @@ func TestDeleteRemoteRepo(t *testing.T) { require.Len(t, longLivingCl.List(), 1) // Check that the other repo's remote data is unaffected - remoteStore = longLivingRepo.GetRemoteStore() + remoteStore = longLivingRepo.getRemoteStore() require.NotNil(t, remoteStore) meta, err = remoteStore.GetSized(data.CanonicalRootRole.String(), store.NoSizeLimit) require.NoError(t, err) From 0b3f3cb67f292588fff066cbe4dd0bded59c79b5 Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Tue, 28 Feb 2017 21:28:02 +0100 Subject: [PATCH 12/13] Error on invalid baseURL for a remote store Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 4 +++- client/helpers.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/client/client.go b/client/client.go index 8be53412e..596c739cc 100644 --- a/client/client.go +++ b/client/client.go @@ -76,8 +76,10 @@ func NewFileCachedNotaryRepository(baseDir string, gun data.GUN, baseURL string, cryptoService := cryptoservice.NewCryptoService(keyStores...) remoteStore, err := getRemoteStore(baseURL, gun, rt) + // baseURL is syntactically invalid if err != nil { logrus.Error(err) + return nil, err } cl, err := changelist.NewFileChangelist(filepath.Join( @@ -1150,7 +1152,7 @@ func DeleteTrustData(baseDir string, gun data.GUN, URL string, rt http.RoundTrip if deleteRemote { remote, err := getRemoteStore(URL, gun, rt) if err != nil { - logrus.Error("unable to instantiate remote store") + logrus.Error("unable to instantiate a remote store: %v", err) return err } if err := remote.RemoveAll(); err != nil { diff --git a/client/helpers.go b/client/helpers.go index de55533d6..5ec0384e2 100644 --- a/client/helpers.go +++ b/client/helpers.go @@ -26,7 +26,7 @@ func getRemoteStore(baseURL string, gun data.GUN, rt http.RoundTripper) (store.R if err != nil { return store.OfflineStore{}, err } - return s, err + return s, nil } func applyChangelist(repo *tuf.Repo, invalid *tuf.Repo, cl changelist.Changelist) error { From b70d671f6671763e39514bfdf050582c1cece99f Mon Sep 17 00:00:00 2001 From: Nassim 'Nass' Eddequiouaq Date: Wed, 1 Mar 2017 04:23:53 +0100 Subject: [PATCH 13/13] Remove unnecessary error logging on invalid repo baseURL Signed-off-by: Nassim 'Nass' Eddequiouaq --- client/client.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/client/client.go b/client/client.go index 596c739cc..2a27e32a0 100644 --- a/client/client.go +++ b/client/client.go @@ -76,9 +76,8 @@ func NewFileCachedNotaryRepository(baseDir string, gun data.GUN, baseURL string, cryptoService := cryptoservice.NewCryptoService(keyStores...) remoteStore, err := getRemoteStore(baseURL, gun, rt) - // baseURL is syntactically invalid if err != nil { - logrus.Error(err) + // baseURL is syntactically invalid return nil, err }