Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add dependency injection of remote store for NotaryRepository initial… #1094

Merged
merged 13 commits into from
Mar 1, 2017
Merged

Add dependency injection of remote store for NotaryRepository initial… #1094

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
ddfe727
Add dependency injection of remote store for NotaryRepository initial…
n4ss Feb 9, 2017
e4cacd0
Remove helper and test for changing remote store base URL once instan…
n4ss Feb 10, 2017
ae43610
Remove the RoundTripper object out of NotaryRepository
n4ss Feb 13, 2017
7d8ec9f
making changelist injectable
Feb 19, 2017
1070d54
fixing an ineffassign error
Feb 19, 2017
e8d8eae
Minor fixes from reviews
n4ss Feb 24, 2017
144a9ad
Add a getter for RemoteStore of a repo returning a valid remote store…
n4ss Feb 25, 2017
1d03a50
Minor gofmt fix
n4ss Feb 25, 2017
9b5aeb7
Remove unncesserary error return value from repo's remoteStore getter
n4ss Feb 28, 2017
10577ed
Let getRemoteStore helper function caller decide wether to log and ex…
n4ss Feb 28, 2017
1afae72
Remove export of repo's remoteStore getter
n4ss Feb 28, 2017
0b3f3cb
Error on invalid baseURL for a remote store
n4ss Feb 28, 2017
b70d671
Remove unnecessary error logging on invalid repo baseURL
n4ss Mar 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions client/changelist/changelist.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand Down
5 changes: 5 additions & 0 deletions client/changelist/file_changelist.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
3 changes: 3 additions & 0 deletions client/changelist/interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
141 changes: 73 additions & 68 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder how this passed the gofmt before

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trustmanager was in use at line 92 prior to this PR.

"github.com/docker/notary/tuf"
"github.com/docker/notary/tuf/data"
Expand All @@ -42,8 +41,9 @@ type NotaryRepository struct {
baseDir string
gun data.GUN
baseURL string
tufRepoPath string
changelist changelist.Changelist
cache store.MetadataStore
remoteStore store.RemoteStore
CryptoService signed.CryptoService
tufRepo *tuf.Repo
invalid *tuf.Repo // known data that was parsable but deemed invalid
Expand All @@ -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) {
Expand All @@ -65,47 +67,57 @@ 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...)

remoteStore, err := getRemoteStore(baseURL, gun, rt)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we still seem to be using this initializer in the CLI for both reading notary data and publishing/initializing notary data, would it make sense to log this error instead? That way later the actual NotaryRepository object itself can decide whether to fail if it's offline (in the case of publishing and initializing) or continue reading from cache?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes sense to me, will fix!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually added the error logging directly inside getRemoteStore without being too sure about this decision.. Should we let caller decide on wether to logging instead? It seems more reasonable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let the caller decide on logging.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(but in this specific case, always log the error)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cyli actually we should probably log when we get an OfflineStore only from getRemoteStore I think. If we get an err from getRemoteStore and hence from NewHTTPStore, it means that baseURL was incorrect, not that it was unreachable.

Copy link
Contributor

@cyli cyli Feb 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From your comment in slack:

We have the following behavior for NewHTTPStore:

  • if we have an invalid url from baseURL+gun then we get a nil store and a non-nil err -> this should error in callers, not switched to an OfflineStore

That makes sense to change this (from master, it seem sot already do so in this PR) to error on actual invalid values.

  • if we have an invalid http.roundtrip, we get an OfflineStore and a nil err -> this should be logged in callers

If we were looking just at NewHTTPStore, I'm wondering if we should actually convert a nil http.RoundTrip to a http.DefaultTransport, or count it as invalid input and return an error (I don't feel strongly either way).

But in our usage of this function, we seem to set the roundtrip to nil when doing offline operations, namely updating the changelist, so we don't want it to error or probably care about it logging. I'd argue that eventually we should probably just directly call NewNotaryRepository instead with a nil remote store or something. I think it doesn't even need the cache, or trust pinning, or a cryptoservice, so those could probably also all be nil.

In the meantime (unless we wanted to make that change here), since our usage is specifically "a nil http.Roundtrip means this is intended to be an offline operation", maybe we should explicitly add a parameter to denote that, or just ignore invalid roundtrips?

  • we have a valid HTTPStore -> all good

We don't check if the remote is actually reachable

👍

// baseURL is syntactically invalid
if err != nil {
logrus.Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: I'm not sure we have to log as well as returning the error.

Also it looks like in the CLI, tuf.go's tokenAuth function also parses the URL and checks for validity, which is probably a better validity check than NewHTTPStore's, since it looks like a url like http:asdga gets through the url.Parse + checking for the existence of a scheme?:

$  bin/notary -D -s https:asdgadg list repo
DEBU[0000] Configuration file not found, using defaults 
DEBU[0000] Using the following trust directory: /Users/cyli/.notary 
ERRO[0000] could not reach https:asdgadg: Get https:///v2/: http: no Host in request URL 
INFO[0000] continuing in offline mode                   
DEBU[0000] No yubikey found, using alternative key storage: loaded library /usr/local/lib/libykcs11.dylib, but no HSM slots found 
DEBU[0000] Making dir path: /Users/cyli/.notary/tuf/repo/changelist 

* fatal: client is offline

We can probably fix that in a later PR, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just fixed it right now in a last small commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks! That was my only comment! This otherwise looks good to merge on green!

return nil, err
}

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)
}

// 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) {
// 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
Copy link
Contributor

@cyli cyli Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: Perhaps these should be listed as examples rather than normal defaults, since in this particular function there is no default for the baseDir (these are just defaults for the CLI tool that uses it and in docker, which uses it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Apologies, I know this wasn't your change, I just hadn't noticed it before)

// docker content trust).
// 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, cl changelist.Changelist) (
*NotaryRepository, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: this signature is kind of hard to read - not sure if you chose it or if gofmt imposed it to be this way. Maybe we could move the return values up a line such that we have 2 lines instead of 3?


cryptoService := cryptoservice.NewCryptoService(keyStores...)
// Repo's remote store is either a valid remote store or an OfflineStore
if remoteStore == nil {
remoteStore = store.OfflineStore{}
}

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,
roundTrip: rt,
trustPinning: trustPin,
trustPinning: trustPinning,
LegacyVersions: 0, // By default, don't sign with legacy roles
}

nRepo.cache = cache

return nRepo, nil
}

Expand Down Expand Up @@ -274,10 +286,13 @@ func (r *NotaryRepository) initializeRoles(rootKeys []data.PublicKey, localRoles
)
}
}

remote := r.getRemoteStore()

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, remote)
if err != nil {
return
}
Expand All @@ -302,8 +317,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}
}
Expand Down Expand Up @@ -344,11 +358,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}
Expand All @@ -360,22 +369,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
Expand Down Expand Up @@ -533,13 +537,19 @@ 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 r.changelist, nil
}

// getRemoteStore returns the remoteStore of a repository if valid or
// or an OfflineStore otherwise
func (r *NotaryRepository) getRemoteStore() store.RemoteStore {
if r.remoteStore != nil {
return r.remoteStore
}
return cl, nil

r.remoteStore = &store.OfflineStore{}

return r.remoteStore
}

// RoleWithSignatures is a Role with its associated signatures
Expand Down Expand Up @@ -590,18 +600,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 {
if err := r.publish(r.changelist); err != nil {
return err
}
if err = r.publish(cl); 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
}
Expand Down Expand Up @@ -686,10 +692,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.getRemoteStore()

return remote.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles))
Copy link
Contributor

@riyazdf riyazdf Feb 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: return r.remoteStore.SetMulti(data.MetadataRoleMapToStringMap(updatedFiles))

(no need for the extra remote var)

}
Expand Down Expand Up @@ -970,10 +973,9 @@ 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.getRemoteStore()

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.

Expand Down Expand Up @@ -1037,7 +1039,8 @@ 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)
remote := r.getRemoteStore()
pubKey, err = rotateRemoteKey(role, remote)
pubKeyList = make(data.KeyList, 0, 1)
pubKeyList = append(pubKeyList, pubKey)
if err != nil {
Expand All @@ -1062,7 +1065,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)
}
Expand Down Expand Up @@ -1139,15 +1142,17 @@ 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, err := getRemoteStore(r.baseURL, r.gun, r.roundTrip)
remote, err := getRemoteStore(URL, gun, rt)
if err != nil {
logrus.Error("unable to instantiate a remote store: %v", err)
return err
}
if err := remote.RemoveAll(); err != nil {
Expand Down
Loading