fix: abandon updates if timestamp.json isn't new (#387)

Adds a new test for this case: if a client sees a new `timestamp.json`
file with the same version as its current `timestamp.json` file, it
should do nothing (no update, but also no error).

A few other tests were implicitly relying on the fact that the client
did a full update each time, so they've been updated to commit a new
timestamp.

This updates go-tuf for TUF specification v1.0.30 (fixes #321). The
only substantive change was
[theupdateframework/specification#209][tuf-spec-209], which clarifies
the intended behavior for updating metadata files.

Updates for other roles were already in compliance:

- Root metadata: https://github.com/theupdateframework/go-tuf/blob/13eff30efd6c61f165e1bf06e8c0e72f5a0e5703/client/client.go#L258
- Timestamp, checking snapshot version: https://github.com/theupdateframework/go-tuf/blob/13eff30efd6c61f165e1bf06e8c0e72f5a0e5703/client/client.go#L751
- Snapshot, must match version from timestamp: https://github.com/theupdateframework/go-tuf/blob/13eff30efd6c61f165e1bf06e8c0e72f5a0e5703/client/client.go#L667
- Snapshot, no rollback of targets: https://github.com/theupdateframework/go-tuf/blob/13eff30efd6c61f165e1bf06e8c0e72f5a0e5703/client/client.go#L685
- Targets: https://github.com/theupdateframework/go-tuf/blob/13eff30efd6c61f165e1bf06e8c0e72f5a0e5703/client/client.go#L643

[tuf-spec-209]: (theupdateframework/specification#209).

Signed-off-by: Zachary Newman <[email protected]>

Signed-off-by: Zachary Newman <[email protected]>

Author: znewman01

client/client.go

@@ -143,7 +143,13 @@ func (c *Client) Update() (data.TargetFiles, error) {
 	}
 	// 5.4.(2,3 and 4) - Verify timestamp against various attacks
 	// Returns the extracted snapshot metadata
-	snapshotMeta, err := c.decodeTimestamp(timestampJSON)
+	snapshotMeta, sameTimestampVersion, err := c.decodeTimestamp(timestampJSON)
+	if sameTimestampVersion {
+		// The new timestamp.json file had the same version; we don't need to
+		// update, so bail early.
+		return c.targets, nil
+	}
+
 	if err != nil {
 		return nil, err
 	}
@@ -740,22 +746,31 @@ func (c *Client) decodeTargets(b json.RawMessage) (data.TargetFiles, error) {
 }
 
 // decodeTimestamp decodes and verifies timestamp metadata, and returns the
-// new snapshot file meta.
-func (c *Client) decodeTimestamp(b json.RawMessage) (data.TimestampFileMeta, error) {
+// new snapshot file meta and signals whether the update should be aborted early
+// (the new timestamp has the same version as the old one, so there's no need to
+// complete the update).
+func (c *Client) decodeTimestamp(b json.RawMessage) (data.TimestampFileMeta, bool, error) {
 	timestamp := &data.Timestamp{}
+
 	if err := c.db.Unmarshal(b, timestamp, "timestamp", c.timestampVer); err != nil {
-		return data.TimestampFileMeta{}, ErrDecodeFailed{"timestamp.json", err}
+		return data.TimestampFileMeta{}, false, ErrDecodeFailed{"timestamp.json", err}
+	}
+	// 5.4.3.1  - Check for timestamp rollback attack
+	// We already checked for timestamp.Version < c.timestampVer in the Unmarshal call above.
+	// Here, we're checking for version equality, which indicates that we can abandon this update.
+	if timestamp.Version == c.timestampVer {
+		return data.TimestampFileMeta{}, true, nil
 	}
 	// 5.4.3.2 - Check for snapshot rollback attack
 	// Verify that the current snapshot meta version is less than or equal to the new one
 	if timestamp.Meta["snapshot.json"].Version < c.snapshotVer {
-		return data.TimestampFileMeta{}, verify.ErrLowVersion{Actual: timestamp.Meta["snapshot.json"].Version, Current: c.snapshotVer}
+		return data.TimestampFileMeta{}, false, verify.ErrLowVersion{Actual: timestamp.Meta["snapshot.json"].Version, Current: c.snapshotVer}
 	}
-	// At this point we can trust the new timestamp and the snaphost version it refers to
+	// At this point we can trust the new timestamp and the snapshot version it refers to
 	// so we can update the client's trusted versions and proceed with persisting the new timestamp
 	c.timestampVer = timestamp.Version
 	c.snapshotVer = timestamp.Meta["snapshot.json"].Version
-	return timestamp.Meta["snapshot.json"], nil
+	return timestamp.Meta["snapshot.json"], false, nil
 }
 
 // hasMetaFromSnapshot checks whether local metadata has the given meta

client/client_test.go

@@ -896,6 +896,7 @@ func (s *ClientSuite) TestUpdateMixAndMatchAttack(c *C) {
 	c.Assert(s.repo.AddTargetWithExpires("foo.txt", nil, expires), IsNil)
 	c.Assert(s.repo.Snapshot(), IsNil)
 	c.Assert(s.repo.Timestamp(), IsNil)
+	c.Assert(s.repo.Commit(), IsNil)
 	s.syncRemote(c)
 	client := s.updatedClient(c)
 
@@ -909,6 +910,7 @@ func (s *ClientSuite) TestUpdateMixAndMatchAttack(c *C) {
 	c.Assert(s.repo.AddTargetWithExpires("bar.txt", nil, expires), IsNil)
 	c.Assert(s.repo.Snapshot(), IsNil)
 	c.Assert(s.repo.Timestamp(), IsNil)
+	c.Assert(s.repo.Commit(), IsNil)
 	s.syncRemote(c)
 	newTargets, ok := s.remote.meta["targets.json"]
 	if !ok {
@@ -964,6 +966,41 @@ func (s *ClientSuite) TestUpdateReplayAttack(c *C) {
 	})
 }
 
+func (s *ClientSuite) TestUpdateForkTimestamp(c *C) {
+	client := s.updatedClient(c)
+
+	// grab the remote timestamp.json
+	oldTimestamp, ok := s.remote.meta["timestamp.json"]
+	if !ok {
+		c.Fatal("missing remote timestamp.json")
+	}
+
+	// generate a new timestamp and sync with the client
+	version := client.timestampVer
+	c.Assert(version > 0, Equals, true)
+	c.Assert(s.repo.Timestamp(), IsNil)
+	s.syncRemote(c)
+	_, err := client.Update()
+	c.Assert(err, IsNil)
+	newVersion := client.timestampVer
+	c.Assert(newVersion > version, Equals, true)
+
+	// generate a new, different timestamp with the *same version*
+	s.remote.meta["timestamp.json"] = oldTimestamp
+	c.Assert(s.repo.Timestamp(), IsNil)
+	c.Assert(client.timestampVer, Equals, newVersion) // double-check: same version?
+	s.syncRemote(c)
+
+	oldMeta, err := client.local.GetMeta()
+	c.Assert(err, IsNil)
+	_, err = client.Update()
+	c.Assert(err, IsNil) // no error: the targets.json version didn't change, so there was no update!
+	// Client shouldn't update!
+	newMeta, err := client.local.GetMeta()
+	c.Assert(err, IsNil)
+	c.Assert(oldMeta, DeepEquals, newMeta)
+}
+
 func (s *ClientSuite) TestUpdateTamperedTargets(c *C) {
 	client := s.newClient(c)
 
@@ -998,6 +1035,7 @@ func (s *ClientSuite) TestUpdateTamperedTargets(c *C) {
 	c.Assert(err, IsNil)
 	s.store.SetMeta("targets.json", tamperedJSON)
 	s.store.Commit(false, nil, nil)
+	c.Assert(s.repo.Timestamp(), IsNil) // unless timestamp changes, the client doesn't even look at "targets.json"
 	s.syncRemote(c)
 	_, err = client.Update()
 	c.Assert(err, DeepEquals, ErrWrongSize{"targets.json", int64(len(tamperedJSON)), int64(len(targetsJSON))})