diff --git a/client/client.go b/client/client.go index 65b4cf7f4c..49cd0481ac 100644 --- a/client/client.go +++ b/client/client.go @@ -875,44 +875,47 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error { switch { // We currently support locally or remotely managing snapshot keys... case role == data.CanonicalSnapshotRole: + break + // locally managing targets keys only - case role == data.CanonicalTargetsRole: - if serverManagesKey { - return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole} - } + case role == data.CanonicalTargetsRole && !serverManagesKey: + break + case role == data.CanonicalTargetsRole && serverManagesKey: + return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole} + // and remotely managing timestamp keys only - case role == data.CanonicalTimestampRole: - if !serverManagesKey { - return ErrInvalidLocalRole{Role: data.CanonicalTimestampRole} - } + case role == data.CanonicalTimestampRole && serverManagesKey: + break + case role == data.CanonicalTimestampRole && !serverManagesKey: + return ErrInvalidLocalRole{Role: data.CanonicalTimestampRole} + default: return fmt.Errorf("notary does not currently permit rotating the %s key", role) } - if serverManagesKey { - pubKey, err := getRemoteKey(r.baseURL, r.gun, role, r.roundTrip) - if err != nil { - return fmt.Errorf("unable to rotate remote key: %s", err) - } - cl := changelist.NewMemChangelist() - if err := r.rootFileKeyChange(cl, role, changelist.ActionCreate, pubKey); err != nil { - return err - } - return r.publish(cl) + var ( + pubKey data.PublicKey + err error + errFmtMsg string + ) + switch serverManagesKey { + case true: + pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip) + errFmtMsg = "unable to rotate remote key: %s" + default: + pubKey, err = r.CryptoService.Create(role, data.ECDSAKey) + errFmtMsg = "unable to generate key: %s" } - pubKey, err := r.CryptoService.Create(role, data.ECDSAKey) if err != nil { - return fmt.Errorf("unable to generate key: %s", err) + return fmt.Errorf(errFmtMsg, err) } - cl, err := changelist.NewFileChangelist(filepath.Join(r.tufRepoPath, "changelist")) - if err != nil { + cl := changelist.NewMemChangelist() + if err := r.rootFileKeyChange(cl, role, changelist.ActionCreate, pubKey); err != nil { return err } - defer cl.Close() - - return r.rootFileKeyChange(cl, role, changelist.ActionCreate, pubKey) + return r.publish(cl) } func (r *NotaryRepository) rootFileKeyChange(cl changelist.Changelist, role, action string, key data.PublicKey) error { diff --git a/client/client_test.go b/client/client_test.go index 5b81972e95..b0135f3420 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -2579,47 +2579,6 @@ func TestRotateKeyInvalidRole(t *testing.T) { } } -// Initialize repo to have local signing of snapshots. Rotate the key to have -// the server manage the snapshot key. Assert that this publishes the key change -// but not any other changes. -func TestRemoteRotationOnlyPublishesKeyChanges(t *testing.T) { - ts := fullTestServer(t) - defer ts.Close() - - repo1, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, false) - defer os.RemoveAll(repo1.baseDir) - - oldSnapshotRole, err := repo1.tufRepo.GetBaseRole(data.CanonicalSnapshotRole) - assert.NoError(t, err) - - // Add and make sure it's not published when the key is rotated - addTarget(t, repo1, "latest", "../fixtures/intermediate-ca.crt") - cl, err := repo1.GetChangelist() - assert.NoError(t, err) - assert.Len(t, cl.List(), 1) - - assert.NoError(t, repo1.RotateKey(data.CanonicalSnapshotRole, true)) - assert.Len(t, cl.List(), 1, "rotating key published other changes") - - // ensure that the key rotation was public by pulling from a new repo - repo2, _ := newRepoToTestRepo(t, repo1, true) - defer os.RemoveAll(repo2.baseDir) - tgts, err := repo2.ListTargets() - assert.NoError(t, err) - assert.Len(t, tgts, 0, "No targets should have been published") - - newSnapshotRole, err := repo2.tufRepo.GetBaseRole(data.CanonicalSnapshotRole) - assert.NoError(t, err) - - // assert that the snapshot key has changed - assert.Len(t, oldSnapshotRole.Keys, 1) - assert.Len(t, newSnapshotRole.Keys, 1) - for k := range oldSnapshotRole.Keys { - _, ok := newSnapshotRole.Keys[k] - assert.False(t, ok) - } -} - // If remotely rotating key fails, the failure is propagated func TestRemoteRotationError(t *testing.T) { ts, _, _ := simpleTestServer(t) @@ -2637,8 +2596,7 @@ func TestRemoteRotationError(t *testing.T) { // Rotates the keys. After the rotation, downloading the latest metadata // and assert that the keys have changed -func assertRotationSuccessful(t *testing.T, repo1 *NotaryRepository, - keysToRotate map[string]bool, alreadyPublished bool) { +func assertRotationSuccessful(t *testing.T, repo1 *NotaryRepository, keysToRotate map[string]bool) { // Create 2 new repos: 1 will download repo data before the publish, // and one only downloads after the publish. This reflects a client // that has some previous trust data (but is not the publisher), and a @@ -2648,35 +2606,26 @@ func assertRotationSuccessful(t *testing.T, repo1 *NotaryRepository, repos := []*NotaryRepository{repo1, repo2} - if alreadyPublished { - repo3, _ := newRepoToTestRepo(t, repo1, true) - defer os.RemoveAll(repo2.baseDir) - - // force a pull on repo3 - _, err := repo3.GetTargetByName("latest") - assert.NoError(t, err) - - repos = append(repos, repo3) - } - oldKeyIDs := make(map[string][]string) for role := range keysToRotate { keyIDs := repo1.tufRepo.Root.Signed.Roles[role].KeyIDs oldKeyIDs[role] = keyIDs } + // Confirm no changelists get published + changesPre := getChanges(t, repo1) + // Do rotation for role, serverManaged := range keysToRotate { assert.NoError(t, repo1.RotateKey(role, serverManaged)) } - // Publish - err := repo1.Publish() - assert.NoError(t, err) + changesPost := getChanges(t, repo1) + assert.Equal(t, changesPre, changesPost) // Download data from remote and check that keys have changed for _, repo := range repos { - _, err := repo.GetTargetByName("latest") // force a pull + _, err := repo.Update(true) assert.NoError(t, err) for role, isRemoteKey := range keysToRotate { @@ -2704,12 +2653,6 @@ func assertRotationSuccessful(t *testing.T, repo1 *NotaryRepository, assert.NotNil(t, key) } } - - // Confirm changelist dir empty (on repo1, it should be empty after - // after publishing changes, on repo2, there should never have been - // any changelists) - changes := getChanges(t, repo) - assert.Len(t, changes, 0, "wrong number of changelist files found") } } @@ -2725,11 +2668,17 @@ func TestRotateBeforePublishFromRemoteKeyToLocalKey(t *testing.T) { defer os.RemoveAll(repo.baseDir) // Adding a target will allow us to confirm the repository is still valid - // after rotating the keys. + // after rotating the keys when we publish (and that rotation doesn't publish + // non-key-rotation changes) addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt") + assertRotationSuccessful(t, repo, map[string]bool{ data.CanonicalTargetsRole: false, - data.CanonicalSnapshotRole: false}, false) + data.CanonicalSnapshotRole: false}) + + assert.NoError(t, repo.Publish()) + _, err := repo.GetTargetByName("latest") + assert.NoError(t, err) } // Initialize a repo, locally signed snapshots @@ -2777,12 +2726,13 @@ func testRotateKeySuccess(t *testing.T, serverManagesSnapshotInit bool, // rotating the keys. addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt") + assertRotationSuccessful(t, repo, keysToRotate) + // Publish assert.NoError(t, repo.Publish()) - // Get root.json and capture targets + snapshot key IDs - repo.GetTargetByName("latest") // force a pull - assertRotationSuccessful(t, repo, keysToRotate, true) + _, err := repo.GetTargetByName("latest") + assert.NoError(t, err) var keysToExpectCreated []string for role, serverManaged := range keysToRotate { diff --git a/cmd/notary/integration_test.go b/cmd/notary/integration_test.go index e741975e70..0b6668e289 100644 --- a/cmd/notary/integration_test.go +++ b/cmd/notary/integration_test.go @@ -871,28 +871,13 @@ func TestClientKeyGenerationRotation(t *testing.T) { assertSuccessfullyPublish(t, tempDir, server.URL, "gun", target, tempfiles[0]) // rotate the signing keys - _, err = runCommand(t, tempDir, "key", "rotate", "gun", data.CanonicalSnapshotRole) + _, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", data.CanonicalSnapshotRole) assert.NoError(t, err) - _, err = runCommand(t, tempDir, "key", "rotate", "gun", data.CanonicalTargetsRole) + _, err = runCommand(t, tempDir, "-s", server.URL, "key", "rotate", "gun", data.CanonicalTargetsRole) assert.NoError(t, err) - root, sign := assertNumKeys(t, tempDir, 1, 4, true) + root, sign := assertNumKeys(t, tempDir, 1, 2, true) assert.Equal(t, origRoot[0], root[0]) - // there should be the new keys and the old keys - for _, origKey := range origSign { - found := false - for _, key := range sign { - if key == origKey { - found = true - } - } - assert.True(t, found, "Old key not found in list of old and new keys") - } - // publish the key rotation - _, err = runCommand(t, tempDir, "-s", server.URL, "publish", "gun") - assert.NoError(t, err) - root, sign = assertNumKeys(t, tempDir, 1, 2, true) - assert.Equal(t, origRoot[0], root[0]) // just do a cursory rotation check that the keys aren't equal anymore for _, origKey := range origSign { for _, key := range sign { diff --git a/cmd/notary/keys.go b/cmd/notary/keys.go index f4df2c7082..a0cfa89bde 100644 --- a/cmd/notary/keys.go +++ b/cmd/notary/keys.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "io/ioutil" - "net/http" "os" "path/filepath" "strconv" @@ -38,7 +37,7 @@ var cmdKeyListTemplate = usageTemplate{ var cmdRotateKeyTemplate = usageTemplate{ Use: "rotate [ GUN ] [ key role ]", Short: "Rotate a signing (non-root) key of the given type for the given Globally Unique Name and role.", - Long: "Generates a new signing key (non-root) for the given Globally Unique Name and role. Rotating to a server-managed key is an online-only operation: a new key is requested from the server rather than generated, and if successful, the key rotation is immediately published. Rotating to a locally-managed key is an offline operation only: `notary publish` must be executed manually afterward to publish to the remote server.\nThe role must be one of \"snapshot\", \"targets\", or \"timestamp\".", + Long: "Generates a new key for the given Globally Unique Name and role (one of \"snapshot\", \"targets\", or \"timestamp\"). If rotating to a server-managed key, a new key is requested from the server rather than generated. If the generation or key request is successful, the key rotation only is immediately published.", } var cmdKeyGenerateRootKeyTemplate = usageTemplate{ @@ -415,15 +414,11 @@ func (k *keyCommander) keysRotate(cmd *cobra.Command, args []string) error { gun := args[0] rotateKeyRole := args[1] - var rt http.RoundTripper - if k.rotateKeyServerManaged { - // this does not actually push the changes, just creates the keys, but - // it creates a key remotely so it needs a transport - rt, err = getTransport(config, gun, false) - if err != nil { - return err - } + rt, err := getTransport(config, gun, false) + if err != nil { + return err } + nRepo, err := notaryclient.NewNotaryRepository( config.GetString("trust_dir"), gun, getRemoteTrustServer(config), rt, k.getRetriever()) diff --git a/cmd/notary/keys_test.go b/cmd/notary/keys_test.go index ca02ad225a..f3e5940bc0 100644 --- a/cmd/notary/keys_test.go +++ b/cmd/notary/keys_test.go @@ -414,14 +414,13 @@ func TestRotateKeyBothKeys(t *testing.T) { ret := passphrase.ConstantRetriever("pass") ts, initialKeys := setUpRepo(t, tempBaseDir, gun, ret) - // we won't need this anymore since we are creating keys locally - ts.Close() + defer ts.Close() k := &keyCommander{ configGetter: func() (*viper.Viper, error) { v := viper.New() v.SetDefault("trust_dir", tempBaseDir) - // won't need a remote server URL, since we are creating local keys + v.SetDefault("remote_server.url", ts.URL) return v, nil }, getRetriever: func() passphrase.Retriever { return ret }, @@ -434,30 +433,32 @@ func TestRotateKeyBothKeys(t *testing.T) { cl, err := repo.GetChangelist() assert.NoError(t, err, "unable to get changelist: %v", err) - assert.Len(t, cl.List(), 2) + assert.Len(t, cl.List(), 0) - // two new keys have been created, and the old keys should still be there + // two new keys have been created, and the old keys should still be gone newKeys := repo.CryptoService.ListAllKeys() + // there should be 3 keys - snapshot, targets, and root + assert.Len(t, newKeys, 3) + + // the old snapshot/targets keys should be gone for keyID, role := range initialKeys { r, ok := newKeys[keyID] - assert.True(t, ok, "original key %s missing", keyID) - assert.Equal(t, role, r) - delete(newKeys, keyID) + switch r { + case data.CanonicalSnapshotRole, data.CanonicalTargetsRole: + assert.False(t, ok, "original key %s still there", keyID) + case data.CanonicalRootRole: + assert.Equal(t, role, r) + assert.True(t, ok, "old root key has changed") + } } - // there should be 2 keys left - assert.Len(t, newKeys, 2) - // one for each role - var targetsFound, snapshotFound bool + + found := make(map[string]bool) for _, role := range newKeys { - switch role { - case data.CanonicalTargetsRole: - targetsFound = true - case data.CanonicalSnapshotRole: - snapshotFound = true - } + found[role] = true } - assert.True(t, targetsFound, "targets key was not created") - assert.True(t, snapshotFound, "snapshot key was not created") + assert.True(t, found[data.CanonicalTargetsRole], "targets key was not created") + assert.True(t, found[data.CanonicalSnapshotRole], "snapshot key was not created") + assert.True(t, found[data.CanonicalRootRole], "root key was removed somehow") } func TestChangeKeyPassphraseInvalidID(t *testing.T) {