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

Jump to bottom

Notary repository lazy initialization #1105

Merged
merged 19 commits into from
Mar 8, 2017
Merged

Notary repository lazy initialization #1105

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
c046614
Add lazy initialization of a repository at publish time
n4ss Feb 14, 2017
9a4d4b8
Syntax fix by gofmt
n4ss Feb 14, 2017
d11ad89
Add lazy initialization of a repository at publish time
n4ss Feb 14, 2017
dd059a6
lots of failing tests
Feb 20, 2017
e556e94
Fix rootkey IDs list creation causing faulty access at repo init
n4ss Feb 24, 2017
5a9636a
TestNotInitializedOnPublish doesn't work with repo lazy init at publi…
n4ss Feb 22, 2017
52a07d3
Modify lazy init logic to bootstrap if metadata on disk
n4ss Feb 23, 2017
1da4d7f
Fix deprecated test accordingly to the new logic
n4ss Feb 23, 2017
4182ca0
Fix mishandled conflicts with types update PR
n4ss Feb 24, 2017
afc4985
Update deprecated tests for repo lazy init behavior
n4ss Feb 24, 2017
1702384
Move the decision to init repo based on cached content to its own me…
n4ss Mar 2, 2017
6a1f1c1
Move private keys retrieval from a crypto service to a helper function
n4ss Mar 2, 2017
2e8d5ad
Fix test for rotation without explicit init by using a full test server
n4ss Mar 3, 2017
9ff8415
Remove the root key cli parameter to 'notary publish'
n4ss Mar 3, 2017
73f678c
Add comments for metadata lookup function and initializeFromCache
n4ss Mar 3, 2017
556e2b4
Refactor the metadata retrieval to bootstrap repo from cache or from …
n4ss Mar 3, 2017
7e74573
Remove duplicate test for repo init and publish
n4ss Mar 3, 2017
d6ba9f9
Add debug logs in publish
n4ss Mar 3, 2017
dd1ef8b
Improve logs and adjust log-level for repo bootstrap
n4ss Mar 4, 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
31 changes: 17 additions & 14 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ func NewNotaryRepository(baseDir string, gun data.GUN, baseURL string, remoteSto
return nRepo, nil
}

// GetGUN is a getter for the GUN object from a NotaryRepository
func (r *NotaryRepository) GetGUN() data.GUN {
return r.gun
}

// Target represents a simplified version of the data TUF operates on, so external
// applications don't have to depend on TUF data types.
type Target struct {
Expand Down Expand Up @@ -174,13 +179,10 @@ func rootCertKey(gun data.GUN, privKey data.PrivateKey) (data.PublicKey, error)
// result is only stored on local disk, not published to the server. To do that,
// use r.Publish() eventually.
func (r *NotaryRepository) Initialize(rootKeyIDs []string, serverManagedRoles ...data.RoleName) error {
privKeys := make([]data.PrivateKey, 0, len(rootKeyIDs))
for _, keyID := range rootKeyIDs {
privKey, _, err := r.CryptoService.GetPrivateKey(keyID)
if err != nil {
return err
}
privKeys = append(privKeys, privKey)

privKeys, err := getAllPrivKeys(rootKeyIDs, r.CryptoService)
if err != nil {
return err
}

// currently we only support server managing timestamps and snapshots, and
Expand Down Expand Up @@ -254,7 +256,6 @@ func (r *NotaryRepository) Initialize(rootKeyIDs []string, serverManagedRoles ..

func (r *NotaryRepository) initializeRoles(rootKeys []data.PublicKey, localRoles, remoteRoles []data.RoleName) (
root, targets, snapshot, timestamp data.BaseRole, err error) {

root = data.NewBaseRole(
data.CanonicalRootRole,
notary.MinThreshold,
Expand Down Expand Up @@ -618,17 +619,19 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error {
// update first before publishing
if err := r.Update(true); err != nil {
// If the remote is not aware of the repo, then this is being published
// for the first time. Try to load from disk instead for publishing.
// for the first time. Try to initialize the repository before publishing.
if _, ok := err.(ErrRepositoryNotExist); ok {
err := r.bootstrapRepo()
if _, ok := err.(store.ErrMetaNotFound); ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(fixed)

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Will fix!

logrus.Infof("No TUF data found locally or remotely - initializing repository %s for the first time", r.gun.String())
err = r.Initialize(nil)
}

if err != nil {
logrus.Debugf("Unable to load repository from local files: %s",
err.Error())
if _, ok := err.(store.ErrMetaNotFound); ok {
return ErrRepoNotInitialized{}
}
logrus.WithError(err).Debugf("Unable to load or initialize repository during first publish: %s", err.Error())
return err
}

// Ensure we will push the initial root and targets file. Either or
// both of the root and targets may not be marked as Dirty, since
// there may not be any changes that update them, so use a
Expand Down
52 changes: 9 additions & 43 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1650,14 +1650,13 @@ func testPublishNoData(t *testing.T, rootType string, clearCache, serverManagesS
}
}

// Publishing an uninitialized repo will fail, but initializing and republishing
// after should succeed
// Publishing an uninitialized repo should not fail
func TestPublishUninitializedRepo(t *testing.T) {
var gun data.GUN = "docker.com/notary"
ts := fullTestServer(t)
defer ts.Close()

// uninitialized repo should fail to publish
// uninitialized repo should not fail to publish
tempBaseDir, err := ioutil.TempDir("", "notary-tests")
require.NoError(t, err)
defer os.RemoveAll(tempBaseDir)
Expand All @@ -1666,25 +1665,12 @@ func TestPublishUninitializedRepo(t *testing.T) {
http.DefaultTransport, passphraseRetriever, trustpinning.TrustPinConfig{})
require.NoError(t, err, "error creating repository: %s", err)
err = repo.Publish()
require.Error(t, err)

// no metadata created
requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, false)
requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, false)
requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, false)

// now, initialize and republish in the same directory
rootPubKey, err := repo.CryptoService.Create(data.CanonicalRootRole, repo.gun, data.ECDSAKey)
require.NoError(t, err, "error generating root key: %s", err)

require.NoError(t, repo.Initialize([]string{rootPubKey.ID()}))
require.NoError(t, err)

// now metadata is created
// metadata is created
requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, true)
requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, true)
requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, true)

require.NoError(t, repo.Publish())
}

// Create a repo, instantiate a notary server, and publish the repo with
Expand Down Expand Up @@ -1985,26 +1971,6 @@ func testPublishBadMetadata(t *testing.T, roleName string, repo *NotaryRepositor
}
}

// If the repo is not initialized, calling repo.Publish() should return ErrRepoNotInitialized
func TestNotInitializedOnPublish(t *testing.T) {
// Temporary directory where test files will be created
tempBaseDir, err := ioutil.TempDir("", "notary-test-")
defer os.RemoveAll(tempBaseDir)
require.NoError(t, err, "failed to create a temporary directory: %s", err)

gun := "docker.com/notary"
ts := fullTestServer(t)
defer ts.Close()

repo, _, _ := createRepoAndKey(t, data.ECDSAKey, tempBaseDir, gun, ts.URL)

addTarget(t, repo, "v1", "../fixtures/intermediate-ca.crt")

err = repo.Publish()
require.Error(t, err)
require.IsType(t, ErrRepoNotInitialized{}, err)
}

type cannotCreateKeys struct {
signed.CryptoService
}
Expand Down Expand Up @@ -2650,17 +2616,17 @@ func TestRemoteRotationNoRootKey(t *testing.T) {
require.IsType(t, signed.ErrInsufficientSignatures{}, err)
}

// The repo hasn't been initialized, so we can't rotate
func TestRemoteRotationNonexistentRepo(t *testing.T) {
ts, _, _ := simpleTestServer(t)
// The repo should initialize successfully at publish time after
// rotating the key.
func TestRemoteRotationNoInit(t *testing.T) {
ts := fullTestServer(t)
defer ts.Close()

repo := newBlankRepo(t, ts.URL)
defer os.RemoveAll(repo.baseDir)

err := repo.RotateKey(data.CanonicalTimestampRole, true, nil)
require.Error(t, err)
require.IsType(t, ErrRepoNotInitialized{}, err)
require.NoError(t, err)
}

// Rotates the keys. After the rotation, downloading the latest metadata
Expand Down
36 changes: 36 additions & 0 deletions client/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
store "github.com/docker/notary/storage"
"github.com/docker/notary/tuf"
"github.com/docker/notary/tuf/data"
"github.com/docker/notary/tuf/signed"
"github.com/docker/notary/tuf/utils"
)

Expand Down Expand Up @@ -268,3 +269,38 @@ func serializeCanonicalRole(tufRepo *tuf.Repo, role data.RoleName, extraSigningK

return json.Marshal(s)
}

func getAllPrivKeys(rootKeyIDs []string, cryptoService signed.CryptoService) ([]data.PrivateKey, error) {
if cryptoService == nil {
return nil, fmt.Errorf("no crypto service available to get private keys from")
}

privKeys := make([]data.PrivateKey, 0, len(rootKeyIDs))
for _, keyID := range rootKeyIDs {
privKey, _, err := cryptoService.GetPrivateKey(keyID)
if err != nil {
return nil, err
}
privKeys = append(privKeys, privKey)
}
if len(privKeys) == 0 {
var rootKeyID string
rootKeyList := cryptoService.ListKeys(data.CanonicalRootRole)
if len(rootKeyList) == 0 {
rootPublicKey, err := cryptoService.Create(data.CanonicalRootRole, "", data.ECDSAKey)
if err != nil {
return nil, err
}
rootKeyID = rootPublicKey.ID()
} else {
rootKeyID = rootKeyList[0]
}
privKey, _, err := cryptoService.GetPrivateKey(rootKeyID)
if err != nil {
return nil, err
}
privKeys = append(privKeys, privKey)
}

return privKeys, nil
}
74 changes: 39 additions & 35 deletions cmd/notary/tuf.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ func (t *tufCommander) AddToCommand(cmd *cobra.Command) {
cmd.AddCommand(cmdReset)

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

cmd.AddCommand(cmdTUFLookupTemplate.ToCommand(t.tufLookup))

cmdTUFList := cmdTUFListTemplate.ToCommand(t.tufList)
Expand Down Expand Up @@ -385,6 +386,35 @@ func (t *tufCommander) tufDeleteGUN(cmd *cobra.Command, args []string) error {
return nil
}

func importRootKey(cmd *cobra.Command, rootKey string, nRepo *notaryclient.NotaryRepository, retriever notary.PassRetriever) ([]string, error) {
var rootKeyList []string

if rootKey != "" {
privKey, err := readKey(data.CanonicalRootRole, rootKey, retriever)
if err != nil {
return nil, err
}
err = nRepo.CryptoService.AddKey(data.CanonicalRootRole, "", privKey)
if err != nil {
return nil, fmt.Errorf("Error importing key: %v", err)
}
rootKeyList = []string{privKey.ID()}
} else {
rootKeyList = nRepo.CryptoService.ListKeys(data.CanonicalRootRole)
}

if len(rootKeyList) > 0 {
// Chooses the first root key available, which is initialization specific
// but should return the HW one first.
rootKeyID := rootKeyList[0]
cmd.Printf("Root key found, using: %s\n", rootKeyID)

return []string{rootKeyID}, nil
}

return []string{}, nil
}

func (t *tufCommander) tufInit(cmd *cobra.Command, args []string) error {
if len(args) < 1 {
cmd.Usage()
Expand Down Expand Up @@ -413,38 +443,12 @@ func (t *tufCommander) tufInit(cmd *cobra.Command, args []string) error {
return err
}

var rootKeyList []string

if t.rootKey != "" {
privKey, err := readKey(data.CanonicalRootRole, t.rootKey, t.retriever)
if err != nil {
return err
}
err = nRepo.CryptoService.AddKey(data.CanonicalRootRole, "", privKey)
if err != nil {
return fmt.Errorf("Error importing key: %v", err)
}
rootKeyList = []string{privKey.ID()}
} else {
rootKeyList = nRepo.CryptoService.ListKeys(data.CanonicalRootRole)
}

var rootKeyID string
if len(rootKeyList) < 1 {
cmd.Println("No root keys found. Generating a new root key...")
rootPublicKey, err := nRepo.CryptoService.Create(data.CanonicalRootRole, "", data.ECDSAKey)
if err != nil {
return err
}
rootKeyID = rootPublicKey.ID()
} else {
// Chooses the first root key available, which is initialization specific
// but should return the HW one first.
rootKeyID = rootKeyList[0]
cmd.Printf("Root key found, using: %s\n", rootKeyID)
rootKeyIDs, err := importRootKey(cmd, t.rootKey, nRepo, t.retriever)
if err != nil {
return err
}

if err = nRepo.Initialize([]string{rootKeyID}); err != nil {
if err = nRepo.Initialize(rootKeyIDs); err != nil {
return err
}

Expand Down Expand Up @@ -686,7 +690,7 @@ func (t *tufCommander) tufPublish(cmd *cobra.Command, args []string) error {
return err
}

return publishAndPrintToCLI(cmd, nRepo, gun)
return publishAndPrintToCLI(cmd, nRepo)
}

func (t *tufCommander) tufRemove(cmd *cobra.Command, args []string) error {
Expand Down Expand Up @@ -1031,14 +1035,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())
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

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 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..

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 mean the contract at the publishAndPrintToCLI level only, which can be called with different values for repo.gun and gun.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, that makes sense.

return publishAndPrintToCLI(cmd, nRepo)
}

func publishAndPrintToCLI(cmd *cobra.Command, nRepo *notaryclient.NotaryRepository, gun data.GUN) error {
func publishAndPrintToCLI(cmd *cobra.Command, nRepo *notaryclient.NotaryRepository) error {
if err := nRepo.Publish(); err != nil {
return err
}
cmd.Printf("Successfully published changes for repository %s\n", gun)
cmd.Printf("Successfully published changes for repository %s\n", nRepo.GetGUN())
return nil
}