Repository: theupdateframework/go-tuf
Pull request #385 (2022-09-16 16:11:57)

feat: pass logger into repo and client
Signed-off-by: Asra Ali <asraa@google.com>

Fixes https://github.com/theupdateframework/go-tuf/issues/376

Please fill in the fields below to submit a pull request.  The more information that is provided, the better.

Release Notes: 
```
* feat: Allows passing an optional logger into `repo`. Default uses `io.Discard` to swallow logs.
* feat: Exposes a `repo.NewRepoWithOpts` that can be used to instantiate a repo with repo options like prefix, indent, hash algorithms, and logger.
```

**Types of changes**:
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ x ] 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**:
Client and Repo can now pass in an optional logger. This will not break any existing clients, the argument is optional.

**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

**This is pretty preliminary, please let me know if you agree/disagree with my approach. 

I would still need to fix the CLI as a follow-up, this PR as it stands totally removes logging since none is explicitly passed.
**



Patch:
diff --git a/.golangci.yml b/.golangci.yml
index 6e8bf3c..570c05d 100644
--- a/.golangci.yml
+++ b/.golangci.yml
@@ -13,3 +13,4 @@ linters:
     - gosimple
     - unused
     - typecheck
+    - forbidigo
diff --git a/cmd/tuf-client/main.go b/cmd/tuf-client/main.go
index 15d03fa..25a3200 100644
--- a/cmd/tuf-client/main.go
+++ b/cmd/tuf-client/main.go
@@ -3,6 +3,7 @@ package main
 import (
 	"fmt"
 	"log"
+	"os"
 
 	docopt "github.com/flynn/go-docopt"
 	tuf "github.com/theupdateframework/go-tuf/client"
@@ -32,7 +33,7 @@ See "tuf-client help <command>" for more information on a specific command.
 
 	if cmd == "help" {
 		if len(cmdArgs) == 0 { // `tuf-client help`
-			fmt.Println(usage)
+			fmt.Fprint(os.Stderr, usage)
 			return
 		} else { // `tuf-client help <command>`
 			cmd = cmdArgs[0]
diff --git a/cmd/tuf/gen_key.go b/cmd/tuf/gen_key.go
index bd4334a..2ad77a5 100644
--- a/cmd/tuf/gen_key.go
+++ b/cmd/tuf/gen_key.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"os"
 	"time"
 
 	"github.com/flynn/go-docopt"
@@ -39,7 +40,7 @@ func cmdGenKey(args *docopt.Args, repo *tuf.Repo) error {
 		string(data.KeySchemeRSASSA_PSS_SHA256):
 		keyScheme = data.KeyScheme(t)
 	default:
-		fmt.Println("Using default key scheme", keyScheme)
+		fmt.Fprint(os.Stderr, "Using default key scheme", keyScheme)
 	}
 
 	var err error
@@ -57,7 +58,7 @@ func cmdGenKey(args *docopt.Args, repo *tuf.Repo) error {
 		return err
 	}
 	for _, id := range keyids {
-		fmt.Println("Generated", role, keyScheme, "key with ID", id)
+		fmt.Fprintf(os.Stdout, "Generated %s %s key with ID %s", role, keyScheme, id)
 	}
 	return nil
 }
diff --git a/cmd/tuf/get_threshold.go b/cmd/tuf/get_threshold.go
index e40ec26..a0d78fd 100644
--- a/cmd/tuf/get_threshold.go
+++ b/cmd/tuf/get_threshold.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"os"
 
 	"github.com/flynn/go-docopt"
 	"github.com/theupdateframework/go-tuf"
@@ -23,6 +24,6 @@ func cmdGetThreshold(args *docopt.Args, repo *tuf.Repo) error {
 		return err
 	}
 
-	fmt.Println("The threshold for", role, "role is", threshold)
+	fmt.Fprintf(os.Stdout, "The threshold for %s role is %d", role, threshold)
 	return nil
 }
diff --git a/cmd/tuf/main.go b/cmd/tuf/main.go
index f2b7397..6ee220b 100644
--- a/cmd/tuf/main.go
+++ b/cmd/tuf/main.go
@@ -58,7 +58,7 @@ See "tuf help <command>" for more information on a specific command
 
 	if cmd == "help" {
 		if len(cmdArgs) == 0 { // `tuf help`
-			fmt.Println(usage)
+			fmt.Fprint(os.Stderr, usage)
 			return
 		} else { // `tuf help <command>`
 			cmd = cmdArgs[0]
@@ -115,7 +115,11 @@ func runCommand(name string, args []string, dir string, insecure bool) error {
 	if !insecure {
 		p = getPassphrase
 	}
-	repo, err := tuf.NewRepo(tuf.FileSystemStore(dir, p))
+	logger := log.New(os.Stdout, "", 0)
+	storeOpts := tuf.StoreOpts{Logger: logger, PassFunc: p}
+
+	repo, err := tuf.NewRepoWithOpts(tuf.FileSystemStoreWithOpts(dir, storeOpts),
+		tuf.WithLogger(logger))
 	if err != nil {
 		return err
 	}
diff --git a/cmd/tuf/payload.go b/cmd/tuf/payload.go
index 8cc0c2f..3ae2c89 100644
--- a/cmd/tuf/payload.go
+++ b/cmd/tuf/payload.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"os"
 
 	"github.com/flynn/go-docopt"
 	"github.com/theupdateframework/go-tuf"
@@ -20,6 +21,6 @@ func cmdPayload(args *docopt.Args, repo *tuf.Repo) error {
 	if err != nil {
 		return err
 	}
-	fmt.Print(string(p))
+	fmt.Fprint(os.Stdout, string(p))
 	return nil
 }
diff --git a/cmd/tuf/set_threshold.go b/cmd/tuf/set_threshold.go
index 57754d2..29149ff 100644
--- a/cmd/tuf/set_threshold.go
+++ b/cmd/tuf/set_threshold.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"fmt"
+	"os"
 	"strconv"
 
 	"github.com/flynn/go-docopt"
@@ -28,6 +29,6 @@ func cmdSetThreshold(args *docopt.Args, repo *tuf.Repo) error {
 		return err
 	}
 
-	fmt.Println("The threshold for", role, "role is now", threshold)
+	fmt.Fprintf(os.Stdout, "The threshold for %s role is now %d", role, threshold)
 	return nil
 }
diff --git a/cmd/tuf/sign_payload.go b/cmd/tuf/sign_payload.go
index 8da5642..6772972 100644
--- a/cmd/tuf/sign_payload.go
+++ b/cmd/tuf/sign_payload.go
@@ -36,7 +36,7 @@ func cmdSignPayload(args *docopt.Args, repo *tuf.Repo) error {
 	if err != nil {
 		return err
 	}
-	fmt.Print(string(bytes))
+	fmt.Fprint(os.Stdout, string(bytes))
 
 	fmt.Fprintln(os.Stderr, "tuf: signed with", numKeys, "key(s)")
 	return nil
diff --git a/local_store.go b/local_store.go
index 1b4a7f6..fee03f3 100644
--- a/local_store.go
+++ b/local_store.go
@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"io/fs"
+	"log"
 	"os"
 	"path/filepath"
 	"strings"
@@ -197,18 +198,44 @@ type persistedKeys struct {
 	Data      json.RawMessage `json:"data"`
 }
 
+type StoreOpts struct {
+	Logger   *log.Logger
+	PassFunc util.PassphraseFunc
+}
+
 func FileSystemStore(dir string, p util.PassphraseFunc) LocalStore {
 	return &fileSystemStore{
 		dir:            dir,
 		passphraseFunc: p,
+		logger:         log.New(io.Discard, "", 0),
 		signerForKeyID: make(map[string]keys.Signer),
 		keyIDsForRole:  make(map[string][]string),
 	}
 }
 
+func FileSystemStoreWithOpts(dir string, opts ...StoreOpts) LocalStore {
+	store := &fileSystemStore{
+		dir:            dir,
+		passphraseFunc: nil,
+		logger:         log.New(io.Discard, "", 0),
+		signerForKeyID: make(map[string]keys.Signer),
+		keyIDsForRole:  make(map[string][]string),
+	}
+	for _, opt := range opts {
+		if opt.Logger != nil {
+			store.logger = opt.Logger
+		}
+		if opt.PassFunc != nil {
+			store.passphraseFunc = opt.PassFunc
+		}
+	}
+	return store
+}
+
 type fileSystemStore struct {
 	dir            string
 	passphraseFunc util.PassphraseFunc
+	logger         *log.Logger
 
 	signerForKeyID map[string]keys.Signer
 	keyIDsForRole  map[string][]string
@@ -526,7 +553,7 @@ func (f *fileSystemStore) ChangePassphrase(role string) error {
 	keys, _, err := f.loadPrivateKeys(role)
 	if err != nil {
 		if os.IsNotExist(err) {
-			fmt.Printf("Failed to change passphrase. Missing keys file for %s role. \n", role)
+			f.logger.Printf("Failed to change passphrase. Missing keys file for %s role. \n", role)
 		}
 		return err
 	}
@@ -548,7 +575,7 @@ func (f *fileSystemStore) ChangePassphrase(role string) error {
 	if err := util.AtomicallyWriteFile(f.keysPath(role), append(data, '\n'), 0600); err != nil {
 		return err
 	}
-	fmt.Printf("Successfully changed passphrase for %s keys file\n", role)
+	f.logger.Printf("Successfully changed passphrase for %s keys file\n", role)
 	return nil
 }
 
diff --git a/pkg/keys/deprecated_ecdsa.go b/pkg/keys/deprecated_ecdsa.go
index 4a8f151..6d48c9d 100644
--- a/pkg/keys/deprecated_ecdsa.go
+++ b/pkg/keys/deprecated_ecdsa.go
@@ -9,7 +9,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 
 	"github.com/theupdateframework/go-tuf/data"
 )
@@ -98,6 +97,5 @@ func (p *deprecatedP256Verifier) UnmarshalPublicKey(key *data.PublicKey) error {
 	}
 
 	p.key = key
-	fmt.Fprintln(os.Stderr, "tuf: warning using deprecated ecdsa hex-encoded keys")
 	return nil
 }
diff --git a/repo.go b/repo.go
index 603785f..cce0020 100644
--- a/repo.go
+++ b/repo.go
@@ -7,6 +7,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"log"
 	"path"
 	"sort"
 	"strings"
@@ -48,18 +49,47 @@ type Repo struct {
 	meta           map[string]json.RawMessage
 	prefix         string
 	indent         string
+	logger         *log.Logger
+}
+
+type RepoOpts func(r *Repo)
+
+func WithLogger(logger *log.Logger) RepoOpts {
+	return func(r *Repo) {
+		r.logger = logger
+	}
+}
+
+func WithHashAlgorithms(hashAlgorithms ...string) RepoOpts {
+	return func(r *Repo) {
+		r.hashAlgorithms = hashAlgorithms
+	}
+}
+
+func WithPrefix(prefix string) RepoOpts {
+	return func(r *Repo) {
+		r.prefix = prefix
+	}
+}
+
+func WithIndex(indent string) RepoOpts {
+	return func(r *Repo) {
+		r.indent = indent
+	}
 }
 
 func NewRepo(local LocalStore, hashAlgorithms ...string) (*Repo, error) {
 	return NewRepoIndent(local, "", "", hashAlgorithms...)
 }
 
-func NewRepoIndent(local LocalStore, prefix string, indent string, hashAlgorithms ...string) (*Repo, error) {
+func NewRepoIndent(local LocalStore, prefix string, indent string,
+	hashAlgorithms ...string) (*Repo, error) {
 	r := &Repo{
 		local:          local,
 		hashAlgorithms: hashAlgorithms,
 		prefix:         prefix,
 		indent:         indent,
+		logger:         log.New(io.Discard, "", 0),
 	}
 
 	var err error
@@ -70,6 +100,17 @@ func NewRepoIndent(local LocalStore, prefix string, indent string, hashAlgorithm
 	return r, nil
 }
 
+func NewRepoWithOpts(local LocalStore, opts ...RepoOpts) (*Repo, error) {
+	r, err := NewRepo(local)
+	if err != nil {
+		return nil, err
+	}
+	for _, opt := range opts {
+		opt(r)
+	}
+	return r, nil
+}
+
 func (r *Repo) Init(consistentSnapshot bool) error {
 	t, err := r.topLevelTargets()
 	if err != nil {
@@ -91,7 +132,7 @@ func (r *Repo) Init(consistentSnapshot bool) error {
 		return err
 	}
 
-	fmt.Println("Repository initialized")
+	r.logger.Println("Repository initialized")
 	return nil
 }
 
@@ -533,7 +574,7 @@ func (r *Repo) RevokeKeyWithExpires(keyRole, id string, expires time.Time) error
 
 	err = r.setMeta("root.json", root)
 	if err == nil {
-		fmt.Println("Revoked", keyRole, "key with ID", id, "in root metadata")
+		r.logger.Println("Revoked", keyRole, "key with ID", id, "in root metadata")
 	}
 	return err
 }
@@ -783,7 +824,7 @@ func (r *Repo) Sign(roleFilename string) error {
 	r.meta[roleFilename] = b
 	err = r.local.SetMeta(roleFilename, b)
 	if err == nil {
-		fmt.Println("Signed", roleFilename, "with", numKeys, "key(s)")
+		r.logger.Println("Signed", roleFilename, "with", numKeys, "key(s)")
 	}
 	return err
 }
@@ -1223,7 +1264,7 @@ func (r *Repo) removeTargetsWithExpiresFromMeta(metaName string, paths []string,
 		for _, path := range paths {
 			path = util.NormalizeTarget(path)
 			if _, ok := t.Targets[path]; !ok {
-				fmt.Printf("[%v] The following target is not present: %v\n", metaName, path)
+				r.logger.Printf("[%v] The following target is not present: %v\n", metaName, path)
 				continue
 			}
 			removed = true
@@ -1243,17 +1284,17 @@ func (r *Repo) removeTargetsWithExpiresFromMeta(metaName string, paths []string,
 
 	err = r.setMeta(metaName, t)
 	if err == nil {
-		fmt.Printf("[%v] Removed targets:\n", metaName)
+		r.logger.Printf("[%v] Removed targets:\n", metaName)
 		for _, v := range removed_targets {
-			fmt.Println("*", v)
+			r.logger.Println("*", v)
 		}
 		if len(t.Targets) != 0 {
-			fmt.Printf("[%v] Added/staged targets:\n", metaName)
+			r.logger.Printf("[%v] Added/staged targets:\n", metaName)
 			for k := range t.Targets {
-				fmt.Println("*", k)
+				r.logger.Println("*", k)
 			}
 		} else {
-			fmt.Printf("[%v] There are no added/staged targets\n", metaName)
+			r.logger.Printf("[%v] There are no added/staged targets\n", metaName)
 		}
 	}
 	return err
@@ -1307,7 +1348,7 @@ func (r *Repo) SnapshotWithExpires(expires time.Time) error {
 	}
 	err = r.setMeta("snapshot.json", snapshot)
 	if err == nil {
-		fmt.Println("Staged snapshot.json metadata with expiration date:", snapshot.Expires)
+		r.logger.Println("Staged snapshot.json metadata with expiration date:", snapshot.Expires)
 	}
 	return err
 }
@@ -1339,7 +1380,7 @@ func (r *Repo) TimestampWithExpires(expires time.Time) error {
 
 	err = r.setMeta("timestamp.json", timestamp)
 	if err == nil {
-		fmt.Println("Staged timestamp.json metadata with expiration date:", timestamp.Expires)
+		r.logger.Println("Staged timestamp.json metadata with expiration date:", timestamp.Expires)
 	}
 	return err
 }
@@ -1505,7 +1546,7 @@ func (r *Repo) Commit() error {
 
 	err = r.local.Commit(root.ConsistentSnapshot, versions, hashes)
 	if err == nil {
-		fmt.Println("Committed successfully")
+		r.logger.Println("Committed successfully")
 	}
 	return err
 }
@@ -1513,7 +1554,7 @@ func (r *Repo) Commit() error {
 func (r *Repo) Clean() error {
 	err := r.local.Clean()
 	if err == nil {
-		fmt.Println("Removed all staged metadata and target files")
+		r.logger.Println("Removed all staged metadata and target files")
 	}
 	return err
 }


Test patch:
diff --git a/internal/fsutil/perm_test.go b/internal/fsutil/perm_test.go
index 6061d29..f80ef94 100644
--- a/internal/fsutil/perm_test.go
+++ b/internal/fsutil/perm_test.go
@@ -4,7 +4,6 @@
 package fsutil
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"testing"
@@ -59,7 +58,6 @@ func TestEnsureMaxPermissions(t *testing.T) {
 	assert.NoError(t, err)
 	err = EnsureMaxPermissions(fi, os.FileMode(0222))
 	assert.Error(t, err)
-	fmt.Println(err)
 
 	// Check matching due to more restrictive perms on file
 	err = os.Chmod(p, 0444)
diff --git a/repo_test.go b/repo_test.go
index 2f3aebb..d841bb5 100644
--- a/repo_test.go
+++ b/repo_test.go
@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"log"
 	"os"
 	"path"
 	"path/filepath"
@@ -1422,7 +1423,12 @@ func (rs *RepoSuite) TestKeyPersistence(c *C) {
 	// Test changing the passphrase
 	// 1. Create a secure store with a passphrase (create new object and temp folder so we discard any previous state)
 	tmp = newTmpDir(c)
-	store = FileSystemStore(tmp.path, testPassphraseFunc)
+	var logBytes bytes.Buffer
+	storeOpts := StoreOpts{
+		Logger:   log.New(&logBytes, "", 0),
+		PassFunc: testPassphraseFunc,
+	}
+	store = FileSystemStoreWithOpts(tmp.path, storeOpts)
 
 	// 1.5. Changing passphrase works for top-level and delegated roles.
 	r, err := NewRepo(store)
@@ -1433,6 +1439,7 @@ func (rs *RepoSuite) TestKeyPersistence(c *C) {
 
 	// 2. Test changing the passphrase when the keys file does not exist - should FAIL
 	c.Assert(store.(PassphraseChanger).ChangePassphrase("root"), NotNil)
+	c.Assert(strings.Contains(logBytes.String(), "Missing keys file"), Equals, true)
 
 	// 3. Generate a new key
 	signer, err = keys.GenerateEd25519Key()