Repository: theupdateframework/go-tuf
Pull request #210 (2022-01-25 21:00:35)

Add client method for initializing from root metadata
This will allow users of go-tuf to initialize a client
without requiring a remote connection. This is useful
for when a root has been updated and the local root
verification keys can no longer verify the the latest
remote root, except by verifying the chain with
client.Update().

Ref #208

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>

haydentherapper: cc @asraa 

Open question: Should we remove `client.Init` in favor of this method? Given that go-tuf does not have a 1.x.x release, I'd assume it'd be fine to drop support for `client.Init` without having to worry about deprecation.
joshuagl: 
> Open question: Should we remove `client.Init` in favor of this method? Given that go-tuf does not have a 1.x.x release, I'd assume it'd be fine to drop support for `client.Init` without having to worry about deprecation.

The absence of a formal. 1.x.x release does technically absolve us of having to worry about deprecation. However, go-tuf has active users who have been bitten by API changes (i.e. https://github.com/theupdateframework/go-tuf/issues/203). We should be wary about making any breaking API and behaviour changes until we have made a release that can be pinned to and formalised our releases. See https://github.com/theupdateframework/go-tuf/issues/200 and https://github.com/theupdateframework/go-tuf/issues/114
hosseinsia: To the point that @joshuagl made about active users, we are using the existing init ... so removing it would break us ...
trishankatdatadog: Do we have a release process (which includes making breaking MINOR 0.MINOR.PATCH releases for now) formalized? If so, we should formalize it first before merging this.
coveralls: ## Pull Request Test Coverage Report for [Build 1747687722](https://coveralls.io/builds/45977457)

* **8** of **8**   **(100.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage decreased (**-2.6%**) to **70.412%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/45977457/badge)](https://coveralls.io/builds/45977457) |
| :-- | --: |
| Change from base [Build 1741844387](https://coveralls.io/builds/45894834): |  -2.6% |
| Covered Lines: | 2168 |
| Relevant Lines: | 3079 |

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

haydentherapper: @joshuagl - Do you know when this release process will be formalized? I agree that it'd be nice to document breaking changes in a changelog. One thing to note is this would be a noticeable breaking change, so users who pull in the latest changes would be able to recognize that `Init`'s params have changed.

+1 to @asraa's comment about this following the spec. It calls out that you should be loading a "trusted root metadata file", not just the root keys. It's worth mentioning again that ideally by default, `Init` would just "load", by initializing state from a known root, and update mechanisms should be kept separate.

@hosseinsia, as part of the refactor, we can change all of our usages of `client.Init`. I see we're using it in the `tuf-client init` [command](https://github.com/theupdateframework/go-tuf/blob/master/cmd/tuf-client/init.go). We can deprecate the usage of the `root-keys-file` flag and switch to a `root-metadata` flag. This should actually make the command easier to use, since the [keys format](https://github.com/theupdateframework/go-tuf/blob/master/data/types.go#L42) has some complexity, and users should always have the root metadata file available. Note that this won't impact `tuf init`, since that uses `repo.Init`.
haydentherapper: I've updated the PR, changing `Init` to only take the root metadata JSON. This commit could be split into its own PR, but I think that we should decide when to make the breaking change to change the parameters of `Init`. My preference would be making the change in this PR.
hosseinsia: @asraa 
Yes please. That would not break us and makes the transition much smoother. Thank you!
haydentherapper: @hosseinsia, how should we communicate the differences between `Init` and `InitLocal` to convince users to switch over? And furthermore, if users do start switching over to `InitLocal`, then we'll end up breaking them when we remove the method in the future.

This change shouldn't break anyone who's using a pinned version of go-tuf. Once someone updates to the latest commit in go-tuf, their build will break, but it should be clear what the issue is. Either we can include changelog with this commit, or just assume that the documentation on the function is clear.

It's also worth noting that impact is low according to https://cs.github.com/?scopeName=All+repos&scope=&q=%22github.com%2Ftheupdateframework%2Fgo-tuf%2Fclient%22+AND+%22.Init%22+AND+-path%3Acosign+AND+-repo%3Atheupdateframework%2Fgo-tuf - I've looked through these and mostly see either usage with Sigstore maintainers or go-tuf maintainers.

I'd prefer that we make the breaking change now. This will make Init align to the TUF specification, since Init does not go through the secure root update flow.
hosseinsia: Potentially, in a release note, we can specify that the `InitLocal` is recommended over the existing method. Also, we can update the comment the `init` and tag it as deprecated to decentivize the usage (see deprecated here https://go.dev/blog/godoc). regardless of whether we keep or remove the `init`, we have to do it in two PR to make the roll-back easier. WDYT?
haydentherapper: @hosseinsia I've updated the PR and added a deprecation comment to Init. Please take another look! 

I'll leave #208 open until a versioning system is in place, when we can revisit this issue and remove `Init`. 
asraa: @hosseinsia staticcheck doesn't like seeing usage of `Init` in the codebase now that it's deprecated. Can we leave `Init` as an public method of the client API, and switch over the CLI and tests to use `InitLocal`? That shouldn't break your usage of go-tuf's Client as it's deprecated. 

(Also, this still won't break anyone unless they manually update the pinned sha of go-tuf)

@trishankatdatadog 
hosseinsia: @asraa good resolution. Thanks!
haydentherapper: Thanks all for the feedback! Updated the client to switch to InitLocal.

Patch:
diff --git a/client/client.go b/client/client.go
index b4ea7f9..893e161 100644
--- a/client/client.go
+++ b/client/client.go
@@ -111,6 +111,8 @@ func NewClient(local LocalStore, remote RemoteStore) *Client {
 // The latest root.json is fetched from remote storage, verified using rootKeys
 // and threshold, and then saved in local storage. It is expected that rootKeys
 // were securely distributed with the software being updated.
+//
+// Deprecated: Use c.InitLocal and c.Update to initialize a local repository.
 func (c *Client) Init(rootKeys []*data.PublicKey, threshold int) error {
 	if len(rootKeys) < threshold {
 		return ErrInsufficientKeys
@@ -147,6 +149,20 @@ func (c *Client) Init(rootKeys []*data.PublicKey, threshold int) error {
 	return c.local.SetMeta("root.json", rootJSON)
 }
 
+// InitLocal initializes a local repository from root metadata.
+//
+// The root's keys are extracted from the root and saved in local storage.
+// Root expiration is not checked.
+// It is expected that rootJSON was securely distributed with the software
+// being updated.
+func (c *Client) InitLocal(rootJSON []byte) error {
+	err := c.loadAndVerifyRootMeta(rootJSON, true /*ignoreExpiredCheck*/)
+	if err != nil {
+		return err
+	}
+	return c.local.SetMeta("root.json", rootJSON)
+}
+
 // Update downloads and verifies remote metadata and returns updated targets.
 // It always performs root update (5.2 and 5.3) section of the v1.0.19 spec.
 //
@@ -430,6 +446,12 @@ func (c *Client) loadAndVerifyLocalRootMeta(ignoreExpiredCheck bool) error {
 	if !ok {
 		return ErrNoRootKeys
 	}
+	return c.loadAndVerifyRootMeta(rootJSON, ignoreExpiredCheck)
+}
+
+// loadAndVerifyRootMeta decodes and verifies root metadata and loads the top-level keys.
+// This method first clears the DB for top-level keys and then loads the new keys.
+func (c *Client) loadAndVerifyRootMeta(rootJSON []byte, ignoreExpiredCheck bool) error {
 	// unmarshal root.json without verifying as we need the root
 	// keys first
 	s := &data.Signed{}
diff --git a/cmd/tuf-client/init.go b/cmd/tuf-client/init.go
index 6b1296a..f043c0b 100644
--- a/cmd/tuf-client/init.go
+++ b/cmd/tuf-client/init.go
@@ -1,28 +1,26 @@
 package main
 
 import (
-	"encoding/json"
 	"io"
 	"os"
 
 	"github.com/flynn/go-docopt"
 	tuf "github.com/theupdateframework/go-tuf/client"
-	"github.com/theupdateframework/go-tuf/data"
 )
 
 func init() {
 	register("init", cmdInit, `
-usage: tuf-client init [-s|--store=<path>] <url> [<root-keys-file>]
+usage: tuf-client init [-s|--store=<path>] <url> [<root-metadata-file>]
 
 Options:
   -s <path>    The path to the local file store [default: tuf.db]
 
-Initialize the local file store with root keys.
+Initialize the local file store with root metadata.
   `)
 }
 
 func cmdInit(args *docopt.Args, client *tuf.Client) error {
-	file := args.String["<root-keys-file>"]
+	file := args.String["<root-metadata-file>"]
 	var in io.Reader
 	if file == "" || file == "-" {
 		in = os.Stdin
@@ -33,9 +31,9 @@ func cmdInit(args *docopt.Args, client *tuf.Client) error {
 			return err
 		}
 	}
-	var rootKeys []*data.PublicKey
-	if err := json.NewDecoder(in).Decode(&rootKeys); err != nil {
+	bytes, err := io.ReadAll(in)
+	if err != nil {
 		return err
 	}
-	return client.Init(rootKeys, len(rootKeys))
+	return client.InitLocal(bytes)
 }


Test patch:
diff --git a/client/client_test.go b/client/client_test.go
index c0be244..5ad38b3 100644
--- a/client/client_test.go
+++ b/client/client_test.go
@@ -280,6 +280,50 @@ func (s *ClientSuite) TestInit(c *C) {
 	c.Assert(err, IsNil)
 }
 
+func (s *ClientSuite) TestInitLocalAllowsExpired(c *C) {
+	s.genKeyExpired(c, "targets")
+	c.Assert(s.repo.Snapshot(), IsNil)
+	c.Assert(s.repo.Timestamp(), IsNil)
+	c.Assert(s.repo.Commit(), IsNil)
+	s.syncRemote(c)
+	client := NewClient(MemoryLocalStore(), s.remote)
+	bytes, err := io.ReadAll(s.remote.meta["root.json"])
+	c.Assert(err, IsNil)
+	s.withMetaExpired(func() {
+		c.Assert(client.InitLocal(bytes), IsNil)
+	})
+}
+
+func (s *ClientSuite) TestInitLocal(c *C) {
+	client := NewClient(MemoryLocalStore(), s.remote)
+	bytes, err := io.ReadAll(s.remote.meta["root.json"])
+	c.Assert(err, IsNil)
+	dataSigned := &data.Signed{}
+	c.Assert(json.Unmarshal(bytes, dataSigned), IsNil)
+	root := &data.Root{}
+	c.Assert(json.Unmarshal(dataSigned.Signed, root), IsNil)
+
+	// check Update() returns ErrNoRootKeys when uninitialized
+	_, err = client.Update()
+	c.Assert(err, Equals, ErrNoRootKeys)
+
+	// check InitLocal() returns ErrInvalid when the root's signature is
+	// invalid
+	// modify root and marshal without regenerating signatures
+	root.Version = root.Version + 1
+	rootBytes, err := json.Marshal(root)
+	c.Assert(err, IsNil)
+	dataSigned.Signed = rootBytes
+	dataBytes, err := json.Marshal(dataSigned)
+	c.Assert(err, IsNil)
+	c.Assert(client.InitLocal(dataBytes), Equals, verify.ErrInvalid)
+
+	// check Update() does not return ErrNoRootKeys after initialization
+	c.Assert(client.InitLocal(bytes), IsNil)
+	_, err = client.Update()
+	c.Assert(err, IsNil)
+}
+
 func (s *ClientSuite) TestFirstUpdate(c *C) {
 	files, err := s.newClient(c).Update()
 	c.Assert(err, IsNil)
diff --git a/client/delegations_test.go b/client/delegations_test.go
index 13625d1..801858d 100644
--- a/client/delegations_test.go
+++ b/client/delegations_test.go
@@ -268,19 +268,7 @@ func initTestDelegationClient(t *testing.T, dirPrefix string) (*Client, func() e
 	c := NewClient(MemoryLocalStore(), remote)
 	rawFile, err := ioutil.ReadFile(initialStateDir + "/" + "root.json")
 	assert.Nil(t, err)
-	s := &data.Signed{}
-	root := &data.Root{}
-	assert.Nil(t, json.Unmarshal(rawFile, s))
-	assert.Nil(t, json.Unmarshal(s.Signed, root))
-	var keys []*data.PublicKey
-	for _, sig := range s.Signatures {
-		k, ok := root.Keys[sig.KeyID]
-		if ok {
-			keys = append(keys, k)
-		}
-	}
-
-	assert.Nil(t, c.Init(keys, 1))
+	assert.Nil(t, c.InitLocal(rawFile))
 	files, err := ioutil.ReadDir(initialStateDir)
 	assert.Nil(t, err)
 
diff --git a/client/interop_test.go b/client/interop_test.go
index dfe90e9..19a8d9b 100644
--- a/client/interop_test.go
+++ b/client/interop_test.go
@@ -2,8 +2,8 @@ package client
 
 import (
 	"bytes"
-	"encoding/json"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"net"
 	"net/http"
@@ -11,7 +11,6 @@ import (
 	"path/filepath"
 	"strconv"
 
-	"github.com/theupdateframework/go-tuf/data"
 	"github.com/theupdateframework/go-tuf/util"
 	. "gopkg.in/check.v1"
 
@@ -150,9 +149,12 @@ func (t *testCase) runStep(c *C, stepName string) {
 	c.Assert(err, IsNil)
 
 	client := NewClient(t.local, remote)
-	// initiate a client with the root keys
-	keys := getKeys(c, remote)
-	c.Assert(client.Init(keys, 1), IsNil)
+	// initiate a client with the root metadata
+	ioReader, _, err := remote.GetMeta("root.json")
+	c.Assert(err, IsNil)
+	rootJsonBytes, err := io.ReadAll(ioReader)
+	c.Assert(err, IsNil)
+	c.Assert(client.InitLocal(rootJsonBytes), IsNil)
 
 	// check update returns the correct updated targets
 	files, err := client.Update()
@@ -191,31 +193,3 @@ func startFileServer(c *C, dir string) (string, func() error) {
 	go http.Serve(l, http.FileServer(http.Dir(dir)))
 	return addr, l.Close
 }
-
-func getKeys(c *C, remote RemoteStore) []*data.PublicKey {
-	r, _, err := remote.GetMeta("root.json")
-	c.Assert(err, IsNil)
-
-	type SignedRoot struct {
-		Signed data.Root
-	}
-	root := &SignedRoot{}
-	err = json.NewDecoder(r).Decode(&root)
-	c.Assert(err, IsNil)
-
-	rootRole, exists := root.Signed.Roles["root"]
-	c.Assert(exists, Equals, true)
-
-	rootKeys := []*data.PublicKey{}
-
-	for _, keyID := range rootRole.KeyIDs {
-		key, exists := root.Signed.Keys[keyID]
-		c.Assert(exists, Equals, true)
-
-		rootKeys = append(rootKeys, key)
-	}
-
-	c.Assert(rootKeys, Not(HasLen), 0)
-
-	return rootKeys
-}
diff --git a/client/python_interop/python_interop_test.go b/client/python_interop/python_interop_test.go
index 590f456..7a4abdc 100644
--- a/client/python_interop/python_interop_test.go
+++ b/client/python_interop/python_interop_test.go
@@ -2,7 +2,6 @@ package client
 
 import (
 	"bytes"
-	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -14,10 +13,7 @@ import (
 
 	tuf "github.com/theupdateframework/go-tuf"
 	client "github.com/theupdateframework/go-tuf/client"
-	"github.com/theupdateframework/go-tuf/data"
-	"github.com/theupdateframework/go-tuf/pkg/keys"
 	"github.com/theupdateframework/go-tuf/util"
-	"golang.org/x/crypto/ed25519"
 	. "gopkg.in/check.v1"
 )
 
@@ -59,17 +55,11 @@ func (InteropSuite) TestGoClientPythonGenerated(c *C) {
 		)
 		c.Assert(err, IsNil)
 
-		// initiate a client with the root keys
-		f, err := os.Open(filepath.Join(testDataDir, dir, "keystore", "root_key.pub"))
-		c.Assert(err, IsNil)
-		key := &data.PublicKey{}
-		c.Assert(json.NewDecoder(f).Decode(key), IsNil)
-		c.Assert(key.Type, Equals, "ed25519")
-		pk, err := keys.GetVerifier(key)
-		c.Assert(err, IsNil)
-		c.Assert(pk.Public(), HasLen, ed25519.PublicKeySize)
+		// initiate a client with the root metadata
 		client := client.NewClient(client.MemoryLocalStore(), remote)
-		c.Assert(client.Init([]*data.PublicKey{key}, 1), IsNil)
+		rootJSON, err := ioutil.ReadFile(filepath.Join(testDataDir, dir, "repository", "metadata", "root.json"))
+		c.Assert(err, IsNil)
+		c.Assert(client.InitLocal(rootJSON), IsNil)
 
 		// check update returns the correct updated targets
 		files, err := client.Update()