Merge branch 'master' into ecdsa-key

.github/workflows/tests.yml

@@ -59,9 +59,11 @@ jobs:
     needs: get-go-versions
     steps:
       - uses: actions/setup-go@268d8c0ca0432bb2cf416faae41297df9d262d7f
+        with:
+          go-version: ${{ matrix.go-version }}
       - uses: actions/checkout@v3
       - name: golangci-lint
         uses: golangci/golangci-lint-action@537aa1903e5d359d0b27dbc19ddd22c5087f3fbc
         with:
-          version: v1.45.2
+          version: v1.49.0
           args: --timeout 3m

.golangci.yml

@@ -1,11 +1,15 @@
+run:
+  # Lint using Go 1.17, since some linters are disabled by default for Go 1.18
+  # until generics are supported.
+  # See https://github.com/golangci/golangci-lint/issues/2649
+  go: '1.17'
+
 linters:
   disable-all: true
   enable:
     - staticcheck
     - gofmt
     - govet
     - gosimple
-    - structcheck
-    - varcheck
     - unused
     - typecheck

client/client.go

@@ -5,7 +5,6 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"io"
-	"io/ioutil"
 
 	"github.com/theupdateframework/go-tuf/data"
 	"github.com/theupdateframework/go-tuf/util"
@@ -551,7 +550,7 @@ func (c *Client) downloadMetaUnsafe(name string, maxMetaSize int64) ([]byte, err
 	// although the size has been checked above, use a LimitReader in case
 	// the reported size is inaccurate, or size is -1 which indicates an
 	// unknown length
-	return ioutil.ReadAll(io.LimitReader(r, maxMetaSize))
+	return io.ReadAll(io.LimitReader(r, maxMetaSize))
 }
 
 // remoteGetFunc is the type of function the download method uses to download
@@ -622,7 +621,7 @@ func (c *Client) downloadMeta(name string, version int64, m data.FileMeta) ([]by
 		stream = r
 	}
 
-	return ioutil.ReadAll(stream)
+	return io.ReadAll(stream)
 }
 
 func (c *Client) downloadMetaFromSnapshot(name string, m data.SnapshotFileMeta) ([]byte, error) {
@@ -673,17 +672,6 @@ func (c *Client) downloadMetaFromTimestamp(name string, m data.TimestampFileMeta
 	return b, nil
 }
 
-// decodeRoot decodes and verifies root metadata.
-func (c *Client) decodeRoot(b json.RawMessage) error {
-	root := &data.Root{}
-	if err := c.db.Unmarshal(b, root, "root", c.rootVer); err != nil {
-		return ErrDecodeFailed{"root.json", err}
-	}
-	c.rootVer = root.Version
-	c.consistentSnapshot = root.ConsistentSnapshot
-	return nil
-}
-
 // decodeSnapshot decodes and verifies snapshot metadata, and returns the new
 // root and targets file meta.
 func (c *Client) decodeSnapshot(b json.RawMessage) (data.SnapshotFiles, error) {
@@ -790,36 +778,6 @@ func (c *Client) localMetaFromSnapshot(name string, m data.SnapshotFileMeta) (js
 	return b, err == nil
 }
 
-// hasTargetsMeta checks whether local metadata has the given snapshot meta
-//lint:ignore U1000 unused
-func (c *Client) hasTargetsMeta(m data.SnapshotFileMeta) bool {
-	b, ok := c.localMeta["targets.json"]
-	if !ok {
-		return false
-	}
-	meta, err := util.GenerateSnapshotFileMeta(bytes.NewReader(b), m.Hashes.HashAlgorithms()...)
-	if err != nil {
-		return false
-	}
-	err = util.SnapshotFileMetaEqual(meta, m)
-	return err == nil
-}
-
-// hasSnapshotMeta checks whether local metadata has the given meta
-//lint:ignore U1000 unused
-func (c *Client) hasMetaFromTimestamp(name string, m data.TimestampFileMeta) bool {
-	b, ok := c.localMeta[name]
-	if !ok {
-		return false
-	}
-	meta, err := util.GenerateTimestampFileMeta(bytes.NewReader(b), m.Hashes.HashAlgorithms()...)
-	if err != nil {
-		return false
-	}
-	err = util.TimestampFileMetaEqual(meta, m)
-	return err == nil
-}
-
 type Destination interface {
 	io.Writer
 	Delete() error
@@ -829,11 +787,11 @@ type Destination interface {
 //
 // dest will be deleted and an error returned in the following situations:
 //
-//   * The target does not exist in the local targets.json
-//   * Failed to fetch the chain of delegations accessible from local snapshot.json
-//   * The target does not exist in any targets
-//   * Metadata cannot be generated for the downloaded data
-//   * Generated metadata does not match local metadata for the given file
+//   - The target does not exist in the local targets.json
+//   - Failed to fetch the chain of delegations accessible from local snapshot.json
+//   - The target does not exist in any targets
+//   - Metadata cannot be generated for the downloaded data
+//   - Generated metadata does not match local metadata for the given file
 func (c *Client) Download(name string, dest Destination) (err error) {
 	// delete dest if there is an error
 	defer func() {

client/client_test.go

@@ -2,11 +2,12 @@ package client
 
 import (
 	"bytes"
+	"crypto/sha256"
+	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"net"
 	"net/http"
 	"os"
@@ -398,7 +399,7 @@ func newClientWithMeta(baseDir string, relPath string, serverAddr string) (*Clie
 	c := NewClient(MemoryLocalStore(), remote)
 	for _, m := range []string{"root.json", "snapshot.json", "timestamp.json", "targets.json"} {
 		if _, err := os.Stat(initialStateDir + "/" + m); err == nil {
-			metadataJSON, err := ioutil.ReadFile(initialStateDir + "/" + m)
+			metadataJSON, err := os.ReadFile(initialStateDir + "/" + m)
 			if err != nil {
 				return nil, err
 			}
@@ -1211,7 +1212,7 @@ func generateRepoFS(c *C, dir string, files map[string][]byte, consistentSnapsho
 	for file, data := range files {
 		path := filepath.Join(dir, "staged", "targets", file)
 		c.Assert(os.MkdirAll(filepath.Dir(path), 0755), IsNil)
-		c.Assert(ioutil.WriteFile(path, data, 0644), IsNil)
+		c.Assert(os.WriteFile(path, data, 0644), IsNil)
 		c.Assert(repo.AddTarget(file, nil), IsNil)
 	}
 	c.Assert(repo.Snapshot(), IsNil)
@@ -1237,3 +1238,136 @@ func (s *ClientSuite) TestVerifyDigest(c *C) {
 
 	c.Assert(client.VerifyDigest(hash, "sha256", size, digest), IsNil)
 }
+
+type StateLessSuite struct{}
+
+var _ = Suite(&StateLessSuite{})
+
+func (s *StateLessSuite) TestRejectsMultiSignaturesSameKeyDifferentIDs(c *C) {
+	// In this test Alice and Bob want to create a TUF repo
+	// where a root key rotation would require both their signatures.
+	// Alice uses an old version of Go-TUF where each key gets assigned several IDs.
+	// Bob uses a modern version of Go-TUF that does not produce the same list of IDs for a same key.
+	// This test checks that the TUF client
+	// will not accept a root rotation
+	// signed twice with Alice's key with different key IDs each time.
+	// This test was failing with https://github.com/theupdateframework/go-tuf/tree/ac7b5d7bce18cca5a84a28b021bd6372f450b35b
+	// because the signature verification code was assuming that the key IDs used in the metadata
+	// were the same as the one the TUF library of the client would generate,
+	// breaking the security of threshold signatures.
+
+	// The attack works just the same if Alice is malicious from the beginning
+	// and convinces Bob to sign an initial "root.json"
+	// with additional key IDs for her only key,
+	// but this scenario show that the vulnerability can even impact situations
+	// where Alice is not malicious at all,
+	// she was simply using an old client and an attacker stole her key.
+	// The purpose of threshold signatures in TUF is precisely
+	// to make sure that an attacker cannot forge signatures
+	// if they did not steal a large enough number of keys.
+
+	alice, err := keys.GenerateEd25519Key()
+	if err != nil {
+		panic(err)
+	}
+
+	root := data.NewRoot()
+	root.Version = 1
+	root.Roles["root"] = &data.Role{
+		KeyIDs:    []string{},
+		Threshold: 2, // Note the threshold
+	}
+
+	// reproduces how IDs were computed in
+	// https://github.com/theupdateframework/go-tuf/blob/8e84384bebe3/data/types.go#L50
+	oldTUFIDs := func(k *data.PublicKey) []string {
+		bytes, _ := cjson.EncodeCanonical(k)
+		digest := sha256.Sum256(bytes)
+		ids := []string{hex.EncodeToString(digest[:])}
+
+		if k.Scheme != "" || len(k.Algorithms) != 0 {
+			bytes, _ = cjson.EncodeCanonical(&data.PublicKey{
+				Type:  k.Type,
+				Value: k.Value,
+			})
+			digest = sha256.Sum256(bytes)
+			ids = append(ids, hex.EncodeToString(digest[:]))
+		}
+
+		return ids
+	}
+
+	// Alice adds her key using an old version of go-tuf
+	// which will use several IDs
+	for _, keyID := range oldTUFIDs(alice.PublicData()) {
+		root.Keys[keyID] = alice.PublicData()
+		root.Roles["root"].KeyIDs = append(root.Roles["root"].KeyIDs, keyID)
+	}
+
+	bob, err := keys.GenerateEd25519Key()
+	if err != nil {
+		panic(err)
+	}
+
+	root.AddKey(bob.PublicData())
+	root.Roles["root"].KeyIDs = append(
+		root.Roles["root"].KeyIDs,
+		bob.PublicData().IDs()...,
+	)
+
+	// signer for the other roles, not important
+	delegatedSigner, _ := keys.GenerateEd25519Key()
+	root.AddKey(delegatedSigner.PublicData())
+	for _, role := range []string{"targets", "snapshot", "timestamp"} {
+		root.Roles[role] = &data.Role{
+			KeyIDs:    delegatedSigner.PublicData().IDs(),
+			Threshold: 1,
+		}
+	}
+
+	signedRoot, err := sign.Marshal(root, alice, bob)
+	c.Assert(err, IsNil)
+	rootJSON, err := json.Marshal(signedRoot)
+	c.Assert(err, IsNil)
+
+	// producing evil root using only Alice's key
+
+	evilRoot := root
+	evilRoot.Version = 2
+
+	canonical, err := cjson.EncodeCanonical(evilRoot)
+	c.Assert(err, IsNil)
+	sig, err := alice.SignMessage(canonical)
+	c.Assert(err, IsNil)
+	signedEvilRoot := &data.Signed{
+		Signed:     canonical,
+		Signatures: make([]data.Signature, 0),
+	}
+	for _, keyID := range oldTUFIDs(alice.PublicData()) {
+		signedEvilRoot.Signatures = append(signedEvilRoot.Signatures, data.Signature{
+			Signature: sig,
+			KeyID:     keyID,
+		})
+	}
+	evilRootJSON, err := json.Marshal(signedEvilRoot)
+	c.Assert(err, IsNil)
+
+	// checking that client does not accept root rotation
+	// to evil root
+
+	localStore := MemoryLocalStore()
+	err = localStore.SetMeta("root.json", rootJSON)
+	c.Assert(err, IsNil)
+
+	remoteStore := newFakeRemoteStore()
+	remoteStore.meta["2.root.json"] = newFakeFile(evilRootJSON)
+
+	client := NewClient(localStore, remoteStore)
+
+	err = client.UpdateRoots()
+	if err != nil {
+		c.Assert(err, DeepEquals, verify.ErrRoleThreshold{Expected: 2, Actual: 1})
+	} else {
+		c.Fatalf("client returned no error when updating with evil root")
+	}
+}

client/delegations_test.go

@@ -5,9 +5,9 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"net"
 	"net/http"
+	"os"
 	"strings"
 	"testing"
 	"time"
@@ -273,10 +273,10 @@ func initTestDelegationClient(t *testing.T, dirPrefix string) (*Client, func() e
 	assert.Nil(t, err)
 
 	c := NewClient(MemoryLocalStore(), remote)
-	rawFile, err := ioutil.ReadFile(initialStateDir + "/" + "root.json")
+	rawFile, err := os.ReadFile(initialStateDir + "/" + "root.json")
 	assert.Nil(t, err)
 	assert.Nil(t, c.Init(rawFile))
-	files, err := ioutil.ReadDir(initialStateDir)
+	files, err := os.ReadDir(initialStateDir)
 	assert.Nil(t, err)
 
 	// load local files
@@ -287,7 +287,7 @@ func initTestDelegationClient(t *testing.T, dirPrefix string) (*Client, func() e
 		name := f.Name()
 		// ignoring consistent snapshot when loading initial state
 		if len(strings.Split(name, ".")) == 1 && strings.HasSuffix(name, ".json") {
-			rawFile, err := ioutil.ReadFile(initialStateDir + "/" + name)
+			rawFile, err := os.ReadFile(initialStateDir + "/" + name)
 			assert.Nil(t, err)
 			assert.Nil(t, c.local.SetMeta(name, rawFile))
 		}

client/interop_test.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"fmt"
 	"io"
-	"io/ioutil"
 	"net"
 	"net/http"
 	"os"
@@ -33,7 +32,7 @@ func checkGoIdentity(c *C, consistentSnapshot bool) {
 	c.Assert(err, IsNil)
 	testDataDir := filepath.Join(cwd, "testdata")
 
-	tempDir, err := ioutil.TempDir("", "")
+	tempDir, err := os.MkdirTemp("", "")
 	c.Assert(err, IsNil)
 	defer os.RemoveAll(tempDir)
 
@@ -59,7 +58,7 @@ func computeHashes(c *C, dir string) map[string]string {
 			return nil
 		}
 
-		bytes, err := ioutil.ReadFile(path)
+		bytes, err := os.ReadFile(path)
 		if err != nil {
 			return err
 		}
@@ -108,7 +107,7 @@ func newTestCase(c *C, name string, consistentSnapshot bool, options *HTTPRemote
 	c.Assert(err, IsNil)
 	testDir := filepath.Join(cwd, "testdata", name, fmt.Sprintf("consistent-snapshot-%t", consistentSnapshot))
 
-	dirEntries, err := ioutil.ReadDir(testDir)
+	dirEntries, err := os.ReadDir(testDir)
 	c.Assert(err, IsNil)
 	c.Assert(dirEntries, Not(HasLen), 0)