Repository: theupdateframework/go-tuf
Pull request #197 (2021-12-10 23:06:21)

[Delegations prereq 7] Make signers addressible by key ID in LocalStore
Splitting up https://github.com/theupdateframework/go-tuf/pull/175

Changes:
1. Move `LocalStore` struct definition into `local_store.go`
2. Refactor the `signers` ("signers for role") map into two maps, `keyIDsForRole` and `signerForKeyID`.
3. Add `SignersForKeyIDs` to `LocalStore`, which will be used to get signers for key IDs specified in a targets delegation

coveralls: ## Pull Request Test Coverage Report for [Build 1565533779](https://coveralls.io/builds/44928261)

* **86** of **92**   **(93.48%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.5%**) to **70.594%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [local_store.go](https://coveralls.io/builds/44928261/source?filename=local_store.go#L150) | 86 | 92 | 93.48%
<!-- | **Total:** | **86** | **92** | **93.48%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/44928261/badge)](https://coveralls.io/builds/44928261) |
| :-- | --: |
| Change from base [Build 1565516238](https://coveralls.io/builds/44928146): |  0.5% |
| Covered Lines: | 2187 |
| Relevant Lines: | 3098 |

---
##### 💛  - [Coveralls](https://coveralls.io)

ethan-lowman-dd: Realized I'm missing new tests here. Will add.
ethan-lowman-dd: > @ethan-lowman-dd did you want to add tests?

@asraa The functionality of `SignersForKeyIDs` is tested to some extent in existing tests (since this PR just adds a step of indirection for key lookup) but I will add some dedicated tests too.
ethan-lowman-dd: @asraa Added tests for the changed functionality.
ethan-lowman-dd: Rebasing dismissed reviews. There shouldn't have been any material changes in the rebase.

Patch:
diff --git a/client/delegations.go b/client/delegations.go
index f1ec67a..de3e664 100644
--- a/client/delegations.go
+++ b/client/delegations.go
@@ -32,7 +32,7 @@ func (c *Client) getTargetFileMeta(target string) (data.TargetFileMeta, error) {
 		}
 
 		// covers 5.6.{1,2,3,4,5,6}
-		targets, err := c.loadDelegatedTargets(snapshot, d)
+		targets, err := c.loadDelegatedTargets(snapshot, d.Delegatee.Name, d.DB)
 		if err != nil {
 			return data.TargetFileMeta{}, err
 		}
@@ -79,9 +79,7 @@ func (c *Client) loadLocalSnapshot() (*data.Snapshot, error) {
 }
 
 // loadDelegatedTargets downloads, decodes, verifies and stores targets
-func (c *Client) loadDelegatedTargets(snapshot *data.Snapshot, delegation targets.Delegation) (*data.Targets, error) {
-	role := delegation.Delegatee.Name
-
+func (c *Client) loadDelegatedTargets(snapshot *data.Snapshot, role string, db *verify.DB) (*data.Targets, error) {
 	var err error
 	fileName := role + ".json"
 	fileMeta, ok := snapshot.Meta[fileName]
@@ -104,7 +102,7 @@ func (c *Client) loadDelegatedTargets(snapshot *data.Snapshot, delegation target
 	// 5.6.3 verify signature with parent public keys
 	// 5.6.5 verify that the targets is not expired
 	// role "targets" is a top role verified by root keys loaded in the client db
-	err = delegation.DB.Unmarshal(raw, targets, role, fileMeta.Version)
+	err = db.Unmarshal(raw, targets, role, fileMeta.Version)
 	if err != nil {
 		return nil, ErrDecodeFailed{fileName, err}
 	}
diff --git a/local_store.go b/local_store.go
index 139f436..bd1175b 100644
--- a/local_store.go
+++ b/local_store.go
@@ -12,20 +12,49 @@ import (
 
 	"github.com/theupdateframework/go-tuf/data"
 	"github.com/theupdateframework/go-tuf/encrypted"
+	"github.com/theupdateframework/go-tuf/internal/sets"
 	"github.com/theupdateframework/go-tuf/pkg/keys"
 	"github.com/theupdateframework/go-tuf/util"
 )
 
-func signers(privateKeys []*data.PrivateKey) []keys.Signer {
-	res := make([]keys.Signer, 0, len(privateKeys))
-	for _, k := range privateKeys {
-		signer, err := keys.GetSigner(k)
-		if err != nil {
-			continue
-		}
-		res = append(res, signer)
-	}
-	return res
+type LocalStore interface {
+	// GetMeta returns a map from metadata file names (e.g. root.json) to their raw JSON payload or an error.
+	GetMeta() (map[string]json.RawMessage, error)
+
+	// SetMeta is used to update a metadata file name with a JSON payload.
+	SetMeta(name string, meta json.RawMessage) error
+
+	// WalkStagedTargets calls targetsFn for each staged target file in paths.
+	// If paths is empty, all staged target files will be walked.
+	WalkStagedTargets(paths []string, targetsFn TargetsWalkFunc) error
+
+	// FileIsStaged determines if a metadata file is currently staged, to avoid incrementing
+	// version numbers repeatedly while staged.
+	FileIsStaged(filename string) bool
+
+	// Commit is used to publish staged files to the repository
+	//
+	// This will also reset the staged meta to signal incrementing version numbers.
+	// TUF 1.0 requires that the root metadata version numbers in the repository does not
+	// gaps. To avoid this, we will only increment the number once until we commit.
+	Commit(bool, map[string]int, map[string]data.Hashes) error
+
+	// GetSigners return a list of signers for a role.
+	GetSigners(role string) ([]keys.Signer, error)
+
+	// SaveSigner adds a signer to a role.
+	SaveSigner(role string, signer keys.Signer) error
+
+	// SignersForRole return a list of signing keys for a role.
+	SignersForKeyIDs(keyIDs []string) []keys.Signer
+
+	// Clean is used to remove all staged manifests.
+	Clean() error
+}
+
+type PassphraseChanger interface {
+	// ChangePassphrase changes the passphrase for a role keys file.
+	ChangePassphrase(string) error
 }
 
 func MemoryStore(meta map[string]json.RawMessage, files map[string][]byte) LocalStore {
@@ -33,10 +62,11 @@ func MemoryStore(meta map[string]json.RawMessage, files map[string][]byte) Local
 		meta = make(map[string]json.RawMessage)
 	}
 	return &memoryStore{
-		meta:       meta,
-		stagedMeta: make(map[string]json.RawMessage),
-		files:      files,
-		signers:    make(map[string][]keys.Signer),
+		meta:           meta,
+		stagedMeta:     make(map[string]json.RawMessage),
+		files:          files,
+		signerForKeyID: make(map[string]keys.Signer),
+		keyIDsForRole:  make(map[string][]string),
 	}
 }
 
@@ -44,7 +74,9 @@ type memoryStore struct {
 	meta       map[string]json.RawMessage
 	stagedMeta map[string]json.RawMessage
 	files      map[string][]byte
-	signers    map[string][]keys.Signer
+
+	signerForKeyID map[string]keys.Signer
+	keyIDsForRole  map[string][]string
 }
 
 func (m *memoryStore) GetMeta() (map[string]json.RawMessage, error) {
@@ -105,14 +137,53 @@ func (m *memoryStore) Commit(consistentSnapshot bool, versions map[string]int, h
 }
 
 func (m *memoryStore) GetSigners(role string) ([]keys.Signer, error) {
-	return m.signers[role], nil
+	keyIDs, ok := m.keyIDsForRole[role]
+	if ok {
+		return m.SignersForKeyIDs(keyIDs), nil
+	}
+
+	return nil, nil
 }
 
 func (m *memoryStore) SaveSigner(role string, signer keys.Signer) error {
-	m.signers[role] = append(m.signers[role], signer)
+	keyIDs := signer.PublicData().IDs()
+
+	for _, keyID := range keyIDs {
+		m.signerForKeyID[keyID] = signer
+	}
+
+	mergedKeyIDs := sets.DeduplicateStrings(append(m.keyIDsForRole[role], keyIDs...))
+	m.keyIDsForRole[role] = mergedKeyIDs
 	return nil
 }
 
+func (m *memoryStore) SignersForKeyIDs(keyIDs []string) []keys.Signer {
+	signers := []keys.Signer{}
+	keyIDsSeen := map[string]struct{}{}
+
+	for _, keyID := range keyIDs {
+		signer, ok := m.signerForKeyID[keyID]
+		if !ok {
+			continue
+		}
+		addSigner := false
+
+		for _, skid := range signer.PublicData().IDs() {
+			if _, seen := keyIDsSeen[skid]; !seen {
+				addSigner = true
+			}
+
+			keyIDsSeen[skid] = struct{}{}
+		}
+
+		if addSigner {
+			signers = append(signers, signer)
+		}
+	}
+
+	return signers
+}
+
 func (m *memoryStore) Clean() error {
 	return nil
 }
@@ -126,7 +197,8 @@ func FileSystemStore(dir string, p util.PassphraseFunc) LocalStore {
 	return &fileSystemStore{
 		dir:            dir,
 		passphraseFunc: p,
-		signers:        make(map[string][]keys.Signer),
+		signerForKeyID: make(map[string]keys.Signer),
+		keyIDsForRole:  make(map[string][]string),
 	}
 }
 
@@ -134,8 +206,8 @@ type fileSystemStore struct {
 	dir            string
 	passphraseFunc util.PassphraseFunc
 
-	// signers is a cache of persisted keys to avoid decrypting multiple times
-	signers map[string][]keys.Signer
+	signerForKeyID map[string]keys.Signer
+	keyIDsForRole  map[string][]string
 }
 
 func (f *fileSystemStore) repoDir() string {
@@ -333,18 +405,63 @@ func (f *fileSystemStore) Commit(consistentSnapshot bool, versions map[string]in
 }
 
 func (f *fileSystemStore) GetSigners(role string) ([]keys.Signer, error) {
-	if keys, ok := f.signers[role]; ok {
-		return keys, nil
+	keyIDs, ok := f.keyIDsForRole[role]
+	if ok {
+		return f.SignersForKeyIDs(keyIDs), nil
 	}
-	keys, _, err := f.loadPrivateKeys(role)
+
+	privKeys, _, err := f.loadPrivateKeys(role)
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil, nil
 		}
 		return nil, err
 	}
-	f.signers[role] = signers(keys)
-	return f.signers[role], nil
+
+	signers := []keys.Signer{}
+	for _, key := range privKeys {
+		signer, err := keys.GetSigner(key)
+		if err != nil {
+			return nil, err
+		}
+
+		// Cache the signers.
+		for _, keyID := range signer.PublicData().IDs() {
+			f.keyIDsForRole[role] = append(f.keyIDsForRole[role], keyID)
+			f.signerForKeyID[keyID] = signer
+		}
+		signers = append(signers, signer)
+	}
+
+	return signers, nil
+}
+
+func (f *fileSystemStore) SignersForKeyIDs(keyIDs []string) []keys.Signer {
+	signers := []keys.Signer{}
+	keyIDsSeen := map[string]struct{}{}
+
+	for _, keyID := range keyIDs {
+		signer, ok := f.signerForKeyID[keyID]
+		if !ok {
+			continue
+		}
+
+		addSigner := false
+
+		for _, skid := range signer.PublicData().IDs() {
+			if _, seen := keyIDsSeen[skid]; !seen {
+				addSigner = true
+			}
+
+			keyIDsSeen[skid] = struct{}{}
+		}
+
+		if addSigner {
+			signers = append(signers, signer)
+		}
+	}
+
+	return signers
 }
 
 // ChangePassphrase changes the passphrase for a role keys file. Implements
@@ -391,7 +508,7 @@ func (f *fileSystemStore) SaveSigner(role string, signer keys.Signer) error {
 	}
 
 	// add the key to the existing keys (if any)
-	keys, pass, err := f.loadPrivateKeys(role)
+	privKeys, pass, err := f.loadPrivateKeys(role)
 	if err != nil && !os.IsNotExist(err) {
 		return err
 	}
@@ -399,7 +516,7 @@ func (f *fileSystemStore) SaveSigner(role string, signer keys.Signer) error {
 	if err != nil {
 		return err
 	}
-	keys = append(keys, key)
+	privKeys = append(privKeys, key)
 
 	// if loadPrivateKeys didn't return a passphrase (because no keys yet exist)
 	// and passphraseFunc is set, get the passphrase so the keys file can
@@ -414,13 +531,13 @@ func (f *fileSystemStore) SaveSigner(role string, signer keys.Signer) error {
 
 	pk := &persistedKeys{}
 	if pass != nil {
-		pk.Data, err = encrypted.Marshal(keys, pass)
+		pk.Data, err = encrypted.Marshal(privKeys, pass)
 		if err != nil {
 			return err
 		}
 		pk.Encrypted = true
 	} else {
-		pk.Data, err = json.MarshalIndent(keys, "", "\t")
+		pk.Data, err = json.MarshalIndent(privKeys, "", "\t")
 		if err != nil {
 			return err
 		}
@@ -432,7 +549,27 @@ func (f *fileSystemStore) SaveSigner(role string, signer keys.Signer) error {
 	if err := util.AtomicallyWriteFile(f.keysPath(role), append(data, '\n'), 0600); err != nil {
 		return err
 	}
-	f.signers[role] = append(f.signers[role], signer)
+
+	// Merge privKeys into f.keyIDsForRole and register signers with
+	// f.signerForKeyID.
+	keyIDsForRole := f.keyIDsForRole[role]
+	for _, key := range privKeys {
+		signer, err := keys.GetSigner(key)
+		if err != nil {
+			return err
+		}
+
+		keyIDs := signer.PublicData().IDs()
+
+		for _, keyID := range keyIDs {
+			f.signerForKeyID[keyID] = signer
+		}
+
+		keyIDsForRole = append(keyIDsForRole, keyIDs...)
+	}
+
+	f.keyIDsForRole[role] = sets.DeduplicateStrings(keyIDsForRole)
+
 	return nil
 }
 
diff --git a/repo.go b/repo.go
index 482cdf0..c436195 100644
--- a/repo.go
+++ b/repo.go
@@ -34,44 +34,6 @@ var topLevelMetadata = []string{
 // names and generate target file metadata with additional custom metadata.
 type TargetsWalkFunc func(path string, target io.Reader) error
 
-type LocalStore interface {
-	// GetMeta returns a map from metadata file names (e.g. root.json) to their raw JSON payload or an error.
-	GetMeta() (map[string]json.RawMessage, error)
-
-	// SetMeta is used to update a metadata file name with a JSON payload.
-	SetMeta(string, json.RawMessage) error
-
-	// WalkStagedTargets calls targetsFn for each staged target file in paths.
-	//
-	// If paths is empty, all staged target files will be walked.
-	WalkStagedTargets(paths []string, targetsFn TargetsWalkFunc) error
-
-	// FileIsStaged determines if a metadata file is currently staged, to avoid incrementing
-	// version numbers repeatedly while staged.
-	FileIsStaged(filename string) bool
-
-	// Commit is used to publish staged files to the repository
-	//
-	// This will also reset the staged meta to signal incrementing version numbers.
-	// TUF 1.0 requires that the root metadata version numbers in the repository does not
-	// gaps. To avoid this, we will only increment the number once until we commit.
-	Commit(bool, map[string]int, map[string]data.Hashes) error
-
-	// GetSigners return a list of signers for a role.
-	GetSigners(string) ([]keys.Signer, error)
-
-	// SaveSigner adds a signer to a role.
-	SaveSigner(string, keys.Signer) error
-
-	// Clean is used to remove all staged metadata files.
-	Clean() error
-}
-
-type PassphraseChanger interface {
-	// ChangePassphrase changes the passphrase for a role keys file.
-	ChangePassphrase(string) error
-}
-
 type Repo struct {
 	local          LocalStore
 	hashAlgorithms []string


Test patch:
diff --git a/local_store_test.go b/local_store_test.go
new file mode 100644
index 0000000..8badcfc
--- /dev/null
+++ b/local_store_test.go
@@ -0,0 +1,70 @@
+package tuf
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/theupdateframework/go-tuf/pkg/keys"
+)
+
+func TestLocalStoreSigners(t *testing.T) {
+	tmpdir, err := os.MkdirTemp("", "")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		os.RemoveAll(tmpdir)
+	}()
+
+	stores := map[string]LocalStore{
+		"MemoryStore":     MemoryStore(nil, nil),
+		"FileSystemStore": FileSystemStore(tmpdir, nil),
+	}
+
+	for name, store := range stores {
+		t.Run(name, func(t *testing.T) {
+			signers, err := store.GetSigners("abc")
+			assert.NoError(t, err)
+			assert.Equal(t, len(signers), 0)
+
+			// Add two signers to role "a".
+			aSigner1, err := keys.GenerateEd25519Key()
+			assert.NoError(t, err)
+			err = store.SaveSigner("a", aSigner1)
+			assert.NoError(t, err)
+
+			aSigner2, err := keys.GenerateEd25519Key()
+			assert.NoError(t, err)
+			err = store.SaveSigner("a", aSigner2)
+			assert.NoError(t, err)
+
+			// Add one signer to role "b".
+			bSigner, err := keys.GenerateEd25519Key()
+			assert.NoError(t, err)
+			err = store.SaveSigner("b", bSigner)
+			assert.NoError(t, err)
+
+			// Add to b again to test deduplication.
+			err = store.SaveSigner("b", bSigner)
+			assert.NoError(t, err)
+
+			signers, err = store.GetSigners("a")
+			assert.NoError(t, err)
+			assert.ElementsMatch(t, []keys.Signer{aSigner1, aSigner2}, signers)
+
+			signers, err = store.GetSigners("b")
+			assert.NoError(t, err)
+			assert.ElementsMatch(t, []keys.Signer{bSigner}, signers)
+
+			a1KeyIDs := aSigner1.PublicData().IDs()
+			a2KeyIDs := aSigner2.PublicData().IDs()
+			bKeyIDs := bSigner.PublicData().IDs()
+
+			assert.Equal(t, []keys.Signer{aSigner1}, store.SignersForKeyIDs(a1KeyIDs))
+			assert.Equal(t, []keys.Signer{aSigner2}, store.SignersForKeyIDs(a2KeyIDs))
+			assert.ElementsMatch(t, []keys.Signer{aSigner1, aSigner2}, store.SignersForKeyIDs(append(a1KeyIDs, a2KeyIDs...)))
+			assert.Equal(t, []keys.Signer{bSigner}, store.SignersForKeyIDs(bKeyIDs))
+		})
+	}
+}