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 fccc74221b..777901f38d 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -21,7 +21,6 @@ import ( "github.com/Sirupsen/logrus" ctxu "github.com/docker/distribution/context" "github.com/docker/go/canonical/json" - "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/net/context" @@ -2580,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) @@ -2632,14 +2590,13 @@ func TestRemoteRotationError(t *testing.T) { // server has died, so this should fail err := repo.RotateKey(data.CanonicalTimestampRole, true) - assert.Error(t, err) - assert.Contains(t, err.Error(), "unable to rotate remote key") + require.Error(t, err) + require.Contains(t, err.Error(), "unable to rotate remote key") } // Rotates the keys. After the rotation, downloading the latest metadata // and require that the keys have changed -func requireRotationSuccessful(t *testing.T, repo1 *NotaryRepository, - keysToRotate map[string]bool, alreadyPublished bool) { +func requireRotationSuccessful(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 @@ -2649,35 +2606,26 @@ func requireRotationSuccessful(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") - require.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 { require.NoError(t, repo1.RotateKey(role, serverManaged)) } - // Publish - err := repo1.Publish() - require.NoError(t, err) + changesPost := getChanges(t, repo1) + require.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) require.NoError(t, err) for role, isRemoteKey := range keysToRotate { @@ -2705,12 +2653,6 @@ func requireRotationSuccessful(t *testing.T, repo1 *NotaryRepository, require.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) - require.Len(t, changes, 0, "wrong number of changelist files found") } } @@ -2726,11 +2668,16 @@ 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") requireRotationSuccessful(t, repo, map[string]bool{ data.CanonicalTargetsRole: false, - data.CanonicalSnapshotRole: false}, false) + data.CanonicalSnapshotRole: false}) + + require.NoError(t, repo.Publish()) + _, err := repo.GetTargetByName("latest") + require.NoError(t, err) } // Initialize a repo, locally signed snapshots @@ -2778,12 +2725,13 @@ func testRotateKeySuccess(t *testing.T, serverManagesSnapshotInit bool, // rotating the keys. addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt") + requireRotationSuccessful(t, repo, keysToRotate) + // Publish require.NoError(t, repo.Publish()) - // Get root.json and capture targets + snapshot key IDs - repo.GetTargetByName("latest") // force a pull - requireRotationSuccessful(t, repo, keysToRotate, true) + _, err := repo.GetTargetByName("latest") + require.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 89816f5b45..6356d8196f 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..5c7869afbf 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 is immediately published. No other changes, even if they are staged, will be 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..8e45dbdd81 100644 --- a/cmd/notary/keys_test.go +++ b/cmd/notary/keys_test.go @@ -328,7 +328,7 @@ func setUpRepo(t *testing.T, tempBaseDir, gun string, ret passphrase.Retriever) cryptoService := cryptoservice.NewCryptoService( "", trustmanager.NewKeyMemoryStore(ret)) - ts := httptest.NewServer(server.RootHandler(nil, ctx, cryptoService)) + ts := httptest.NewServer(server.RootHandler(nil, ctx, cryptoService, nil, nil)) repo, err := client.NewNotaryRepository( tempBaseDir, gun, ts.URL, http.DefaultTransport, ret) @@ -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) - } - // there should be 2 keys left - assert.Len(t, newKeys, 2) - // one for each role - var targetsFound, snapshotFound bool - for _, role := range newKeys { - switch role { - case data.CanonicalTargetsRole: - targetsFound = true - case data.CanonicalSnapshotRole: - snapshotFound = true + 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") } } - assert.True(t, targetsFound, "targets key was not created") - assert.True(t, snapshotFound, "snapshot key was not created") + + found := make(map[string]bool) + for _, role := range newKeys { + found[role] = true + } + 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) {