diff options
author | Jenny Kim <yjeankim@gitlab.com> | 2024-01-08 19:39:02 +0300 |
---|---|---|
committer | Jenny Kim <yjeankim@gitlab.com> | 2024-01-08 19:39:02 +0300 |
commit | 25f049564a77a91ae016d2c4bed9202ae5555f9b (patch) | |
tree | 973933231840b70bfbe0dbe03288c35f68183f06 | |
parent | 268f800241e9ddf5b65e5931fb830f1cde82ea2d (diff) | |
parent | d7de749b2850dfbadafcc640e168af9cf38e8787 (diff) |
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: ×tamppb.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: ×tamppb.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: ×tamppb.Timestamp{Seconds: 1584564725}, + Timezone: []byte("+0100"), + }, + Committer: &gitalypb.CommitAuthor{ + Name: []byte("Bug Fixer"), + Email: []byte("bugfixer@email.com"), + Date: ×tamppb.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: ×tamppb.Timestamp{Seconds: 1584564725}, + Timezone: []byte("+0100"), + }, + Committer: &gitalypb.CommitAuthor{ + Name: []byte("Bug Fixer"), + Email: []byte("bugfixer@email.com"), + Date: ×tamppb.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: ×tamppb.Timestamp{Seconds: 1584564725}, + Timezone: []byte("+0100"), + }, + Committer: &gitalypb.CommitAuthor{ + Name: []byte("Bug Fixer"), + Email: []byte("bugfixer@email.com"), + Date: ×tamppb.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: ×tamppb.Timestamp{Seconds: 1584564725}, + Timezone: []byte("+0100"), + }, + Committer: &gitalypb.CommitAuthor{ + Name: []byte("Bug Fixer"), + Email: []byte("bugfixer@email.com"), + Date: ×tamppb.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: ×tamppb.Timestamp{Seconds: 1584564725}, + Timezone: []byte("+0100"), + }, + Committer: &gitalypb.CommitAuthor{ + Name: []byte("Bug Fixer"), + Email: []byte("bugfixer@email.com"), + Date: ×tamppb.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: ×tamppb.Timestamp{Seconds: 1584564725}, + Timezone: []byte("+0100"), + }, + Committer: &gitalypb.CommitAuthor{ + Name: []byte("Bug Fixer"), + Email: []byte("bugfixer@email.com"), + Date: ×tamppb.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: ×tamppb.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: ×tamppb.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 } |