From 896fbde0d354f576a0e487700eee961bc264fb59 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 28 Sep 2016 16:05:48 -0700 Subject: [PATCH 1/2] Be able to read keys even if they are not in the root/non-root subdirs Signed-off-by: Ying Li --- trustmanager/keystore.go | 53 ++++------ trustmanager/keystore_test.go | 175 ++++++++++++++++++++++------------ 2 files changed, 129 insertions(+), 99 deletions(-) diff --git a/trustmanager/keystore.go b/trustmanager/keystore.go index 624949efa..03d9d9687 100644 --- a/trustmanager/keystore.go +++ b/trustmanager/keystore.go @@ -173,26 +173,12 @@ func (s *GenericKeyStore) AddKey(keyInfo KeyInfo, privKey data.PrivateKey) error func (s *GenericKeyStore) GetKey(name string) (data.PrivateKey, string, error) { s.Lock() defer s.Unlock() - // If this is a bare key ID without the gun, prepend the gun so the filestore lookup succeeds - if keyInfo, ok := s.keyInfoMap[name]; ok { - name = filepath.Join(keyInfo.Gun, name) - } - cachedKeyEntry, ok := s.cachedKeys[name] if ok { return cachedKeyEntry.key, cachedKeyEntry.alias, nil } - keyAlias, legacy, err := getKeyRole(s.store, name) - if err != nil { - return nil, "", err - } - - if legacy { - name = name + "_" + keyAlias - } - - keyBytes, err := s.store.Get(filepath.Join(getSubdir(keyAlias), name)) + keyBytes, _, keyAlias, err := getKey(s.store, name) if err != nil { return nil, "", err } @@ -218,25 +204,18 @@ func (s *GenericKeyStore) ListKeys() map[string]KeyInfo { func (s *GenericKeyStore) RemoveKey(keyID string) error { s.Lock() defer s.Unlock() - // If this is a bare key ID without the gun, prepend the gun so the filestore lookup succeeds - if keyInfo, ok := s.keyInfoMap[keyID]; ok { - keyID = filepath.Join(keyInfo.Gun, keyID) - } - role, legacy, err := getKeyRole(s.store, keyID) - if err != nil { + _, filename, _, err := getKey(s.store, keyID) + switch err.(type) { + case ErrKeyNotFound, nil: + break + default: return err } delete(s.cachedKeys, keyID) - name := keyID - if legacy { - name = keyID + "_" + role - } - - // being in a subdirectory is for backwards compatibliity - err = s.store.Remove(filepath.Join(getSubdir(role), name)) + err = s.store.Remove(filename) // removing a file that doesn't exist doesn't fail if err != nil { return err } @@ -276,11 +255,11 @@ func KeyInfoFromPEM(pemBytes []byte, filename string) (string, KeyInfo, error) { return keyID, KeyInfo{Gun: gun, Role: role}, nil } -// getKeyRole finds the role for the given keyID. It attempts to look -// both in the newer format PEM headers, and also in the legacy filename -// format. It returns: the role, whether it was found in the legacy format -// (true == legacy), and an error -func getKeyRole(s Storage, keyID string) (string, bool, error) { +// getKey finds the key and role for the given keyID. It attempts to +// look both in the newer format PEM headers, and also in the legacy filename +// format. It returns: the key bytes, the filename it was found under, the role, +// and an error +func getKey(s Storage, keyID string) ([]byte, string, string, error) { name := strings.TrimSpace(strings.TrimSuffix(filepath.Base(keyID), filepath.Ext(keyID))) for _, file := range s.ListFiles() { @@ -289,21 +268,21 @@ func getKeyRole(s Storage, keyID string) (string, bool, error) { if strings.HasPrefix(filename, name) { d, err := s.Get(file) if err != nil { - return "", false, err + return nil, "", "", err } block, _ := pem.Decode(d) if block != nil { if role, ok := block.Headers["role"]; ok { - return role, false, nil + return d, file, role, nil } } role := strings.TrimPrefix(filename, name+"_") - return role, true, nil + return d, file, role, nil } } - return "", false, ErrKeyNotFound{KeyID: keyID} + return nil, "", "", ErrKeyNotFound{KeyID: keyID} } // Assumes 2 subdirectories, 1 containing root keys and 1 containing TUF keys diff --git a/trustmanager/keystore_test.go b/trustmanager/keystore_test.go index 12ecb2297..c09b7995b 100644 --- a/trustmanager/keystore_test.go +++ b/trustmanager/keystore_test.go @@ -2,10 +2,12 @@ package trustmanager import ( "crypto/rand" + "encoding/pem" "errors" "fmt" "io/ioutil" "os" + "path" "path/filepath" "testing" @@ -167,29 +169,39 @@ func TestGet(t *testing.T) { gun := "docker.io/notary" - // Root role needs to go in the rootKeySubdir to be read. - // All other roles can go in the nonRootKeysSubdir, possibly under a GUN + // Root role currently goes into the rootKeySubdir, and all other roles go + // in the nonRootKeysSubdir, possibly under a GUN. nonRootKeysSubdirWithGUN := filepath.Clean(filepath.Join(notary.NonRootKeysSubdir, gun)) - - testGetKeyWithRole(t, "", data.CanonicalRootRole, notary.RootKeysSubdir, true) + testGetKeyWithRole(t, "", data.CanonicalRootRole, filepath.Join(notary.PrivDir, notary.RootKeysSubdir), true) for _, role := range nonRootRolesToTest { - testGetKeyWithRole(t, "", role, notary.NonRootKeysSubdir, true) - testGetKeyWithRole(t, gun, role, nonRootKeysSubdirWithGUN, true) + testGetKeyWithRole(t, "", role, filepath.Join(notary.PrivDir, notary.NonRootKeysSubdir), true) + testGetKeyWithRole(t, gun, role, filepath.Join(notary.PrivDir, nonRootKeysSubdirWithGUN), true) } - // Root cannot go in the nonRootKeysSubdir, or it won't be able to be read, - // and vice versa - testGetKeyWithRole(t, "", data.CanonicalRootRole, notary.NonRootKeysSubdir, false) - testGetKeyWithRole(t, gun, data.CanonicalRootRole, nonRootKeysSubdirWithGUN, false) - for _, role := range nonRootRolesToTest { - testGetKeyWithRole(t, "", role, notary.RootKeysSubdir, false) + // However, keys of any role can be read from anywhere in the private dir so long as + // it has the right ID + for _, expectedSubdir := range []string{ + notary.PrivDir, + filepath.Join(notary.PrivDir, nonRootKeysSubdirWithGUN), + filepath.Join(notary.PrivDir, notary.RootKeysSubdir), + filepath.Join(notary.PrivDir, notary.RootKeysSubdir, gun), + filepath.Join(notary.PrivDir, notary.NonRootKeysSubdir), + } { + for _, role := range append(nonRootRolesToTest, data.CanonicalRootRole) { + testGetKeyWithRole(t, "", role, expectedSubdir, true) + testGetKeyWithRole(t, gun, role, expectedSubdir, true) + } } -} -func testGetKeyWithRole(t *testing.T, gun, role, expectedSubdir string, success bool) { - testData := []byte(fmt.Sprintf(`-----BEGIN RSA PRIVATE KEY----- -role: %s + // keys outside of the private dir cannot be read + for _, role := range append(nonRootRolesToTest, data.CanonicalRootRole) { + testGetKeyWithRole(t, "", role, "otherDir", false) + testGetKeyWithRole(t, gun, role, "otherDir", false) + } +} +func writeKeyFile(t *testing.T, perms os.FileMode, filename, roleInPEM string) []byte { + testData := []byte(`-----BEGIN RSA PRIVATE KEY----- MIIEogIBAAKCAQEAyUIXjsrWRrvPa4Bzp3VJ6uOUGPay2fUpSV8XzNxZxIG/Opdr +k3EQi1im6WOqF3Y5AS1UjYRxNuRN+cAZeo3uS1pOTuoSupBXuchVw8s4hZJ5vXn TRmGb+xY7tZ1ZVgPfAZDib9sRSUsL/gC+aSyprAjG/YBdbF06qKbfOfsoCEYW1OQ @@ -216,7 +228,24 @@ EkqpAoGAJWe8PC0XK2RE9VkbSPg9Ehr939mOLWiHGYTVWPttUcum/rTKu73/X/mj WxnPWGtzM1pHWypSokW90SP4/xedMxludvBvmz+CTYkNJcBGCrJumy11qJhii9xp EMl3eFOJXjIch/wIesRSN+2dGOsl7neercjMh1i9RvpCwHDx/E0= -----END RSA PRIVATE KEY----- -`, role)) +`) + + if roleInPEM != "" { + block, _ := pem.Decode(testData) + require.NotNil(t, block) + block.Headers = map[string]string{ + "role": roleInPEM, + } + testData = pem.EncodeToMemory(block) + } + + os.MkdirAll(filepath.Dir(filename), perms) + err := ioutil.WriteFile(filename, testData, perms) + require.NoError(t, err, "failed to write test file") + return testData +} + +func testGetKeyWithRole(t *testing.T, gun, role, expectedSubdir string, success bool) { testName := "keyID" testExt := "key" perms := os.FileMode(0755) @@ -229,10 +258,8 @@ EMl3eFOJXjIch/wIesRSN+2dGOsl7neercjMh1i9RvpCwHDx/E0= defer os.RemoveAll(tempBaseDir) // Since we're generating this manually we need to add the extension '.' - filePath := filepath.Join(tempBaseDir, notary.PrivDir, expectedSubdir, testName+"."+testExt) - os.MkdirAll(filepath.Dir(filePath), perms) - err = ioutil.WriteFile(filePath, testData, perms) - require.NoError(t, err, "failed to write test file") + filePath := filepath.Join(tempBaseDir, expectedSubdir, testName+"."+testExt) + testData := writeKeyFile(t, perms, filePath, role) // Create our store store, err := NewKeyFileStore(tempBaseDir, emptyPassphraseRetriever) @@ -240,7 +267,7 @@ EMl3eFOJXjIch/wIesRSN+2dGOsl7neercjMh1i9RvpCwHDx/E0= // Call the GetKey function if gun != "" { - testName = gun + "/keyID" + testName = path.Join(gun, "keyID") } privKey, _, err := store.GetKey(testName) if success { @@ -263,34 +290,6 @@ EMl3eFOJXjIch/wIesRSN+2dGOsl7neercjMh1i9RvpCwHDx/E0= // TestGetLegacyKey ensures we can still load keys where the role // is stored as part of the filename (i.e. _.key func TestGetLegacyKey(t *testing.T) { - testData := []byte(`-----BEGIN RSA PRIVATE KEY----- -MIIEogIBAAKCAQEAyUIXjsrWRrvPa4Bzp3VJ6uOUGPay2fUpSV8XzNxZxIG/Opdr -+k3EQi1im6WOqF3Y5AS1UjYRxNuRN+cAZeo3uS1pOTuoSupBXuchVw8s4hZJ5vXn -TRmGb+xY7tZ1ZVgPfAZDib9sRSUsL/gC+aSyprAjG/YBdbF06qKbfOfsoCEYW1OQ -82JqHzQH514RFYPTnEGpvfxWaqmFQLmv0uMxV/cAYvqtrGkXuP0+a8PknlD2obw5 -0rHE56Su1c3Q42S7L51K38tpbgWOSRcTfDUWEj5v9wokkNQvyKBwbS996s4EJaZd -7r6M0h1pHnuRxcSaZLYRwgOe1VNGg2VfWzgd5QIDAQABAoIBAF9LGwpygmj1jm3R -YXGd+ITugvYbAW5wRb9G9mb6wspnwNsGTYsz/UR0ZudZyaVw4jx8+jnV/i3e5PC6 -QRcAgqf8l4EQ/UuThaZg/AlT1yWp9g4UyxNXja87EpTsGKQGwTYxZRM4/xPyWOzR -mt8Hm8uPROB9aA2JG9npaoQG8KSUj25G2Qot3ukw/IOtqwN/Sx1EqF0EfCH1K4KU -a5TrqlYDFmHbqT1zTRec/BTtVXNsg8xmF94U1HpWf3Lpg0BPYT7JiN2DPoLelRDy -a/A+a3ZMRNISL5wbq/jyALLOOyOkIqa+KEOeW3USuePd6RhDMzMm/0ocp5FCwYfo -k4DDeaECgYEA0eSMD1dPGo+u8UTD8i7ZsZCS5lmXLNuuAg5f5B/FGghD8ymPROIb -dnJL5QSbUpmBsYJ+nnO8RiLrICGBe7BehOitCKi/iiZKJO6edrfNKzhf4XlU0HFl -jAOMa975pHjeCoZ1cXJOEO9oW4SWTCyBDBSqH3/ZMgIOiIEk896lSmkCgYEA9Xf5 -Jqv3HtQVvjugV/axAh9aI8LMjlfFr9SK7iXpY53UdcylOSWKrrDok3UnrSEykjm7 -UL3eCU5jwtkVnEXesNn6DdYo3r43E6iAiph7IBkB5dh0yv3vhIXPgYqyTnpdz4pg -3yPGBHMPnJUBThg1qM7k6a2BKHWySxEgC1DTMB0CgYAGvdmF0J8Y0k6jLzs/9yNE -4cjmHzCM3016gW2xDRgumt9b2xTf+Ic7SbaIV5qJj6arxe49NqhwdESrFohrKaIP -kM2l/o2QaWRuRT/Pvl2Xqsrhmh0QSOQjGCYVfOb10nAHVIRHLY22W4o1jk+piLBo -a+1+74NRaOGAnu1J6/fRKQKBgAF180+dmlzemjqFlFCxsR/4G8s2r4zxTMXdF+6O -3zKuj8MbsqgCZy7e8qNeARxwpCJmoYy7dITNqJ5SOGSzrb2Trn9ClP+uVhmR2SH6 -AlGQlIhPn3JNzI0XVsLIloMNC13ezvDE/7qrDJ677EQQtNEKWiZh1/DrsmHr+irX -EkqpAoGAJWe8PC0XK2RE9VkbSPg9Ehr939mOLWiHGYTVWPttUcum/rTKu73/X/mj -WxnPWGtzM1pHWypSokW90SP4/xedMxludvBvmz+CTYkNJcBGCrJumy11qJhii9xp -EMl3eFOJXjIch/wIesRSN+2dGOsl7neercjMh1i9RvpCwHDx/E0= ------END RSA PRIVATE KEY----- -`) testName := "docker.com/notary/root" testExt := "key" testAlias := "root" @@ -305,10 +304,7 @@ EMl3eFOJXjIch/wIesRSN+2dGOsl7neercjMh1i9RvpCwHDx/E0= // Since we're generating this manually we need to add the extension '.' filePath := filepath.Join(tempBaseDir, notary.PrivDir, notary.RootKeysSubdir, testName+"_"+testAlias+"."+testExt) - - os.MkdirAll(filepath.Dir(filePath), perms) - err = ioutil.WriteFile(filePath, testData, perms) - require.NoError(t, err, "failed to write test file") + writeKeyFile(t, perms, filePath, "") // Create our store store, err := NewKeyFileStore(tempBaseDir, emptyPassphraseRetriever) @@ -356,12 +352,6 @@ func TestListKeys(t *testing.T) { require.Equal(t, role, listedInfo.Role) } - // Write an invalid filename to the directory - filePath := filepath.Join(tempBaseDir, notary.PrivDir, notary.RootKeysSubdir, "fakekeyname.key") - err = ioutil.WriteFile(filePath, []byte("data"), perms) - require.NoError(t, err, "failed to write test file") - - // Check to see if the keystore still lists two keys keyMap := store.ListKeys() require.Len(t, keyMap, len(roles)) @@ -372,6 +362,31 @@ func TestListKeys(t *testing.T) { _, err = store.GetKeyInfo(keyID) require.NoError(t, err) } + + require.Len(t, store.ListKeys(), len(roles)) + + // ListKeys() works even if the keys are in non-standard locations + for i, loc := range []string{ + filepath.Join(tempBaseDir, notary.PrivDir, notary.RootKeysSubdir), + filepath.Join(tempBaseDir, notary.PrivDir, notary.NonRootKeysSubdir), + filepath.Join(tempBaseDir, notary.PrivDir, notary.RootKeysSubdir, testName), + filepath.Join(tempBaseDir, notary.PrivDir, testName), + filepath.Join(tempBaseDir, notary.PrivDir), + tempBaseDir, // this key won't be read + } { + fp := filepath.Join(loc, fmt.Sprintf("keyID%d.key", i)) + writeKeyFile(t, perms, fp, "") + + // Ensure file exists + _, err := ioutil.ReadFile(fp) + require.NoError(t, err, "expected file not found") + } + + // update our store so we read from the FS again + store, err = NewKeyFileStore(tempBaseDir, passphraseRetriever) + require.NoError(t, err, "failed to create new key filestore") + + require.Len(t, store.ListKeys(), len(roles)+5) } func TestAddGetKeyMemStore(t *testing.T) { @@ -567,6 +582,42 @@ func TestRemoveKey(t *testing.T) { testRemoveKeyWithRole(t, data.CanonicalSnapshotRole, filepath.Join(notary.NonRootKeysSubdir, gun)) testRemoveKeyWithRole(t, "targets/a/b/c", notary.NonRootKeysSubdir) testRemoveKeyWithRole(t, "invalidRole", notary.NonRootKeysSubdir) + + // create another store for other testing + tempBaseDir, err := ioutil.TempDir("", "notary-test-") + require.NoError(t, err, "failed to create a temporary directory") + defer os.RemoveAll(tempBaseDir) + + store, err := NewKeyFileStore(tempBaseDir, passphraseRetriever) + require.NoError(t, err, "failed to create new key filestore") + + // write keys to non-standard locations - since we're generating keys manually + // we need to add the extenxion + perms := os.FileMode(0755) + for i, loc := range []string{ + filepath.Join(tempBaseDir, notary.PrivDir, notary.RootKeysSubdir), + filepath.Join(tempBaseDir, notary.PrivDir, notary.NonRootKeysSubdir), + filepath.Join(tempBaseDir, notary.PrivDir, notary.RootKeysSubdir, gun), + filepath.Join(tempBaseDir, notary.PrivDir, gun), + filepath.Join(tempBaseDir, notary.PrivDir), + } { + fp := filepath.Join(loc, fmt.Sprintf("keyID%d.key", i)) + writeKeyFile(t, perms, fp, "") + + // Ensure file exists + _, err := ioutil.ReadFile(fp) + require.NoError(t, err, "expected file not found") + + err = store.RemoveKey(fmt.Sprintf("keyID%d", i)) + require.NoError(t, err) + + // File should no longer exist + _, err = ioutil.ReadFile(fp) + require.True(t, os.IsNotExist(err), "file should not exist") + } + + // removing a non-existant key should not error + require.NoError(t, store.RemoveKey("nope")) } func testRemoveKeyWithRole(t *testing.T, role, expectedSubdir string) { @@ -599,9 +650,9 @@ func testRemoveKeyWithRole(t *testing.T, role, expectedSubdir string) { err = store.RemoveKey(privKey.ID()) require.NoError(t, err, "unable to remove key") - // Check to see if file still exists + // File should no longer exist _, err = ioutil.ReadFile(expectedFilePath) - require.Error(t, err, "file should not exist") + require.True(t, os.IsNotExist(err), "file should not exist") } func TestKeysAreCached(t *testing.T) { From 128bf0c17fdee74be03f00a5ce71a8ea721090a1 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 28 Sep 2016 16:09:11 -0700 Subject: [PATCH 2/2] Update changelog Signed-off-by: Ying Li --- CHANGELOG.md | 1 + trustmanager/keystore_test.go | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a4323a1f9..7b68d9696 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ + Preliminary Windows support for notary client [#970](https://github.com/docker/notary/pull/970) + Output message to CLI when repo changes have been successfully published [#974](https://github.com/docker/notary/pull/974) + Improved error messages for client authentication errors and for the witness command [#972](https://github.com/docker/notary/pull/972) ++ Support for finding keys that are anywhere in the notary directory's "private" directory, not just under "private/root_keys" or "private/tuf_keys" [#981](https://github.com/docker/notary/pull/981) ## [v0.4.0](https://github.com/docker/notary/releases/tag/v0.4.0) 9/21/2016 + Server-managed key rotations [#889](https://github.com/docker/notary/pull/889) diff --git a/trustmanager/keystore_test.go b/trustmanager/keystore_test.go index c09b7995b..5fd6a8937 100644 --- a/trustmanager/keystore_test.go +++ b/trustmanager/keystore_test.go @@ -616,7 +616,7 @@ func TestRemoveKey(t *testing.T) { require.True(t, os.IsNotExist(err), "file should not exist") } - // removing a non-existant key should not error + // removing a non-existent key should not error require.NoError(t, store.RemoveKey("nope")) }