From 77c9fca2d05f8e6e93d15e457ff37b86ae852bc4 Mon Sep 17 00:00:00 2001 From: Savely Krasovsky Date: Tue, 23 Aug 2022 14:41:55 +0000 Subject: feat(gitaly-git2go): sign commits with OpenPGP key --- internal/git2go/apply.go | 7 +++- internal/git2go/apply_test.go | 14 +++---- internal/git2go/cherry_pick.go | 10 +++-- internal/git2go/commit.go | 36 ++++------------- internal/git2go/commit_test.go | 69 +++++++++++++++++++++++++++++--- internal/git2go/executor.go | 2 + internal/git2go/merge.go | 3 ++ internal/git2go/rebase.go | 4 ++ internal/git2go/resolve_conflicts.go | 2 + internal/git2go/revert.go | 4 ++ internal/git2go/submodule.go | 7 +++- internal/git2go/testdata/publicKey.gpg | Bin 0 -> 260 bytes internal/git2go/testdata/signingKey.gpg | Bin 0 -> 297 bytes 13 files changed, 112 insertions(+), 46 deletions(-) create mode 100644 internal/git2go/testdata/publicKey.gpg create mode 100644 internal/git2go/testdata/signingKey.gpg (limited to 'internal/git2go') diff --git a/internal/git2go/apply.go b/internal/git2go/apply.go index 2bee12857..5c2b70da2 100644 --- a/internal/git2go/apply.go +++ b/internal/git2go/apply.go @@ -104,8 +104,13 @@ func (b *Executor) Apply(ctx context.Context, repo repository.GitRepo, params Ap execEnv := b.gitCmdFactory.GetExecutionEnvironment(ctx) + args := []string{"-git-binary-path", execEnv.BinaryPath} + if b.signingKey != "" { + args = append(args, "-signing-key", b.signingKey) + } + var result Result - output, err := b.run(ctx, repo, reader, "apply", "-git-binary-path", execEnv.BinaryPath) + output, err := b.run(ctx, repo, reader, "apply", args...) if err != nil { return "", fmt.Errorf("run: %w", err) } diff --git a/internal/git2go/apply_test.go b/internal/git2go/apply_test.go index 2888dab8b..af99a58e9 100644 --- a/internal/git2go/apply_test.go +++ b/internal/git2go/apply_test.go @@ -41,7 +41,7 @@ func TestExecutor_Apply(t *testing.T) { author := NewSignature("Test Author", "test.author@example.com", time.Now()) committer := NewSignature("Test Committer", "test.committer@example.com", time.Now()) - parentCommitSHA, err := executor.Commit(ctx, repo, CommitParams{ + parentCommitSHA, err := executor.Commit(ctx, repo, CommitCommand{ Repository: repoPath, Author: author, Committer: committer, @@ -50,7 +50,7 @@ func TestExecutor_Apply(t *testing.T) { }) require.NoError(t, err) - noCommonAncestor, err := executor.Commit(ctx, repo, CommitParams{ + noCommonAncestor, err := executor.Commit(ctx, repo, CommitCommand{ Repository: repoPath, Author: author, Committer: committer, @@ -59,7 +59,7 @@ func TestExecutor_Apply(t *testing.T) { }) require.NoError(t, err) - updateToA, err := executor.Commit(ctx, repo, CommitParams{ + updateToA, err := executor.Commit(ctx, repo, CommitCommand{ Repository: repoPath, Author: author, Committer: committer, @@ -69,7 +69,7 @@ func TestExecutor_Apply(t *testing.T) { }) require.NoError(t, err) - updateToB, err := executor.Commit(ctx, repo, CommitParams{ + updateToB, err := executor.Commit(ctx, repo, CommitCommand{ Repository: repoPath, Author: author, Committer: committer, @@ -79,7 +79,7 @@ func TestExecutor_Apply(t *testing.T) { }) require.NoError(t, err) - updateFromAToB, err := executor.Commit(ctx, repo, CommitParams{ + updateFromAToB, err := executor.Commit(ctx, repo, CommitCommand{ Repository: repoPath, Author: author, Committer: committer, @@ -89,7 +89,7 @@ func TestExecutor_Apply(t *testing.T) { }) require.NoError(t, err) - otherFile, err := executor.Commit(ctx, repo, CommitParams{ + otherFile, err := executor.Commit(ctx, repo, CommitCommand{ Repository: repoPath, Author: author, Committer: committer, @@ -214,7 +214,7 @@ func TestExecutor_Apply(t *testing.T) { Author: author, Committer: committer, Message: tc.patches[len(tc.patches)-1].Message, - }, getCommit(t, ctx, repo, commitID)) + }, getCommit(t, ctx, repo, commitID, false)) gittest.RequireTree(t, cfg, repoPath, commitID.String(), tc.tree) }) } diff --git a/internal/git2go/cherry_pick.go b/internal/git2go/cherry_pick.go index 6724d082a..7e954f50e 100644 --- a/internal/git2go/cherry_pick.go +++ b/internal/git2go/cherry_pick.go @@ -8,9 +8,9 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/git/repository" ) -// CherryPickCommand contains parameters to perform a cherry pick. +// CherryPickCommand contains parameters to perform a cherry-pick. type CherryPickCommand struct { - // Repository is the path where to execute the cherry pick. + // Repository is the path where to execute the cherry-pick. Repository string // CommitterName is the committer name for the resulting commit. CommitterName string @@ -26,9 +26,13 @@ type CherryPickCommand struct { Commit string // Mainline is the parent to be considered the mainline Mainline uint + // SigningKey is a path to the key to sign commit using OpenPGP + SigningKey string } -// CherryPick performs a cherry pick via gitaly-git2go. +// CherryPick performs a cherry-pick via gitaly-git2go. func (b *Executor) CherryPick(ctx context.Context, repo repository.GitRepo, m CherryPickCommand) (git.ObjectID, error) { + m.SigningKey = b.signingKey + return b.runWithGob(ctx, repo, "cherry-pick", m) } diff --git a/internal/git2go/commit.go b/internal/git2go/commit.go index 13edb4faa..7197e5a37 100644 --- a/internal/git2go/commit.go +++ b/internal/git2go/commit.go @@ -1,9 +1,7 @@ package git2go import ( - "bytes" "context" - "encoding/gob" "fmt" "gitlab.com/gitlab-org/gitaly/v15/internal/git" @@ -42,8 +40,8 @@ func (err DirectoryExistsError) Error() string { return fmt.Sprintf("directory exists: %q", string(err)) } -// CommitParams contains the information and the steps to build a commit. -type CommitParams struct { +// CommitCommand contains the information and the steps to build a commit. +type CommitCommand struct { // Repository is the path of the repository to operate on. Repository string // Author is the author of the commit. @@ -56,34 +54,14 @@ type CommitParams struct { Parent string // Actions are the steps to build the commit. Actions []Action + // SigningKey is a path to the key to sign commit using OpenPGP + SigningKey string } // Commit builds a commit from the actions, writes it to the object database and // returns its object id. -func (b *Executor) Commit(ctx context.Context, repo repository.GitRepo, params CommitParams) (git.ObjectID, error) { - input := &bytes.Buffer{} - if err := gob.NewEncoder(input).Encode(params); err != nil { - return "", err - } +func (b *Executor) Commit(ctx context.Context, repo repository.GitRepo, c CommitCommand) (git.ObjectID, error) { + c.SigningKey = b.signingKey - output, err := b.run(ctx, repo, input, "commit") - if err != nil { - return "", err - } - - var result Result - if err := gob.NewDecoder(output).Decode(&result); err != nil { - return "", err - } - - if result.Err != nil { - return "", result.Err - } - - commitID, err := git.ObjectHashSHA1.FromHex(result.CommitID) - if err != nil { - return "", fmt.Errorf("could not parse commit ID: %w", err) - } - - return commitID, nil + return b.runWithGob(ctx, repo, "commit", c) } diff --git a/internal/git2go/commit_test.go b/internal/git2go/commit_test.go index 0fef58310..48deee815 100644 --- a/internal/git2go/commit_test.go +++ b/internal/git2go/commit_test.go @@ -7,11 +7,14 @@ import ( "context" "errors" "fmt" + "os" "strconv" "strings" "testing" "time" + "github.com/ProtonMail/go-crypto/openpgp" + "github.com/ProtonMail/go-crypto/openpgp/packet" "github.com/stretchr/testify/require" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest" @@ -63,8 +66,9 @@ func TestExecutor_Commit(t *testing.T) { executor := NewExecutor(cfg, gittest.NewCommandFactory(t, cfg), config.NewLocator(cfg)) for _, tc := range []struct { - desc string - steps []step + desc string + steps []step + signAndVerify bool }{ { desc: "create directory", @@ -455,14 +459,34 @@ func TestExecutor_Commit(t *testing.T) { }, }, }, + { + desc: "update created file, sign commit and verify signature", + steps: []step{ + { + actions: []Action{ + CreateFile{Path: "file", OID: originalFile.String()}, + UpdateFile{Path: "file", OID: updatedFile.String()}, + }, + treeEntries: []gittest.TreeEntry{ + {Mode: DefaultMode, Path: "file", Content: "updated"}, + }, + }, + }, + signAndVerify: true, + }, } { t.Run(tc.desc, func(t *testing.T) { author := NewSignature("Author Name", "author.email@example.com", time.Now()) committer := NewSignature("Committer Name", "committer.email@example.com", time.Now()) + + if tc.signAndVerify { + executor.signingKey = testhelper.SigningKeyPath + } + var parentCommit git.ObjectID for i, step := range tc.steps { message := fmt.Sprintf("commit %d", i+1) - commitID, err := executor.Commit(ctx, repo, CommitParams{ + commitID, err := executor.Commit(ctx, repo, CommitCommand{ Repository: repoPath, Author: author, Committer: committer, @@ -483,7 +507,7 @@ func TestExecutor_Commit(t *testing.T) { Author: author, Committer: committer, Message: message, - }, getCommit(t, ctx, repo, commitID)) + }, getCommit(t, ctx, repo, commitID, tc.signAndVerify)) gittest.RequireTree(t, cfg, repoPath, commitID.String(), step.treeEntries) parentCommit = commitID @@ -492,24 +516,40 @@ func TestExecutor_Commit(t *testing.T) { } } -func getCommit(tb testing.TB, ctx context.Context, repo *localrepo.Repo, oid git.ObjectID) commit { +func getCommit(tb testing.TB, ctx context.Context, repo *localrepo.Repo, oid git.ObjectID, verifySignature bool) commit { tb.Helper() data, err := repo.ReadObject(ctx, oid) require.NoError(tb, err) + var ( + gpgsig, dataWithoutGpgSig string + gpgsigStarted bool + ) + var commit commit lines := strings.Split(string(data), "\n") for i, line := range lines { if line == "" { commit.Message = strings.Join(lines[i+1:], "\n") + dataWithoutGpgSig += "\n" + commit.Message break } + if gpgsigStarted && strings.HasPrefix(line, " ") { + gpgsig += strings.TrimSpace(line) + "\n" + continue + } + split := strings.SplitN(line, " ", 2) require.Len(tb, split, 2, "invalid commit: %q", data) field, value := split[0], split[1] + + if field != "gpgsig" { + dataWithoutGpgSig += line + "\n" + } + switch field { case "parent": require.Empty(tb, commit.Parent, "multi parent parsing not implemented") @@ -520,10 +560,29 @@ func getCommit(tb testing.TB, ctx context.Context, repo *localrepo.Repo, oid git case "committer": require.Empty(tb, commit.Committer, "commit contained multiple committers") commit.Committer = unmarshalSignature(tb, value) + case "gpgsig": + gpgsig = value + "\n" + gpgsigStarted = true default: } } + if gpgsig != "" || verifySignature { + file, err := os.Open("testdata/publicKey.gpg") + require.NoError(tb, err) + + keyring, err := openpgp.ReadKeyRing(file) + require.NoError(tb, err) + + _, err = openpgp.CheckArmoredDetachedSignature( + keyring, + strings.NewReader(dataWithoutGpgSig), + strings.NewReader(gpgsig), + &packet.Config{}, + ) + require.NoError(tb, err) + } + return commit } diff --git a/internal/git2go/executor.go b/internal/git2go/executor.go index 30c8ff0a3..b97a9f3b0 100644 --- a/internal/git2go/executor.go +++ b/internal/git2go/executor.go @@ -31,6 +31,7 @@ var ( // Executor executes gitaly-git2go. type Executor struct { binaryPath string + signingKey string gitCmdFactory git.CommandFactory locator storage.Locator logFormat, logLevel string @@ -41,6 +42,7 @@ type Executor struct { func NewExecutor(cfg config.Cfg, gitCmdFactory git.CommandFactory, locator storage.Locator) *Executor { return &Executor{ binaryPath: cfg.BinaryPath(BinaryName), + signingKey: cfg.Git.SigningKey, gitCmdFactory: gitCmdFactory, locator: locator, logFormat: cfg.Logging.Format, diff --git a/internal/git2go/merge.go b/internal/git2go/merge.go index 7086cfa76..8669a18eb 100644 --- a/internal/git2go/merge.go +++ b/internal/git2go/merge.go @@ -47,6 +47,8 @@ type MergeCommand struct { // If set to `true`, then the resulting commit will have `Ours` as its only parent. // Otherwise, a merge commit will be created with `Ours` and `Theirs` as its parents. Squash bool + // SigningKey is a path to the key to sign commit using OpenPGP + SigningKey string } // MergeResult contains results from a merge. @@ -60,6 +62,7 @@ func (b *Executor) Merge(ctx context.Context, repo repository.GitRepo, m MergeCo if err := m.verify(); err != nil { return MergeResult{}, fmt.Errorf("merge: %w: %s", ErrInvalidArgument, err.Error()) } + m.SigningKey = b.signingKey commitID, err := b.runWithGob(ctx, repo, "merge", m) if err != nil { diff --git a/internal/git2go/rebase.go b/internal/git2go/rebase.go index 9cd4ec814..8b041dadb 100644 --- a/internal/git2go/rebase.go +++ b/internal/git2go/rebase.go @@ -30,9 +30,13 @@ type RebaseCommand struct { // and which are thus empty to be skipped. If unset, empty commits will cause the rebase to // fail. SkipEmptyCommits bool + // SigningKey is a path to the key to sign commit using OpenPGP + SigningKey string } // Rebase performs the rebase via gitaly-git2go func (b *Executor) Rebase(ctx context.Context, repo repository.GitRepo, r RebaseCommand) (git.ObjectID, error) { + r.SigningKey = b.signingKey + return b.runWithGob(ctx, repo, "rebase", r) } diff --git a/internal/git2go/resolve_conflicts.go b/internal/git2go/resolve_conflicts.go index 69d8ed14f..cd1ad0d03 100644 --- a/internal/git2go/resolve_conflicts.go +++ b/internal/git2go/resolve_conflicts.go @@ -28,6 +28,8 @@ type ResolveResult struct { // Resolve will attempt merging and resolving conflicts for the provided request func (b *Executor) Resolve(ctx context.Context, repo repository.GitRepo, r ResolveCommand) (ResolveResult, error) { + r.SigningKey = b.signingKey + if err := r.verify(); err != nil { return ResolveResult{}, fmt.Errorf("resolve: %w: %s", ErrInvalidArgument, err.Error()) } diff --git a/internal/git2go/revert.go b/internal/git2go/revert.go index 7442e1566..4c42e706d 100644 --- a/internal/git2go/revert.go +++ b/internal/git2go/revert.go @@ -26,9 +26,13 @@ type RevertCommand struct { Revert string // Mainline is the parent to be considered the mainline Mainline uint + // SigningKey is a path to the key to sign commit using OpenPGP + SigningKey string } // Revert reverts a commit via gitaly-git2go. func (b *Executor) Revert(ctx context.Context, repo repository.GitRepo, r RevertCommand) (git.ObjectID, error) { + r.SigningKey = b.signingKey + return b.runWithGob(ctx, repo, "revert", r) } diff --git a/internal/git2go/submodule.go b/internal/git2go/submodule.go index e420ad5c5..ce4aad37f 100644 --- a/internal/git2go/submodule.go +++ b/internal/git2go/submodule.go @@ -37,6 +37,9 @@ type SubmoduleCommand struct { Submodule string // Branch where to commit submodule update Branch string + + // SigningKey is a path to the key to sign commit using OpenPGP + SigningKey string } // SubmoduleResult contains results from a committing a submodule update @@ -47,6 +50,8 @@ type SubmoduleResult struct { // Submodule attempts to commit the request submodule change func (b *Executor) Submodule(ctx context.Context, repo repository.GitRepo, s SubmoduleCommand) (SubmoduleResult, error) { + s.SigningKey = b.signingKey + if err := s.verify(); err != nil { return SubmoduleResult{}, fmt.Errorf("submodule: %w", err) } @@ -58,7 +63,7 @@ func (b *Executor) Submodule(ctx context.Context, repo repository.GitRepo, s Sub } // Ideally we would use `b.runWithGob` here to avoid the gob encoding - // boilerplate but it is not possible here because `runWithGob` adds error + // boilerplate, but it is not possible here because `runWithGob` adds error // prefixes and the `LegacyErrPrefix*` errors must match exactly. stdout, err := b.run(ctx, repo, input, cmd) if err != nil { diff --git a/internal/git2go/testdata/publicKey.gpg b/internal/git2go/testdata/publicKey.gpg new file mode 100644 index 000000000..98d02a71e Binary files /dev/null and b/internal/git2go/testdata/publicKey.gpg differ diff --git a/internal/git2go/testdata/signingKey.gpg b/internal/git2go/testdata/signingKey.gpg new file mode 100644 index 000000000..841fbc76a Binary files /dev/null and b/internal/git2go/testdata/signingKey.gpg differ -- cgit v1.2.3