-
Notifications
You must be signed in to change notification settings - Fork 514
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
Changes from 12 commits
c046614
9a4d4b8
d11ad89
dd059a6
e556e94
5a9636a
52a07d3
1da4d7f
4182ca0
afc4985
1702384
6a1f1c1
2e8d5ad
9ff8415
73f678c
556e2b4
7e74573
d6ba9f9
dd1ef8b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
|
@@ -1666,14 +1665,31 @@ 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) | ||
require.NoError(t, err) | ||
|
||
// no metadata created | ||
requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, false) | ||
requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, false) | ||
requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, false) | ||
// metadata is created | ||
requireRepoHasExpectedMetadata(t, repo, data.CanonicalRootRole, true) | ||
requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, true) | ||
requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, true) | ||
} | ||
|
||
// Tnitializing a repo and republishing after should succeed | ||
func TestPublishInitializedRepo(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this test case may already be covered by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. indeed, will fix. |
||
var gun data.GUN = "docker.com/notary" | ||
ts := fullTestServer(t) | ||
defer ts.Close() | ||
|
||
// uninitialized repo should not fail to publish | ||
tempBaseDir, err := ioutil.TempDir("", "notary-tests") | ||
require.NoError(t, err) | ||
defer os.RemoveAll(tempBaseDir) | ||
|
||
// initialized repo should not fail to publish either | ||
repo, err := NewFileCachedNotaryRepository(tempBaseDir, gun, ts.URL, | ||
http.DefaultTransport, passphraseRetriever, trustpinning.TrustPinConfig{}) | ||
require.NoError(t, err, "error creating repository: %s", err) | ||
|
||
// now, initialize and republish in the same directory | ||
// now, initialize and publish | ||
rootPubKey, err := repo.CryptoService.Create(data.CanonicalRootRole, repo.gun, data.ECDSAKey) | ||
require.NoError(t, err, "error generating root key: %s", err) | ||
|
||
|
@@ -1684,6 +1700,7 @@ func TestPublishUninitializedRepo(t *testing.T) { | |
requireRepoHasExpectedMetadata(t, repo, data.CanonicalSnapshotRole, true) | ||
requireRepoHasExpectedMetadata(t, repo, data.CanonicalTargetsRole, true) | ||
|
||
// check we can just call publish again | ||
require.NoError(t, repo.Publish()) | ||
} | ||
|
||
|
@@ -1985,26 +2002,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 | ||
} | ||
|
@@ -2650,8 +2647,10 @@ 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) { | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I follow why this is the case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @endophage found the issue, we needed a |
||
func TestRemoteRotationNoInit(t *testing.T) { | ||
ts, _, _ := simpleTestServer(t) | ||
defer ts.Close() | ||
|
||
|
@@ -2660,7 +2659,7 @@ func TestRemoteRotationNonexistentRepo(t *testing.T) { | |
|
||
err := repo.RotateKey(data.CanonicalTimestampRole, true, nil) | ||
require.Error(t, err) | ||
require.IsType(t, ErrRepoNotInitialized{}, err) | ||
require.IsType(t, store.ErrMetaNotFound{}, err) | ||
} | ||
|
||
// Rotates the keys. After the rotation, downloading the latest metadata | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
) | ||
|
||
|
@@ -268,3 +269,59 @@ func serializeCanonicalRole(tufRepo *tuf.Repo, role data.RoleName, extraSigningK | |
|
||
return json.Marshal(s) | ||
} | ||
|
||
func isMetaCached(cache store.MetadataStore) (bool, error) { | ||
if cache == nil { | ||
return false, nil | ||
} | ||
|
||
for _, role := range data.BaseRoles { | ||
_, err := cache.GetSized(role.String(), store.NoSizeLimit) | ||
if err != nil { | ||
if _, ok := err.(store.ErrMetaNotFound); ok { | ||
continue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this intended? This function seems to return true if a cache exists but none of the roles have metadata in it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n4ss - sorry yes, you're correct. Could we add a docstring-style comment to this function? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will do! |
||
} | ||
|
||
return false, err | ||
} | ||
|
||
return true, nil | ||
} | ||
|
||
return false, nil | ||
} | ||
|
||
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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I think this can stay as is now that the flag is gone There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sounds good! |
||
cmdTUFPublish.Flags().StringVar(&t.rootKey, "rootkey", "", "Root key to initialize the repository with") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: can the flag description indicate that it's only used if this is the first publish? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll actually remove this as we don't really need it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm it depends on whether we want lazy initialization to support this (we'd have to inject the root key into this call to I'm ok with removing this for now, since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
cmd.AddCommand(cmdTUFPublish) | ||
|
||
cmd.AddCommand(cmdTUFLookupTemplate.ToCommand(t.tufLookup)) | ||
|
||
cmdTUFList := cmdTUFListTemplate.ToCommand(t.tufList) | ||
|
@@ -385,6 +388,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() | ||
|
@@ -413,38 +445,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 | ||
} | ||
|
||
|
@@ -686,7 +692,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 { | ||
|
@@ -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()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Providing a different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think you need the getter because we use the
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update: unless you mean the contract between the gun and There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean the contract at the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit typo:
Initializing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix.