fix: sign-payload shouldn't recanonicalize payload (#479)

Fixes #475.

- 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`.

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

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
 }

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 {

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)

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
 }