Merge branch 'id-change-committer-of-gitaly-commits' into 'master'

Specify GitLab as Committer for commits created by Gitaly

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

Merged-by: Will Chandler <wchandler@gitlab.com>
Approved-by: John Cai <jcai@gitlab.com>
Reviewed-by: Igor Drozdov <idrozdov@gitlab.com>
Reviewed-by: Will Chandler <wchandler@gitlab.com>
Reviewed-by: John Cai <jcai@gitlab.com>
Co-authored-by: Igor Drozdov <idrozdov@gitlab.com>

13 files changed, 78 insertions, 23 deletions

diff --git a/internal/git/localrepo/commit.go b/internal/git/localrepo/commit.go
index 5f941af0c..df5effc2a 100644
--- a/internal/git/localrepo/commit.go
+++ b/internal/git/localrepo/commit.go
@@ -14,6 +14,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/perm"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v16/proto/go/gitalypb"
@@ -94,7 +95,7 @@ type WriteCommitConfig struct {
 	TreeEntries        []TreeEntry
 	TreeID             git.ObjectID
 	AlternateObjectDir string
-	SigningKey         string
+	GitConfig          config.Git
 }
 
 func validateWriteCommitConfig(cfg WriteCommitConfig) error {
@@ -158,10 +159,20 @@ func (repo *Repo) WriteCommit(ctx context.Context, cfg WriteCommitConfig) (git.O
 		fmt.Sprintf("GIT_AUTHOR_NAME=%s", cfg.AuthorName),
 		fmt.Sprintf("GIT_AUTHOR_EMAIL=%s", cfg.AuthorEmail),
 		fmt.Sprintf("GIT_COMMITTER_DATE=%s", git.FormatTime(cfg.CommitterDate)),
-		fmt.Sprintf("GIT_COMMITTER_NAME=%s", cfg.CommitterName),
-		fmt.Sprintf("GIT_COMMITTER_EMAIL=%s", cfg.CommitterEmail),
 	)
 
+	if featureflag.GPGSigning.IsEnabled(ctx) && cfg.GitConfig.CommitterName != "" && cfg.GitConfig.CommitterEmail != "" {
+		env = append(env,
+			fmt.Sprintf("GIT_COMMITTER_NAME=%s", cfg.GitConfig.CommitterName),
+			fmt.Sprintf("GIT_COMMITTER_EMAIL=%s", cfg.GitConfig.CommitterEmail),
+		)
+	} else {
+		env = append(env,
+			fmt.Sprintf("GIT_COMMITTER_NAME=%s", cfg.CommitterName),
+			fmt.Sprintf("GIT_COMMITTER_EMAIL=%s", cfg.CommitterEmail),
+		)
+	}
+
 	var flags []git.Option
 
 	for _, parent := range cfg.Parents {
@@ -179,8 +190,8 @@ func (repo *Repo) WriteCommit(ctx context.Context, cfg WriteCommitConfig) (git.O
 		git.WithEnv(env...),
 	}
 
-	if featureflag.GPGSigning.IsEnabled(ctx) && cfg.SigningKey != "" {
-		flags = append(flags, git.Flag{Name: "--gpg-sign=" + cfg.SigningKey})
+	if featureflag.GPGSigning.IsEnabled(ctx) && cfg.GitConfig.SigningKey != "" {
+		flags = append(flags, git.Flag{Name: "--gpg-sign=" + cfg.GitConfig.SigningKey})
 		opts = append(opts, git.WithGitalyGPG())
 	}
 
diff --git a/internal/git/localrepo/commit_test.go b/internal/git/localrepo/commit_test.go
index da54e06ff..c92cb6f9f 100644
--- a/internal/git/localrepo/commit_test.go
+++ b/internal/git/localrepo/commit_test.go
@@ -14,6 +14,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/signature"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/testhelper"
@@ -343,6 +344,39 @@ func testWriteCommit(t *testing.T, ctx context.Context) {
 		})
 	}
 
+	t.Run("committer email and name are set", func(tb *testing.T) {
+		if !featureflag.GPGSigning.IsEnabled(ctx) {
+			tb.Skip()
+		}
+
+		cfg := WriteCommitConfig{
+			TreeID:         treeA.OID,
+			AuthorName:     gittest.DefaultCommitterName,
+			AuthorEmail:    gittest.DefaultCommitterMail,
+			CommitterName:  gittest.DefaultCommitterName,
+			CommitterEmail: gittest.DefaultCommitterMail,
+			AuthorDate:     gittest.DefaultCommitTime,
+			CommitterDate:  gittest.DefaultCommitTime,
+			Message:        "my custom message",
+			GitConfig: config.Git{
+				CommitterEmail: "noreply@gitlab.com",
+				CommitterName:  "GitLab",
+			},
+		}
+
+		oid, err := repo.WriteCommit(ctx, cfg)
+		require.Nil(t, err)
+
+		commit, err := repo.ReadCommit(ctx, git.Revision(oid))
+		require.NoError(t, err)
+
+		require.Equal(t, gittest.DefaultCommitAuthor, commit.Author)
+		require.Equal(t, "my custom message", string(commit.Body))
+
+		require.Equal(t, "noreply@gitlab.com", string(commit.Committer.Email))
+		require.Equal(t, "GitLab", string(commit.Committer.Name))
+	})
+
 	t.Run("signed", func(tb *testing.T) {
 		if !featureflag.GPGSigning.IsEnabled(ctx) {
 			tb.Skip()
@@ -357,7 +391,9 @@ func testWriteCommit(t *testing.T, ctx context.Context) {
 			AuthorDate:     gittest.DefaultCommitTime,
 			CommitterDate:  gittest.DefaultCommitTime,
 			Message:        "my custom message",
-			SigningKey:     "testdata/signing_gpg_key",
+			GitConfig: config.Git{
+				SigningKey: "testdata/signing_gpg_key",
+			},
 		}
 
 		oid, err := repo.WriteCommit(ctx, cfg)
diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index 49280cd3d..1e125bc8e 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -308,8 +308,15 @@ type Git struct {
 	BinPath            string      `toml:"bin_path,omitempty" json:"bin_path"`
 	CatfileCacheSize   int         `toml:"catfile_cache_size,omitempty" json:"catfile_cache_size"`
 	Config             []GitConfig `toml:"config,omitempty" json:"config"`
-	SigningKey         string      `toml:"signing_key,omitempty" json:"signing_key"`
-	RotatedSigningKeys []string    `toml:"rotated_signing_keys,omitempty" json:"rotated_signing_keys"`
+	// SigningKey is the private key used for signing commits created by Gitaly
+	SigningKey string `toml:"signing_key,omitempty" json:"signing_key"`
+	// RotatedSigningKeys are the private keys that have used for commit signing before.
+	// The keys from the SigningKey field is moved into this field for some time to rotate signing keys.
+	RotatedSigningKeys []string `toml:"rotated_signing_keys,omitempty" json:"rotated_signing_keys"`
+	// CommitterEmail is the committer email of the commits created by Gitaly, e.g. `noreply@gitlab.com`
+	CommitterEmail string `toml:"committer_email,omitempty" json:"committer_email"`
+	// CommitterName is the committer name of the commits created by Gitaly, e.g. `GitLab`
+	CommitterName string `toml:"committer_name,omitempty" json:"committer_name"`
 }
 
 // Validate runs validation on all fields and compose all found errors.
diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index 417047951..f0225e4c3 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -487,7 +487,9 @@ func TestLoadConfigCommand(t *testing.T) {
 					{
 						"git": {
 							"signing_key": "signing_key",
-							"rotated_signing_keys": ["rotated_key_1", "rotated_key_2"]
+							"rotated_signing_keys": ["rotated_key_1", "rotated_key_2"],
+							"committer_email": "noreply@gitlab.com",
+							"committer_name": "GitLab"
 						}
 					}
 					EOF
@@ -505,6 +507,8 @@ func TestLoadConfigCommand(t *testing.T) {
 						cfg.Git.BinPath = "foo/bar"
 						cfg.Git.SigningKey = "signing_key"
 						cfg.Git.RotatedSigningKeys = []string{"rotated_key_1", "rotated_key_2"}
+						cfg.Git.CommitterEmail = "noreply@gitlab.com"
+						cfg.Git.CommitterName = "GitLab"
 					}),
 				}
 			},
diff --git a/internal/gitaly/service/commit/commit_signatures_test.go b/internal/gitaly/service/commit/commit_signatures_test.go
index 7be69ecb3..f0925629f 100644
--- a/internal/gitaly/service/commit/commit_signatures_test.go
+++ b/internal/gitaly/service/commit/commit_signatures_test.go
@@ -297,7 +297,7 @@ func testGetCommitSignatures(t *testing.T, ctx context.Context) {
 					AuthorDate:     gittest.DefaultCommitTime,
 					CommitterDate:  gittest.DefaultCommitTime,
 					Message:        "message",
-					SigningKey:     cfg.Git.SigningKey,
+					GitConfig:      cfg.Git,
 				})
 				require.NoError(t, err)
 
@@ -310,7 +310,9 @@ func testGetCommitSignatures(t *testing.T, ctx context.Context) {
 					AuthorDate:     gittest.DefaultCommitTime,
 					CommitterDate:  gittest.DefaultCommitTime,
 					Message:        "rotated key commit message",
-					SigningKey:     cfg.Git.RotatedSigningKeys[0],
+					GitConfig: config.Git{
+						SigningKey: cfg.Git.RotatedSigningKeys[0],
+					},
 				})
 				require.NoError(t, err)
 
diff --git a/internal/gitaly/service/operations/cherry_pick.go b/internal/gitaly/service/operations/cherry_pick.go
index 27de0544d..715ddd77f 100644
--- a/internal/gitaly/service/operations/cherry_pick.go
+++ b/internal/gitaly/service/operations/cherry_pick.go
@@ -114,7 +114,7 @@ func (s *Server) UserCherryPick(ctx context.Context, req *gitalypb.UserCherryPic
 			CommitterName:  committerSignature.Name,
 			CommitterEmail: committerSignature.Email,
 			CommitterDate:  committerSignature.When,
-			SigningKey:     s.signingKey,
+			GitConfig:      s.gitConfig,
 		},
 	)
 	if err != nil {
diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go
index 6ef7fcecf..3e8389ea7 100644
--- a/internal/gitaly/service/operations/commit_files.go
+++ b/internal/gitaly/service/operations/commit_files.go
@@ -485,7 +485,7 @@ func (s *Server) userCommitFilesGit(
 		CommitterEmail: committerSignature.Email,
 		Message:        string(header.CommitMessage),
 		TreeID:         treeish,
-		SigningKey:     s.signingKey,
+		GitConfig:      s.gitConfig,
 	}
 
 	if cfg.AuthorName == "" {
diff --git a/internal/gitaly/service/operations/merge_branch.go b/internal/gitaly/service/operations/merge_branch.go
index 677c96e6f..faf99de62 100644
--- a/internal/gitaly/service/operations/merge_branch.go
+++ b/internal/gitaly/service/operations/merge_branch.go
@@ -74,7 +74,6 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc
 		revision.String(),
 		firstRequest.CommitId,
 		false,
-		true,
 	)
 	if err != nil {
 		var conflictErr *localrepo.MergeTreeConflictError
diff --git a/internal/gitaly/service/operations/merge_to_ref.go b/internal/gitaly/service/operations/merge_to_ref.go
index 73510142b..23ddeb5cb 100644
--- a/internal/gitaly/service/operations/merge_to_ref.go
+++ b/internal/gitaly/service/operations/merge_to_ref.go
@@ -100,7 +100,6 @@ func (s *Server) UserMergeToRef(ctx context.Context, request *gitalypb.UserMerge
 		oid.String(),
 		sourceOID.String(),
 		false,
-		false,
 	)
 	if err != nil {
 		s.logger.WithError(err).WithFields(
diff --git a/internal/gitaly/service/operations/merge_utils.go b/internal/gitaly/service/operations/merge_utils.go
index a71c2c35f..51442beeb 100644
--- a/internal/gitaly/service/operations/merge_utils.go
+++ b/internal/gitaly/service/operations/merge_utils.go
@@ -17,7 +17,6 @@ func (s *Server) merge(
 	ours string,
 	theirs string,
 	squash bool,
-	sign bool,
 ) (string, error) {
 	treeOID, err := quarantineRepo.MergeTree(ctx, ours, theirs, localrepo.WithAllowUnrelatedHistories(), localrepo.WithConflictingFileNamesOnly())
 	if err != nil {
@@ -39,9 +38,7 @@ func (s *Server) merge(
 		CommitterName:  committer.Name,
 		CommitterEmail: committer.Email,
 		CommitterDate:  committer.When,
-	}
-	if sign {
-		cfg.SigningKey = s.signingKey
+		GitConfig:      s.gitConfig,
 	}
 	c, err := quarantineRepo.WriteCommit(
 		ctx,
diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go
index fcd1813fd..90d03a7dd 100644
--- a/internal/gitaly/service/operations/revert.go
+++ b/internal/gitaly/service/operations/revert.go
@@ -117,7 +117,7 @@ func (s *Server) UserRevert(ctx context.Context, req *gitalypb.UserRevertRequest
 			CommitterName:  committerSignature.Name,
 			CommitterEmail: committerSignature.Email,
 			CommitterDate:  committerSignature.When,
-			SigningKey:     s.signingKey,
+			GitConfig:      s.gitConfig,
 		},
 	)
 	if err != nil {
diff --git a/internal/gitaly/service/operations/server.go b/internal/gitaly/service/operations/server.go
index 9673bb2da..e29665a0a 100644
--- a/internal/gitaly/service/operations/server.go
+++ b/internal/gitaly/service/operations/server.go
@@ -7,6 +7,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/git/quarantine"
+	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/hook/updateref"
 	"gitlab.com/gitlab-org/gitaly/v16/internal/gitaly/service"
@@ -29,7 +30,7 @@ type Server struct {
 	gitCmdFactory git.CommandFactory
 	catfileCache  catfile.Cache
 	updater       *updateref.UpdaterWithHooks
-	signingKey    string
+	gitConfig     config.Git
 }
 
 // NewServer creates a new instance of a grpc OperationServiceServer
@@ -43,7 +44,7 @@ func NewServer(deps *service.Dependencies) *Server {
 		gitCmdFactory: deps.GetGitCmdFactory(),
 		catfileCache:  deps.GetCatfileCache(),
 		updater:       deps.GetUpdaterWithHooks(),
-		signingKey:    deps.GetCfg().Git.SigningKey,
+		gitConfig:     deps.GetCfg().Git,
 	}
 }
 
diff --git a/internal/gitaly/service/operations/squash.go b/internal/gitaly/service/operations/squash.go
index 0b06c5030..0afb47be2 100644
--- a/internal/gitaly/service/operations/squash.go
+++ b/internal/gitaly/service/operations/squash.go
@@ -145,7 +145,6 @@ func (s *Server) userSquash(ctx context.Context, req *gitalypb.UserSquashRequest
 		startCommit.String(),
 		endCommit.String(),
 		true,
-		true,
 	)
 	if err != nil {
 		var mergeConflictErr *localrepo.MergeTreeConflictError