Repository: theupdateframework/go-tuf
Pull request #345 (2022-07-18 15:48:41)

fix: require length and hashes for target metadata
Signed-off-by: Asra Ali <asraa@google.com>

Please fill in the fields below to submit a pull request.  The more information that is provided, the better.

Fixes #<Issue>
Release Notes: <!-- What comments/remarks should we include in the release notes for this change? -->
```
* breaking change: `data.FileMeta` no longer contains a `Custom` field
```

Rust's `tough` requires length in the JSON, and go-tuf omits if empty. always marshal a length, even if `0`. Some files need to be signed as empty because there is explicitly no content there.

cc @raulcabello


**Types of changes**:
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ x ] Bug fix (non-breaking change which fixes an issue)
- [  ] New feature (non-breaking change which adds functionality)
- [  ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

**Description of the changes being introduced by the pull request**:

**Please verify and check that the pull request fulfills the following requirements**:

- [x] Tests have been added for the bug fix or new feature
- [ ] Docs have been added for the bug fix or new feature


asraa: @trishankatdatadog @mnm678 @rdimitrov could you please review this change? it's breaking consumption of sigstore metadata from `tuftool` because a target file had length 0.
rdimitrov: > > As per the spec, both length and hashes should be optional - [ref](https://theupdateframework.github.io/specification/latest/#metapath-length).
> 
> I don't think that's the correct reference. That's for a metapath length like snapshotted metadata. Target files https://theupdateframework.github.io/specification/latest/#target-files do not mention length being optional.

Yeah, sorry, I was about to edit my comment. I think the problem is that we use the same `FileMeta` object to describe both metafiles and targets. As you said, it's optional for things like snapshot meta (which is what I had in mind initially), but shouldn't be the same for targets.
asraa: > As you said, it's optional for things like snapshot meta (which is what I had in mind initially), but shouldn't be the same for targets.

I'll update this PR to be a larger fix then to separate these two metas. I'll add a test for a 0 length target and an optionally omitted  lenghth on snapshot meta hopefully.
rdimitrov: > > As you said, it's optional for things like snapshot meta (which is what I had in mind initially), but shouldn't be the same for targets.
> 
> I'll update this PR to be a larger fix then to separate these two metas. I'll add a test for a 0 length target and an optionally omitted lenghth on snapshot meta hopefully.

Sounds great! 👍 
asraa: > The names of the types feel a little less identifiable here, as this is already a break change (right?) could we rename TargetFileMeta to TargetFile?

The only breaking change here is that `FileMeta` (the base FileMeta type) no longer contains `Custom`: added this to the release note. All other names are preserved! I introduced `MetapathFileMeta`, analogous to `TargetFileMeta`, which `SnapshotFileMeta` and `TimestampFileMeta` alias to.

I'd rather keep the wording `XXXXFileMeta` since that's how `(Targets/Snapshot/Timestamp)FileMeta` are all worded in go-tuf. Should I make `MetapathFileMeta` private and only expose `(Targets/Snapshot/Timestamp)FileMeta`? I'd rather not break API and remove `(Snapshot/Timestamp)FileMeta` in favor or something like `MetaFile`
asraa: thanks! all set with @joshua's comment on `metapathFileMeta` privatized.

Patch:
diff --git a/client/client.go b/client/client.go
index 080deef..6785096 100644
--- a/client/client.go
+++ b/client/client.go
@@ -669,7 +669,7 @@ func (c *Client) downloadMeta(name string, version int64, m data.FileMeta) ([]by
 }
 
 func (c *Client) downloadMetaFromSnapshot(name string, m data.SnapshotFileMeta) ([]byte, error) {
-	b, err := c.downloadMeta(name, m.Version, m.FileMeta)
+	b, err := c.downloadMeta(name, m.Version, data.FileMeta{Length: m.Length, Hashes: m.Hashes})
 	if err != nil {
 		return nil, err
 	}
@@ -679,7 +679,7 @@ func (c *Client) downloadMetaFromSnapshot(name string, m data.SnapshotFileMeta)
 		return nil, ErrDownloadFailed{name, err}
 	}
 
-	meta, err := util.GenerateSnapshotFileMeta(bytes.NewReader(b), m.HashAlgorithms()...)
+	meta, err := util.GenerateSnapshotFileMeta(bytes.NewReader(b), m.Hashes.HashAlgorithms()...)
 	if err != nil {
 		return nil, err
 	}
@@ -693,7 +693,7 @@ func (c *Client) downloadMetaFromSnapshot(name string, m data.SnapshotFileMeta)
 }
 
 func (c *Client) downloadMetaFromTimestamp(name string, m data.TimestampFileMeta) ([]byte, error) {
-	b, err := c.downloadMeta(name, m.Version, m.FileMeta)
+	b, err := c.downloadMeta(name, m.Version, data.FileMeta{Length: m.Length, Hashes: m.Hashes})
 	if err != nil {
 		return nil, err
 	}
@@ -703,7 +703,7 @@ func (c *Client) downloadMetaFromTimestamp(name string, m data.TimestampFileMeta
 		return nil, ErrDownloadFailed{name, err}
 	}
 
-	meta, err := util.GenerateTimestampFileMeta(bytes.NewReader(b), m.HashAlgorithms()...)
+	meta, err := util.GenerateTimestampFileMeta(bytes.NewReader(b), m.Hashes.HashAlgorithms()...)
 	if err != nil {
 		return nil, err
 	}
@@ -825,7 +825,7 @@ func (c *Client) localMetaFromSnapshot(name string, m data.SnapshotFileMeta) (js
 	if !ok {
 		return nil, false
 	}
-	meta, err := util.GenerateSnapshotFileMeta(bytes.NewReader(b), m.HashAlgorithms()...)
+	meta, err := util.GenerateSnapshotFileMeta(bytes.NewReader(b), m.Hashes.HashAlgorithms()...)
 	if err != nil {
 		return nil, false
 	}
@@ -840,7 +840,7 @@ func (c *Client) hasTargetsMeta(m data.SnapshotFileMeta) bool {
 	if !ok {
 		return false
 	}
-	meta, err := util.GenerateSnapshotFileMeta(bytes.NewReader(b), m.HashAlgorithms()...)
+	meta, err := util.GenerateSnapshotFileMeta(bytes.NewReader(b), m.Hashes.HashAlgorithms()...)
 	if err != nil {
 		return false
 	}
@@ -855,7 +855,7 @@ func (c *Client) hasMetaFromTimestamp(name string, m data.TimestampFileMeta) boo
 	if !ok {
 		return false
 	}
-	meta, err := util.GenerateTimestampFileMeta(bytes.NewReader(b), m.HashAlgorithms()...)
+	meta, err := util.GenerateTimestampFileMeta(bytes.NewReader(b), m.Hashes.HashAlgorithms()...)
 	if err != nil {
 		return false
 	}
diff --git a/data/types.go b/data/types.go
index 6ec2fc3..ebf7e5c 100644
--- a/data/types.go
+++ b/data/types.go
@@ -147,29 +147,26 @@ func (r *Role) AddKeyIDs(ids []string) bool {
 	return changed
 }
 
-type Files map[string]FileMeta
-
-type FileMeta struct {
-	Length int64            `json:"length,omitempty"`
-	Hashes Hashes           `json:"hashes,omitempty"`
-	Custom *json.RawMessage `json:"custom,omitempty"`
-}
+type Files map[string]TargetFileMeta
 
 type Hashes map[string]HexBytes
 
-func (f FileMeta) HashAlgorithms() []string {
-	funcs := make([]string, 0, len(f.Hashes))
-	for name := range f.Hashes {
+func (f Hashes) HashAlgorithms() []string {
+	funcs := make([]string, 0, len(f))
+	for name := range f {
 		funcs = append(funcs, name)
 	}
 	return funcs
 }
 
-type SnapshotFileMeta struct {
-	FileMeta
-	Version int64 `json:"version"`
+type metapathFileMeta struct {
+	Length  int64  `json:"length,omitempty"`
+	Hashes  Hashes `json:"hashes,omitempty"`
+	Version int64  `json:"version"`
 }
 
+type SnapshotFileMeta metapathFileMeta
+
 type SnapshotFiles map[string]SnapshotFileMeta
 
 type Snapshot struct {
@@ -190,14 +187,20 @@ func NewSnapshot() *Snapshot {
 	}
 }
 
+type FileMeta struct {
+	Length int64  `json:"length"`
+	Hashes Hashes `json:"hashes"`
+}
+
 type TargetFiles map[string]TargetFileMeta
 
 type TargetFileMeta struct {
 	FileMeta
+	Custom *json.RawMessage `json:"custom,omitempty"`
 }
 
 func (f TargetFileMeta) HashAlgorithms() []string {
-	return f.FileMeta.HashAlgorithms()
+	return f.FileMeta.Hashes.HashAlgorithms()
 }
 
 type Targets struct {
@@ -300,10 +303,7 @@ func NewTargets() *Targets {
 	}
 }
 
-type TimestampFileMeta struct {
-	FileMeta
-	Version int64 `json:"version"`
-}
+type TimestampFileMeta metapathFileMeta
 
 type TimestampFiles map[string]TimestampFileMeta
 
diff --git a/repo.go b/repo.go
index 887a04a..90d1692 100644
--- a/repo.go
+++ b/repo.go
@@ -1027,7 +1027,7 @@ func (r *Repo) AddTargetsWithDigest(digest string, digestAlg string, length int6
 	// This is the targets role that needs to sign the target file.
 	targetsRoleName := delegation.Delegatee.Name
 
-	meta := data.FileMeta{Length: length, Hashes: make(data.Hashes, 1)}
+	meta := data.TargetFileMeta{FileMeta: data.FileMeta{Length: length, Hashes: make(data.Hashes, 1)}}
 	meta.Hashes[digestAlg], err = hex.DecodeString(digest)
 	if err != nil {
 		return err
@@ -1044,7 +1044,7 @@ func (r *Repo) AddTargetsWithDigest(digest string, digestAlg string, length int6
 	// What does G2 mean? Copying and pasting this comment from elsewhere in this file.
 	// G2 -> we no longer desire any readers to ever observe non-prefix targets.
 	delete(targetsMeta.Targets, "/"+path)
-	targetsMeta.Targets[path] = data.TargetFileMeta{FileMeta: meta}
+	targetsMeta.Targets[path] = meta
 
 	targetsMeta.Expires = expires.Round(time.Second)
 
diff --git a/util/util.go b/util/util.go
index 7ccb0d3..0878c82 100644
--- a/util/util.go
+++ b/util/util.go
@@ -247,8 +247,9 @@ func GenerateSnapshotFileMeta(r io.Reader, hashAlgorithms ...string) (data.Snaps
 		return data.SnapshotFileMeta{}, err
 	}
 	return data.SnapshotFileMeta{
-		FileMeta: m,
-		Version:  v,
+		Length:  m.Length,
+		Hashes:  m.Hashes,
+		Version: v,
 	}, nil
 }
 
@@ -268,8 +269,9 @@ func GenerateTimestampFileMeta(r io.Reader, hashAlgorithms ...string) (data.Time
 		return data.TimestampFileMeta{}, err
 	}
 	return data.TimestampFileMeta{
-		FileMeta: m,
-		Version:  v,
+		Length:  m.Length,
+		Hashes:  m.Hashes,
+		Version: v,
 	}, nil
 }
 


Test patch:
diff --git a/repo_test.go b/repo_test.go
index a8f4405..8fcfabc 100644
--- a/repo_test.go
+++ b/repo_test.go
@@ -1282,12 +1282,12 @@ func (rs *RepoSuite) TestHashAlgorithm(c *C) {
 		c.Assert(err, IsNil)
 		for name, file := range map[string]data.FileMeta{
 			"foo.txt":       targets.Targets["foo.txt"].FileMeta,
-			"targets.json":  snapshot.Meta["targets.json"].FileMeta,
-			"snapshot.json": timestamp.Meta["snapshot.json"].FileMeta,
+			"targets.json":  {Length: snapshot.Meta["targets.json"].Length, Hashes: snapshot.Meta["targets.json"].Hashes},
+			"snapshot.json": {Length: timestamp.Meta["snapshot.json"].Length, Hashes: timestamp.Meta["snapshot.json"].Hashes},
 		} {
 			for _, hashAlgorithm := range test.expected {
 				if _, ok := file.Hashes[hashAlgorithm]; !ok {
-					c.Fatalf("expected %s hash to contain hash func %s, got %s", name, hashAlgorithm, file.HashAlgorithms())
+					c.Fatalf("expected %s hash to contain hash func %s, got %s", name, hashAlgorithm, file.Hashes.HashAlgorithms())
 				}
 			}
 		}
@@ -2654,3 +2654,50 @@ func (rs *RepoSuite) TestSnapshotWithInvalidRoot(c *C) {
 	c.Assert(r.Timestamp(), IsNil)
 	c.Assert(r.Commit(), IsNil)
 }
+
+// Regression test: Do not omit length in target metadata files.
+func (rs *RepoSuite) TestTargetMetadataLength(c *C) {
+	files := map[string][]byte{"foo.txt": []byte("")}
+	local := MemoryStore(make(map[string]json.RawMessage), files)
+	r, err := NewRepo(local)
+	c.Assert(err, IsNil)
+
+	// Init should create targets.json, but not signed yet
+	r.Init(false)
+
+	genKey(c, r, "root")
+	genKey(c, r, "targets")
+	genKey(c, r, "snapshot")
+	genKey(c, r, "timestamp")
+	c.Assert(r.AddTarget("foo.txt", nil), IsNil)
+	c.Assert(r.Snapshot(), IsNil)
+	c.Assert(r.Timestamp(), IsNil)
+	c.Assert(r.Commit(), IsNil)
+
+	// Check length field of foo.txt exists.
+	meta, err := local.GetMeta()
+	c.Assert(err, IsNil)
+	targetsJSON, ok := meta["targets.json"]
+	if !ok {
+		c.Fatal("missing targets metadata")
+	}
+	s := &data.Signed{}
+	c.Assert(json.Unmarshal(targetsJSON, s), IsNil)
+	fmt.Fprintf(os.Stderr, string(s.Signed))
+	var objMap map[string]json.RawMessage
+	c.Assert(json.Unmarshal(s.Signed, &objMap), IsNil)
+	targetsMap, ok := objMap["targets"]
+	if !ok {
+		c.Fatal("missing targets field in targets metadata")
+	}
+	c.Assert(json.Unmarshal(targetsMap, &objMap), IsNil)
+	targetsMap, ok = objMap["foo.txt"]
+	if !ok {
+		c.Fatal("missing foo.txt in targets")
+	}
+	c.Assert(json.Unmarshal(targetsMap, &objMap), IsNil)
+	targetsMap, ok = objMap["length"]
+	if !ok {
+		c.Fatal("missing length field in foo.txt file meta")
+	}
+}
diff --git a/util/util_test.go b/util/util_test.go
index 2e687c4..ca691ef 100644
--- a/util/util_test.go
+++ b/util/util_test.go
@@ -118,10 +118,8 @@ func (UtilSuite) TestSnapshotFileMetaEqual(c *C) {
 
 	fileMeta := func(version int64, length int64, hashes map[string]string) data.SnapshotFileMeta {
 		return data.SnapshotFileMeta{
-			FileMeta: data.FileMeta{
-				Length: length,
-				Hashes: makeHashes(c, hashes),
-			},
+			Length:  length,
+			Hashes:  makeHashes(c, hashes),
 			Version: version,
 		}
 	}
@@ -158,9 +156,10 @@ func (UtilSuite) TestSnapshotFileMetaEqual(c *C) {
 	}
 
 	for _, t := range testMetaFileCases(c) {
-		actual := data.SnapshotFileMeta{FileMeta: t.actual}
-		expected := data.SnapshotFileMeta{FileMeta: t.expected}
-		c.Assert(SnapshotFileMetaEqual(actual, expected), DeepEquals, t.err(t), Commentf("name = %s", t.name))
+		actual := data.SnapshotFileMeta{Length: t.actual.Length, Hashes: t.actual.Hashes}
+		expected := data.SnapshotFileMeta{Length: t.expected.Length, Hashes: t.expected.Hashes}
+		c.Assert(SnapshotFileMetaEqual(actual, expected), DeepEquals, t.err(t), Commentf("name = %s %d %d", t.name, t.actual.Length, t.expected.Length))
+		c.Assert(FileMetaEqual(t.actual, t.expected), DeepEquals, t.err(t), Commentf("name = %s", t.name))
 	}
 }