Signer: fix usage of crypto.Signer interface

wlynch · wlynch · commit 255dc05646ad · 2024-02-13T15:51:28.000-05:00
crypto.Signer was incorrectly used before. Signer documentation says that Signer.Sign should be used on digests, whereas we were using this on message bodies. To fix this, create our own Signer interface (+ signableObject borrowed from #705) that describes more accurately what we want. As before, the expectation is that signer implementations only need to worry about acting on encoded message bodies rather than needing to encode objects themselves. This is technically a breaking change from the previous Signer implementation, but since this is new and hasn't made it into cut release yet, this seems like an acceptible change. Also adds example test showing how signers can be made (uses base64 for consistent outputs).
diff --git a/options.go b/options.go
@@ -1,7 +1,6 @@
 package git
 
 import (
-	"crypto"
 	"errors"
 	"fmt"
 	"regexp"
@@ -516,7 +515,7 @@ type CommitOptions struct {
 	// Signer denotes a cryptographic signer to sign the commit with.
 	// A nil value here means the commit will not be signed.
 	// Takes precedence over SignKey.
-	Signer crypto.Signer
+	Signer Signer
 	// Amend will create a new commit object and replace the commit that HEAD currently
 	// points to. Cannot be used with All nor Parents.
 	Amend bool
diff --git a/signer.go b/signer.go
@@ -0,0 +1,33 @@
+package git
+
+import (
+	"io"
+
+	"github.com/go-git/go-git/v5/plumbing"
+)
+
+// signableObject is an object which can be signed.
+type signableObject interface {
+	EncodeWithoutSignature(o plumbing.EncodedObject) error
+}
+
+// Signer is an interface for signing git objects.
+// message is a reader containing the encoded object to be signed.
+// Implementors should return the encoded signature and an error if any.
+// See https://git-scm.com/docs/gitformat-signature for more information.
+type Signer interface {
+	Sign(message io.Reader) ([]byte, error)
+}
+
+func signObject(signer Signer, obj signableObject) ([]byte, error) {
+	encoded := &plumbing.MemoryObject{}
+	if err := obj.EncodeWithoutSignature(encoded); err != nil {
+		return nil, err
+	}
+	r, err := encoded.Reader()
+	if err != nil {
+		return nil, err
+	}
+
+	return signer.Sign(r)
+}
diff --git a/signer_test.go b/signer_test.go
@@ -0,0 +1,56 @@
+package git
+
+import (
+	"encoding/base64"
+	"fmt"
+	"io"
+	"time"
+
+	"github.com/go-git/go-billy/v5/memfs"
+	"github.com/go-git/go-git/v5/plumbing/object"
+	"github.com/go-git/go-git/v5/storage/memory"
+)
+
+type b64signer struct{}
+
+// This is not secure, and is only used as an example for testing purposes.
+// Please don't do this.
+func (b64signer) Sign(message io.Reader) ([]byte, error) {
+	b, err := io.ReadAll(message)
+	if err != nil {
+		return nil, err
+	}
+	out := make([]byte, base64.StdEncoding.EncodedLen(len(b)))
+	base64.StdEncoding.Encode(out, b)
+	return out, nil
+}
+
+func ExampleSigner() {
+	repo, err := Init(memory.NewStorage(), memfs.New())
+	if err != nil {
+		panic(err)
+	}
+	w, err := repo.Worktree()
+	if err != nil {
+		panic(err)
+	}
+	commit, err := w.Commit("example commit", &CommitOptions{
+		Author: &object.Signature{
+			Name:  "John Doe",
+			Email: "john@example.com",
+			When:  time.UnixMicro(1234567890).UTC(),
+		},
+		Signer:            b64signer{},
+		AllowEmptyCommits: true,
+	})
+	if err != nil {
+		panic(err)
+	}
+
+	obj, err := repo.CommitObject(commit)
+	if err != nil {
+		panic(err)
+	}
+	fmt.Println(obj.PGPSignature)
+	// Output: dHJlZSA0YjgyNWRjNjQyY2I2ZWI5YTA2MGU1NGJmOGQ2OTI4OGZiZWU0OTA0CmF1dGhvciBKb2huIERvZSA8am9obkBleGFtcGxlLmNvbT4gMTIzNCArMDAwMApjb21taXR0ZXIgSm9obiBEb2UgPGpvaG5AZXhhbXBsZS5jb20+IDEyMzQgKzAwMDAKCmV4YW1wbGUgY29tbWl0
+}
diff --git a/worktree_commit.go b/worktree_commit.go
@@ -2,8 +2,6 @@ package git
 
 import (
 	"bytes"
-	"crypto"
-	"crypto/rand"
 	"errors"
 	"io"
 	"path"
@@ -135,7 +133,7 @@ func (w *Worktree) buildCommitObject(msg string, opts *CommitOptions, tree plumb
 		signer = &gpgSigner{key: opts.SignKey}
 	}
 	if signer != nil {
-		sig, err := w.buildCommitSignature(commit, signer)
+		sig, err := signObject(signer, commit)
 		if err != nil {
 			return plumbing.ZeroHash, err
 		}
@@ -151,44 +149,17 @@ func (w *Worktree) buildCommitObject(msg string, opts *CommitOptions, tree plumb
 
 type gpgSigner struct {
 	key *openpgp.Entity
+	cfg *packet.Config
 }
 
-func (s *gpgSigner) Public() crypto.PublicKey {
-	return s.key.PrimaryKey
-}
-
-func (s *gpgSigner) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) {
-	var cfg *packet.Config
-	if opts != nil {
-		cfg = &packet.Config{
-			DefaultHash: opts.HashFunc(),
-		}
-	}
-
+func (s *gpgSigner) Sign(message io.Reader) ([]byte, error) {
 	var b bytes.Buffer
-	if err := openpgp.ArmoredDetachSign(&b, s.key, bytes.NewReader(digest), cfg); err != nil {
+	if err := openpgp.ArmoredDetachSign(&b, s.key, message, s.cfg); err != nil {
 		return nil, err
 	}
 	return b.Bytes(), nil
 }
 
-func (w *Worktree) buildCommitSignature(commit *object.Commit, signer crypto.Signer) ([]byte, error) {
-	encoded := &plumbing.MemoryObject{}
-	if err := commit.Encode(encoded); err != nil {
-		return nil, err
-	}
-	r, err := encoded.Reader()
-	if err != nil {
-		return nil, err
-	}
-	b, err := io.ReadAll(r)
-	if err != nil {
-		return nil, err
-	}
-
-	return signer.Sign(rand.Reader, b, nil)
-}
-
 // buildTreeHelper converts a given index.Index file into multiple git objects
 // reading the blobs from the given filesystem and creating the trees from the
 // index structure. The created objects are pushed to a given Storer.
