Repository: theupdateframework/go-tuf (go)

Issue:
Payload functionality broken ( invalid character '\n' in string literal)
This is tested with commit 7cddf58, built with go version go1.20.2 darwin/arm64

Steps to reproduce:

```
% tuf init                                                       
Repository initialized
% tuf gen-key --scheme=ecdsa-sha2-nistp256 root     
Enter root keys passphrase: 
Repeat root keys passphrase: 
Generated root ecdsa-sha2-nistp256 key with ID 3919d40168ff00a08fe585a20e9ebeeafbde74c30da09a71e70fb9cb6c59281f
% tuf payload root.json > root.json.payload 
 % tuf sign-payload --role=root root.json.payload > root.json.sigs
ERROR: json: error calling MarshalJSON for type json.RawMessage: invalid character '\n' in string literal
```


Pull request description:
fix: sign-payload shouldn't recanonicalize payload


- Add new `Repo.SignRaw` which doesn't canonicalize before signing (and just returns Signatures)
- Rename `SignPayload` to `CanonicalizeAndSign` (old name is deprecated; it probably doesn't actually get used so we can rip out next major release.)
- Add `sign.MakeSignatures` which does not canonicalize; refactor `sign.Sign` to use it.
- Modify offline flow to test this property.
- Use `SignRaw` in `tuf sign-payload`.

Please fill in the fields below to submit a pull request.  The more information that is provided, the better.

Fixes #475.

Release Notes:
- (API) `Repo.SignPayload` is deprecated; please use `CanonicalizeAndSign` instead.
- (CLI) Fix `tuf sign-payload` to not re-canonicalize the input.

**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). **Please ensure that your PR title** is a [Conventional Commit](https://www.conventionalcommits.org/en/v1.0.0/) breaking change (with a `!`, as in `feat!: change foo`). 

**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
- [x] Docs have been added for the bug fix or new feature


Patch:
diff --git a/cmd/tuf/sign_payload.go b/cmd/tuf/sign_payload.go
index 6772972..1905fd2 100644
--- a/cmd/tuf/sign_payload.go
+++ b/cmd/tuf/sign_payload.go
@@ -7,7 +7,6 @@ import (
 
 	"github.com/flynn/go-docopt"
 	tuf "github.com/theupdateframework/go-tuf"
-	tufdata "github.com/theupdateframework/go-tuf/data"
 )
 
 func init() {
@@ -25,19 +24,18 @@ func cmdSignPayload(args *docopt.Args, repo *tuf.Repo) error {
 	if err != nil {
 		return err
 	}
-	signed := tufdata.Signed{Signed: payload, Signatures: make([]tufdata.Signature, 0)}
 
-	numKeys, err := repo.SignPayload(args.String["--role"], &signed)
+	signatures, err := repo.SignRaw(args.String["--role"], payload)
 	if err != nil {
 		return err
 	}
+	fmt.Fprintln(os.Stderr, "tuf: signed")
 
-	bytes, err := json.Marshal(signed.Signatures)
+	bytes, err := json.Marshal(signatures)
 	if err != nil {
 		return err
 	}
 	fmt.Fprint(os.Stdout, string(bytes))
 
-	fmt.Fprintln(os.Stderr, "tuf: signed with", numKeys, "key(s)")
 	return nil
 }
diff --git a/repo.go b/repo.go
index c6a23de..db2ac66 100644
--- a/repo.go
+++ b/repo.go
@@ -782,11 +782,13 @@ func (r *Repo) setMeta(roleFilename string, meta interface{}) error {
 	return r.local.SetMeta(roleFilename, b)
 }
 
-// SignPayload signs the given payload using the key(s) associated with role.
+// CanonicalizeAndSign canonicalizes the signed portion of signed, then signs it using the key(s) associated with role.
+//
+// It appends the signature to signed.
 //
 // It returns the total number of keys used for signing, 0 (along with
 // ErrNoKeys) if no keys were found, or -1 (along with an error) in error cases.
-func (r *Repo) SignPayload(role string, payload *data.Signed) (int, error) {
+func (r *Repo) CanonicalizeAndSign(role string, signed *data.Signed) (int, error) {
 	keys, err := r.signersForRole(role)
 	if err != nil {
 		return -1, err
@@ -795,13 +797,46 @@ func (r *Repo) SignPayload(role string, payload *data.Signed) (int, error) {
 		return 0, ErrNoKeys{role}
 	}
 	for _, k := range keys {
-		if err = sign.Sign(payload, k); err != nil {
+		if err = sign.Sign(signed, k); err != nil {
 			return -1, err
 		}
 	}
 	return len(keys), nil
 }
 
+// SignPayload canonicalizes the signed portion of payload, then signs it using the key(s) associated with role.
+//
+// It returns the total number of keys used for signing, 0 (along with
+// ErrNoKeys) if no keys were found, or -1 (along with an error) in error cases.
+//
+// DEPRECATED: please use CanonicalizeAndSign instead.
+func (r *Repo) SignPayload(role string, payload *data.Signed) (int, error) {
+	return r.CanonicalizeAndSign(role, payload)
+}
+
+// SignRaw signs the given (pre-canonicalized) payload using the key(s) associated with role.
+//
+// It returns the new data.Signatures.
+func (r *Repo) SignRaw(role string, payload []byte) ([]data.Signature, error) {
+	keys, err := r.signersForRole(role)
+	if err != nil {
+		return nil, err
+	}
+	if len(keys) == 0 {
+		return nil, ErrNoKeys{role}
+	}
+
+	allSigs := make([]data.Signature, 0, len(keys))
+	for _, k := range keys {
+		sigs, err := sign.MakeSignatures(payload, k)
+		if err != nil {
+			return nil, err
+		}
+		allSigs = append(allSigs, sigs...)
+	}
+	return allSigs, nil
+}
+
 func (r *Repo) Sign(roleFilename string) error {
 	signed, err := r.SignedMeta(roleFilename)
 	if err != nil {
diff --git a/sign/sign.go b/sign/sign.go
index 6b15b6b..e31b546 100644
--- a/sign/sign.go
+++ b/sign/sign.go
@@ -2,46 +2,65 @@ package sign
 
 import (
 	"encoding/json"
+	"errors"
 
 	"github.com/secure-systems-lab/go-securesystemslib/cjson"
 	"github.com/theupdateframework/go-tuf/data"
 	"github.com/theupdateframework/go-tuf/pkg/keys"
 )
 
-func Sign(s *data.Signed, k keys.Signer) error {
+const maxSignatures = 1024
+
+// MakeSignatures creates data.Signatures for canonical using signer k.
+//
+// There will be one data.Signature for each of k's IDs, each wih the same
+// signature data.
+func MakeSignatures(canonical []byte, k keys.Signer) ([]data.Signature, error) {
+	sigData, err := k.SignMessage(canonical)
+	if err != nil {
+		return nil, err
+	}
+
 	ids := k.PublicData().IDs()
-	signatures := make([]data.Signature, 0, len(s.Signatures)+1)
-	for _, sig := range s.Signatures {
-		found := false
-		for _, id := range ids {
-			if sig.KeyID == id {
-				found = true
-				break
-			}
-		}
-		if !found {
-			signatures = append(signatures, sig)
-		}
+	signatures := make([]data.Signature, 0, len(ids))
+	for _, id := range ids {
+		signatures = append(signatures, data.Signature{
+			KeyID:     id,
+			Signature: sigData,
+		})
 	}
 
+	return signatures, nil
+}
+
+// Sign signs the to-be-signed part of s using the signer k.
+//
+// The new signature(s) (one for each of k's key IDs) are appended to
+// s.Signatures. Existing signatures for the Key IDs are replaced.
+func Sign(s *data.Signed, k keys.Signer) error {
 	canonical, err := cjson.EncodeCanonical(s.Signed)
 	if err != nil {
 		return err
 	}
 
-	sig, err := k.SignMessage(canonical)
+	size := len(s.Signatures)
+	if size > maxSignatures-1 {
+		return errors.New("value too large")
+	}
+	signatures := make([]data.Signature, 0, size+1)
+	for _, oldSig := range s.Signatures {
+		if !k.PublicData().ContainsID(oldSig.KeyID) {
+			signatures = append(signatures, oldSig)
+		}
+	}
+
+	newSigs, err := MakeSignatures(canonical, k)
 	if err != nil {
 		return err
 	}
+	signatures = append(signatures, newSigs...)
 
 	s.Signatures = signatures
-	for _, id := range ids {
-		s.Signatures = append(s.Signatures, data.Signature{
-			KeyID:     id,
-			Signature: sig,
-		})
-	}
-
 	return nil
 }
 


Test patch:
diff --git a/repo_test.go b/repo_test.go
index ad69e66..4d7d73f 100644
--- a/repo_test.go
+++ b/repo_test.go
@@ -2601,7 +2601,8 @@ func (rs *RepoSuite) TestOfflineFlow(c *C) {
 	r, err := NewRepo(local)
 	c.Assert(err, IsNil)
 	c.Assert(r.Init(false), IsNil)
-	_, err = r.GenKey("root")
+	// Use ECDSA because it has a newline which is a difference between JSON and cJSON.
+	_, err = r.GenKeyWithSchemeAndExpires("root", data.DefaultExpires("root"), data.KeySchemeECDSA_SHA2_P256)
 	c.Assert(err, IsNil)
 
 	// Get the payload to sign
@@ -2621,15 +2622,14 @@ func (rs *RepoSuite) TestOfflineFlow(c *C) {
 	}
 
 	// Sign the payload
-	signed := data.Signed{Signed: payload}
-	_, err = r.SignPayload("targets", &signed)
+	_, err = r.SignRaw("targets", payload)
 	c.Assert(err, Equals, ErrNoKeys{"targets"})
-	numKeys, err := r.SignPayload("root", &signed)
+	signatures, err := r.SignRaw("root", payload)
 	c.Assert(err, IsNil)
-	c.Assert(numKeys, Equals, 1)
+	c.Assert(len(signatures), Equals, 1)
 
 	// Add the payload signatures back
-	for _, sig := range signed.Signatures {
+	for _, sig := range signatures {
 		// This method checks that the signature verifies!
 		err = r.AddOrUpdateSignature("root.json", sig)
 		c.Assert(err, IsNil)