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.

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

Notary repository lazy initialization #1105

Merged
merged 19 commits into from Mar 8, 2017

Conversation

@n4ss
Copy link
Contributor

commented Feb 24, 2017

This work depends on the work done in #1094, allowing us to inject a remote store and a cache in a NotaryRepository object.
Since this PR is stacked on top of #1094 which just got merged, make sure you pull the latests changes.

We are now initializing the repository when publishing, if needed. We do this for three reasons:

  1. reduce the amount of operations necessary for notary users to setup their repo
  2. avoid metadata corruption when calling init several times on the same repo - #1071
  3. give more flexibility on how to initialize

The lazy initialization is done either from scratch by NotaryRepository.Initialize() when starting from a blank state or by NotaryRepository.bootstrapRepo() if metadata is found on disk (example: when someone does a notary key rotate as a first operation).

Some updates have been added to the way change lists were handled, we can now inject one in aNotaryRepository object.

@n4ss n4ss force-pushed the n4ss:repo-lazy-init branch from b174084 to 348cde4 Mar 2, 2017

@n4ss n4ss changed the title WIP: Notary repository lazy initialization - depends on https://github.com/docker/notary/pull/1094 Notary repository lazy initialization Mar 2, 2017

@n4ss n4ss requested review from cyli, endophage and riyazdf Mar 2, 2017

@n4ss n4ss force-pushed the n4ss:repo-lazy-init branch from 9fa5c09 to 7512f85 Mar 2, 2017

@GordonTheTurtle GordonTheTurtle removed the dco/no label Mar 2, 2017

n4ss and others added 12 commits Feb 14, 2017
Add lazy initialization of a repository at publish time
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Add lazy initialization of a repository at publish time
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Syntax fix by gofmt
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
lots of failing tests
Signed-off-by: David Lawrence <david.lawrence@docker.com> (github: endophage)
TestNotInitializedOnPublish doesn't work with repo lazy init at publi…
…sh time

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Modify lazy init logic to bootstrap if metadata on disk
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Fix deprecated test accordingly to the new logic
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Fix rootkey IDs list creation causing faulty access at repo init
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Fix mishandled conflicts with types update PR
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Update deprecated tests for repo lazy init behavior
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Move the decision to init repo based on cached content to its own method
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Move private keys retrieval from a crypto service to a helper function
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

@n4ss n4ss force-pushed the n4ss:repo-lazy-init branch from 7512f85 to 6a1f1c1 Mar 2, 2017

requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, true)
}

// Tnitializing a repo and republishing after should succeed

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 2, 2017

Contributor

nit typo: Initializing

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

Will fix.

func TestRemoteRotationNonexistentRepo(t *testing.T) {
// The repo is initialized at publish time after
// rotating the key. We should be denied the access
// to metadata by the server when trying to retrieve it.

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 2, 2017

Contributor

I'm not sure I follow why this is the case?

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

I made a mistake here, my bad. This is the current behavior which is buggy, I need to fix this. It seems like operations' order is not the right one in this code path.

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

@endophage found the issue, we needed a fullTestServer rather than a simpleTestServer.

_, err := cache.GetSized(role.String(), store.NoSizeLimit)
if err != nil {
if _, ok := err.(store.ErrMetaNotFound); ok {
continue

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 2, 2017

Contributor

is this intended? This function seems to return true if a cache exists but none of the roles have metadata in it

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

No, for each roles, we search if there is metadata. We keep searching for other roles only if the error we get while searching is that there is no metadata in it.

Is there a bug in the logic or implementation in your opinion?

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 3, 2017

Contributor

@n4ss - sorry yes, you're correct. Could we add a docstring-style comment to this function?

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

will do!

@@ -142,7 +142,10 @@ func (t *tufCommander) AddToCommand(cmd *cobra.Command) {
cmdReset.Flags().BoolVar(&t.resetAll, "all", false, "Reset all changes shown in the status list")
cmd.AddCommand(cmdReset)

cmd.AddCommand(cmdTUFPublishTemplate.ToCommand(t.tufPublish))
cmdTUFPublish := cmdTUFPublishTemplate.ToCommand(t.tufPublish)
cmdTUFPublish.Flags().StringVar(&t.rootKey, "rootkey", "", "Root key to initialize the repository with")

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 2, 2017

Contributor

nit: can the flag description indicate that it's only used if this is the first publish?

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

I'll actually remove this as we don't really need it.

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 3, 2017

Contributor

hm it depends on whether we want lazy initialization to support this (we'd have to inject the root key into this call to Initialize), and it will get messy with the auto-publish flag.

I'm ok with removing this for now, since notary init will still allow for this functionality. We can figure out the best way to handle this for lazy init on a followup PR 👍

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

sounds good, I tried to draft it but there is no clean way to propagate the root key to Initialize in this code path.. we should talk about it later indeed

@@ -1031,14 +1037,14 @@ func maybeAutoPublish(cmd *cobra.Command, doPublish bool, gun data.GUN, config *
return err
}

cmd.Println("Auto-publishing changes to", gun)
return publishAndPrintToCLI(cmd, nRepo, gun)
cmd.Println("Auto-publishing changes to", nRepo.GetGUN())

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 2, 2017

Contributor

👍

This comment has been minimized.

Copy link
@endophage

endophage Mar 3, 2017

Contributor

gun is already an argument to this function. No need to create a new getter for it on NotaryRepository.

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 3, 2017

Contributor

ah you're right, the getter isn't needed - for some reason thought I had seen it somewhere else 😅

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

repo.Publish() will use repo.gun for every operations (in bootstrapClient in oldKeysForLegacyClientSupport for example) and not necessarily the one provided as an argument here.

Providing a different gun that the one inside the repository object could lead to misleading logging entry here, this is why I removed it from the prototype and used the "internal" one instead. Does it make sense?

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 3, 2017

Contributor

I don't think you need the getter because we use thegun argument to get the repository here

nRepo, err := notaryclient.NewFileCachedNotaryRepository(
 		config.GetString("trust_dir"), gun, getRemoteTrustServer(config), rt, passRetriever, trustPin)

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

I was thinking about the case where this function might be called elsewhere in the same package, we have no "contract" on the fact that the gun in the argument is the same as the one used for this repo.

It's just less error-prone if we don't allow someone to provide an unrelated gun. For now it's only used properly because we get it from this same repo but people might call it in a different way..

This comment has been minimized.

Copy link
@cyli

cyli Mar 3, 2017

Contributor

I was thinking about the case where this function might be called elsewhere in the same package, we have no "contract" on the fact that the gun in the argument is the same as the one used for this repo.

If the gun passed in to the constructor of the repo, wouldn't that gun and the gun on the repo by contract of the NotaryRepository have to be the same?

This comment has been minimized.

Copy link
@cyli

cyli Mar 3, 2017

Contributor

Update: unless you mean the contract between the gun and publishAndPrintToCLI?

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

I mean the contract at the publishAndPrintToCLI level only, which can be called with different values for repo.gun and gun.

This comment has been minimized.

Copy link
@cyli

cyli Mar 3, 2017

Contributor

Ah ok, that makes sense.

n4ss added 3 commits Mar 3, 2017
Fix test for rotation without explicit init by using a full test server
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Remove the root key cli parameter to 'notary publish'
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Add comments for metadata lookup function and initializeFromCache
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
}

// Initializing a repo and republishing after should succeed
func TestPublishInitializedRepo(t *testing.T) {

This comment has been minimized.

Copy link
@cyli

cyli Mar 3, 2017

Contributor

I think this test case may already be covered by TestPublishBareRepo

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

indeed, will fix.

@@ -837,6 +834,29 @@ func (r *NotaryRepository) bootstrapRepo() error {
return nil
}

// initializeFromCache looks for cached metadata to bootstrap from

This comment has been minimized.

Copy link
@cyli

cyli Mar 3, 2017

Contributor

Non-blocking: Since isMetaCached basically running the same loop as bootstrapRepo, I wonder if we can modify bootstrapRepo to just call Initialize if it encounters an ErrMetaNotFound error that is not related to a missing timestamp or snapshot.

That would eliminate the need for initializeFromCache.

This comment has been minimized.

Copy link
@cyli

cyli Mar 3, 2017

Contributor

Alternately, in the publish logic, if ErrMetadataNotFound is returned by bootstrapRepo, we can then call Initialize rather than return the previous ErrRepoNotInitialized.

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

Will fix with the second solution. Thanks that's way cleaner that way.

n4ss added 2 commits Mar 3, 2017
Refactor the metadata retrieval to bootstrap repo from cache or from …
…scratch

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Remove duplicate test for repo init and publish
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
@@ -142,7 +142,9 @@ func (t *tufCommander) AddToCommand(cmd *cobra.Command) {
cmdReset.Flags().BoolVar(&t.resetAll, "all", false, "Reset all changes shown in the status list")
cmd.AddCommand(cmdReset)

cmd.AddCommand(cmdTUFPublishTemplate.ToCommand(t.tufPublish))

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 3, 2017

Contributor

nit: I think this can stay as is now that the flag is gone

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

sounds good!

if _, ok := err.(ErrRepositoryNotExist); ok {
err := r.bootstrapRepo()
if _, ok := err.(store.ErrMetaNotFound); ok {

This comment has been minimized.

Copy link
@riyazdf

riyazdf Mar 3, 2017

Contributor

Great cleanup! Could we add some debug logs for these cases?

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

sure, good idea

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 3, 2017

Author Contributor

(fixed)

This comment has been minimized.

Copy link
@cyli

cyli Mar 4, 2017

Contributor

Non-blocking: I'm wondering if this should be logged as Info so that users can see it. Mainly because while trying this out, I'm seeing something like:

$  bin/notary -c cmd/notary/config.json delegation add repo2 targets/releases --all-paths fixtures/notary-server.crt 

Addition of delegation role targets/releases with keys [d67a3bf8194b6cccbce29c859dc3677fedfb0a5e236d24b57c37edc202cc73dd], with paths ["" <all paths>], to repository "repo2" staged for next publish.

$  bin/notary -c cmd/notary/config.json publish repo2
Pushing changes to repo2
Enter passphrase for root key with ID f6ff23d: 
Enter passphrase for new targets key with ID 4788565: 
Repeat passphrase for new targets key with ID 4788565: 
Passphrases do not match. Please retry.
Enter passphrase for new targets key with ID 4788565: 
Repeat passphrase for new targets key with ID 4788565: 
Enter passphrase for new snapshot key with ID b5c6cd5: 
Repeat passphrase for new snapshot key with ID b5c6cd5: 
Successfully published changes for repository repo2

And there's not a lot of difference between "passphrase for root key" and "passphrase for new targets key", etc. It may be useful for users to see a note about what's going on, instead of wondering why they're being asked for all these passphrases (since it's hard to distinguish between the output of the lazy init vs publishing an actual change).

Possibly the log message could be "No TUF data found locally or remotely - initializing repository %s for the first time" or something? (also totally cool with "from scratch", but "first time" might make it more clear to users since we document how notary generates keys when initializing repos for the first time.

This comment has been minimized.

Copy link
@n4ss

n4ss Mar 4, 2017

Author Contributor

Very good point, I didn't realize how disturbing it might be regarding the passphrases.

Will fix!

Add debug logs in publish
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
@cyli

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

Other than minor bikeshedding about a log message, this LGTM! Thanks for all your work on this!

@endophage

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2017

This came out a LOT shorter than I was anticipating! Great work. It looks good but I'm reviewing it late-ish at night so will take another look when I'm a bit more alert.

@endophage
Copy link
Contributor

left a comment

LGTM! This looks great.

Improve logs and adjust log-level for repo bootstrap
Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

@n4ss n4ss force-pushed the n4ss:repo-lazy-init branch from f6fab38 to dd1ef8b Mar 8, 2017

@endophage endophage merged commit 9917b88 into theupdateframework:master Mar 8, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project Absolute coverage decreased by -0.09% but relative coverage increased by +0.44% compared to c04e3e6
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed
test with yubikey hardware tests succeeded with yubikey hardware
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.