Merge branch 'security-signature-validation-master' into 'master'

commit: Fix bug in commit signature parsing

See merge request https://gitlab.com/gitlab-org/security/gitaly/-/merge_requests/92

Merged-by: Jenny Kim <yjeankim@gitlab.com>
Approved-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Karthik Nayak <knayak@gitlab.com>

22 files changed, 828 insertions, 367 deletions

diff --git a/internal/git/catfile/commit.go b/internal/git/catfile/commit.go
index 0af44fee8..94277bb10 100644
--- a/internal/git/catfile/commit.go
+++ b/internal/git/catfile/commit.go
@@ -14,7 +14,7 @@ import (
 )
 
 // GetCommit looks up a commit by revision using an existing Batch instance.
-func GetCommit(ctx context.Context, objectReader ObjectContentReader, revision git.Revision) (*gitalypb.GitCommit, error) {
+func GetCommit(ctx context.Context, objectReader ObjectContentReader, revision git.Revision) (*Commit, error) {
 	object, err := objectReader.Object(ctx, revision+"^{commit}")
 	if err != nil {
 		return nil, err
@@ -63,7 +63,7 @@ func GetCommitWithTrailers(
 		}
 	}
 
-	return commit, nil
+	return commit.GitCommit, nil
 }
 
 // GetCommitMessage looks up a commit message and returns it in its entirety.
diff --git a/internal/git/catfile/commit_test.go b/internal/git/catfile/commit_test.go
index 55d6012d9..d4a51457e 100644
--- a/internal/git/catfile/commit_test.go
+++ b/internal/git/catfile/commit_test.go
@@ -55,8 +55,11 @@ func TestGetCommit(t *testing.T) {
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			commit, err := GetCommit(ctx, objectReader, git.Revision(tc.revision))
-			require.Equal(t, tc.expectedErr, err)
-			testhelper.ProtoEqual(t, tc.expectedCommit, commit)
+			if tc.expectedErr != nil || err != nil {
+				require.Equal(t, tc.expectedErr, err)
+			} else {
+				testhelper.ProtoEqual(t, tc.expectedCommit, commit.GitCommit)
+			}
 		})
 	}
 }
diff --git a/internal/git/catfile/parse_commit.go b/internal/git/catfile/parse_commit.go
new file mode 100644
index 000000000..1b22d9d4a
--- /dev/null
+++ b/internal/git/catfile/parse_commit.go
@@ -0,0 +1,252 @@
+package catfile
+
+import (
+	"bytes"
+	"errors"
+	"fmt"
+	"io"
+	"strings"
+
+	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
+)
+
+const (
+	gpgSignaturePrefix       = "gpgsig"
+	gpgSignaturePrefixSha256 = "gpgsig-sha256"
+)
+
+// parseCommitState is a type used to define the current state while parsing
+// a commit message.
+//
+// To understand the state macihne of parsing commit messages, we need
+// to understand the different sections of a commit message.
+//
+// # Sections of a commit message
+//
+// Let's consider a sample commit message
+//
+//	tree 798e5474fafac9754ee6b82ab17af8d70df4fbd3
+//	parent 86f06b3f55e6334abb99fc168e2dd925895c4e49
+//	author John doe <bugfixer@email.com> 1699964265 +0100
+//	committer John doe <bugfixer@email.com> 1699964265 +0100
+//	gpgsig -----BEGIN PGP SIGNATURE-----
+//
+//	 iHUEABYKAB0WIQReCOKeBZren2AFN0T+9BKLUsDX/wUCZVNlaQAKCRD+9BKLUsDX
+//	 /219AP9j8jfQuLieg0Fl8xrOS74eJguYqIsPYI6lPDUvM5XmgQEAkhDUoWFd0ypR
+//	 vXTEU/0CxcaXmlco/ThX2rCYwEUT6wA=
+//	 =Wt+j
+//	 -----END PGP SIGNATURE-----
+//
+//	Commit subject
+//
+// With this, we can see that commit messages have
+//   - The first section consiting of headers. This is everything before
+//     commit message. In our example this consists of the tree, parent,
+//     author, committer and gpgsig.
+//   - The headers can also contain the signature. This is either gpgsig
+//     or gpgsig-256.
+//   - After the first line, all lines of the signature start with a ' '
+//     space character.
+//   - Any headers post the signature are not parsed but will still be
+//     considered as part of the signature payload.
+//   - Post the headers, there is a newline to differentiate the upcoming
+//     commit body. The commit body consists of the commit subject and the
+//     message.
+//
+// Using this information we can now write a parser which represents a state
+// machine which can be used to parse commits.
+type parseCommitState uint
+
+const (
+	parseCommitStateHeader parseCommitState = iota
+	parseCommitStateSignature
+	parseCommitStateUnexpected
+	parseCommitStateBody
+	parseCommitStateEnd
+)
+
+// SignatureData holds the raw data used to validate a signed commit.
+type SignatureData struct {
+	// Signatures refers to the signatures present in the commit. Note that
+	// Git only considers the first signature when parsing commits
+	Signatures [][]byte
+	// Payload refers to the commit data which is signed by the signature,
+	// generally this is everything apart from the signature in the commit.
+	// Headers present after the signature are not considered in the payload.
+	Payload []byte
+}
+
+// Commit wraps the gitalypb.GitCommit structure and includes signature information.
+type Commit struct {
+	*gitalypb.GitCommit
+	SignatureData SignatureData
+}
+
+// ParseCommit implements a state machine to parse the various sections
+// of a commit. To understand the state machine, see the definition
+// for parseState above.
+//
+// The goal is to maintain feature parity with how git [1] (see
+// parse_buffer_signed_by_header()) itself parses commits. This ensures
+// that we throw errors only wherever git does.
+//
+// [1]: https://gitlab.com/gitlab-org/git/-/blob/master/commit.c
+func (p *parser) ParseCommit(object git.Object) (*Commit, error) {
+	commit := &gitalypb.GitCommit{Id: object.ObjectID().String()}
+	var payload []byte
+	currentSignatureIndex := 0
+	signatures := [][]byte{}
+
+	bytesRemaining := object.ObjectSize()
+	p.bufferedReader.Reset(object)
+
+	for state := parseCommitStateHeader; state != parseCommitStateEnd; {
+		receivedEOF := false
+
+		line, err := p.bufferedReader.ReadString('\n')
+		if errors.Is(err, io.EOF) {
+			receivedEOF = true
+		} else if err != nil {
+			return nil, fmt.Errorf("parse raw commit: %w", err)
+		}
+		bytesRemaining -= int64(len(line))
+
+		// If the line only consists of a newline, we can skip
+		// the state to commit body.
+		if line == "\n" {
+			state = parseCommitStateBody
+		}
+
+		switch state {
+		case parseCommitStateHeader:
+			key, value, ok := strings.Cut(line, " ")
+			if !ok {
+				// TODO: Current tests allow empty commits, we might want
+				// to change this behavior.
+				goto loopEnd
+			}
+
+			// For headers, we trim the newline to make it easier
+			// to parse.
+			value = strings.TrimSuffix(value, "\n")
+
+			switch key {
+			case "parent":
+				commit.ParentIds = append(commit.ParentIds, value)
+			case "author":
+				commit.Author = parseCommitAuthor(value)
+			case "committer":
+				commit.Committer = parseCommitAuthor(value)
+			case "tree":
+				commit.TreeId = value
+			case "encoding":
+				commit.Encoding = value
+			case gpgSignaturePrefix, gpgSignaturePrefixSha256:
+				// Since Git only considers the first signature, we only
+				// capture the first signature's type.
+				commit.SignatureType = detectSignatureType(value)
+
+				state = parseCommitStateSignature
+				signatures = append(signatures, []byte(value+"\n"))
+
+				goto loopEnd
+			}
+
+			payload = append(payload, []byte(line)...)
+
+		case parseCommitStateSignature:
+			if after, ok := strings.CutPrefix(line, " "); ok {
+				// All signature lines, must start with a ' ' (space).
+				signatures[currentSignatureIndex] = append(signatures[currentSignatureIndex], []byte(after)...)
+				goto loopEnd
+			} else {
+				currentSignatureIndex++
+
+				// Multiple signatures might be present in the commit.
+				if key, value, ok := strings.Cut(line, " "); ok {
+					if key == gpgSignaturePrefix || key == gpgSignaturePrefixSha256 {
+						signatures = append(signatures, []byte(value))
+						goto loopEnd
+					}
+				}
+
+				// If there is no ' ' (space), it means there is some unexpected
+				// data.
+				//
+				// Note that we don't go back to parsing headers. This is because
+				// any headers which are present after the signature are not parsed
+				// by Git as information. But, they still constitute to the signature
+				// payload. So any data after the signature and before the commit body
+				// is considered unexpected.
+				state = parseCommitStateUnexpected
+			}
+
+			fallthrough
+
+		case parseCommitStateUnexpected:
+			// If the line is only a newline, that means we have reached
+			// the commit body. If not, we keep looping till we do.
+			if line != "\n" {
+				payload = append(payload, []byte(line)...)
+				goto loopEnd
+			}
+
+			fallthrough
+
+		case parseCommitStateBody:
+			payload = append(payload, []byte(line)...)
+
+			body := make([]byte, bytesRemaining)
+			if _, err := io.ReadFull(p.bufferedReader, body); err != nil {
+				return nil, fmt.Errorf("reading commit message: %w", err)
+			}
+
+			// After we have copied the body, we must make sure that there really is no
+			// additional data. For once, this is to detect bugs in our implementation where we
+			// would accidentally have truncated the commit message. On the other hand, we also
+			// need to do this such that we observe the EOF, which we must observe in order to
+			// unblock reading the next object.
+			//
+			// This all feels a bit complicated, where it would be much easier to just read into
+			// a preallocated `bytes.Buffer`. But this complexity is indeed required to optimize
+			// allocations. So if you want to change this, please make sure to execute the
+			// `BenchmarkListAllCommits` benchmark.
+			if n, err := io.Copy(io.Discard, p.bufferedReader); err != nil {
+				return nil, fmt.Errorf("reading commit message: %w", err)
+			} else if n != 0 {
+				return nil, fmt.Errorf(
+					"commit message exceeds expected length %v by %v bytes",
+					object.ObjectSize(), n,
+				)
+			}
+
+			if len(body) > 0 {
+				commit.Subject = subjectFromBody(body)
+				commit.BodySize = int64(len(body))
+				commit.Body = body
+				if max := helper.MaxCommitOrTagMessageSize; len(body) > max {
+					commit.Body = commit.Body[:max]
+				}
+				payload = append(payload, body...)
+			}
+
+			state = parseCommitStateEnd
+		}
+
+	loopEnd:
+		if receivedEOF {
+			state = parseCommitStateEnd
+		}
+	}
+
+	for i, signature := range signatures {
+		signatures[i] = bytes.TrimSuffix(signature, []byte("\n"))
+	}
+
+	return &Commit{
+		GitCommit:     commit,
+		SignatureData: SignatureData{Signatures: signatures, Payload: payload},
+	}, nil
+}
diff --git a/internal/git/catfile/parse_commit_test.go b/internal/git/catfile/parse_commit_test.go
new file mode 100644
index 000000000..2e80dc38f
--- /dev/null
+++ b/internal/git/catfile/parse_commit_test.go
@@ -0,0 +1,531 @@
+package catfile
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
+	"google.golang.org/protobuf/types/known/timestamppb"
+)
+
+const (
+	pgpSignature = `-----BEGIN PGP SIGNATURE-----
+Version: ObjectivePGP
+Comment: https://www.objectivepgp.com
+Charset: UTF-8
+
+wsFcBAABCgAGBQJecon1AAoJEDYMjTn1G2THmSsP/At/jskLdF0i7p0nKf4JLjeeqRJ4k2IUg87U
+ZwV6mbLo5XFm8Sq7CJBAGAhlOZE4BAwKALuawmgs5XMEZwK2z6AIgosGTVpmxDTTI11bXt4XIOdz
+qF7c/gUrJOZzjFXOqDsd5UuPRupwznC5eKlLbfImR+NYxKryo8JGdF5t52ph4kChcQsKlSkXuYNI
++9UgbaMclEjb0OLm+mcP9QxW+Cs9JS2Jb4Jh6XONWW1nDN3ZTDDskguIqqF47UxIgSImrmpMcEj9
+YSNU0oMoHM4+1DoXp1t99EGPoAMvO+a5g8gd1jouCIrI6KOX+GeG/TFFM0mQwg/d/N9LR049m8ed
+vgqg/lMiWUxQGL2IPpYPcgiUEqfn7ete+NMzQV5zstxF/q7Yj2BhM2L7FPHxKaoy/w5Q/DcAO4wN
+5gxVmIvbCDk5JOx8I+boIS8ZxSvIlJ5IWaPrcjg5Mc40it+WHvMqxVnCzH0c6KcXaJ2SibVb59HR
+pdRhEXXw/hRN65l/xwyM8sklQalAGu755gNJZ4k9ApBVUssZyiu+te2+bDirAcmK8/x1jvMQY6bn
+DFxBE7bMHDp24IFPaVID84Ryt3vSSBEkrUGm7OkyDESTpHCr4sfD5o3LCUCIibTqv/CAhe59mhbB
+2AXL7X+EzylKy6C1N5KUUiMTW94AuF6f8FqBoxnf
+=U6zM
+-----END PGP SIGNATURE-----`
+
+	sshSignature = `-----BEGIN SSH SIGNATURE-----
+U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgtc+Qk8jhMwVZk/jFEFCM16LNQb
+30q5kK30bbetfjyTMAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5
+AAAAQADE1oOMKxqQu86XUQbhCoWx8GnnYHQ/i3mHdA0zPycIlDv8N6BRVDS6b0ja2Avj+s
+uNvjRqSEGQJ4q6vhKOnQw=
+-----END SSH SIGNATURE-----`
+)
+
+func TestParseCommits(t *testing.T) {
+	t.Parallel()
+
+	type setupData struct {
+		content        string
+		oid            git.ObjectID
+		expectedCommit *Commit
+		expectedErr    error
+	}
+
+	// Valid-but-interesting commits should be test at the FindCommit level.
+	// Invalid objects (that Git would complain about during fsck) can be
+	// tested here.
+	//
+	// Once a repository contains a pathological object it can be hard to get
+	// rid of it. Because of this I think it's nicer to ignore such objects
+	// than to throw hard errors.
+	for _, tc := range []struct {
+		desc  string
+		setup func(t *testing.T) setupData
+	}{
+		{
+			desc: "empty commit object",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit:     &gitalypb.GitCommit{Id: gittest.DefaultObjectHash.EmptyTreeOID.String()},
+						SignatureData: SignatureData{Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "no email",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "author Jane Doe",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id:     gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe")},
+						},
+						SignatureData: SignatureData{Payload: []byte("author Jane Doe"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "normal author",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "author Au Thor <au.thor@example.com> 1625121079 +0000",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Au Thor"),
+								Email:    []byte("au.thor@example.com"),
+								Date:     timestamppb.New(time.Unix(1625121079, 0)),
+								Timezone: []byte("+0000"),
+							},
+						},
+						SignatureData: SignatureData{Payload: []byte("author Au Thor <au.thor@example.com> 1625121079 +0000"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "author with missing mail",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "author Au Thor <> 1625121079 +0000",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Au Thor"),
+								Date:     timestamppb.New(time.Unix(1625121079, 0)),
+								Timezone: []byte("+0000"),
+							},
+						},
+						SignatureData: SignatureData{Payload: []byte("author Au Thor <> 1625121079 +0000"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "author with missing date",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "author Au Thor <au.thor@example.com>",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:  []byte("Au Thor"),
+								Email: []byte("au.thor@example.com"),
+							},
+						},
+						SignatureData: SignatureData{Payload: []byte("author Au Thor <au.thor@example.com>"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "unmatched <",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "author Jane Doe <janedoe@example.com",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id:     gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe")},
+						},
+						SignatureData: SignatureData{Payload: []byte("author Jane Doe <janedoe@example.com"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "unmatched >",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "author Jane Doe janedoe@example.com>",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id:     gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe janedoe@example.com>")},
+						},
+						SignatureData: SignatureData{Payload: []byte("author Jane Doe janedoe@example.com>"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "date too high",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "author Jane Doe <janedoe@example.com> 9007199254740993 +0200",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Jane Doe"),
+								Email:    []byte("janedoe@example.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 9223371974719179007},
+								Timezone: []byte("+0200"),
+							},
+						},
+						SignatureData: SignatureData{Payload: []byte("author Jane Doe <janedoe@example.com> 9007199254740993 +0200"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "date negative",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "author Jane Doe <janedoe@example.com> -1 +0200",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Jane Doe"),
+								Email:    []byte("janedoe@example.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 9223371974719179007},
+								Timezone: []byte("+0200"),
+							},
+						},
+						SignatureData: SignatureData{Payload: []byte("author Jane Doe <janedoe@example.com> -1 +0200"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "huge",
+			setup: func(_ *testing.T) setupData {
+				repeat := strings.Repeat("A", 100000)
+				content := "author " + repeat
+
+				return setupData{
+					content: content,
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name: []byte(repeat),
+							},
+						},
+						SignatureData: SignatureData{Payload: []byte(content), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "has encoding",
+			setup: func(_ *testing.T) setupData {
+				return setupData{
+					content: "encoding Windows-1251",
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id:       gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Encoding: "Windows-1251",
+						},
+						SignatureData: SignatureData{Payload: []byte("encoding Windows-1251"), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "PGP signed commit",
+			setup: func(_ *testing.T) setupData {
+				commitData, signedCommitData := createSignedCommitData(gpgSignaturePrefix, pgpSignature, "random commit message")
+
+				return setupData{
+					content: signedCommitData,
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							Committer: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							SignatureType: gitalypb.SignatureType_PGP,
+							TreeId:        gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Subject:       []byte("random commit message"),
+							Body:          []byte("random commit message\n"),
+							BodySize:      22,
+						},
+						SignatureData: SignatureData{Payload: []byte(commitData), Signatures: [][]byte{[]byte(pgpSignature)}},
+					},
+				}
+			},
+		},
+		{
+			desc: "PGP SHA256-signed signed commit",
+			setup: func(_ *testing.T) setupData {
+				commitData, signedCommitData := createSignedCommitData(gpgSignaturePrefixSha256, pgpSignature, "random commit message")
+
+				return setupData{
+					content: signedCommitData,
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							Committer: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							SignatureType: gitalypb.SignatureType_PGP,
+							TreeId:        gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Subject:       []byte("random commit message"),
+							Body:          []byte("random commit message\n"),
+							BodySize:      22,
+						},
+						SignatureData: SignatureData{Payload: []byte(commitData), Signatures: [][]byte{[]byte(pgpSignature)}},
+					},
+				}
+			},
+		},
+		{
+			desc: "SSH signed commit",
+			setup: func(_ *testing.T) setupData {
+				commitData, signedCommitData := createSignedCommitData(gpgSignaturePrefix, sshSignature, "random commit message")
+
+				return setupData{
+					content: signedCommitData,
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							Committer: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							SignatureType: gitalypb.SignatureType_SSH,
+							TreeId:        gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Subject:       []byte("random commit message"),
+							Body:          []byte("random commit message\n"),
+							BodySize:      22,
+						},
+						SignatureData: SignatureData{Payload: []byte(commitData), Signatures: [][]byte{[]byte(sshSignature)}},
+					},
+				}
+			},
+		},
+		{
+			desc: "garbage signed commit",
+			setup: func(_ *testing.T) setupData {
+				_, signedCommitData := createSignedCommitData("gpgsig-garbage", sshSignature, "garbage-signed commit message")
+
+				return setupData{
+					content: signedCommitData,
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							Committer: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							TreeId:   gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Subject:  []byte("garbage-signed commit message"),
+							Body:     []byte("garbage-signed commit message\n"),
+							BodySize: 30,
+						},
+						SignatureData: SignatureData{Payload: []byte(signedCommitData), Signatures: [][]byte{}},
+					},
+				}
+			},
+		},
+		{
+			desc: "commits with multiple signatures",
+			setup: func(_ *testing.T) setupData {
+				commitData, signedCommitData := createSignedCommitData("gpgsig", pgpSignature, "commit message")
+
+				signatureLines := strings.Split(sshSignature, "\n")
+				for i, signatureLine := range signatureLines {
+					signatureLines[i] = " " + signatureLine
+				}
+
+				signedCommitData = strings.Replace(signedCommitData, "\n\n", fmt.Sprintf("\n%s%s\n\n", "gpgsig-sha256", strings.Join(signatureLines, "\n")), 1)
+
+				return setupData{
+					content: signedCommitData,
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							Committer: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							SignatureType: gitalypb.SignatureType_PGP,
+							TreeId:        gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Subject:       []byte("commit message"),
+							Body:          []byte("commit message\n"),
+							BodySize:      15,
+						},
+						SignatureData: SignatureData{Payload: []byte(commitData), Signatures: [][]byte{[]byte(pgpSignature), []byte(sshSignature)}},
+					},
+				}
+			},
+		},
+		{
+			desc: "PGP signed commit with headers after the signature",
+			setup: func(_ *testing.T) setupData {
+				commitMessage := "random commit message"
+				commitData, signedCommitData := createSignedCommitData(gpgSignaturePrefix, pgpSignature, "random commit message")
+
+				// Headers after the signature shouldn't be ignored and should be
+				// part of the commit data. If ignored, malicious users can draft a
+				// commit similar to a previously signed commit but with additional
+				// headers and the new commit would still show as verified since we
+				// won't add the additional headers to commit data.
+				postSignatureHeader := fmt.Sprintf("parent %s", gittest.DefaultObjectHash.EmptyTreeOID)
+				modifyCommitData := func(data string) string {
+					return strings.Replace(
+						data,
+						fmt.Sprintf("\n%s", commitMessage),
+						fmt.Sprintf("%s\n\n%s", postSignatureHeader, commitMessage),
+						1,
+					)
+				}
+
+				// We expect that these post signature headers are added to the commit data,
+				// that way any modified commit will fail verification.
+				signedCommitData = modifyCommitData(signedCommitData)
+				commitData = modifyCommitData(commitData)
+
+				return setupData{
+					content: signedCommitData,
+					oid:     gittest.DefaultObjectHash.EmptyTreeOID,
+					expectedCommit: &Commit{
+						GitCommit: &gitalypb.GitCommit{
+							Id: gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Author: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							Committer: &gitalypb.CommitAuthor{
+								Name:     []byte("Bug Fixer"),
+								Email:    []byte("bugfixer@email.com"),
+								Date:     &timestamppb.Timestamp{Seconds: 1584564725},
+								Timezone: []byte("+0100"),
+							},
+							SignatureType: gitalypb.SignatureType_PGP,
+							TreeId:        gittest.DefaultObjectHash.EmptyTreeOID.String(),
+							Subject:       []byte("random commit message"),
+							Body:          []byte("random commit message\n"),
+							BodySize:      22,
+						},
+						SignatureData: SignatureData{Payload: []byte(commitData), Signatures: [][]byte{[]byte(pgpSignature)}},
+					},
+				}
+			},
+		},
+	} {
+		tc := tc
+
+		t.Run(tc.desc, func(t *testing.T) {
+			t.Parallel()
+
+			setup := tc.setup(t)
+
+			commit, err := NewParser().ParseCommit(newStaticObject(setup.content, "commit", setup.oid))
+			require.Equal(t, setup.expectedErr, err)
+			testhelper.ProtoEqual(t, setup.expectedCommit.GitCommit, commit.GitCommit)
+			require.Equal(t, setup.expectedCommit.SignatureData.Payload, commit.SignatureData.Payload)
+			require.Equal(t, setup.expectedCommit.SignatureData.Signatures, commit.SignatureData.Signatures)
+		})
+	}
+}
+
+func createSignedCommitData(signatureField, signature, commitMessage string) (string, string) {
+	commitData := fmt.Sprintf(`tree %s
+author Bug Fixer <bugfixer@email.com> 1584564725 +0100
+committer Bug Fixer <bugfixer@email.com> 1584564725 +0100
+
+%s
+`, gittest.DefaultObjectHash.EmptyTreeOID, commitMessage)
+
+	// Each line of the signature needs to start with a space so that Git recognizes it as a continuation of the
+	// field.
+	signatureLines := strings.Split(signature, "\n")
+	for i, signatureLine := range signatureLines {
+		signatureLines[i] = " " + signatureLine
+	}
+
+	signedCommitData := strings.Replace(commitData, "\n\n", fmt.Sprintf("\n%s%s\n\n", signatureField, strings.Join(signatureLines, "\n")), 1)
+
+	return commitData, signedCommitData
+}
diff --git a/internal/git/catfile/parser.go b/internal/git/catfile/parser.go
index 5df14954d..b64fe16eb 100644
--- a/internal/git/catfile/parser.go
+++ b/internal/git/catfile/parser.go
@@ -11,14 +11,13 @@ import (
 	"time"
 
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
 // Parser parses Git objects into their gitalypb representations.
 type Parser interface {
-	ParseCommit(object git.Object) (*gitalypb.GitCommit, error)
+	ParseCommit(object git.Object) (*Commit, error)
 	ParseTag(object git.Object) (*gitalypb.Tag, error)
 }
 
@@ -37,95 +36,6 @@ func newParser() *parser {
 	}
 }
 
-// ParseCommit parses the commit data from the Reader.
-func (p *parser) ParseCommit(object git.Object) (*gitalypb.GitCommit, error) {
-	commit := &gitalypb.GitCommit{Id: object.ObjectID().String()}
-
-	var lastLine bool
-	p.bufferedReader.Reset(object)
-
-	bytesRemaining := object.ObjectSize()
-	for !lastLine {
-		line, err := p.bufferedReader.ReadString('\n')
-		if errors.Is(err, io.EOF) {
-			lastLine = true
-		} else if err != nil {
-			return nil, fmt.Errorf("parse raw commit: header: %w", err)
-		}
-		bytesRemaining -= int64(len(line))
-
-		if len(line) == 0 || line[0] == ' ' {
-			continue
-		}
-		// A blank line indicates the start of the commit body
-		if line == "\n" {
-			break
-		}
-
-		// There might not be a final line break if there was an EOF
-		if line[len(line)-1] == '\n' {
-			line = line[:len(line)-1]
-		}
-
-		key, value, ok := strings.Cut(line, " ")
-		if !ok {
-			continue
-		}
-
-		switch key {
-		case "parent":
-			commit.ParentIds = append(commit.ParentIds, value)
-		case "author":
-			commit.Author = parseCommitAuthor(value)
-		case "committer":
-			commit.Committer = parseCommitAuthor(value)
-		case "gpgsig", "gpgsig-sha256":
-			commit.SignatureType = detectSignatureType(value)
-		case "tree":
-			commit.TreeId = value
-		case "encoding":
-			commit.Encoding = value
-		}
-	}
-
-	if !lastLine {
-		body := make([]byte, bytesRemaining)
-		if _, err := io.ReadFull(p.bufferedReader, body); err != nil {
-			return nil, fmt.Errorf("reading commit message: %w", err)
-		}
-
-		// After we have copied the body, we must make sure that there really is no
-		// additional data. For once, this is to detect bugs in our implementation where we
-		// would accidentally have truncated the commit message. On the other hand, we also
-		// need to do this such that we observe the EOF, which we must observe in order to
-		// unblock reading the next object.
-		//
-		// This all feels a bit complicated, where it would be much easier to just read into
-		// a preallocated `bytes.Buffer`. But this complexity is indeed required to optimize
-		// allocations. So if you want to change this, please make sure to execute the
-		// `BenchmarkListAllCommits` benchmark.
-		if n, err := io.Copy(io.Discard, p.bufferedReader); err != nil {
-			return nil, fmt.Errorf("reading commit message: %w", err)
-		} else if n != 0 {
-			return nil, fmt.Errorf(
-				"commit message exceeds expected length %v by %v bytes",
-				object.ObjectSize(), n,
-			)
-		}
-
-		if len(body) > 0 {
-			commit.Subject = subjectFromBody(body)
-			commit.BodySize = int64(len(body))
-			commit.Body = body
-			if max := helper.MaxCommitOrTagMessageSize; len(body) > max {
-				commit.Body = commit.Body[:max]
-			}
-		}
-	}
-
-	return commit, nil
-}
-
 const maxUnixCommitDate = 1 << 53
 
 // fallbackTimeValue is the value returned in case there is a parse error. It's the maximum
diff --git a/internal/git/catfile/parser_test.go b/internal/git/catfile/parser_test.go
index 6d2e6d53c..b54251965 100644
--- a/internal/git/catfile/parser_test.go
+++ b/internal/git/catfile/parser_test.go
@@ -1,215 +1,16 @@
 package catfile
 
 import (
-	"bytes"
 	"fmt"
-	"strings"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
-	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
 	"google.golang.org/protobuf/types/known/timestamppb"
 )
 
-func TestParser_ParseCommit(t *testing.T) {
-	t.Parallel()
-
-	info := &ObjectInfo{
-		Oid:  gittest.DefaultObjectHash.EmptyTreeOID,
-		Type: "commit",
-	}
-
-	// Valid-but-interesting commits should be test at the FindCommit level.
-	// Invalid objects (that Git would complain about during fsck) can be
-	// tested here.
-	//
-	// Once a repository contains a pathological object it can be hard to get
-	// rid of it. Because of this I think it's nicer to ignore such objects
-	// than to throw hard errors.
-	for _, tc := range []struct {
-		desc string
-		in   string
-		out  *gitalypb.GitCommit
-	}{
-		{
-			desc: "empty commit object",
-			in:   "",
-			out:  &gitalypb.GitCommit{Id: info.Oid.String()},
-		},
-		{
-			desc: "no email",
-			in:   "author Jane Doe",
-			out: &gitalypb.GitCommit{
-				Id:     info.Oid.String(),
-				Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe")},
-			},
-		},
-		{
-			desc: "unmatched <",
-			in:   "author Jane Doe <janedoe@example.com",
-			out: &gitalypb.GitCommit{
-				Id:     info.Oid.String(),
-				Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe")},
-			},
-		},
-		{
-			desc: "unmatched >",
-			in:   "author Jane Doe janedoe@example.com>",
-			out: &gitalypb.GitCommit{
-				Id:     info.Oid.String(),
-				Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe janedoe@example.com>")},
-			},
-		},
-		{
-			desc: "missing date",
-			in:   "author Jane Doe <janedoe@example.com> ",
-			out: &gitalypb.GitCommit{
-				Id:     info.Oid.String(),
-				Author: &gitalypb.CommitAuthor{Name: []byte("Jane Doe"), Email: []byte("janedoe@example.com")},
-			},
-		},
-		{
-			desc: "date too high",
-			in:   "author Jane Doe <janedoe@example.com> 9007199254740993 +0200",
-			out: &gitalypb.GitCommit{
-				Id: info.Oid.String(),
-				Author: &gitalypb.CommitAuthor{
-					Name:     []byte("Jane Doe"),
-					Email:    []byte("janedoe@example.com"),
-					Date:     &timestamppb.Timestamp{Seconds: 9223371974719179007},
-					Timezone: []byte("+0200"),
-				},
-			},
-		},
-		{
-			desc: "date negative",
-			in:   "author Jane Doe <janedoe@example.com> -1 +0200",
-			out: &gitalypb.GitCommit{
-				Id: info.Oid.String(),
-				Author: &gitalypb.CommitAuthor{
-					Name:     []byte("Jane Doe"),
-					Email:    []byte("janedoe@example.com"),
-					Date:     &timestamppb.Timestamp{Seconds: 9223371974719179007},
-					Timezone: []byte("+0200"),
-				},
-			},
-		},
-		{
-			desc: "ssh signature",
-			in: `gpgsig -----BEGIN SSH SIGNATURE-----
-U1NIU0lHAAAAAQAAADMAAAALc3NoLWVkMjU1MTkAAAAgtc+Qk8jhMwVZk/jFEFCM16LNQb
-30q5kK30bbetfjyTMAAAADZ2l0AAAAAAAAAAZzaGE1MTIAAABTAAAAC3NzaC1lZDI1NTE5
-AAAAQLSyv010gOFwIs9QTtDvlfIEWiAw2iQL/T9usGcxHXn/W5l0cOFCd7O+WaMDg0t0nW
-fF3T79iV8paT4/OfX8Ygg=
------END SSH SIGNATURE-----`,
-			out: &gitalypb.GitCommit{
-				Id:            info.Oid.String(),
-				SignatureType: gitalypb.SignatureType_SSH,
-			},
-		},
-		{
-			desc: "SHA256 signature",
-			in: fmt.Sprintf(`gpgsig-sha256 -----BEGIN PGP SIGNATURE-----
-Version: ObjectivePGP
-Comment: https://www.objectivepgp.com
-Charset: UTF-8
-%c
-wsFcBAABCgAGBQJecon1AAoJEDYMjTn1G2THmSsP/At/jskLdF0i7p0nKf4JLjeeqRJ4k2IUg87U
-ZwV6mbLo5XFm8Sq7CJBAGAhlOZE4BAwKALuawmgs5XMEZwK2z6AIgosGTVpmxDTTI11bXt4XIOdz
-qF7c/gUrJOZzjFXOqDsd5UuPRupwznC5eKlLbfImR+NYxKryo8JGdF5t52ph4kChcQsKlSkXuYNI
-+9UgbaMclEjb0OLm+mcP9QxW+Cs9JS2Jb4Jh6XONWW1nDN3ZTDDskguIqqF47UxIgSImrmpMcEj9
-YSNU0oMoHM4+1DoXp1t99EGPoAMvO+a5g8gd1jouCIrI6KOX+GeG/TFFM0mQwg/d/N9LR049m8ed
-vgqg/lMiWUxQGL2IPpYPcgiUEqfn7ete+NMzQV5zstxF/q7Yj2BhM2L7FPHxKaoy/w5Q/DcAO4wN
-5gxVmIvbCDk5JOx8I+boIS8ZxSvIlJ5IWaPrcjg5Mc40it+WHvMqxVnCzH0c6KcXaJ2SibVb59HR
-pdRhEXXw/hRN65l/xwyM8sklQalAGu755gNJZ4k9ApBVUssZyiu+te2+bDirAcmK8/x1jvMQY6bn
-DFxBE7bMHDp24IFPaVID84Ryt3vSSBEkrUGm7OkyDESTpHCr4sfD5o3LCUCIibTqv/CAhe59mhbB
-2AXL7X+EzylKy6C1N5KUUiMTW94AuF6f8FqBoxnf
-=U6zM
------END PGP SIGNATURE-----`, ' '),
-			out: &gitalypb.GitCommit{
-				Id:            info.Oid.String(),
-				SignatureType: gitalypb.SignatureType_PGP,
-			},
-		},
-		{
-			desc: "huge",
-			in:   "author " + strings.Repeat("A", 100000),
-			out: &gitalypb.GitCommit{
-				Id: info.Oid.String(),
-				Author: &gitalypb.CommitAuthor{
-					Name: bytes.Repeat([]byte("A"), 100000),
-				},
-			},
-		},
-		{
-			desc: "has encoding",
-			in:   "encoding Windows-1251",
-			out: &gitalypb.GitCommit{
-				Id:       info.Oid.String(),
-				Encoding: "Windows-1251",
-			},
-		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			info.Size = int64(len(tc.in))
-			out, err := NewParser().ParseCommit(newStaticObject(tc.in, "commit", info.Oid))
-			require.NoError(t, err, "parse error")
-			require.Equal(t, tc.out, out)
-		})
-	}
-}
-
-func TestParseCommitAuthor(t *testing.T) {
-	t.Parallel()
-
-	for _, tc := range []struct {
-		desc     string
-		author   string
-		expected *gitalypb.CommitAuthor
-	}{
-		{
-			desc:     "empty author",
-			author:   "",
-			expected: &gitalypb.CommitAuthor{},
-		},
-		{
-			desc:   "normal author",
-			author: "Au Thor <au.thor@example.com> 1625121079 +0000",
-			expected: &gitalypb.CommitAuthor{
-				Name:     []byte("Au Thor"),
-				Email:    []byte("au.thor@example.com"),
-				Date:     timestamppb.New(time.Unix(1625121079, 0)),
-				Timezone: []byte("+0000"),
-			},
-		},
-		{
-			desc:   "author with missing mail",
-			author: "Au Thor <> 1625121079 +0000",
-			expected: &gitalypb.CommitAuthor{
-				Name:     []byte("Au Thor"),
-				Date:     timestamppb.New(time.Unix(1625121079, 0)),
-				Timezone: []byte("+0000"),
-			},
-		},
-		{
-			desc:   "author with missing date",
-			author: "Au Thor <au.thor@example.com>",
-			expected: &gitalypb.CommitAuthor{
-				Name:  []byte("Au Thor"),
-				Email: []byte("au.thor@example.com"),
-			},
-		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			testhelper.ProtoEqual(t, tc.expected, parseCommitAuthor(tc.author))
-		})
-	}
-}
-
 func TestParser_ParseTag(t *testing.T) {
 	t.Parallel()
 
diff --git a/internal/git/catfile/tag.go b/internal/git/catfile/tag.go
index 0dfbe8755..afce2f26e 100644
--- a/internal/git/catfile/tag.go
+++ b/internal/git/catfile/tag.go
@@ -61,15 +61,17 @@ func buildAnnotatedTag(ctx context.Context, objectReader ObjectContentReader, ob
 
 	switch tagged.objectType {
 	case "commit":
-		tag.TargetCommit, err = GetCommit(ctx, objectReader, git.Revision(tagged.objectID))
+		commit, err := GetCommit(ctx, objectReader, git.Revision(tagged.objectID))
 		if err != nil {
 			return nil, fmt.Errorf("buildAnnotatedTag error when getting target commit: %w", err)
 		}
+		tag.TargetCommit = commit.GitCommit
 
 	case "tag":
-		tag.TargetCommit, err = dereferenceTag(ctx, objectReader, git.Revision(tagged.objectID))
-		if err != nil {
+		if commit, err := dereferenceTag(ctx, objectReader, git.Revision(tagged.objectID)); err != nil {
 			return nil, fmt.Errorf("buildAnnotatedTag error when dereferencing tag: %w", err)
+		} else if commit != nil {
+			tag.TargetCommit = commit.GitCommit
 		}
 	}
 
@@ -79,7 +81,7 @@ func buildAnnotatedTag(ctx context.Context, objectReader ObjectContentReader, ob
 // dereferenceTag recursively dereferences annotated tags until it finds a non-tag object. If it is
 // a commit, then it will parse and return this commit. Otherwise, if the tagged object is not a
 // commit, it will simply discard the object and not return an error.
-func dereferenceTag(ctx context.Context, objectReader ObjectContentReader, oid git.Revision) (*gitalypb.GitCommit, error) {
+func dereferenceTag(ctx context.Context, objectReader ObjectContentReader, oid git.Revision) (*Commit, error) {
 	object, err := objectReader.Object(ctx, oid+"^{}")
 	if err != nil {
 		return nil, fmt.Errorf("peeling tag: %w", err)
diff --git a/internal/git/localrepo/commit.go b/internal/git/localrepo/commit.go
index df5effc2a..2f77069ef 100644
--- a/internal/git/localrepo/commit.go
+++ b/internal/git/localrepo/commit.go
@@ -68,7 +68,10 @@ func (repo *Repo) ReadCommit(ctx context.Context, revision git.Revision, opts ..
 	if cfg.withTrailers {
 		commit, err = catfile.GetCommitWithTrailers(ctx, repo.gitCmdFactory, repo, objectReader, revision)
 	} else {
-		commit, err = catfile.GetCommit(ctx, objectReader, revision)
+		var c *catfile.Commit
+		if c, err = catfile.GetCommit(ctx, objectReader, revision); err == nil {
+			commit = c.GitCommit
+		}
 	}
 
 	if err != nil {
diff --git a/internal/git/log/last_commit.go b/internal/git/log/last_commit.go
index d92251b32..eb5333668 100644
--- a/internal/git/log/last_commit.go
+++ b/internal/git/log/last_commit.go
@@ -21,7 +21,7 @@ func LastCommitForPath(
 	revision git.Revision,
 	path string,
 	options *gitalypb.GlobalOptions,
-) (*gitalypb.GitCommit, error) {
+) (*catfile.Commit, error) {
 	var stdout strings.Builder
 	cmd, err := gitCmdFactory.New(ctx, repo, git.Command{
 		Name:        "log",
diff --git a/internal/git/log/parser.go b/internal/git/log/parser.go
index 1c9e9d566..dce154770 100644
--- a/internal/git/log/parser.go
+++ b/internal/git/log/parser.go
@@ -55,7 +55,7 @@ func (parser *Parser) Parse(ctx context.Context) bool {
 		return false
 	}
 
-	parser.currentCommit = commit
+	parser.currentCommit = commit.GitCommit
 	return true
 }
 
diff --git a/internal/gitaly/service/commit/commit_signatures.go b/internal/gitaly/service/commit/commit_signatures.go
index 9dc5a9819..122f48c45 100644
--- a/internal/gitaly/service/commit/commit_signatures.go
+++ b/internal/gitaly/service/commit/commit_signatures.go
@@ -1,7 +1,6 @@
 package commit
 
 import (
-	"bufio"
 	"bytes"
 	"errors"
 	"fmt"
@@ -47,6 +46,7 @@ func (s *server) GetCommitSignatures(request *gitalypb.GetCommitSignaturesReques
 		}
 	}
 
+	parser := catfile.NewParser()
 	for _, commitID := range request.CommitIds {
 		commitObj, err := objectReader.Object(ctx, git.Revision(commitID)+"^{commit}")
 		if err != nil {
@@ -56,19 +56,26 @@ func (s *server) GetCommitSignatures(request *gitalypb.GetCommitSignaturesReques
 			return structerr.NewInternal("%w", err)
 		}
 
-		signatureKey, commitText, err := extractSignature(commitObj)
+		commit, err := parser.ParseCommit(commitObj)
 		if err != nil {
 			return structerr.NewInternal("%w", err)
 		}
 
+		signature := []byte{}
+		if len(commit.SignatureData.Signatures) > 0 {
+			// While there could be potentially multiple signatures in a Git
+			// commit, like Git, we only consider the first.
+			signature = commit.SignatureData.Signatures[0]
+		}
+
 		signer := gitalypb.GetCommitSignaturesResponse_SIGNER_USER
 		if signingKeys != nil {
-			if err := signingKeys.Verify(signatureKey, commitText); err == nil {
+			if err := signingKeys.Verify(signature, commit.SignatureData.Payload); err == nil {
 				signer = gitalypb.GetCommitSignaturesResponse_SIGNER_SYSTEM
 			}
 		}
 
-		if err = sendResponse(commitID, signatureKey, commitText, signer, stream); err != nil {
+		if err = sendResponse(commitID, signature, commit.SignatureData.Payload, signer, stream); err != nil {
 			return structerr.NewInternal("%w", err)
 		}
 	}
@@ -76,55 +83,6 @@ func (s *server) GetCommitSignatures(request *gitalypb.GetCommitSignaturesReques
 	return nil
 }
 
-func extractSignature(reader io.Reader) ([]byte, []byte, error) {
-	commitText := []byte{}
-	signatureKey := []byte{}
-	sawSignature := false
-	inSignature := false
-	lineBreak := []byte("\n")
-	whiteSpace := []byte(" ")
-	bufferedReader := bufio.NewReader(reader)
-
-	for {
-		line, err := bufferedReader.ReadBytes('\n')
-
-		if errors.Is(err, io.EOF) {
-			commitText = append(commitText, line...)
-			break
-		}
-		if err != nil {
-			return nil, nil, err
-		}
-
-		if !sawSignature && !inSignature {
-			for _, signatureField := range [][]byte{[]byte("gpgsig "), []byte("gpgsig-sha256 ")} {
-				if !bytes.HasPrefix(line, signatureField) {
-					continue
-				}
-
-				sawSignature, inSignature = true, true
-				line = bytes.TrimPrefix(line, signatureField)
-				break
-			}
-		}
-
-		if inSignature && !bytes.Equal(line, lineBreak) {
-			line = bytes.TrimPrefix(line, whiteSpace)
-			signatureKey = append(signatureKey, line...)
-		} else if inSignature {
-			inSignature = false
-			commitText = append(commitText, line...)
-		} else {
-			commitText = append(commitText, line...)
-		}
-	}
-
-	// Remove last line break from signature
-	signatureKey = bytes.TrimSuffix(signatureKey, lineBreak)
-
-	return signatureKey, commitText, nil
-}
-
 func sendResponse(
 	commitID string,
 	signatureKey []byte,
diff --git a/internal/gitaly/service/commit/find_commits.go b/internal/gitaly/service/commit/find_commits.go
index e2d717b25..15425330c 100644
--- a/internal/gitaly/service/commit/find_commits.go
+++ b/internal/gitaly/service/commit/find_commits.go
@@ -195,7 +195,7 @@ func (g *GetCommits) Commit(ctx context.Context, trailers, shortStat, refs bool)
 		}
 	}
 
-	return commit, nil
+	return commit.GitCommit, nil
 }
 
 func streamCommits(getCommits *GetCommits, stream gitalypb.CommitService_FindCommitsServer, trailers, shortStat bool, refs bool) error {
diff --git a/internal/gitaly/service/commit/last_commit_for_path.go b/internal/gitaly/service/commit/last_commit_for_path.go
index e0f69a97c..555a34e77 100644
--- a/internal/gitaly/service/commit/last_commit_for_path.go
+++ b/internal/gitaly/service/commit/last_commit_for_path.go
@@ -57,7 +57,7 @@ func (s *server) lastCommitForPath(ctx context.Context, in *gitalypb.LastCommitF
 		return &gitalypb.LastCommitForPathResponse{}, nil
 	}
 
-	return &gitalypb.LastCommitForPathResponse{Commit: commit}, err
+	return &gitalypb.LastCommitForPathResponse{Commit: commit.GitCommit}, err
 }
 
 func validateLastCommitForPathRequest(locator storage.Locator, in *gitalypb.LastCommitForPathRequest) error {
diff --git a/internal/gitaly/service/commit/list_all_commits.go b/internal/gitaly/service/commit/list_all_commits.go
index 0ee326c23..62633d6e5 100644
--- a/internal/gitaly/service/commit/list_all_commits.go
+++ b/internal/gitaly/service/commit/list_all_commits.go
@@ -79,7 +79,7 @@ func (s *server) ListAllCommits(
 			return structerr.NewInternal("parsing commit: %w", err)
 		}
 
-		if err := chunker.Send(commit); err != nil {
+		if err := chunker.Send(commit.GitCommit); err != nil {
 			return structerr.NewInternal("sending commit: %w", err)
 		}
 	}
diff --git a/internal/gitaly/service/commit/list_commits.go b/internal/gitaly/service/commit/list_commits.go
index 1a16158a5..09f627873 100644
--- a/internal/gitaly/service/commit/list_commits.go
+++ b/internal/gitaly/service/commit/list_commits.go
@@ -139,7 +139,7 @@ func (s *server) ListCommits(
 			return structerr.NewInternal("parsing commit: %w", err)
 		}
 
-		if err := chunker.Send(commit); err != nil {
+		if err := chunker.Send(commit.GitCommit); err != nil {
 			return structerr.NewInternal("sending commit: %w", err)
 		}
 	}
diff --git a/internal/gitaly/service/commit/list_commits_by_oid.go b/internal/gitaly/service/commit/list_commits_by_oid.go
index 3175c4307..f97c2aca5 100644
--- a/internal/gitaly/service/commit/list_commits_by_oid.go
+++ b/internal/gitaly/service/commit/list_commits_by_oid.go
@@ -50,7 +50,7 @@ func (s *server) ListCommitsByOid(in *gitalypb.ListCommitsByOidRequest, stream g
 			return err
 		}
 
-		if err := sender.Send(commit); err != nil {
+		if err := sender.Send(commit.GitCommit); err != nil {
 			return err
 		}
 	}
diff --git a/internal/gitaly/service/commit/list_commits_by_ref_name.go b/internal/gitaly/service/commit/list_commits_by_ref_name.go
index e77c9f916..6c6d25b07 100644
--- a/internal/gitaly/service/commit/list_commits_by_ref_name.go
+++ b/internal/gitaly/service/commit/list_commits_by_ref_name.go
@@ -37,7 +37,7 @@ func (s *server) ListCommitsByRefName(in *gitalypb.ListCommitsByRefNameRequest,
 		}
 
 		commitByRef := &gitalypb.ListCommitsByRefNameResponse_CommitForRef{
-			Commit: commit, RefName: refName,
+			Commit: commit.GitCommit, RefName: refName,
 		}
 
 		if err := sender.Send(commitByRef); err != nil {
diff --git a/internal/gitaly/service/commit/list_last_commits_for_tree.go b/internal/gitaly/service/commit/list_last_commits_for_tree.go
index fa7e7d0f7..85a5d2a69 100644
--- a/internal/gitaly/service/commit/list_last_commits_for_tree.go
+++ b/internal/gitaly/service/commit/list_last_commits_for_tree.go
@@ -74,7 +74,7 @@ func (s *server) listLastCommitsForTree(in *gitalypb.ListLastCommitsForTreeReque
 
 		commitForTree := &gitalypb.ListLastCommitsForTreeResponse_CommitForTree{
 			PathBytes: []byte(entry.Path),
-			Commit:    commit,
+			Commit:    commit.GitCommit,
 		}
 
 		batch = append(batch, commitForTree)
diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go
index 83b2d7b1f..bc78ca810 100644
--- a/internal/gitaly/service/operations/tags.go
+++ b/internal/gitaly/service/operations/tags.go
@@ -331,7 +331,7 @@ func (s *Server) createTag(
 		if err != nil {
 			return nil, "", structerr.NewInternal("getting commit: %w", err)
 		}
-		tagObject.TargetCommit = peeledTargetCommit
+		tagObject.TargetCommit = peeledTargetCommit.GitCommit
 	}
 
 	return tagObject, refObjectID, nil
diff --git a/internal/gitaly/service/ref/find_all_tags.go b/internal/gitaly/service/ref/find_all_tags.go
index 2091b0011..e58917c15 100644
--- a/internal/gitaly/service/ref/find_all_tags.go
+++ b/internal/gitaly/service/ref/find_all_tags.go
@@ -100,10 +100,11 @@ func (s *server) findAllTags(ctx context.Context, repo *localrepo.Repo, sortFiel
 			// which refers to a commit object. Otherwise, we discard the object's
 			// contents.
 			if peeledTag.ObjectType() == "commit" {
-				result.TargetCommit, err = parser.ParseCommit(peeledTag)
+				commit, err := parser.ParseCommit(peeledTag)
 				if err != nil {
 					return fmt.Errorf("parsing tagged commit: %w", err)
 				}
+				result.TargetCommit = commit.GitCommit
 			} else {
 				if _, err := io.Copy(io.Discard, peeledTag); err != nil {
 					return fmt.Errorf("discarding tagged object contents: %w", err)
@@ -117,7 +118,7 @@ func (s *server) findAllTags(ctx context.Context, repo *localrepo.Repo, sortFiel
 
 			result = &gitalypb.Tag{
 				Id:           tag.ObjectID().String(),
-				TargetCommit: commit,
+				TargetCommit: commit.GitCommit,
 			}
 		default:
 			if _, err := io.Copy(io.Discard, tag); err != nil {
diff --git a/internal/gitaly/service/ref/find_tag.go b/internal/gitaly/service/ref/find_tag.go
index 69f3d93f8..1c700bd65 100644
--- a/internal/gitaly/service/ref/find_tag.go
+++ b/internal/gitaly/service/ref/find_tag.go
@@ -57,7 +57,7 @@ func parseTagLine(ctx context.Context, objectReader catfile.ObjectContentReader,
 		if err != nil {
 			return nil, fmt.Errorf("getting commit catfile: %w", err)
 		}
-		tag.TargetCommit = commit
+		tag.TargetCommit = commit.GitCommit
 		return tag, nil
 	default:
 		return tag, nil
diff --git a/internal/gitaly/service/ref/util.go b/internal/gitaly/service/ref/util.go
index 4f90c3b0d..144d83308 100644
--- a/internal/gitaly/service/ref/util.go
+++ b/internal/gitaly/service/ref/util.go
@@ -35,7 +35,7 @@ func buildAllBranchesBranch(ctx context.Context, objectReader catfile.ObjectCont
 
 	return &gitalypb.FindAllBranchesResponse_Branch{
 		Name:   elements[0],
-		Target: target,
+		Target: target.GitCommit,
 	}, nil
 }
 
@@ -47,7 +47,7 @@ func buildBranch(ctx context.Context, objectReader catfile.ObjectContentReader,
 
 	return &gitalypb.Branch{
 		Name:         elements[0],
-		TargetCommit: target,
+		TargetCommit: target.GitCommit,
 	}, nil
 }