cleanup: Initialize new root and targets at version 1

[asraa]

Currently, the API for NewRoot() and NewTargets() initialized version at 0, whereas they should generally be started at 1.

Clean this up, and existing tests.
https://github.com/theupdateframework/go-tuf/pull/175/files#r850775514

[abs007]

Hi, it seems that the spec versions are already initialized to 1 in types.go

[asraa]

Hi, it seems that the spec versions are already initialized to 1 in types.go

Hey! It's not spec version, but the version of the metadata file.

[abs007]

Could you point me to the file where the metadata version is specified? I've also looked at repo.go and there seems to be a version getting assigned in line 125 and 130 though I'm not sure if its the metadata version.

[znewman01]

See context here: https://github.com/theupdateframework/go-tuf/pull/175/files#r850775514

Those lines you've pointed out do set the root/targets to 1! This issue is meant to track moving that over to NewTargets and NewRoot into data.go.

For instance, look at NewRoot:

go-tuf/data/types.go

Lines 120 to 129 in 4f55897

	func NewRoot() *Root {
	return &Root{
	Type: "root",
	SpecVersion: "1.0",
	Expires: DefaultExpires("root"),
	Keys: make(map[string]*PublicKey),
	Roles: make(map[string]*Role),
	ConsistentSnapshot: true,
	}
	}

It doesn't set Version, which means that it gets set to the default int64 value of 0.

The fix for this issue is easy: just set Version to 1 in NewRoot (data.go) and NewTargets, and remove the lines that you found it repo.go. The hard part is that a lot of existing tests break when you do that 🙂 So you have to fix those too.

[abs007]

Oh, gotcha. The review wasn't loading for me earlier. Thanks for the explanation!

[abs007]

Currently, the version is incremented the first time the metadata is staged to the local store.

Could I get some more info as to where exactly does the version get incremented automatically?

Asking this as tests check to see if the version results to 1. With me adding Version: 1 in NewRoot() and NewTargets(), the extra incrementation takes the Version to 2. Hence I'm confused between dealing with the incrementation or checking for a result of 2 instead.

[rdimitrov]

@abs007 - Sure, here are a few tips, but do check out the rest of the code too 👍

• go-tuf/repo.go

  Line 125 in b4b954d

  root.Version = 1

• go-tuf/repo.go

  Line 236 in b4b954d

  root.Version++

• go-tuf/repo.go

  Line 461 in b4b954d

  root.Version++

• go-tuf/repo.go

  Line 572 in b4b954d

  root.Version++

[abs007]

@ethan-lowman-dd I'm attempting to solve this issue and I have set Version to 1 for NewRoot() and NewTargets(). So now I'm wondering if I should remove the code lines that increment the version during staging since Version has a value of 1 by default now. Would appreciate your feedback on this.

[ethan-lowman-dd]

@abs007 The relevant code is in repo.go, searching for FileIsStaged, Version++, and Version =. We cant remove the Version increment completely, since it still needs to be incremented somehow when updates are staged. I would first update the NewRoot() and NewTargets() to initialize Version to 1, and then focus on getting the existing unit tests to pass. If I remember correctly, the issue is that if you initialize Version to 1 in NewRoot() and NewTargets(), this metadata is not staged in the local store, so when something like this runs:

	if !r.local.FileIsStaged("root.json") {
		root.Version++
	}

the root.json is incremented to 2, skipping version 1. I would see if it's maybe possible to stage metadata right after where NewRoot() and NewTargets() are called in repo.go.

[abs007]

@ethan-lowman-dd So from what I could understand, in the functions where

	if !r.local.FileIsStaged("root.json") {
		root.Version++
	}

is present, the root() method is being called (which returns a pointer to the data.Root struct) prior to the code block. root() now checks to see if any metadata for "root.json" exists. If it doesn't, then the NewRoot() func is called to return a newly initialized root, else, the json data is unmarshaled and fed to a Root type which is then returned. So, staging metadata after NewRoot() doesn't seem possible since root() would always have to run prior to this block.

Since, Version needs to be incremented whenever the metadata isn't staged, we can check for 2 in the tests instead. I thought of having if !r.local.FileIsStaged("root.json") && root.Version == 0 {} but this doesn't make much sense as Version is going to default to 1 now.

[abs007]

Hi, I'd like a final go-ahead to push a pr with the suggested changes. Thanks.

[znewman01]

Go ahead!

[abs007]

It seems that the role1.json file also gets it's metadata version bumped up when checking in line 2087. Target or root json files getting their version bumped up makes sense because of the change but why is is happening to the role1.json file too? Any context would help, thanks. Also, the same thing can be observed in lines 2365 - 2373 too.

[abs007]

Is this line supposed to exist?

[rdimitrov]

Closing since the code base changed and this is already fixed in the new implementation.

Thanks for raising this 👍