From 8521ea5b6dc9560e405fc5936c0f54a434552808 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Tue, 8 Dec 2015 15:38:03 -0800 Subject: [PATCH 1/5] Convert NotaryRepository.RotateKeys to RotateKey(role, serverManages bool) This should make it possible to delegate snapshot key management to the server for existing repos, or switching back to user managing snapshot keys. Signed-off-by: Ying Li --- client/client.go | 41 ++++++---- client/client_test.go | 170 ++++++++++++++++++++++++++++++------------ 2 files changed, 150 insertions(+), 61 deletions(-) diff --git a/client/client.go b/client/client.go index af5e5985aa..c797c52a7f 100644 --- a/client/client.go +++ b/client/client.go @@ -628,19 +628,34 @@ func (r *NotaryRepository) bootstrapClient() (*tufclient.Client, error) { ), nil } -// RotateKeys removes all existing keys associated with role and adds -// the keys specified by keyIDs to the role. These changes are staged -// in a changelist until publish is called. -func (r *NotaryRepository) RotateKeys() error { - for _, role := range []string{"targets", "snapshot"} { - key, err := r.CryptoService.Create(role, data.ECDSAKey) - if err != nil { - return err - } - err = r.rootFileKeyChange(role, changelist.ActionCreate, key) - if err != nil { - return err - } +// RotateKey removes all existing keys associated with the role, and either +// creates and adds one new key or delegates managing the key to the server. +// These changes are staged in a changelist until publish is called. +func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error { + if role == data.CanonicalRootRole || role == data.CanonicalTimestampRole { + return fmt.Errorf( + "notary does not currently support rotating the %s key", role) + } + if serverManagesKey && role == data.CanonicalTargetsRole { + return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole} + } + + var ( + pubKey data.PublicKey + err error + ) + if serverManagesKey { + pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip) + } else { + pubKey, err = r.CryptoService.Create(role, data.ECDSAKey) + } + if err != nil { + return err + } + + err = r.rootFileKeyChange(role, changelist.ActionCreate, pubKey) + if err != nil { + return err } return nil } diff --git a/client/client_test.go b/client/client_test.go index 3c095db5a4..b90bc5869c 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -1020,73 +1020,85 @@ func TestPublishSnapshotLocalKeysCreatedFirst(t *testing.T) { assert.False(t, requestMade) } +// Rotate invalid roles, or attempt to delegate target signing to the server +func TestRotateKeyInvalidRole(t *testing.T) { + tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-") + assert.NoError(t, err, "failed to create a temporary directory: %s", err) + defer os.RemoveAll(tempBaseDir) + + gun := "docker.com/notary" + ts, _, _ := simpleTestServer(t) + defer ts.Close() + + repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false) + + // the equivalent of: (root, true), (root, false), (timestamp, true), + // (timestamp, false), (targets, true) + for role := range data.ValidRoles { + if role == data.CanonicalSnapshotRole { + continue + } + for _, serverManagesKey := range []bool{true, false} { + if role == data.CanonicalTargetsRole && !serverManagesKey { + continue + } + err = repo.RotateKey(role, serverManagesKey) + assert.Error(t, err, + "Rotating a %s key with server-managing the key as %v should fail", + role, serverManagesKey) + } + } +} + // Rotates the keys. After the rotation, downloading the latest metadata // and assert that the keys have changed -func assertRotationSuccessful(t *testing.T, repo *NotaryRepository) { - // capture targets + snapshot key IDs - targetsKeyIDs := repo.tufRepo.Root.Signed.Roles["targets"].KeyIDs - snapshotKeyIDs := repo.tufRepo.Root.Signed.Roles["snapshot"].KeyIDs - assert.Len(t, targetsKeyIDs, 1) - assert.Len(t, snapshotKeyIDs, 1) +func assertRotationSuccessful(t *testing.T, repo *NotaryRepository, + keysToRotate map[string]bool) { + + oldKeyIDs := make(map[string]string) + for role := range keysToRotate { + keyIDs := repo.tufRepo.Root.Signed.Roles[role].KeyIDs + assert.Len(t, keyIDs, 1) + oldKeyIDs[role] = keyIDs[0] + } // Do rotation - repo.RotateKeys() + for role, serverManaged := range keysToRotate { + assert.NoError(t, repo.RotateKey(role, serverManaged)) + } // Publish err := repo.Publish() assert.NoError(t, err) - // Get root.json. Check targets + snapshot keys have changed - // and that they match those found in the changelist. + // Get root.json. Check keys have changed. _, err = repo.GetTargetByName("latest") // force a pull assert.NoError(t, err) - newTargetsKeyIDs := repo.tufRepo.Root.Signed.Roles["targets"].KeyIDs - newSnapshotKeyIDs := repo.tufRepo.Root.Signed.Roles["snapshot"].KeyIDs - assert.Len(t, newTargetsKeyIDs, 1) - assert.Len(t, newSnapshotKeyIDs, 1) - assert.NotEqual(t, targetsKeyIDs[0], newTargetsKeyIDs[0]) - assert.NotEqual(t, snapshotKeyIDs[0], newSnapshotKeyIDs[0]) + + for role, isRemoteKey := range keysToRotate { + keyIDs := repo.tufRepo.Root.Signed.Roles[role].KeyIDs + assert.Len(t, keyIDs, 1) + assert.NotEqual(t, oldKeyIDs[role], keyIDs[0]) + key, _, err := repo.CryptoService.GetPrivateKey(keyIDs[0]) + if isRemoteKey { + assert.Error(t, err) + assert.Nil(t, key) + } else { + assert.NoError(t, err) + assert.NotNil(t, key) + } + } // Confirm changelist dir empty after publishing changes changes := getChanges(t, repo) assert.Len(t, changes, 0, "wrong number of changelist files found") } -// Initialize a repo -// Publish some content (so that the server has a root.json) -// Rotate keys -// Download the latest metadata and assert that the keys have changed. -func TestRotateAfterPublish(t *testing.T) { - // Temporary directory where test files will be created - tempBaseDir, err := ioutil.TempDir("", "notary-test-") - defer os.RemoveAll(tempBaseDir) - - assert.NoError(t, err, "failed to create a temporary directory: %s", err) - - gun := "docker.com/notary" - - ts := fullTestServer(t) - defer ts.Close() - - repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, false) - - // Adding a target will allow us to confirm the repository is still valid after - // rotating the keys. - addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt") - - // Publish - err = repo.Publish() - assert.NoError(t, err) - - repo.GetTargetByName("latest") // force a pull - assertRotationSuccessful(t, repo) -} - // Initialize repo to have the server sign snapshots (remote snapshot key) // Without downloading a server-signed snapshot file, rotate keys so that // snapshots are locally signed (local snapshot key) // Assert that we can publish. -func TestPublishAfterChangedFromRemoteKeyToLocalKey(t *testing.T) { +func TestRotateBeforePublishFromRemoteKeyToLocalKey(t *testing.T) { // Temporary directory where test files will be created tempBaseDir, err := ioutil.TempDir("", "notary-test-") defer os.RemoveAll(tempBaseDir) @@ -1101,12 +1113,74 @@ func TestPublishAfterChangedFromRemoteKeyToLocalKey(t *testing.T) { // Adding a target will allow us to confirm the repository is still valid // after rotating the keys. addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt") - assertRotationSuccessful(t, repo) + assertRotationSuccessful(t, repo, map[string]bool{ + data.CanonicalTargetsRole: false, + data.CanonicalSnapshotRole: false}) +} + +// Initialize a repo, locally signed snapshots +// Publish some content (so that the server has a root.json), and download root.json +// Rotate keys +// Download the latest metadata and assert that the keys have changed. +func TestRotateKeyAfterPublishNoServerManagementChange(t *testing.T) { + // rotate a single target key + testRotateKeySuccess(t, false, map[string]bool{data.CanonicalTargetsRole: false}) + testRotateKeySuccess(t, false, map[string]bool{data.CanonicalSnapshotRole: false}) + // rotate two at once before publishing + testRotateKeySuccess(t, false, map[string]bool{ + data.CanonicalSnapshotRole: false, + data.CanonicalTargetsRole: false}) +} + +// Tests rotating keys when there's a change from locally managed keys to +// remotely managed keys and vice versa +// Before rotating, publish some content (so that the server has a root.json), +// and download root.json +func TestRotateKeyAfterPublishServerManagementChange(t *testing.T) { + // delegate snapshot key management to the server + testRotateKeySuccess(t, false, map[string]bool{ + data.CanonicalSnapshotRole: true, + data.CanonicalTargetsRole: false, + }) + // reclaim snapshot key management from the server + testRotateKeySuccess(t, true, map[string]bool{ + data.CanonicalSnapshotRole: false, + data.CanonicalTargetsRole: false, + }) +} + +func testRotateKeySuccess(t *testing.T, serverManagesSnapshotInit bool, + keysToRotate map[string]bool) { + + // Temporary directory where test files will be created + tempBaseDir, err := ioutil.TempDir("", "notary-test-") + defer os.RemoveAll(tempBaseDir) + + assert.NoError(t, err, "failed to create a temporary directory: %s", err) + + gun := "docker.com/notary" + ts := fullTestServer(t) + defer ts.Close() + + repo, _ := initializeRepo(t, data.ECDSAKey, tempBaseDir, gun, ts.URL, + serverManagesSnapshotInit) + + // Adding a target will allow us to confirm the repository is still valid after + // rotating the keys. + addTarget(t, repo, "latest", "../fixtures/intermediate-ca.crt") + + // Publish + err = repo.Publish() + assert.NoError(t, err) + + // Get root.json and capture targets + snapshot key IDs + repo.GetTargetByName("latest") // force a pull + assertRotationSuccessful(t, repo, keysToRotate) } // If there is no local cache, notary operations return the remote error code func TestRemoteServerUnavailableNoLocalCache(t *testing.T) { - tempBaseDir, err := ioutil.TempDir("", "notary-test-") + tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-") assert.NoError(t, err, "failed to create a temporary directory: %s", err) defer os.RemoveAll(tempBaseDir) From 2ce02329726e5718afae59cf375f717050a55d12 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Tue, 8 Dec 2015 20:26:19 -0800 Subject: [PATCH 2/5] Refactor notary CLI keys cmds to use less globally mutable state. This way we can test the command functions more easily. Signed-off-by: Ying Li --- cmd/notary/keys.go | 312 +++++++++++++++++++++++++++------------------ cmd/notary/main.go | 12 +- 2 files changed, 197 insertions(+), 127 deletions(-) diff --git a/cmd/notary/keys.go b/cmd/notary/keys.go index b028a606ca..22d7d7adad 100644 --- a/cmd/notary/keys.go +++ b/cmd/notary/keys.go @@ -17,112 +17,149 @@ import ( "github.com/docker/notary/tuf/data" "github.com/spf13/cobra" + "github.com/spf13/viper" ) -func init() { - cmdKey.AddCommand(cmdKeyList) - cmdKey.AddCommand(cmdKeyGenerateRootKey) - - cmdKeysBackup.Flags().StringVarP(&keysExportGUN, "gun", "g", "", "Globally Unique Name to export keys for") - cmdKey.AddCommand(cmdKeysBackup) - cmdKey.AddCommand(cmdKeyExportRoot) - cmdKeyExportRoot.Flags().BoolVarP(&keysExportRootChangePassphrase, "change-passphrase", "p", false, "Set a new passphrase for the key being exported") - cmdKey.AddCommand(cmdKeysRestore) - cmdKey.AddCommand(cmdKeyImportRoot) - cmdKey.AddCommand(cmdRotateKey) - cmdKey.AddCommand(cmdKeyRemove) +type usageTemplate struct { + Use string + Short string + Long string } -var cmdKey = &cobra.Command{ +type cobraRunE func(cmd *cobra.Command, args []string) error + +func (u usageTemplate) ToCommand(run cobraRunE) *cobra.Command { + c := cobra.Command{ + Use: u.Use, + Short: u.Short, + Long: u.Long, + } + if run != nil { + // newer versions of cobra support a run function that returns an error, + // but in the meantime, this should help ease the transition + c.Run = func(cmd *cobra.Command, args []string) { + err := run(cmd, args) + if err != nil { + cmd.Usage() + fatalf(err.Error()) + } + } + } + return &c +} + +var cmdKeyTemplate = usageTemplate{ Use: "key", Short: "Operates on keys.", Long: `Operations on private keys.`, } -var cmdKeyList = &cobra.Command{ +var cmdKeyListTemplate = usageTemplate{ Use: "list", Short: "Lists keys.", Long: "Lists all keys known to notary.", - Run: keysList, } -var cmdRotateKey = &cobra.Command{ +var cmdRotateKeyTemplate = usageTemplate{ Use: "rotate [ GUN ]", Short: "Rotate all the signing (non-root) keys for the given Globally Unique Name.", Long: "Removes all old signing (non-root) keys for the given Globally Unique Name, and generates new ones. This only makes local changes - please use then `notary publish` to push the key rotation changes to the remote server.", - Run: keysRotate, } -var cmdKeyGenerateRootKey = &cobra.Command{ +var cmdKeyGenerateRootKeyTemplate = usageTemplate{ Use: "generate [ algorithm ]", Short: "Generates a new root key with a given algorithm.", Long: "Generates a new root key with a given algorithm. If hardware key storage (e.g. a Yubikey) is available, the key will be stored both on hardware and on disk (so that it can be backed up). Please make sure to back up and then remove this on-key disk immediately afterwards.", - Run: keysGenerateRootKey, } -var keysExportGUN string - -var cmdKeysBackup = &cobra.Command{ +var cmdKeysBackupTemplate = usageTemplate{ Use: "backup [ zipfilename ]", Short: "Backs up all your on-disk keys to a ZIP file.", Long: "Backs up all of your accessible of keys. The keys are reencrypted with a new passphrase. The output is a ZIP file. If the --gun option is passed, only signing keys and no root keys will be backed up. Does not work on keys that are only in hardware (e.g. Yubikeys).", - Run: keysBackup, } -var keysExportRootChangePassphrase bool - -var cmdKeyExportRoot = &cobra.Command{ +var cmdKeyExportRootTemplate = usageTemplate{ Use: "export [ keyID ] [ pemfilename ]", Short: "Export a root key on disk to a PEM file.", Long: "Exports a single root key on disk, without reencrypting. The output is a PEM file. Does not work on keys that are only in hardware (e.g. Yubikeys).", - Run: keysExportRoot, } -var cmdKeysRestore = &cobra.Command{ +var cmdKeysRestoreTemplate = usageTemplate{ Use: "restore [ zipfilename ]", Short: "Restore multiple keys from a ZIP file.", Long: "Restores one or more keys from a ZIP file. If hardware key storage (e.g. a Yubikey) is available, root keys will be imported into the hardware, but not backed up to disk in the same location as the other, non-root keys.", - Run: keysRestore, } -var cmdKeyImportRoot = &cobra.Command{ +var cmdKeyImportRootTemplate = usageTemplate{ Use: "import [ pemfilename ]", Short: "Imports a root key from a PEM file.", Long: "Imports a single root key from a PEM file. If a hardware key storage (e.g. Yubikey) is available, the root key will be imported into the hardware but not backed up on disk again.", - Run: keysImportRoot, } -var cmdKeyRemove = &cobra.Command{ +var cmdKeyRemoveTemplate = usageTemplate{ Use: "remove [ keyID ]", Short: "Removes the key with the given keyID.", Long: "Removes the key with the given keyID. If the key is stored in more than one location, you will be asked which one to remove.", - Run: keyRemove, } -func keysList(cmd *cobra.Command, args []string) { +type keyCommander struct { + // these need to be set + configGetter func() *viper.Viper + retriever passphrase.Retriever + remoteServer string + + // these are for command line parsing - no need to set + keysExportRootChangePassphrase bool + keysExportGUN string +} + +func (k *keyCommander) GetCommand() *cobra.Command { + cmd := cmdKeyTemplate.ToCommand(nil) + cmd.AddCommand(cmdKeyListTemplate.ToCommand(k.keysList)) + cmd.AddCommand(cmdKeyGenerateRootKeyTemplate.ToCommand(k.keysGenerateRootKey)) + cmd.AddCommand(cmdKeysRestoreTemplate.ToCommand(k.keysRestore)) + cmd.AddCommand(cmdKeyImportRootTemplate.ToCommand(k.keysImportRoot)) + cmd.AddCommand(cmdKeyRemoveTemplate.ToCommand(k.keyRemove)) + cmd.AddCommand(cmdRotateKeyTemplate.ToCommand(k.keysRotate)) + + cmdKeysBackup := cmdKeysBackupTemplate.ToCommand(k.keysBackup) + cmdKeysBackup.Flags().StringVarP( + &k.keysExportGUN, "gun", "g", "", "Globally Unique Name to export keys for") + cmd.AddCommand(cmdKeysBackup) + + cmdKeyExportRoot := cmdKeyExportRootTemplate.ToCommand(k.keysExportRoot) + cmdKeyExportRoot.Flags().BoolVarP( + &k.keysExportRootChangePassphrase, "change-passphrase", "p", false, + "Set a new passphrase for the key being exported") + cmd.AddCommand(cmdKeyExportRoot) + return cmd +} + +func (k *keyCommander) keysList(cmd *cobra.Command, args []string) error { if len(args) > 0 { - cmd.Usage() - os.Exit(1) + return fmt.Errorf("") } - parseConfig() + config := k.configGetter() + ks, err := k.getKeyStores(config, true) + if err != nil { + return err + } - stores := getKeyStores(cmd, mainViper.GetString("trust_dir"), retriever, true) cmd.Println("") - prettyPrintKeys(stores, cmd.Out()) + prettyPrintKeys(ks, cmd.Out()) cmd.Println("") + return nil } -func keysGenerateRootKey(cmd *cobra.Command, args []string) { +func (k *keyCommander) keysGenerateRootKey(cmd *cobra.Command, args []string) error { // We require one or no arguments (since we have a default value), but if the // user passes in more than one argument, we error out. if len(args) > 1 { - cmd.Usage() - fatalf("Please provide only one Algorithm as an argument to generate (rsa, ecdsa)") + return fmt.Errorf( + "Please provide only one Algorithm as an argument to generate (rsa, ecdsa)") } - parseConfig() - // If no param is given to generate, generates an ecdsa key by default algorithm := data.ECDSAKey @@ -137,49 +174,50 @@ func keysGenerateRootKey(cmd *cobra.Command, args []string) { } if !allowedCiphers[strings.ToLower(algorithm)] { - fatalf("Algorithm not allowed, possible values are: RSA, ECDSA") + return fmt.Errorf("Algorithm not allowed, possible values are: RSA, ECDSA") } - parseConfig() - - cs := cryptoservice.NewCryptoService( - "", - getKeyStores(cmd, mainViper.GetString("trust_dir"), retriever, true)..., - ) + config := k.configGetter() + ks, err := k.getKeyStores(config, true) + if err != nil { + return err + } + cs := cryptoservice.NewCryptoService("", ks...) pubKey, err := cs.Create(data.CanonicalRootRole, algorithm) if err != nil { - fatalf("Failed to create a new root key: %v", err) + return fmt.Errorf("Failed to create a new root key: %v", err) } cmd.Printf("Generated new %s root key with keyID: %s\n", algorithm, pubKey.ID()) + return nil } // keysBackup exports a collection of keys to a ZIP file -func keysBackup(cmd *cobra.Command, args []string) { +func (k *keyCommander) keysBackup(cmd *cobra.Command, args []string) error { if len(args) < 1 { - cmd.Usage() - fatalf("Must specify output filename for export") + return fmt.Errorf("Must specify output filename for export") } - parseConfig() + config := k.configGetter() + ks, err := k.getKeyStores(config, false) + if err != nil { + return err + } exportFilename := args[0] - cs := cryptoservice.NewCryptoService( - "", - getKeyStores(cmd, mainViper.GetString("trust_dir"), retriever, false)..., - ) + cs := cryptoservice.NewCryptoService("", ks...) exportFile, err := os.Create(exportFilename) if err != nil { - fatalf("Error creating output file: %v", err) + return fmt.Errorf("Error creating output file: %v", err) } // Must use a different passphrase retriever to avoid caching the // unlocking passphrase and reusing that. exportRetriever := getRetriever() - if keysExportGUN != "" { - err = cs.ExportKeysByGUN(exportFile, keysExportGUN, exportRetriever) + if k.keysExportGUN != "" { + err = cs.ExportKeysByGUN(exportFile, k.keysExportGUN, exportRetriever) } else { err = cs.ExportAllKeys(exportFile, exportRetriever) } @@ -188,38 +226,36 @@ func keysBackup(cmd *cobra.Command, args []string) { if err != nil { os.Remove(exportFilename) - fatalf("Error exporting keys: %v", err) + return fmt.Errorf("Error exporting keys: %v", err) } + return nil } // keysExportRoot exports a root key by ID to a PEM file -func keysExportRoot(cmd *cobra.Command, args []string) { +func (k *keyCommander) keysExportRoot(cmd *cobra.Command, args []string) error { if len(args) < 2 { - cmd.Usage() - fatalf("Must specify key ID and output filename for export") + return fmt.Errorf("Must specify key ID and output filename for export") } - parseConfig() - keyID := args[0] exportFilename := args[1] if len(keyID) != idSize { - fatalf("Please specify a valid root key ID") + return fmt.Errorf("Please specify a valid root key ID") } - parseConfig() - - cs := cryptoservice.NewCryptoService( - "", - getKeyStores(cmd, mainViper.GetString("trust_dir"), retriever, false)..., - ) + config := k.configGetter() + ks, err := k.getKeyStores(config, true) + if err != nil { + return err + } + cs := cryptoservice.NewCryptoService("", ks...) exportFile, err := os.Create(exportFilename) if err != nil { - fatalf("Error creating output file: %v", err) + return fmt.Errorf("Error creating output file: %v", err) } - if keysExportRootChangePassphrase { + if k.keysExportRootChangePassphrase { // Must use a different passphrase retriever to avoid caching the // unlocking passphrase and reusing that. exportRetriever := getRetriever() @@ -230,83 +266,106 @@ func keysExportRoot(cmd *cobra.Command, args []string) { exportFile.Close() if err != nil { os.Remove(exportFilename) - fatalf("Error exporting root key: %v", err) + return fmt.Errorf("Error exporting root key: %v", err) } + return nil } // keysRestore imports keys from a ZIP file -func keysRestore(cmd *cobra.Command, args []string) { +func (k *keyCommander) keysRestore(cmd *cobra.Command, args []string) error { if len(args) < 1 { - cmd.Usage() - fatalf("Must specify input filename for import") + return fmt.Errorf("Must specify input filename for import") } importFilename := args[0] - parseConfig() - - cs := cryptoservice.NewCryptoService( - "", - getKeyStores(cmd, mainViper.GetString("trust_dir"), retriever, true)..., - ) + config := k.configGetter() + ks, err := k.getKeyStores(config, true) + if err != nil { + return err + } + cs := cryptoservice.NewCryptoService("", ks...) zipReader, err := zip.OpenReader(importFilename) if err != nil { - fatalf("Opening file for import: %v", err) + return fmt.Errorf("Opening file for import: %v", err) } defer zipReader.Close() err = cs.ImportKeysZip(zipReader.Reader) if err != nil { - fatalf("Error importing keys: %v", err) + return fmt.Errorf("Error importing keys: %v", err) } + return nil } // keysImportRoot imports a root key from a PEM file -func keysImportRoot(cmd *cobra.Command, args []string) { +func (k *keyCommander) keysImportRoot(cmd *cobra.Command, args []string) error { if len(args) != 1 { - cmd.Usage() - fatalf("Must specify input filename for import") + return fmt.Errorf("Must specify input filename for import") } - parseConfig() - - cs := cryptoservice.NewCryptoService( - "", - getKeyStores(cmd, mainViper.GetString("trust_dir"), retriever, true)..., - ) + config := k.configGetter() + ks, err := k.getKeyStores(config, true) + if err != nil { + return err + } + cs := cryptoservice.NewCryptoService("", ks...) importFilename := args[0] importFile, err := os.Open(importFilename) if err != nil { - fatalf("Opening file for import: %v", err) + return fmt.Errorf("Opening file for import: %v", err) } defer importFile.Close() err = cs.ImportRootKey(importFile) if err != nil { - fatalf("Error importing root key: %v", err) + return fmt.Errorf("Error importing root key: %v", err) } + return nil } -func keysRotate(cmd *cobra.Command, args []string) { +func (k *keyCommander) keysRotate(cmd *cobra.Command, args []string) error { if len(args) < 1 { - cmd.Usage() - fatalf("Must specify a GUN and target") + return fmt.Errorf("Must specify a GUN") } - parseConfig() + rotateKeyRole := strings.ToLower(k.rotateKeyRole) + + var rolesToRotate []string + switch rotateKeyRole { + case "": + rolesToRotate = []string{data.CanonicalSnapshotRole, data.CanonicalTargetsRole} + case data.CanonicalSnapshotRole: + rolesToRotate = []string{data.CanonicalSnapshotRole} + case data.CanonicalTargetsRole: + rolesToRotate = []string{data.CanonicalTargetsRole} + default: + cmd.Usage() + fatalf(`key rotation not supported for %s keys`, rotateKeyRole) + } + if k.rotateKeyServerManaged && rotateKeyRole != data.CanonicalSnapshotRole { + return fmt.Errorf( + "remote signing/key management is only supported for the snapshot key") + } + + config := k.configGetter() gun := args[0] - nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, remoteTrustServer, nil, retriever) + nRepo, err := notaryclient.NewNotaryRepository( + config.GetString("trust_dir"), gun, k.remoteServer, nil, retriever) if err != nil { - fatalf(err.Error()) + return err } - if err := nRepo.RotateKeys(); err != nil { - fatalf(err.Error()) + for _, role := range rolesToRotate { + if err := nRepo.RotateKey(role, k.rotateKeyServerManaged); err != nil { + return err + } } + return nil } func removeKeyInteractively(keyStores []trustmanager.KeyStore, keyID string, @@ -382,40 +441,43 @@ func removeKeyInteractively(keyStores []trustmanager.KeyStore, keyID string, } // keyRemove deletes a private key based on ID -func keyRemove(cmd *cobra.Command, args []string) { +func (k *keyCommander) keyRemove(cmd *cobra.Command, args []string) error { if len(args) < 1 { - cmd.Usage() - fatalf("must specify the key ID of the key to remove") + return fmt.Errorf("must specify the key ID of the key to remove") } - parseConfig() + config := k.configGetter() + ks, err := k.getKeyStores(config, true) + if err != nil { + return err + } keyID := args[0] // This is an invalid ID if len(keyID) != idSize { - fatalf("invalid key ID provided: %s", keyID) + return fmt.Errorf("invalid key ID provided: %s", keyID) } - stores := getKeyStores(cmd, mainViper.GetString("trust_dir"), retriever, true) cmd.Println("") - err := removeKeyInteractively(stores, keyID, os.Stdin, cmd.Out()) + err = removeKeyInteractively(ks, keyID, os.Stdin, + cmd.Out()) cmd.Println("") - if err != nil { - fatalf(err.Error()) - } + return err } -func getKeyStores(cmd *cobra.Command, directory string, - ret passphrase.Retriever, withHardware bool) []trustmanager.KeyStore { +func (k *keyCommander) getKeyStores( + config *viper.Viper, withHardware bool) ([]trustmanager.KeyStore, error) { - fileKeyStore, err := trustmanager.NewKeyFileStore(directory, ret) + directory := config.GetString("trust_dir") + fileKeyStore, err := trustmanager.NewKeyFileStore(directory, k.retriever) if err != nil { - fatalf("Failed to create private key store in directory: %s", directory) + return nil, fmt.Errorf( + "Failed to create private key store in directory: %s", directory) } ks := []trustmanager.KeyStore{fileKeyStore} if withHardware { - yubiStore, err := getYubiKeyStore(fileKeyStore, ret) + yubiStore, err := getYubiKeyStore(fileKeyStore, k.retriever) if err == nil && yubiStore != nil { // Note that the order is important, since we want to prioritize // the yubikey store @@ -423,5 +485,5 @@ func getKeyStores(cmd *cobra.Command, directory string, } } - return ks + return ks, nil } diff --git a/cmd/notary/main.go b/cmd/notary/main.go index 64afff1470..e22f5436a4 100644 --- a/cmd/notary/main.go +++ b/cmd/notary/main.go @@ -37,7 +37,7 @@ func init() { retriever = getPassphraseRetriever() } -func parseConfig() { +func parseConfig() *viper.Viper { if verbose { logrus.SetLevel(logrus.DebugLevel) logrus.SetOutput(os.Stderr) @@ -94,6 +94,8 @@ func parseConfig() { mainViper.Set("trust_dir", expandedTrustDir) } logrus.Debugf("Using the following trust directory: %s", mainViper.GetString("trust_dir")) + + return mainViper } func setupCommand(notaryCmd *cobra.Command) { @@ -113,7 +115,13 @@ func setupCommand(notaryCmd *cobra.Command) { notaryCmd.PersistentFlags().BoolVarP(&verbose, "verbose", "v", false, "Verbose output") notaryCmd.PersistentFlags().StringVarP(&remoteTrustServer, "server", "s", "", "Remote trust server location") - notaryCmd.AddCommand(cmdKey) + cmdKeyGenerator := &keyCommander{ + configGetter: parseConfig, + retriever: retriever, + remoteServer: remoteTrustServer, + } + + notaryCmd.AddCommand(cmdKeyGenerator.GetCommand()) notaryCmd.AddCommand(cmdCert) notaryCmd.AddCommand(cmdTufInit) notaryCmd.AddCommand(cmdTufList) From ca1623e17bc017a9144697ef5286b12eee3008b8 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 9 Dec 2015 00:24:10 -0800 Subject: [PATCH 3/5] Update CLI rotate key command to optionally rotate a single key. This makes it possible to delegate snapshots key management to the server, and to reclaim the responsibility. Signed-off-by: Ying Li --- cmd/notary/keys.go | 33 +++++-- cmd/notary/keys_test.go | 185 ++++++++++++++++++++++++++++++++++++++++ cmd/notary/main.go | 1 - cmd/notary/tuf.go | 38 +++++---- 4 files changed, 231 insertions(+), 26 deletions(-) diff --git a/cmd/notary/keys.go b/cmd/notary/keys.go index 22d7d7adad..e690a630ad 100644 --- a/cmd/notary/keys.go +++ b/cmd/notary/keys.go @@ -5,6 +5,7 @@ import ( "bufio" "fmt" "io" + "net/http" "os" "path/filepath" "strconv" @@ -62,8 +63,8 @@ var cmdKeyListTemplate = usageTemplate{ var cmdRotateKeyTemplate = usageTemplate{ Use: "rotate [ GUN ]", - Short: "Rotate all the signing (non-root) keys for the given Globally Unique Name.", - Long: "Removes all old signing (non-root) keys for the given Globally Unique Name, and generates new ones. This only makes local changes - please use then `notary publish` to push the key rotation changes to the remote server.", + Short: "Rotate the signing (non-root) keys for the given Globally Unique Name.", + Long: "Removes all the old signing (non-root) keys for the given Globally Unique Name, and generates new ones. This only makes local changes - please use then `notary publish` to push the key rotation changes to the remote server.", } var cmdKeyGenerateRootKeyTemplate = usageTemplate{ @@ -106,11 +107,12 @@ type keyCommander struct { // these need to be set configGetter func() *viper.Viper retriever passphrase.Retriever - remoteServer string // these are for command line parsing - no need to set keysExportRootChangePassphrase bool keysExportGUN string + rotateKeyRole string + rotateKeyServerManaged bool } func (k *keyCommander) GetCommand() *cobra.Command { @@ -120,7 +122,6 @@ func (k *keyCommander) GetCommand() *cobra.Command { cmd.AddCommand(cmdKeysRestoreTemplate.ToCommand(k.keysRestore)) cmd.AddCommand(cmdKeyImportRootTemplate.ToCommand(k.keysImportRoot)) cmd.AddCommand(cmdKeyRemoveTemplate.ToCommand(k.keyRemove)) - cmd.AddCommand(cmdRotateKeyTemplate.ToCommand(k.keysRotate)) cmdKeysBackup := cmdKeysBackupTemplate.ToCommand(k.keysBackup) cmdKeysBackup.Flags().StringVarP( @@ -132,6 +133,18 @@ func (k *keyCommander) GetCommand() *cobra.Command { &k.keysExportRootChangePassphrase, "change-passphrase", "p", false, "Set a new passphrase for the key being exported") cmd.AddCommand(cmdKeyExportRoot) + + cmdRotateKey := cmdRotateKeyTemplate.ToCommand(k.keysRotate) + cmdRotateKey.Flags().BoolVarP(&k.rotateKeyServerManaged, "server-managed", "r", + false, "Signing and key management will be handled by the remote server. "+ + "(no key will be generated or stored locally) "+ + "Can only be used in conjunction with --key-type.") + cmdRotateKey.Flags().StringVarP(&k.rotateKeyRole, "key-type", "t", "", + `Key type to rotate. Supported values: "targets", "snapshots". `+ + `If not provided, both targets and snapshot keys will be rotated, `+ + `and the new keys will be locally generated and stored.`) + cmd.AddCommand(cmdRotateKey) + return cmd } @@ -344,8 +357,7 @@ func (k *keyCommander) keysRotate(cmd *cobra.Command, args []string) error { case data.CanonicalTargetsRole: rolesToRotate = []string{data.CanonicalTargetsRole} default: - cmd.Usage() - fatalf(`key rotation not supported for %s keys`, rotateKeyRole) + return fmt.Errorf("key rotation not supported for %s keys", k.rotateKeyRole) } if k.rotateKeyServerManaged && rotateKeyRole != data.CanonicalSnapshotRole { return fmt.Errorf( @@ -355,8 +367,15 @@ func (k *keyCommander) keysRotate(cmd *cobra.Command, args []string) error { config := k.configGetter() gun := args[0] + 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 = getTransport(config, gun, true) + } nRepo, err := notaryclient.NewNotaryRepository( - config.GetString("trust_dir"), gun, k.remoteServer, nil, retriever) + config.GetString("trust_dir"), gun, getRemoteTrustServer(config), + rt, k.retriever) if err != nil { return err } diff --git a/cmd/notary/keys_test.go b/cmd/notary/keys_test.go index 1b791aa9a1..111778cce0 100644 --- a/cmd/notary/keys_test.go +++ b/cmd/notary/keys_test.go @@ -3,12 +3,21 @@ package main import ( "bytes" "crypto/rand" + "fmt" "io/ioutil" + "net/http" + "net/http/httptest" + "os" "strings" "testing" + "github.com/docker/notary/client" "github.com/docker/notary/passphrase" "github.com/docker/notary/trustmanager" + "github.com/docker/notary/tuf/data" + "github.com/jfrazelle/go/canonical/json" + "github.com/spf13/cobra" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" ) @@ -216,3 +225,179 @@ func TestRemoveMultikeysRemoveOnlyChosenKey(t *testing.T) { } } } + +// Non-roles, root, and timestamp can't be rotated +func TestRotateKeyInvalidRoles(t *testing.T) { + invalids := []string{ + data.CanonicalRootRole, + data.CanonicalTimestampRole, + "notevenARole", + } + for _, role := range invalids { + for _, serverManaged := range []bool{true, false} { + k := &keyCommander{ + configGetter: viper.New, + retriever: passphrase.ConstantRetriever("pass"), + rotateKeyRole: role, + rotateKeyServerManaged: serverManaged, + } + err := k.keysRotate(&cobra.Command{}, []string{"gun"}) + assert.Error(t, err) + assert.Contains(t, err.Error(), + fmt.Sprintf("key rotation not supported for %s keys", role)) + } + } +} + +// Cannot rotate a targets key and require that the server manage it +func TestRotateKeyTargetCannotBeServerManaged(t *testing.T) { + k := &keyCommander{ + configGetter: viper.New, + retriever: passphrase.ConstantRetriever("pass"), + rotateKeyRole: data.CanonicalTargetsRole, + rotateKeyServerManaged: true, + } + err := k.keysRotate(&cobra.Command{}, []string{"gun"}) + assert.Error(t, err) + assert.Contains(t, err.Error(), + "remote signing/key management is only supported for the snapshot key") +} + +// rotate key must be provided with a gun +func TestRotateKeyNoGUN(t *testing.T) { + k := &keyCommander{ + configGetter: viper.New, + retriever: passphrase.ConstantRetriever("pass"), + rotateKeyRole: data.CanonicalTargetsRole, + } + err := k.keysRotate(&cobra.Command{}, []string{}) + assert.Error(t, err) + assert.Contains(t, err.Error(), "Must specify a GUN") +} + +// initialize a repo with keys, so they can be rotated +func setUpRepo(t *testing.T, tempBaseDir, gun string, ret passphrase.Retriever) ( + *httptest.Server, map[string]string) { + + // server that always returns 200 (and a key) + key, err := trustmanager.GenerateECDSAKey(rand.Reader) + assert.NoError(t, err) + pubKey := data.PublicKeyFromPrivate(key) + jsonBytes, err := json.MarshalCanonical(&pubKey) + assert.NoError(t, err) + keyJSON := string(jsonBytes) + ts := httptest.NewServer(http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + fmt.Fprint(w, keyJSON) + })) + + repo, err := client.NewNotaryRepository( + tempBaseDir, gun, ts.URL, http.DefaultTransport, ret) + assert.NoError(t, err, "error creating repo: %s", err) + + rootPubKey, err := repo.CryptoService.Create("root", data.ECDSAKey) + assert.NoError(t, err, "error generating root key: %s", err) + + err = repo.Initialize(rootPubKey.ID()) + assert.NoError(t, err) + + return ts, repo.CryptoService.ListAllKeys() +} + +// The command line uses NotaryRepository's RotateKey - this is just testing +// that the correct config variables are passed for the client to request a key +// from the remote server. +func TestRotateKeyRemoteServerManagesKey(t *testing.T) { + // Temporary directory where test files will be created + tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-") + defer os.RemoveAll(tempBaseDir) + assert.NoError(t, err, "failed to create a temporary directory: %s", err) + gun := "docker.com/notary" + + ret := passphrase.ConstantRetriever("pass") + + ts, initialKeys := setUpRepo(t, tempBaseDir, gun, ret) + defer ts.Close() + + k := &keyCommander{ + configGetter: func() *viper.Viper { + v := viper.New() + v.SetDefault("trust_dir", tempBaseDir) + v.SetDefault("remote_server.url", ts.URL) + return v + }, + retriever: ret, + rotateKeyRole: data.CanonicalSnapshotRole, + rotateKeyServerManaged: true, + } + err = k.keysRotate(&cobra.Command{}, []string{gun}) + assert.NoError(t, err) + + repo, err := client.NewNotaryRepository(tempBaseDir, gun, ts.URL, nil, ret) + assert.NoError(t, err, "error creating repo: %s", err) + + cl, err := repo.GetChangelist() + assert.NoError(t, err, "unable to get changelist: %v", err) + assert.Len(t, cl.List(), 1) + // no keys have been created, since a remote key was specified + assert.Equal(t, initialKeys, repo.CryptoService.ListAllKeys()) +} + +// The command line uses NotaryRepository's RotateKey - this is just testing +// that the correct config variables are passed for the client to rotate +// both the targets and snapshot key, and create them locally +func TestRotateKeyBothKeys(t *testing.T) { + // Temporary directory where test files will be created + tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-") + defer os.RemoveAll(tempBaseDir) + assert.NoError(t, err, "failed to create a temporary directory: %s", err) + gun := "docker.com/notary" + + 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() + + k := &keyCommander{ + configGetter: func() *viper.Viper { + v := viper.New() + v.SetDefault("trust_dir", tempBaseDir) + // won't need a remote server URL, since we are creating local keys + return v + }, + retriever: ret, + } + err = k.keysRotate(&cobra.Command{}, []string{gun}) + assert.NoError(t, err) + + repo, err := client.NewNotaryRepository(tempBaseDir, gun, ts.URL, nil, ret) + assert.NoError(t, err, "error creating repo: %s", err) + + cl, err := repo.GetChangelist() + assert.NoError(t, err, "unable to get changelist: %v", err) + assert.Len(t, cl.List(), 2) + + // two new keys have been created, and the old keys should still be there + newKeys := repo.CryptoService.ListAllKeys() + 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 + } + } + assert.True(t, targetsFound, "targets key was not created") + assert.True(t, snapshotFound, "snapshot key was not created") +} diff --git a/cmd/notary/main.go b/cmd/notary/main.go index e22f5436a4..efd9eb97cc 100644 --- a/cmd/notary/main.go +++ b/cmd/notary/main.go @@ -118,7 +118,6 @@ func setupCommand(notaryCmd *cobra.Command) { cmdKeyGenerator := &keyCommander{ configGetter: parseConfig, retriever: retriever, - remoteServer: remoteTrustServer, } notaryCmd.AddCommand(cmdKeyGenerator.GetCommand()) diff --git a/cmd/notary/tuf.go b/cmd/notary/tuf.go index 4f59076f00..e67fed333f 100644 --- a/cmd/notary/tuf.go +++ b/cmd/notary/tuf.go @@ -23,6 +23,7 @@ import ( "github.com/docker/notary/tuf/data" "github.com/docker/notary/utils" "github.com/spf13/cobra" + "github.com/spf13/viper" ) var cmdTufList = &cobra.Command{ @@ -94,7 +95,7 @@ func tufAdd(cmd *cobra.Command, args []string) { // no online operations are performed by add so the transport argument // should be nil - nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(), nil, retriever) + nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(mainViper), nil, retriever) if err != nil { fatalf(err.Error()) } @@ -121,7 +122,7 @@ func tufInit(cmd *cobra.Command, args []string) { parseConfig() gun := args[0] - nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(), getTransport(gun, false), retriever) + nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(mainViper), getTransport(mainViper, gun, false), retriever) if err != nil { fatalf(err.Error()) } @@ -157,7 +158,7 @@ func tufList(cmd *cobra.Command, args []string) { parseConfig() gun := args[0] - nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(), getTransport(gun, true), retriever) + nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(mainViper), getTransport(mainViper, gun, true), retriever) if err != nil { fatalf(err.Error()) } @@ -181,7 +182,7 @@ func tufLookup(cmd *cobra.Command, args []string) { gun := args[0] targetName := args[1] - nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(), getTransport(gun, true), retriever) + nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(mainViper), getTransport(mainViper, gun, true), retriever) if err != nil { fatalf(err.Error()) } @@ -203,7 +204,7 @@ func tufStatus(cmd *cobra.Command, args []string) { parseConfig() gun := args[0] - nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(), nil, retriever) + nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(mainViper), nil, retriever) if err != nil { fatalf(err.Error()) } @@ -237,7 +238,7 @@ func tufPublish(cmd *cobra.Command, args []string) { cmd.Println("Pushing changes to", gun) - nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(), getTransport(gun, false), retriever) + nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(mainViper), getTransport(mainViper, gun, false), retriever) if err != nil { fatalf(err.Error()) } @@ -260,7 +261,7 @@ func tufRemove(cmd *cobra.Command, args []string) { // no online operation are performed by remove so the transport argument // should be nil. - repo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(), nil, retriever) + repo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(mainViper), nil, retriever) if err != nil { fatalf(err.Error()) } @@ -288,7 +289,7 @@ func verify(cmd *cobra.Command, args []string) { gun := args[0] targetName := args[1] - nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(), getTransport(gun, true), retriever) + nRepo, err := notaryclient.NewNotaryRepository(mainViper.GetString("trust_dir"), gun, getRemoteTrustServer(mainViper), getTransport(mainViper, gun, true), retriever) if err != nil { fatalf(err.Error()) } @@ -353,9 +354,9 @@ func (ps passwordStore) Basic(u *url.URL) (string, string) { return username, password } -func getTransport(gun string, readOnly bool) http.RoundTripper { +func getTransport(config *viper.Viper, gun string, readOnly bool) http.RoundTripper { // Attempt to get a root CA from the config file. Nil is the host defaults. - rootCAFile := mainViper.GetString("remote_server.root_ca") + rootCAFile := config.GetString("remote_server.root_ca") if rootCAFile != "" { // If we haven't been given an Absolute path, we assume it's relative // from the configuration directory (~/.notary by default) @@ -365,8 +366,8 @@ func getTransport(gun string, readOnly bool) http.RoundTripper { } insecureSkipVerify := false - if mainViper.IsSet("remote_server.skipTLSVerify") { - insecureSkipVerify = mainViper.GetBool("remote_server.skipTLSVerify") + if config.IsSet("remote_server.skipTLSVerify") { + insecureSkipVerify = config.GetBool("remote_server.skipTLSVerify") } tlsConfig, err := utils.ConfigureClientTLS(&utils.ClientTLSOpts{ RootCAFile: rootCAFile, @@ -387,18 +388,19 @@ func getTransport(gun string, readOnly bool) http.RoundTripper { TLSClientConfig: tlsConfig, DisableKeepAlives: true, } - - return tokenAuth(base, gun, readOnly) + return tokenAuth(config, base, gun, readOnly) } -func tokenAuth(baseTransport *http.Transport, gun string, readOnly bool) http.RoundTripper { +func tokenAuth(config *viper.Viper, baseTransport *http.Transport, gun string, + readOnly bool) http.RoundTripper { + // TODO(dmcgowan): add notary specific headers authTransport := transport.NewTransport(baseTransport) pingClient := &http.Client{ Transport: authTransport, Timeout: 5 * time.Second, } - trustServerURL := getRemoteTrustServer() + trustServerURL := getRemoteTrustServer(config) endpoint, err := url.Parse(trustServerURL) if err != nil { fatalf("Could not parse remote trust server url (%s): %s", trustServerURL, err.Error()) @@ -433,9 +435,9 @@ func tokenAuth(baseTransport *http.Transport, gun string, readOnly bool) http.Ro return transport.NewTransport(baseTransport, modifier) } -func getRemoteTrustServer() string { +func getRemoteTrustServer(config *viper.Viper) string { if remoteTrustServer == "" { - configRemote := mainViper.GetString("remote_server.url") + configRemote := config.GetString("remote_server.url") if configRemote != "" { remoteTrustServer = configRemote } else { From 2c7e632925a1236589778855b45c89fb5ede8c4a Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 11 Dec 2015 17:56:00 -0800 Subject: [PATCH 4/5] Amend rotation tests to assert old keys are removed after rotation. Signed-off-by: Ying Li --- client/client.go | 6 +----- client/client_test.go | 17 +++++++++++++---- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/client/client.go b/client/client.go index c797c52a7f..c498a62ddb 100644 --- a/client/client.go +++ b/client/client.go @@ -653,11 +653,7 @@ func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error { return err } - err = r.rootFileKeyChange(role, changelist.ActionCreate, pubKey) - if err != nil { - return err - } - return nil + return r.rootFileKeyChange(role, changelist.ActionCreate, pubKey) } func (r *NotaryRepository) rootFileKeyChange(role, action string, key data.PublicKey) error { diff --git a/client/client_test.go b/client/client_test.go index b90bc5869c..9bbba350be 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -1055,11 +1055,10 @@ func TestRotateKeyInvalidRole(t *testing.T) { func assertRotationSuccessful(t *testing.T, repo *NotaryRepository, keysToRotate map[string]bool) { - oldKeyIDs := make(map[string]string) + oldKeyIDs := make(map[string][]string) for role := range keysToRotate { keyIDs := repo.tufRepo.Root.Signed.Roles[role].KeyIDs - assert.Len(t, keyIDs, 1) - oldKeyIDs[role] = keyIDs[0] + oldKeyIDs[role] = keyIDs } // Do rotation @@ -1078,7 +1077,17 @@ func assertRotationSuccessful(t *testing.T, repo *NotaryRepository, for role, isRemoteKey := range keysToRotate { keyIDs := repo.tufRepo.Root.Signed.Roles[role].KeyIDs assert.Len(t, keyIDs, 1) - assert.NotEqual(t, oldKeyIDs[role], keyIDs[0]) + + // the new key is not the same as any of the old keys, and the + // old keys have been removed not just from the TUF file, but + // from the cryptoservice + for _, oldKeyID := range oldKeyIDs[role] { + assert.NotEqual(t, oldKeyID, keyIDs[0]) + _, _, err := repo.CryptoService.GetPrivateKey(oldKeyID) + assert.Error(t, err) + } + + // the new key is present in the cryptoservice, or not present if remote key, _, err := repo.CryptoService.GetPrivateKey(keyIDs[0]) if isRemoteKey { assert.Error(t, err) From 63f48791c3339ad6bf927d7dccd4d0680fd1be31 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Tue, 15 Dec 2015 10:18:58 -0800 Subject: [PATCH 5/5] Fix docstring for 'key-type' parameter on key rotate. Signed-off-by: Ying Li --- cmd/notary/keys.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/notary/keys.go b/cmd/notary/keys.go index e690a630ad..ddaf53cdb1 100644 --- a/cmd/notary/keys.go +++ b/cmd/notary/keys.go @@ -140,7 +140,7 @@ func (k *keyCommander) GetCommand() *cobra.Command { "(no key will be generated or stored locally) "+ "Can only be used in conjunction with --key-type.") cmdRotateKey.Flags().StringVarP(&k.rotateKeyRole, "key-type", "t", "", - `Key type to rotate. Supported values: "targets", "snapshots". `+ + `Key type to rotate. Supported values: "targets", "snapshot". `+ `If not provided, both targets and snapshot keys will be rotated, `+ `and the new keys will be locally generated and stored.`) cmd.AddCommand(cmdRotateKey)